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

typ validation in JWT JOSE header #160

Open
lmm-git opened this issue Apr 17, 2024 · 4 comments · May be fixed by #161
Open

typ validation in JWT JOSE header #160

lmm-git opened this issue Apr 17, 2024 · 4 comments · May be fixed by #161

Comments

@lmm-git
Copy link

lmm-git commented Apr 17, 2024

Currently, in the verification module the typ header field of the JOSE header of JWT is checked.

However, I am not sure why exactly as I did not find any spec warranting the comment in line 205.

I checked RFC 7515 stating "Use of this Header Parameter is OPTIONAL" and even more that the typ can even be JOSE etc. Also RFC 7519 mentions that even if the header is set it is just recommended that the header should be set to "JWT".

At the moment, there is no possibility builtin to turn off or change the verification. Personally, I would see three different options to improve the situation:

  1. Remove this check altogether. It has no security implications and the checks are only recommended. (It is already skipped if the claim is not set at all)
  2. Allow disabling the check
  3. Allow specifying a list of allowed typ values while defaulting to JWT
@lmm-git lmm-git linked a pull request Apr 17, 2024 that will close this issue
@ramosbugs
Copy link
Owner

Hey, thanks for starting this discussion.

Out of curiosity, what typ value(s) are you seeing in the wild that are causing issues with this crate?

The typ header specifies the kind of content that is attached to the JOSE header, and I don't think it should be ignored if it's specified. It's optional, so we don't reject JWTs without it, but if it's explicitly included and defines a format not understood by this crate, then I think we should reject the JWT as unsupported.

The section you cited from RFC 7515 (JWS, a layer of abstraction underneath RFC 7519 (JWT)) states:

The "typ" value "JOSE" can be used by applications to indicate that
this object is a JWS or JWE using the JWS Compact Serialization or
the JWE Compact Serialization. The "typ" value "JOSE+JSON" can be
used by applications to indicate that this object is a JWS or JWE
using the JWS JSON Serialization or the JWE JSON Serialization.
Other type values can also be used by applications.

The format used in OpenID Connect is defined to be the JWS/JWE Compact Serialization. Based on this, I agree that we should relax the check to accept either JWT or JOSE. We should probably also accept either format with the optional application/ MIME type prefix defined in each of those RFC's Media Type Registrations (case-insensitively).

However, I don't think we should ignore typ altogether by default, because values like JOSE+JSON (JWS/JWE JSON Serialization) are unsupported by OpenID Connect or this crate. Similarly, if we see a MIME type like image/png, it doesn't seem like we should process the JWT further.

At the moment, there is no possibility builtin to turn off or change the verification. Personally, I would see three different options to improve the situation:

  1. Remove this check altogether. It has no security implications and the checks are only recommended. (It is already skipped if the claim is not set at all)
  2. Allow disabling the check
  3. Allow specifying a list of allowed typ values while defaulting to JWT

I think Option 2 probably makes the most sense. Since, as you mentioned, this doesn't have obvious security implications, it's probably not worth the complexity of Option 3.

@lmm-git
Copy link
Author

lmm-git commented Apr 18, 2024

Hey, thanks for your quick response!

I think your reasoning makes sense. If you want, I can adjust the PR accordingly.

The reason I stumbled upon that specific quirk in the implementation as I am currently trying to implement RFC 9068 based on this project as it seems to have a quite nice code base with sane defaults properly following the according standards. Therefore, I took a look on how the verification is done here as within RFC 9068 the check is mandatory. This is also the reason why I started with implementing Option 3 to easily check for other typ values.

@ramosbugs
Copy link
Owner

This is also the reason why I started with implementing Option 3 to easily check for other typ values.

That makes sense. Since the proposed changes are to the JwtClaimsVerifier and not the IdTokenVerifier, I'm willing to merge them to help support other kinds of JWTs. Do you want to mark the PR ready for review so that it's mergeable?

I am currently trying to implement RFC 9068 based on this project

Cool. Are the APIs you need to do that public? I'd like to support these kinds of extensions but hopefully without becoming a general-purpose JWT library. It might even make sense to split this crate at some point so that other libraries can leverage the innards (via a lower-level crate) without adding them to the public interface of this crate. Both crates could still live in this repo as a Rust workspace, though.

@lmm-git
Copy link
Author

lmm-git commented Apr 24, 2024

I will adjust the MR accordingly next week (unfortunately, I have no time to properly finish it until then). But I wanted to give you a short update.

So far, I started implementing a verifying access tokens within your project, check out this branch. This is really a proof of concept and I am personally not sure whether this is the right approach. I tested locally to expose more of the internal API to support such a use case better, but it seems as there would be a lot of additional exports required. Actually, my first try was just to implement the JwtAccessTokenVerifier externally, but this got really messy quite quick (but I can try again as I am now much more into the code base than before). Let me know what you think.

Based on these changes, I implemented a proof of concept verifying access tokens in an API situation, but this code is not ready to share yet. Will do that probably later next week.

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 a pull request may close this issue.

2 participants