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

ADFS created access tokens can't be validated due to missing 'kid' header #370

Closed
ananace opened this issue Sep 10, 2020 · 3 comments
Closed
Assignees

Comments

@ananace
Copy link

ananace commented Sep 10, 2020

We've been working on expanding the use of OpenID Connect to remove some older authentication methods in places, but ran into some issues due to how ADFS creates tokens.

Some de-personalized tokens from an authentication, as example; (with output from https://jwt.ms)

Access Token;

{
  "typ": "JWT",
  "alg": "RS256",
  "x5t": "UtTPYQs5OWuDZ2L0b2EiedFW82Q"
}.{
  "aud": "microsoft:identityserver:1269b850-82b1-4c23-baa2-5e181b077e6d",
  "iss": "http://fs.domain.com/adfs/services/trust",
  "iat": 1599641473,
  "exp": 1599645073,
  "apptype": "Confidential",
  "appid": "1269b850-82b1-4c23-baa2-5e181b077e6d",
  "authmethod": "urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport",
  "auth_time": "2020-09-09T08:50:55.546Z",
  "ver": "1.0",
  "scp": "openid"
}.[Signature]

ID token;

{
  "typ": "JWT",
  "alg": "RS256",
  "x5t": "UtTPYQs5OWuDZ2L0b2EiedFW82Q",
  "kid": "UtTPYQs5OWuDZ2L0b2EiedFW82Q"
}.{
  "aud": "1269b850-82b1-4c23-baa2-5e181b077e6d",
  "iss": "https://fs.domain.com/adfs",
  "iat": 1599641473,
  "exp": 1599645073,
  "auth_time": 1599641455,
  "mfa_auth_time": 1599641473,
  "nonce": "fMmvp_EPZMl2aIcKWoqXWGP0pvdIi9v7xKhwWlzRD5A",
  "sub": "qP6P9bly/v3iZJNYERLrieqDevuo/G2k11uZiPfenQY=",
  "upn": "user@domain.com",
  "unique_name": "AD\\user",
  "sid": "S-1-5-21-797777765-1715453426-19741283-85243",
  "apptype": "Confidential",
  "appid": "1269b850-82b1-4c23-baa2-5e181b077e6d",
  "authmethod": "urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport",
  "ver": "1.0",
  "scp": "openid"
}.[Signature]

As seen, the access token is missing the 'kid' header, and instead only contains an 'x5t' header - albeit with the exact same data as 'kid' would've contained. This caused validation to fail when using JWKs, as the signature verification wasn't able to read the key id.
Here's a simple one-line patch that got around the issue for us, but I really feel like this isn't the correct way to go about with things;

--- /a/lib/jwt/decode.rb    2020-09-10 10:09:12.183103903 +0200
+++ /b/lib/jwt/decode.rb    2020-09-10 10:55:26.259471083 +0200
@@ -34,7 +34,7 @@
 
     def verify_signature
       @key = find_key(&@keyfinder) if @keyfinder
-      @key = ::JWT::JWK::KeyFinder.new(jwks: @options[:jwks]).key_for(header['kid']) if @options[:jwks]
+      @key = ::JWT::JWK::KeyFinder.new(jwks: @options[:jwks]).key_for(header['x5t']) if @options[:jwks]
 
       raise(JWT::IncorrectAlgorithm, 'An algorithm must be specified') if allowed_algorithms.empty?
       raise(JWT::IncorrectAlgorithm, 'Expected a different algorithm') unless options_includes_algo_in_header?
@anakinj anakinj self-assigned this Sep 12, 2020
@anakinj
Copy link
Member

anakinj commented Sep 12, 2020

Interesting. I will look into if we could identify the public key based on the signature.

@anakinj
Copy link
Member

anakinj commented Oct 15, 2020

Hi @ananace. Sorry for a late reply.

Think the gem itself cannot have these kind of fallbacks or exceptions. The JWT RFC states When used with a JWK, the "kid" value is used to match a JWK "kid" parameter value

But you can for your case have the exception by giving the keyfinder as a parameter to the decode method.

jwk = JWT::JWK.new(OpenSSL::PKey::RSA.new(2048))

payload, headers = { data: 'data' }, { x5t: jwk.kid }
token_with_x5t_instead_of_kid = JWT.encode(payload, jwk.keypair, 'RS512', headers)

jwk_loader = ->(options) do
  { keys: [jwk.export] }
end

decoded_payload, decoded_headers = JWT.decode(token_with_x5t_instead_of_kid, nil, true, { algorithms: ['RS512']}) do |header|
  ::JWT::JWK::KeyFinder.new(jwks: jwk_loader).key_for(header['kid'] || header['x5t'])
end

p decoded_payload

@anakinj
Copy link
Member

anakinj commented Dec 1, 2020

Closing this. Think the keyfinder in the gem cannot make these kind of assumptions.

@anakinj anakinj closed this as completed Dec 1, 2020
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

No branches or pull requests

2 participants