Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audience claim verification check with Bearer token #856

Closed
SagarGi opened this issue May 2, 2024 · 14 comments · Fixed by #864
Closed

Audience claim verification check with Bearer token #856

SagarGi opened this issue May 2, 2024 · 14 comments · Fixed by #864

Comments

@SagarGi
Copy link

SagarGi commented May 2, 2024

Description

I am currently in a situation where i have an access_token for nextcloud which i got from keycloak. The user_oidc app allows a setting to make API request as bearer-auth using the access_token. In the access_token i have an audience claim for example another-client other than nextcloud itself.

for example:

{
    "exp": 1714648426,
    "iat": 1714647826,
    "auth_time": 1714647781,
    "jti": "8ebabf79-7a87-4c74-86fe-58e24300677b",
    "iss": "https://keycloak.local/realms/opendesk",
    `"aud": "https://openproject.local"`,
    "sub": "6b65f442-e0a2-4e2d-ac0a-32506dd57f41",
    "typ": "Bearer",
    "azp": "nextcloud",
    "session_state": "28b06352-4cb6-4fca-9487-11795e1ec7a2",
}

In the above payload of the access_token the audience claim is "aud": "https://openproject.local" which is not nextcloud. And i am able to make API request with it.

I have some question regarding it.

  • Does the user_oidc app check and verify the audience claim in the access_token for API request?
  • Or the user_oidc is missing to check the audience claim?
  • Or its intended feature?
  • Or any other reasons why the audience claim is not checked and verified when making API request?
@julien-nc
Copy link
Member

Does the user_oidc app check and verify the audience claim in the access_token for API request?

No, only the token expiration is checked: https://github.com/nextcloud/user_oidc/blob/main/lib/User/Validator/SelfEncodedValidator.php#L67-L71

There are many more checks done when obtaining a token during login: https://github.com/nextcloud/user_oidc/blob/main/lib/Controller/LoginController.php#L444-L484

It is kind of intended. I mean, the goal is to let other services (using the same client from the same IdP) make API requests to NC.

Why are you asking? Do you expect the validation to fail in this case?

@SagarGi
Copy link
Author

SagarGi commented May 3, 2024

@julien-nc Any other services making request with the nextcloud with any audience claim seemed a bit odd. Also i might lack knowledge why the audience claim is not verified and checked in user_oidc. But we want to have a situation in NC+OP integration where the services knows that the token is intended only for it to make requests.

@julien-nc
Copy link
Member

Is the audience supposed to always be the Oidc client ID?
If so, then we can validate the audience and this means only tokens obtained by OP with the same Oidc client than NC could be used to make requests against NC.

@wielinde
Copy link

wielinde commented May 6, 2024

Hey @julien-nc, AFAIK Keycloak's token endpoint allows replacing one token for another. So, if OP has one access token with the OP client as audience it, then OP can request a new token with it to get another access token with the desired audience, the Nextcloud client. For that to work Keycloak's realm needs to have set policies that allow this. The Keycloak docs explain that. See Keycloak's docs (paragraph 7) on token exchange https://www.keycloak.org/docs/latest/securing_apps/index.html#_token-exchange

If you think it from the other way around: if you don't have audience awareness during authorisation you could (mis)use the same token for requests in the other direction. Without an audience check any Keycloak access token from a client within the same realm will authorize in Nextcloud, even though it was originally given to a completely different client, e. g. some untrustworthy service that you would only give little permissions/scopes within Keycloak.

@julien-nc
Copy link
Member

Ok then I guess we can add audience check in the bearer token validation. This would be the same check as when getting the login token, we would check the audience is the Oidc client ID.
Let's make this check optional and enable by default. Wdyt?

@wielinde
Copy link

wielinde commented May 7, 2024

@julien-nc Sounds great! Thank you!

@col-panic
Copy link

Please consider #864 (comment)

You should allow the opt-out of this validation for API too!

@julien-nc
Copy link
Member

@col-panic Let discuss that here.

As mentioned there:
https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation

This validation MAY include that when an azp (authorized party) Claim is present, the Client SHOULD verify that its client_id is the Claim Value.

I'm not sure if the AZP check is wrong or if there is something wrong in your setup.

Can you check the AZP value in the ID token you receive? You can check that by setting the Nextcloud log level to debug (0) and looking at a log line that contains Parsed the JWT payload after trying to log in.
Or you can replace line 507 of lib/Controller/LoginController.php with:

$message = $this->l10n->t('The authorized party does not match ours: ' . $idTokenPayload->azp . ' !== ' . $provider->getClientId());

and this message should be displayed in the browser when trying to log in.

@col-panic
Copy link

col-panic commented Aug 15, 2024

@julien-nc thank you for your feedback:

  1. I could find, that the resp. line is coming from user_oidc/lib/User/Validator/SelfEncodedValidator.php
  2. I found that config:app:set user_oidc selfencoded_bearer_validation_audience_check --value=0 does not seem to be considered. It still performs the check within, which fails with
    This token is not for us, authorized party SelfEncodedValidator.php (azp) is different than the client ID (I addded SelfEncodedValidator.php to differentiate to the LoginController`
  3. I understand, that the check itself is correct - the azp value presented is "azp": "elexis-web-api" which is clearly not nextcloud, and I understand the security implications. We will change the implementation in order to adapt to it - but in the short run I really want to deactivate this.

Why does the above occ command not work to disable

$oidcSystemConfig = $this->config->getSystemValue('user_oidc', []);
?

@col-panic
Copy link

Uncommenting lines 91 to 108 makes it work for me ;)

@col-panic
Copy link

@julien-nc stupid question, in #856 (comment) you refer to the IDToken - but why is an ID Token even sent to an API endpoint? Shouldn't you always use an Access Token here? https://oauth.net/id-tokens-vs-access-tokens/

@igonaf
Copy link

igonaf commented Aug 15, 2024

When you use an accessToken with azp=clientA and aud=API_1, API_2, API_1 (e.g., Nextcloud) and API_2 should verify the token, but the primary focus should be on the aud field rather than the azp.

The aud field indicates which resources (i.e., which APIs) the token is intended for. If the aud field specifies API_1 and API_2, the token can be used to access both APIs. When receiving a request with an accessToken, both API_1 and API_2 should check whether the aud field contains their identifier. If aud includes the API's identifier, then the token is valid for that API.

The azp field indicates who requested the token (in this case, clientA). The azp field confirms which client (e.g., application) initiated the request for the token. This field is primarily used for security purposes so that the resource server knows which client issued the token, but it is not mandatory for all APIs to check. @julien-nc But here, you are comparing azp with your $providerClientId, which is wrong since it breaks the concept. This part should be cut off, and the config selfencoded_bearer_validation_audience_check should validate only the aud . On the other hand, azp validation may stay as a separate option (e.g., selfencoded_bearer_validation_azp_check) but needs to be mapped with a trusted azp list (e.g., from Nextcloud config, settings or env) not $providerClientId.

@julien-nc you mentioned the link. In the context of authentication and authorization, such as in Keycloak, you receive three main types of tokens: idToken, accessToken, and refreshToken. The primary purpose of the idToken is to provide information about the user. It contains encoded information about the identity of the user who has successfully authenticated in the system. The idToken is used by the client application to obtain information about the user and to confirm the user's identity after a successful login.
The accessToken is used to authorize access to protected resources or APIs on behalf of the user. The accessToken is used every time the client application or service attempts to access protected resources or APIs. The resource server checks this token to grant or deny access.
So IDTokenValidation shouldn't be part of user_oidc as @col-panic mentioned.

API_1 and API_2 typically only check aud to ensure that the token is indeed intended for them. Verification of azp is generally not necessary for a resource server (API).

@igonaf
Copy link

igonaf commented Aug 16, 2024

@julien-nc To sum up, if selfencoded_bearer_validation_audience_check=true, then only accessToken received in the Nextcloud application can be used in user_oidc (which breaks SSO principles); if selfencoded_bearer_validation_audience_check=false then many accessTokens could be used for user_oidc without proper permission (aud context).

@julien-nc
Copy link
Member

More granular settings for aud and azp checks: #921

@igonaf In short, are you saying that user_oidc should only accept access tokens as bearer tokens when a client performs a Nextcloud API request?

So IDTokenValidation shouldn't be part of user_oidc as @col-panic mentioned.

You mean only when validating a bearer token, right? We need to validate the ID token on login.

We have collaborators who use ID tokens to perform requests to the NC API. We need to keep this.

It is already possible to prevent ID token validation and only perform the access token validation (user_oidc makes a request to the IdP userinfo endpoint):

'user_oidc' => [
    'userinfo_bearer_validation' => true,
    'selfencoded_bearer_validation' => false,
],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants