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

review V51.4.2 - OAuth and OIDC audience restrictions for tokens #2182

Closed
elarlang opened this issue Oct 22, 2024 · 30 comments
Closed

review V51.4.2 - OAuth and OIDC audience restrictions for tokens #2182

elarlang opened this issue Oct 22, 2024 · 30 comments
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V51 Group issues related to OAuth Will be closed if no response/opposite arguments _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

elarlang commented Oct 22, 2024

From the initial OAuth paragraph draft we have requirements:

# Description L1 L2 L3
51.4.2 [ADDED] Verify that access tokens are restricted to certain resource servers (audience restriction), preferably to a single resource server. Every resource server is obliged to verify, for every request, whether the access token sent with that request was meant to be used for that particular resource server. If not, the resource server must refuse to serve the respective request.

Additionally to some formating improvements, we need to (re)validate the content, the need, the problem to solve and sections.

@elarlang elarlang added the V51 Group issues related to OAuth label Oct 22, 2024
@elarlang elarlang changed the title review V51.4.2 and V51.4.3 review V51.4.2 Oct 22, 2024
@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 labels Oct 22, 2024
@elarlang
Copy link
Collaborator Author

We have also requirement 3.5.6:

# Description L1 L2 L3 CWE
3.5.6 [ADDED] Verify that other, security-sensitive attributes of a stateless token are being verified. For example, in a JWT this may include issuer, subject, and audience. 287

Maybe we should cover it with spliting 3.5.6 to more precise requirements, as recommended in #1967 (comment).

At the same time I don't want the message to be hidden, that aud in an access token points to resource server and aud in ID token points to client_id.

@randomstuff
Copy link
Contributor

randomstuff commented Oct 23, 2024

preferably to a single resource server.

FWIW, this condition is less important (sometimes) when using sender-constrained tokens.

@elarlang
Copy link
Collaborator Author

For direction:

Verify that the resource server validates the access token to be made for that resource server (audience) by checking the 'aud' claim from the access token to be an expected value.

Need to cover:

  • reference token and introspection
  • fix the last part - the aud need to contain expected value referencing to resource server

@elarlang elarlang added the 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos label Oct 29, 2024
@elarlang
Copy link
Collaborator Author

elarlang commented Nov 6, 2024

Updated:

Verify that the resource server validates the access token to be made for that resource server (audience), for example by checking the 'aud' claim from the access token to be an expected value.

ping @randomstuff

elarlang added a commit that referenced this issue Nov 6, 2024
elarlang added a commit that referenced this issue Nov 6, 2024
@randomstuff
Copy link
Contributor

Proposition of minor rewording:

Verify that the resource server validates that the access token is intended to be usable on that resource server (audience), for example by checking the 'aud' claim from the access token to be an expected value.

@randomstuff
Copy link
Contributor

Or:

Verify that the resource server rejects the request if the access token was not intended to be usable on that resource server (audience), for example by checking the 'aud' claim from the access token to be an expected value.

@TobiasAhnoff
Copy link

I vote for the more positive wording

Verify that the resource server validates that the access token is intended to be usable on that resource server (audience), for example by checking the 'aud' claim from the access token to be an expected value.

@randomstuff
Copy link
Contributor

OK for the positive one. Is it clear enough?

@elarlang
Copy link
Collaborator Author

elarlang commented Nov 8, 2024

"to be usable" > "to be used"?

OK for the positive one. Is it clear enough?

Do you want to highlight the problem to solve"? Like "To be sure that access tokens from the same issuer for different resources (audience) can not be cross-used."?

@randomstuff
Copy link
Contributor

I don't know :) it's probably OK like that.

@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos labels Nov 10, 2024
@elarlang elarlang added 6) PR awaiting review and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Nov 13, 2024
elarlang pushed a commit that referenced this issue Nov 18, 2024
@randomstuff
Copy link
Contributor

Nitpicky changes:

# Description L1 L2 L3
51.4.2 [ADDED] Verify that the resource server validates that the access token is intended to be used on that resource server (audience). For access token in JWT format or using JWT from token introspection, it can be done by checking the 'aud' claim.

(added ", it")

Moreover I would not say "JWT from token introspection" because this is not stricly speaking a JWT:

# Description L1 L2 L3
51.4.2 [ADDED] Verify that the resource server validates that the access token is intended to be used on that resource server (audience). For access token in JWT format or using OAuth, it can be done by checking the 'aud' claim.

@elarlang elarlang reopened this Nov 19, 2024
@elarlang elarlang added the 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR label Nov 19, 2024
@elarlang
Copy link
Collaborator Author

elarlang commented Nov 19, 2024

Note that the context for this requirement is OAuth resource server

Proposed update for V51.4.2:

Verify that the resource server ensures that the access token is intended to be used with that server (audience). Validation can be done by checking the 'aud' claim from the access token or from the token introspection response.

@TobiasAhnoff
Copy link

TobiasAhnoff commented Nov 19, 2024

Both 51.4.2 (option 2) and 51.5.4 are good, perhaps add introspection to 51.4.2 to make it more clear

Verify that the resource server validates that the access token is intended to be used on that resource server (audience). For access token in JWT format or using OAuth introspection, it can be done by checking the 'aud' claim.

But this (except the note on OAuth introspection) is also covered by #2363 (comment), maybe suggested 51.4.2 can be "merged" to that?

@elarlang
Copy link
Collaborator Author

except the note on OAuth introspection

This is the main reason why we have a separate requirement for OAuth, as it is a special case. The same applies for 'aud' validation from ID token.

elarlang pushed a commit to elarlang/ASVS that referenced this issue Nov 19, 2024
@elarlang elarlang removed the 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR label Nov 19, 2024
@tghosth
Copy link
Collaborator

tghosth commented Nov 19, 2024

I would like to suggest the following tweaked wording for 3.5.8 and 51.4.2:

# Description L1 L2 L3 CWE
3.5.8 [ADDED] Verify that the service only accepts tokens which are intended for use with that service. For JWTs, this can be achieved by validating the 'aud' claim against an allowlist defined in the service.
51.4.2 [ADDED] Verify that the resource server only accepts access tokens which are intended for use with that server (audience). The audience may be included in a cryptographically secured token or it can be checked using the token introspection endpoint.

@elarlang
Copy link
Collaborator Author

elarlang commented Nov 19, 2024

We have a separate issue for general requirement (proposed 3.5.8): #2363. To keep everything followable, I recommend to add this proposal there taking into account abstract feedback from here.

With this post it would be nice to have some diff of changes and reasons behind that.

The audience may be included in a cryptographically secured token or it can be checked using the token introspection endpoint.

Technically correct is: the audience IS included in access token JWT and MAY be included in introspection response.

"Validation can be done" is not good, but it carries the meaning: "most likely you can use the 'aud' claim, but if something else is used to achieve that, it is also ok".

I don't want to mess with "cryptographically secured token" terminology here - all OAuth / OIDC specs are talking about JWT and "cryptographically secured token" is just a alien here. There is no abstraction needed in that field as it is fixed to use JWT. It is more precise and correct.

In general: both requirements have gone through long discussions to have every word in place. If something different is proposed, I expect it take into account everything that is said before starting the entire discussion from the start.

@randomstuff
Copy link
Contributor

randomstuff commented Nov 19, 2024

I don't want to mess with "cryptographically secured token" terminology here - all OAuth / OIDC specs are talking about JWT and "cryptographically secured token" is just a alien here. There is no abstraction needed in that field as it is fixed to use JWT. It is more precise and correct.

OIDC is fixed to using JWT for ID token and related tokens. For OAuth, JWT is required in many places (DPoP, etc.). For other cases (OAuth access tokens), some people could find it interesting (or have other reasons) to use other token formats such as:

  • SAML ← yes this is still used
  • PASETO ← self-proclaimed better-than-JWT
  • CWT ← often used if the token is transferred as QR code
  • itsdangerous
  • Biscuit ← have funny properties

Many of the requirements about tokens (in Oauth or otherwise) are applicable to them to some extent.

@tghosth
Copy link
Collaborator

tghosth commented Nov 20, 2024

I added a comment here for 3.5.8: #2363 (comment)

My proposal for 51.4.2:

# Description L1 L2 L3 CWE
51.4.2 [ADDED] Verify that the resource server only accepts access tokens which are intended for use with that server (audience). The audience may be included in a cryptographically secured token or it can be checked using the token introspection endpoint.

As @randomstuff says, I don't think OAuth access tokens have to be JWTs which is why I used the "cryptographically secured token" terminology. Similarly, the "aud" claim is not limited to OAuth and might appear differently in different tokens which is why I don't think it should be specifically mentioned here. As further proof, the OAuth token introspection endpoint exists specifically for a situation when the access token is not a cryptographically secured token or a JWT.

@elarlang
Copy link
Collaborator Author

For a starter, some questions to @randomstuff

some people could find it interesting (or have other reasons) to use other token formats such as:

Is it supported by spec or is it something that "I can do it"?

Is is widespread enough to rewrite entire chapter to be abstract and with a danger to not be that clear and understandable?

Is it something "you can technically do if you want" or something that "I want to rewrite OAuth and OIDC chapter"?

If it is supported (not limited) by the OAuth spec, is it enough to just mention it with one line in chapter text?

If someone is using SAML, ist it called OAuth or is it called SAML that by theory requires it's own specific requirements?

@elarlang
Copy link
Collaborator Author

elarlang commented Nov 20, 2024

Answering myself:

The usage of "cryptographically secured" is valid only

  • for the context to provide defense against tampering
  • for requirements dealing with that

.. and it should not carried anywhere else, where it is out of context, potentially incorrect, and potentially limiting the requirement. For example here, if the token is "cryptographically secured" does not mean it has structured information (to provide the aud information).

Proposal update:

Verify that the resource server only accepts access tokens that are intended for use with that service (audience). The audience may be included in a structured access token (such as the 'aud' claim in JWT) or it can be checked using the token introspection endpoint.

Changes:

  • "which" > "that" - grammarly
  • "server (audience)" > "service (audience)" - align with Add token requirement for intented audience #2363
  • "The audience may be included in a cryptographically secured token" > "The audience may be included in a structured access token (such as the 'aud' claim in JWT)"

@tghosth
Copy link
Collaborator

tghosth commented Nov 20, 2024

So it bothers me to allow relying on something in the token without being specific that it would need to be signed in order to rely upon it. Whilst I agree that "if the token is "cryptographically secured" does not mean it has structured information", the opposite cannot be the case as we cannot rely on a structured access token which is not cryptographically secured.

So maybe:

Verify that the resource server only accepts access tokens that are intended for use with that service (audience). The audience may be included in a cryptographically secured, structured access token (such as the 'aud' claim in JWT) or it can be checked using the token introspection endpoint.

@elarlang
Copy link
Collaborator Author

elarlang commented Nov 20, 2024

Do you want to duplicate the crypto part to every requirement that has an integrity check as a pre-condition or requirements in current V3.5 related to crypto and is integrity check enough for that? (The question is rhetorical)

More from https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-12#section-1.4

The authorization server MUST ensure that access tokens cannot be generated, modified, or guessed to produce valid access tokens by unauthorized parties.

Crypto is one solution for achieving that. It is covered somewhere else in the ASVS. Additionally, OAuth itself does not say how it must be achieved. My point with this (and all the other requirements in OAuth and OIDC chapter) is - in case "cryptographically secured token" is used, it is validated and the integrity check for the token is done before reaching to other requirements. We don't need to duplicate it here.

To just explain the idea of duplication, why to limit with "cryptographically secured" tokens?

Verify that the resource server only accepts access tokens that are intended for use with that service (audience). The audience may be included in a cryptographically secured (3.5.3, 3.5.5, 3.5.7), in valid duration (3.5.4), validated type (3.5.9) structured access token (such as the 'aud' claim in JWT) or it can be checked using the token introspection endpoint.

@tghosth
Copy link
Collaborator

tghosth commented Nov 20, 2024

Ok so I am ok with this then:

Verify that the resource server only accepts access tokens that are intended for use with that service (audience). The audience may be included in a structured access token (such as the 'aud' claim in JWT) or it can be checked using the token introspection endpoint.

@elarlang
Copy link
Collaborator Author

@randomstuff - the requirement got updated and merged as:

# Description L1 L2 L3
51.4.2 [ADDED] Verify that the resource server only accepts access tokens that are intended for use with that service (audience). The audience may be included in a structured access token (such as the 'aud' claim in JWT) or it can be checked using the token introspection endpoint.

I'll wait for your feedback or confirmation, does it need further improvements or can we close this?

@randomstuff
Copy link
Contributor

OK for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V51 Group issues related to OAuth Will be closed if no response/opposite arguments _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

4 participants