fix: rfc9068 must condition ignored in introspection

[james-d-elliott]

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

[mitar]

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]

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.

[mitar]

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]

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]

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]

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.

[aeneasr]

Given that this SHOULD (aka optional) validation is a breaking change as we're restricting validation, this can't be merged as is without having - most likely - negative impact on existing customers in Ory.

[aeneasr]

I'm also confused, what does RFC9068 have to do with the standard token introspector? These are completely different specs.

[aeneasr]

I see, we don't implement/support RFC9068 as it is a newer spec from 2021 and wasn't available when this library was written. Back-porting it onto existing implementation will cause serious issues. We can maybe add a feature flag to optionally enable RFC9068 compliance. At the same time, this section reeks of Auth0's weird mix of "it's an access token BUT it also represents a user session well not really because it's not an ID token but hey we rely on oauth2 so let's standardize this somehow".

2.2.1.  Authentication Information Claims

   The claims listed in this section MAY be issued in the context of
   authorization grants involving the resource owner and reflect the
   types and strength of authentication in the access token that the
   authentication server enforced prior to returning the authorization
   response to the client.  Their values are fixed and remain the same
   across all access tokens that derive from a given authorization
   response, whether the access token was obtained directly in the
   response (e.g., via the implicit flow) or after one or more token
   exchanges (e.g., obtaining a fresh access token using a refresh token
   or exchanging one access token for another via [RFC8693] procedures).

   auth_time  OPTIONAL - as defined in Section 2 of [OpenID.Core].

   acr  OPTIONAL - as defined in Section 2 of [OpenID.Core].

   amr  OPTIONAL - as defined in Section 2 of [OpenID.Core].

None of this should be in an access token in my view.

[mitar]

I must say that I didn't get here because this is something written in some standard, but from very concrete experience where current implementation of introspection cannot differentiate between ID tokens or access tokens and I see this as a security issue. I am not sure if it is exploitable, but anything which can be swapped around without detection in my mind opens doors to potential issues.

If some standard figured out how to address this, then I am all for it. It seems RFC 9068 has and so let's do it their way.

I am not advocating necessary to implement full RFC 9068, especially if you feel that it was undue influenced by Auth0's choices. But this particular change, having a flag in authenticated header, feels to me like simply a good security design for any protocol.

Now how we go about adding this in a backwards compatible way? To me, because I see it is a security issue, I feel that the change should be opt-out. fosite should be more secure by default and developers should have an easy way to disable this to keep backwards compatible. So maybe one configuration option to not set and validate header?

[aeneasr]

ID tokens should be signed with another key than jwt access tokens, and they also have a different audience (the auth server and not the auth client). Is it actually possible to introspect ID Tokens in Hydra? I would be quite surprised

[james-d-elliott]

Given that this SHOULD (aka optional) validation is a breaking change as we're restricting validation, this can't be merged as is without having - most likely - negative impact on existing customers in Ory.

From https://www.rfc-editor.org/rfc/rfc9068.html#name-validating-jwt-access-token

Resource servers receiving a JWT access token MUST validate it in the following manner.
The resource server MUST verify that the "typ" header value is "at+jwt" or "application/at+jwt" and reject tokens carrying any other value.

I'm also confused, what does RFC9068 have to do with the standard token introspector? These are completely different specs.

I was not entirely aware the JWT introspector wasn't intended to support RFC9068, I had assumed (probably the issue here) since the typ value used the RFC9068 semantics that it was actually a JWT Profile Access Token. You mentioned a different spec, was this an early version of this spec maybe or another one I'm not aware of?

---

I can understand all the other points. I think in regards to the auth_time, amr, and acr claims this does communicate useful information to an auth server to determine if the authorization has performed enough validation of the resource owner to be trustworthy. These are definitely optional elements/claims though.

As far as I can see it the existing implementation of the JWT Access Tokens fosite mints already satisfies this aspect of the spec. I think based on this fact the chance of it having a negative impact is minimal if not non-existent. I am however not terribly familiar with the implementation on the Hydra side.

I would be more than willing to make the adjustments necessary to have this off by default with an opt-in. This would probably totally mitigate the issues and ory can technically perform a strategic staged migration should that be desirable. Alternatively I'd not be offended if you decided not to take up this PR.

As far as RFC9068, if the intention was not to support it, I think there is a significant functional benefit in adapting to supporting this specific spec to ensure the validation semantics are of a well understood specification rather than what is effectively a proprietary implementation which could very likely have specific validation semantics that may only be supported via an official library, though that being said I fall heavily in the camp of access tokens should never be used for third parties for anything other than API access such as introspection or user information (NOT to directly validate identity like some inexplicably seem to do).

[mitar]

ID tokens should be signed with another key than jwt access tokens

I do not think this is the case in fosite? Does it even support that? At least default configuration uses same key for all JWT signing.

they also have a different audience

They do, but this is not checked in introspection at all. Whole discussion in #410 was about that and we then agreed that opt-in additional parameter to introspection endpoint to tell expected audience is a reasonable addition, so I made #845. And while I agree that checking audience is useful and can prevent this confusion as well, I do think that fundamentally making JWT access tokens and JWT ID tokens different through header is something which should be done by default.

Is it actually possible to introspect ID Tokens in Hydra? I would be quite surprised

In Fosite it was, the last time I checked. I even added TODO to this effect. This PR would resolve that TODO nicely (and in a standard compliant way, which additional audience parameter to the introspection endpoint sadly is not, even if it is very useful).