Access token and code hash are not generated with the right key length

[vivshankar]

Describe the bug

The access token hash should be computed based on the alg header key length and not hard coded to 256. This is from the spec.

at_hash
OPTIONAL. Access Token hash value. Its value is the base64url encoding of the left-most half of the hash of the octets of the ASCII representation of the access_token value, where the hash algorithm used is the hash algorithm used in the alg Header Parameter of the ID Token's JOSE Header. For instance, if the alg is RS256, hash the access_token value with SHA-256, then take the left-most 128 bits and base64url encode them. The at_hash value is a case sensitive string.

Code hash is similar.

c_hash
Code hash value. Its value is the base64url encoding of the left-most half of the hash of the octets of the ASCII representation of the code value, where the hash algorithm used is the hash algorithm used in the alg Header Parameter of the ID Token's JOSE Header. For instance, if the alg is HS512, hash the code value with SHA-512, then take the left-most 256 bits and base64url encode them. The c_hash value is a case sensitive string.
If the ID Token is issued from the Authorization Endpoint with a code, which is the case for the response_type values code id_token and code id_token token, this is REQUIRED; otherwise, its inclusion is OPTIONAL.

To Reproduce

1. Configure the ID token alg header to use, say, RS512
2. Run the flow to produce the id_token
3. Look at the c_hash and at_hash

Expected behavior

Implemented per specification.

Environment

All

Additional context

None

[vivshankar]

The fix is relatively simple and I am happy to contribute it. I would like to hear back on whether this is something that has been considered before and rejected for some reason. I did not find it in the Issue history, so I am assuming not.

[aeneasr]

Could you please link the cited references?

[vivshankar]

https://openid.net/specs/openid-connect-core-1_0.html#HybridIDToken

I am on the phone but you should see this in the other sections of the specification too. It's part of the core

Edit: Please also see section 3.1.3.6 that covers the token endpoint.

[mitar]

I think what you are saying is that fosite currently does not support other algorithms than RS256 and ES256 and that is true. Probably it has some hard-coded assumptions. So are you trying to enable other algorithms and you have issues?

[vivshankar]

No, I am indicating that the code is current hard coding the key length to 256 when computing the hashes.

See - https://github.com/ory/fosite/blob/master/handler/openid/helper.go#L41

Here it is possible to get to the Session and IDTokenHeaders to read the alg. See snippet of code below:

computeHash(ctx context.Context, sess openid.Session, token string) (string, error) {
	var err error
	hashSize := 256
	if alg, ok := sess.IDTokenHeaders().Get("alg").(string); ok {
		hashSize, err = strconv.Atoi(alg[2:])
		if err != nil {
			hashSize = 256
		}
	}

	var hash hash.Hash
	if hashSize == 384 {
		hash = sha512.New384()
	} else if hashSize == 512 {
		hash = sha512.New()
	} else {
		// fallback to 256
		hash = sha256.New()
	}

	buffer := bytes.NewBufferString(token)
	_, err = hash.Write(buffer.Bytes())
	if err != nil {
		return "", err
	}
	hashBuf := bytes.NewBuffer(hash.Sum([]byte{}))

	return base64.RawURLEncoding.EncodeToString(hashBuf.Bytes()[:hashBuf.Len()/2]), nil
}

There are other flows where this is computed without using the helper.

https://github.com/ory/fosite/blob/master/handler/openid/flow_hybrid.go#L150
https://github.com/ory/fosite/blob/master/handler/openid/flow_implicit.go#L101

In both, the strategy used is RS256JWTStrategy and it is hard coded.

The code of the sort I am proposing above would retain backward compatibility while addressing the violation of the spec.

[mitar]

No, I am indicating that the code is current hard coding the key length to 256 when computing the hashes.

Yes, because code is hard-coded to RS256 and ES256. Or am I still not getting it?

[vivshankar]

@mitar Sorry I guess I am not getting your point. The fact that it is hard coded is the problem. It should not be hard coded. It should be based on the alg in the ID token header.

[mitar]

Yes, Fosite currently does not support other algorithms besides RS256 and ES256. Spec also does not require one to support all algorithms. Are you saying you would like fosite to support other algorithms?

[vivshankar]

Fosite offers the ability to specify a JWTStrategy that can be used to support any algorithm. This is something that we use to sign the ID token using our own implementation that consumes Fosite and extend to different key lengths and algorithms. However, that JWTStrategy appears to not be used to compute the hash, which seems simple enough to change.

I am not looking for native implementations equivalent to RS256JWTStrategy. I am proposing that the code should either be modified to respect the alg that can be specified in the IDTokenHeaders or allow an extension to implement a JWTStrategy/HashStrategy that computes the hash.

At the moment, the only way to override this is rather messy. I am happy to continue our discussion over Slack. Also, I am happy contributing the code once we settle on the approach. @mitar

[narg95]

hmm let me give my 5 cents:

• What I understand from @mitar is that out of the box fosite only support RS256 and ES256, therefore SHA-256 is hard-coded as hashing alg for claim at_hash and c_hash. So he is asking if you need fosite to support any other signing algorithm which indirectly will fix @vivshankar issue with hashing.

• What I understand from @vivshankar is that he is not using out-of-the-box fosite signing algorithms, but he is plugging his own JWT signing strategies, but he is having pain on ID Tokens generation because the hashing, which is linked at which signing alg was use, is not equally extensible but hard-coded.

I see two options, either fosite is smart enough and identify and pick the correct hashing algorithm when issuing the IDToken from a list of supported hashing algorithms, or fosite allows to plug a hashing strategy for those claims so that it is consistent with the extensibility of JWT Signing Strategy. I also noted hashing code is not consistent on each flow: c.Enigma.Hash or c.RS256JWTStrategy.Hash or helper.GetAccessTokenHash. A PR to address this issue can also bring consistency here.

[vivshankar]

@narg95 Thanks. This is exactly my proposal. I plug in my own because I support different key lengths and PS algorithms as well.

[mitar]

I think I added ES256 support so from what I remember it was pretty messy adding it. :-) So this is why I think this will not be so simple change. But yea, I also think that fosite should support this somehow. I just think it is a bit of work.

[vivshankar]

Another option is to add a check to see if the hash already exists in the claims and if it does, not overwrite it. Effectively, this allows someone using Fosite to write a custom handler that runs before the Fosite OpenIDConnect handler. @mitar Does that sound better as an interim solution? The change then would be fairly isolated.

[mitar]

I think this is not a very secure design. It is error prone and hard to debug/follow the logic. Generally for every field there should be only one place where it is being set.

But let's see what @aeneasr thinks about this issue.

[vivshankar]

I have offered 3 paths that can be followed:

1. Use the snippet of code I posted a few comments back that computes the hash based on the alg in the session headers.
2. Provide a hash strategy that can be overriden to be symmetric with the JWTStrategy interface.
3. Custom handler that is allowed to compute this hash outside of Fosite core handlers

IMO the first is the cleanest and most correct based on the specification.

[vivshankar]

There is one extension point/interface that does exist and we have modified the provider to use that - the IDTokenStrategy. We effectively wrap the DefaultStrategy.

type idTokenStrategy struct {
	DefaultStrategy *fopenid.DefaultStrategy
}

The only thing in the DefaultStrategy that we end up using is the GenerateIDToken func, which eventually calls into our JWTStrategy to sign the token etc. This approach works OK.

[aeneasr]

The proposed change makes sense to me as specified by the spec! In general, supporting more JWK algs out of the box would also be nice :)

[vivshankar]

Ok great, I will roll in the change I proposed in #630 (comment).

It is effectively two things:

1. Update the IDTokenHelper
2. Modify handlers not using IDTokenHelper to use IDTokenHelper to generate hashes

@aeneasr @mitar Let me know if you have any objections.

[aeneasr]

Sorry, I missed this notification. Is this still an issue?

[vivshankar]

@aeneasr Yes, this is still an issue. I haven't gotten round to submitting a PR yet.

[aeneasr]

Ok :) No pressure! Looking forward to your (as always) great contributions :)

[james-d-elliott]

Relevant spec for matching JWA to elliptic curves etc: https://datatracker.ietf.org/doc/html/rfc7518#section-3.1

Think this is important because the elliptic curve of the key theoretically needs to also match the alg.

Also to add to this it doesn't seem clear how to support both RS256 and ES256 in the same fosite.OAuth2Provider when using compose.Compose(). Is it something not supported via compose, or is it not supported at all, or am I just being daft and missing something? I have a feeling this issue is talking about probably, just expressed in a different way.

I thought it was potentially the fosite.OpenIDConnectClient implementations GetJSONWebKeysURI() or GetJSONWebKeys(), however they appear to only be used to validate client request objects if I'm not mistaken, not to sign id tokens.

[aeneasr]

I’ll have to take a look st the code but the signer objects should be capable of being having alternate implementations with different algs. If I am not mistaken we already support that in Ory Hydra so it should be fairly straight forward. Regarding compose, you’ll probably need to add a new compose struct to support other algs easily

[vivshankar]

Fosite has some really really good extensible interfaces. The JWTStrategy (look under /token/jwt/jwt.go) is generally initialized before calling compose.Compose. On my end (I don't work at ory :-) ), we have our own implementation that supports RS, ES and PS, along with the ability to encrypt the token based on client configuration. It had to be our own because, notwithstanding the missing algorithm implementations, we have our own key management approach.

The ugly part is passing the Client configuration object into the Generate function. So we end up using the Go context object, which I really dislike given how out-of-control that can-of-worms can become. If you choose to configure these at the provider level, i.e. using a wrapper of compose.Config to add additional config properties, it is much simpler. You can just throw that in as part of constructing your JWT strategy.

This particular issue, however, is about the hash that is generated for authorization code and access token (can be extended to s_hash if there's an inclination to support that). It needs to be based on the key length defined in the JWT header (RS512 for example). I am just a little delayed in contributing the code. I hope to get to it in the next couple of weeks once things settle down on my end.

[james-d-elliott]

After reading all this again it makes sense what you're proposing. I must have been particularly tired when I read it. The hash should be the appropriate length for the JWA being used as suggested.

I also now understand the particular issues surrounding the per-client/per-request JWA. The only way the spec effectively handles the ability for the RP to pick a JWA is via dynamic registration with the id_token_signed_response_alg parameter. I think based on this it's probably not desirable to implement things the spec isn't explicit about. Though this is somewhat off-topic so I apologize.

Edit: To clarify, obviously it could be handled it per-client without deviating from the spec, I only meant there is no safe way to determine the entire JWA (you could determine the key length potentially like you suggest) on a per-authorization basis from the spec other than dynamic registration.