Add issuer, audience and azp checks in bearer token validator #864
Conversation
julien-nc
force-pushed
the
enh/856/add-bearer-token-audience-check
branch
from
99ea021
to
bc823aa
Compare
| return null; | ||
| } | ||
| } else { | ||
| $this->logger->debug('Multiple audiences but no authorized party (azp) in the id token'); |
There was a problem hiding this comment.
| $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?
There was a problem hiding this comment.
ID tokens are JWT tokens. Access tokens are more or less equivalent to OAuth tokens. This log message is correct IMO.
| $checkAudience = !isset($oidcSystemConfig['selfencoded_bearer_validation_audience_check']) | ||
| || !in_array($oidcSystemConfig['selfencoded_bearer_validation_audience_check'], [false, 'false', 0, '0'], true); | ||
| if ($checkAudience) { | ||
| if (!($payload->aud === $provider->getClientId() || in_array($provider->getClientId(), $payload->aud, true))) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Audience can be a string or an array. The check is now safer. Any better?
julien-nc
force-pushed
the
enh/856/add-bearer-token-audience-check
branch
from
bc823aa
to
022ff1a
Compare
julien-nc
force-pushed
the
enh/856/add-bearer-token-audience-check
branch
from
022ff1a
to
6984473
Compare
|
@wielinde Could you say again what was your concern about the The current check (when validating the token for API calls) is: 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? |
|
@julien-nc I understand the specification that you have linked above in the following manner: Only when there is an And I am not aware of any such OIDC extension. So from my understanding, I would remove the requirement that the an // FYI @ba1ash |
|
@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. All good? |
|
@julien-nc From my understanding of the spec, that looks good, now. Thank you. |
…ake it possible to disable audience check via config Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
julien-nc
force-pushed
the
enh/856/add-bearer-token-audience-check
branch
from
8cdbd7e
to
91e7712
Compare
The bearer token validation is less complete than the login controller one.
closes #856