Add issuer, audience and azp checks in bearer token validator

[julien-nc]

The bearer token validation is less complete than the login controller one.

• add issuer, audience and azp (authorized party) checks in bearer token validator
• make it possible to disable bearer token audience check via config.php.

closes #856

[SagarGi]

Suggested change

	$this->logger->debug('Multiple audiences but no authorized party (azp) in the id token');
	$this->logger->debug('Multiple audiences but no authorized party (azp) in the access token');

or simple token may be?

[julien-nc]

ID tokens are JWT tokens. Access tokens are more or less equivalent to OAuth tokens. This log message is correct IMO.

[SagarGi]

hi @julien-nc I check out this PR. In case we have a single audience in the access token other than nextcloud.
for example i tried to make request with with access token:

   "aud": "client_other_than_nextcloud"

i got server error with status code 500 as:

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
    <s:exception>TypeError</s:exception>
    <s:message>in_array(): Argument #2 ($haystack) must be of type array, string given</s:message>
</d:error>

It seems like it is checking for multiple audience as an array but for single we have only string. Might be the reason for the error?

[julien-nc]

Audience can be a string or an array. The check is now safer. Any better?

[julien-nc]

@wielinde Could you say again what was your concern about the azp check?

The current check (when validating the token for API calls) is:
If there are multiple audiences in the token (the aud attribute of the token is an array), we want the azp token attribute (authorized party) to be equal to the Oidc client ID.

This was implemented following points 4 and 5 of https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation

Do you think this check only makes sense to authenticate in NC but not when validating a token used to make API calls?

[wielinde]

@julien-nc I understand the specification that you have linked above in the following manner:

Only when there is an azp is given, then it shall be taken into account. However, that it is present in a token is not the default. In a vanilla OIDC setup, the azp shall not be added to a token, unless required by a OIDC extension.

And I am not aware of any such OIDC extension. So from my understanding, I would remove the requirement that the an azp must be given whenever an array of aud is present. And when azp is given, then it needs to be in the list auf aud AND be the Nextcloud client. Does that make sense?

// FYI @ba1ash

[julien-nc]

@wielinde I can't remember where I found a different description of how the azp check should be done. Anyway let's stick to this one.
For both login token and bearer token, the check is now: (Independently of the aud claim) If the token has the azp claim, it should be equal to the Oidc client ID.
If audience check is disabled for bearer token validation, the azp is not checked even if it's present.

All good?

[wielinde]

@julien-nc From my understanding of the spec, that looks good, now. Thank you.

[juliusknorr]

Code looks good 👍

[col-panic]

I'm using user_oidc to protect my API. Now since this patch, I cannot access Nextcloud using API anymore, the output is This token is not for us, authorized party (azp) is different than the client ID

I found the setting selfencoded_bearer_validation_audience_check but it seems to catch only when coming in via https://github.com/nextcloud/user_oidc/blob/f5bb30caef3a31ba19fe3f44da256b46cac90e9f/lib/User/Validator/SelfEncodedValidator.php#L90 not when coming in via

user_oidc/lib/Controller/LoginController.php

Line 506 in f5bb30c

$this->logger->debug('This token is not for us, authorized party (azp) is different than the client ID');

Check for example https://github.com/search?q=repo%3Anextcloud%2Fuser_oidc+%22This+token+is+not+for+us%2C+authorized+party+%28azp%29+is+different+than+the+client+ID%22&type=code

Could you please verify, that this option does also disable azp and aud checkin when used via API?

Should I create a new issue for this?