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

fix: rfc9068 must condition ignored in introspection #767

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

james-d-elliott
Copy link
Contributor

@james-d-elliott james-d-elliott commented Aug 31, 2023

This fixes an outstanding TODO where the requirement for a correctly formed RFC9068 access token MUST have the media type of 'application/at+jwt', and that this media type MUST be appropriately reflected in the typ header as either 'at+jwt' (SHOULD), or as the media type itself. See https://datatracker.ietf.org/doc/html/rfc9068#section-2.1 for more information.

Related Issue or Design Document

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

@james-d-elliott james-d-elliott marked this pull request as draft August 31, 2023 20:19
@james-d-elliott james-d-elliott marked this pull request as ready for review February 13, 2024 08:53
@mitar
Copy link
Contributor

mitar commented Feb 15, 2024

Tests are missing.

Also, I could not find where in fosite we set type for access tokens to at+jwt? It seems to me we only set it to JWT.

@james-d-elliott
Copy link
Contributor Author

I must have missed the tests and the change which makes the JWT Profile default to this typ when picking this out of local changes. Let me see what I have there.

@james-d-elliott james-d-elliott force-pushed the fix-rfc9068-typ-validation branch from 5be54f6 to 760314f Compare February 15, 2024 09:57
This fixes an outstanding TODO where the requirement for a correctly formed RFC9068 access token MUST have the media type of 'application/at+jwt', and that this media type MUST be appropriately reflected in the typ header as either 'at+jwt' (SHOULD), or as the media type itself. See https://datatracker.ietf.org/doc/html/rfc9068#section-2.1 for more information.
@james-d-elliott james-d-elliott force-pushed the fix-rfc9068-typ-validation branch from 760314f to cf41b86 Compare February 15, 2024 09:58
@mitar
Copy link
Contributor

mitar commented Feb 15, 2024

So there is no typ for ID tokens? Have you made a test which makes sure we are not setting now this new typ for ID tokens as well?

@james-d-elliott
Copy link
Contributor Author

So there is no typ for ID tokens? Have you made a test which makes sure we are not setting now this new typ for ID tokens as well?

This change doesn't affect that code path, just GetJWTHeader() *jwt.Headers not IDTokenHeaders() *jwt.Headers.

@mitar
Copy link
Contributor

mitar commented Feb 15, 2024

Any thoughts on how to transition this on a live system? There might be a period when old valid access tokens are returned as invalid because they have old typ?

(This is not a problem for me, but it might be for other users of fosite.)

@james-d-elliott
Copy link
Contributor Author

I have made sure the function is exported for this reason. I think from a practical standpoint they should be considered invalid or the implementer should implement their own validator which allows both based on iat claim. i.e. tokens issued before x should be safe if they have the JWT typ. However I think it would be irresponsible for security to make this a default option, and implementers should consider all tokens absent this typ to be invalid.

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 this pull request may close these issues.

2 participants