user_oidc icon indicating copy to clipboard operation
user_oidc copied to clipboard

Audience claim verification check with Bearer token

Open SagarGi opened this issue 1 year ago • 6 comments

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?

SagarGi avatar May 02 '24 11:05 SagarGi

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?

julien-nc avatar May 02 '24 11:05 julien-nc

@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.

SagarGi avatar May 03 '24 03:05 SagarGi

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.

julien-nc avatar May 06 '24 10:05 julien-nc

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.

wielinde avatar May 06 '24 20:05 wielinde

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?

julien-nc avatar May 07 '24 08:05 julien-nc

@julien-nc Sounds great! Thank you!

wielinde avatar May 07 '24 12:05 wielinde

Please consider https://github.com/nextcloud/user_oidc/pull/864#issuecomment-2286113125

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

col-panic avatar Aug 13 '24 17:08 col-panic

@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.

julien-nc avatar Aug 14 '24 08:08 julien-nc

@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 https://github.com/nextcloud/user_oidc/blob/125e525fb688fa1353e57454ea81ee7fcd03eb2d/lib/User/Validator/SelfEncodedValidator.php#L88?

col-panic avatar Aug 15 '24 06:08 col-panic

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

col-panic avatar Aug 15 '24 07:08 col-panic

@julien-nc stupid question, in https://github.com/nextcloud/user_oidc/issues/856#issuecomment-2288204104 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/

col-panic avatar Aug 15 '24 07:08 col-panic

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 avatar Aug 15 '24 10:08 igonaf

@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).

igonaf avatar Aug 16 '24 09:08 igonaf

More granular settings for aud and azp checks: https://github.com/nextcloud/user_oidc/pull/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,
],

julien-nc avatar Aug 20 '24 16:08 julien-nc