IdentityServer / IdentityServer4

OpenID Connect and OAuth 2.0 Framework for ASP.NET Core
https://identityserver.io
Apache License 2.0
9.23k stars 4.02k forks source link

c_hash generated using wrong hashing algorithm acording to spec #3435

Closed sorenhnielsen closed 5 years ago

sorenhnielsen commented 5 years ago

Hi

I'm using IdentityServer4 with OicdClient2 in a Hybrid Flow setup. There I have problems with validation of the c_hash value.

It looks like IdentityServer4 is always hashing the code value with sha256. This is in DefaultTokenService.HashAdditionalData

while the OicdClient2 is using sha256/384/512 based on the Algorithm from the Token.

Since we get this header token back { "alg": "RS512", "typ": "JWT" } OicdClient2 expects to hash the code with sha512.

This looks like it's according to the specification on https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.2.2.9

Hash the octets of the ASCII representation of the access_token with the hash algorithm specified in JWA [JWA] for the alg Header Parameter of the ID Token's JOSE Header. For instance, if the alg is RS256, the hash algorithm used is SHA-256.

and https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.3.2.8

Right now this is preventing me from using the Hybrid Flow.

leastprivilege commented 5 years ago

Where does the RS512 come from - our code only allows RS256 - have you changed it?

We have to wait til netcore3 for better crypto agility.

sorenhnielsen commented 5 years ago

The RS512 comes from the replacement of builder.AddDeveloperSigningCredential (in Startup).

In production we dont have a Developer certificate, so the example code throw an exception. Therefore we added our own certificate, which is based on RS512. This certificate seems to control which algorithm the JWT token gets based on.

For now I have changed that certificate so we use RS256.

leastprivilege commented 5 years ago

OK - yes I know there is room for improvement here. I will keep this open.

Glad you have a workaround for now.

leastprivilege commented 5 years ago

OK - this is fixed in 3.0

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.