ramosbugs / openidconnect-rs

OpenID Connect Library for Rust
MIT License
404 stars 100 forks source link

`typ` validation in JWT JOSE header #160

Closed lmm-git closed 3 months ago

lmm-git commented 5 months ago

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
ramosbugs commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

ramosbugs commented 3 months ago

~Done in #175~