AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.06k stars 401 forks source link

Token decryption fails if incoming token doesn't have kid (key id) in the protected header. #1431

Closed kesane-msft closed 4 years ago

kesane-msft commented 4 years ago

My application receives encrypted web tokens from AAD. I am using the OWIN middleware for setting up the token decryption parameters. I am setting the TokenValidationParameters.TokenDecryptionKeys with a set of two certificates either of which might be used to encrypt the token. (I am testing the certificate rotation flow). However, when I am testing the code, I see the following error being reported -

Microsoft.Owin.Security.OAuth.OAuthBearerAuthenticationMiddleware Error: 0 : Authentication failed
Microsoft.IdentityModel.Tokens.SecurityTokenKeyWrapException: IDX10659: UnwrapKey failed, exception from cryptographic operation: 'System.Security.Cryptography.CryptographicException'
   at Microsoft.IdentityModel.Tokens.RsaKeyWrapProvider.UnwrapKey(Byte[] keyBytes)
   at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.GetContentEncryptionKeys(JwtSecurityToken jwtToken, TokenValidationParameters validationParameters)
   at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.DecryptToken(JwtSecurityToken jwtToken, TokenValidationParameters validationParameters)
   at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.ValidateToken(String token, TokenValidationParameters validationParameters, SecurityToken& validatedToken)
   at Microsoft.Owin.Security.Jwt.JwtFormat.Unprotect(String protectedText)

If I remove the certificate which is currently not being used to encrypt the tokens then the error goes away.

I believe this has to do with the code here - https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/e4928aff114bf17ee04bd0360b15da6d33694b06/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs#L966

It should not be enclosing the if condition at https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/e4928aff114bf17ee04bd0360b15da6d33694b06/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs#L981

Looking for suggestions of what is the correct way out here.

Currently to unblock myself, I am adding a token decryption key resolver with the following code - (which is heavily influenced with the code in JsonWebTokenHandler)

private List<SecurityKey> TokenDecryptionKeyResolver(string token, SecurityToken securityToken, string kid, TokenValidationParameters validationParameters)
{
    using (X509Store store = new X509Store(StoreName.My, StoreLocation.LocalMachine))
    {
            store.Open(OpenFlags.ReadOnly | OpenFlags.OpenExistingOnly);
            X509Certificate2Collection certs = store.Certificates.Find(X509FindType.FindByThumbprint, <thumbprint_new>, true);
            var tokenEncryptionCert = certs[0];         
            certs = store.Certificates.Find(X509FindType.FindByThumbprint, <thumbprint_old>, true);
            var tokenEncryptionCertOld = certs[0];
    }
    List<X509SecurityKey> tokenDecryptionKeys = new List<X509SecurityKey> {
                                                                    new X509SecurityKey(tokenEncryptionCert),
                                                                    new X509SecurityKey(tokenEncryptionCertOld) };
    if (!string.IsNullOrEmpty(kid))
    {
            foreach (var key in tokenDecryptionKeys)
            {
                    if (key != null && string.Equals(key.KeyId, kid, StringComparison.OrdinalIgnoreCase))
                    {
                            return new List<SecurityKey>() { key };
                    }
            }
    }
    if (securityToken is JwtSecurityToken)
    {
            JwtSecurityToken jwtToken = securityToken as JwtSecurityToken;
            if (!string.IsNullOrEmpty(jwtToken?.Header.X5t))
            {
                    foreach (var key in tokenDecryptionKeys)
                    {
                            if (string.Equals(key.X5t, jwtToken.Header.X5t, StringComparison.OrdinalIgnoreCase))
                            {
                                    return new List<SecurityKey>() { key };
                            }
                    }
            }
    }
    return null;
}
brentschmaltz commented 4 years ago

@kesane-msft nice work in figuring out a workaround. Definitely a bug we should fix.

brentschmaltz commented 4 years ago

@kesane-msft what is happening here is we throw without trying all keys. We should add a try / catch inside GetContentEncryptionKeys.

kesane-msft commented 4 years ago

@brentschmaltz - not sure I get your comment over here. Could you please elaborate a bit more?

brentschmaltz commented 4 years ago

@kesane-msft we throw without trying the rest of the keys, so unless the first one works we fault.