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

IdentityModel extensions for .Net
MIT License
1.03k stars 386 forks source link

[Bug] NotImplementedException throw when verifying JsonWebToken with KeyVaultSecurityKey #2231

Open dezfowler opened 11 months ago

dezfowler commented 11 months ago

Which version of Microsoft.IdentityModel are you using?

6.32.0, 7.0.0

Where is the issue?

Is this a new or an existing app?

c. This is a new app or an experiment.

Repro

// === Set up ===
var azureServiceTokenProvider = new AzureServiceTokenProvider();
var callback = new KeyVaultSecurityKey.AuthenticationCallback (azureServiceTokenProvider.KeyVaultTokenCallback);

// Need to use this crypto provider for KeyVault key operations         
var keyVaultCryptoProviderFactory = new CryptoProviderFactory()
{
    CustomCryptoProvider = new KeyVaultCryptoProvider()
};

var handler = new JsonWebTokenHandler();

// URL for an existing KeyVault RSA key
string keyUrl = "https://contoso.vault.azure.net/keys/example";
var asymmetricKey = new KeyVaultSecurityKey(keyUrl, callback)
{
    // Have to supply the CryptoProviderFactory with the key (see issue #2145)
    CryptoProviderFactory = cryptoProviderFactory,
};

// === Sign step ===

// Signing data with RSA
var sigCreds = new SigningCredentials(asymmetricKey, SecurityAlgorithms.RsaSha512);
var cleartext = "I wrote this";
var jws = handler.CreateToken(cleartext, sigCreds);

// === Verify step ===

var tokenParams = new TokenValidationParameters
{
    ValidateAudience = false,
    ValidateLifetime = false,
    ValidateIssuer = false,
    IssuerSigningKey= asymmetricKey
};
var token = new JsonWebToken(jws);
var result = handler.ValidateToken(token, tokenParams);  // ===> Fails with "Not Implemented" exception

Expected behavior

Verification succeeds.

Actual behavior

A "Not Implemented" exception is thrown.

Possible solution

This fails because JsonWebTokenHandler wants to use this overload of Verify which is not implemented in the KeyVaultSignatureProvider.

It's possible to get this to work by using a custom crypto factory and derived version of KeyVaultSignatureProvider which has an implementation of the method similar to...

        public override bool Verify(byte[] input, int inputOffset, int inputLength, byte[] signature, int signatureOffset, int signatureLength)
        {
            byte[] signatureBytes = new byte[signatureLength];
            Array.Copy(signature, signatureOffset, signatureBytes, 0, signatureLength);

            byte[] inputBytes = new byte[inputLength];
            Array.Copy(input, inputOffset, inputBytes, 0, inputLength);

            return base.Verify(inputBytes, signatureBytes);
        }

Although a full implementation of this seems to need to be much more complex with lots of guards e.g. here.

Additional context / logs / screenshots / links to code N/A

jmprieur commented 11 months ago

Do all the Microsoft.IdentityModel assemblies have the same version?

brentschmaltz commented 11 months ago

@dezfowler this is a bug

dezfowler commented 11 months ago

Do all the Microsoft.IdentityModel assemblies have the same version?

Yes.

V0ldek commented 10 months ago

Note that the KeyVaultSecurityKey part is not relevant. This happens if one tries to use ValidateTokenAsync with JwtSecurityToken regardless of the parameters.

dezfowler commented 10 months ago

Note that the KeyVaultSecurityKey part is not relevant. This happens if one tries to use ValidateTokenAsync with JwtSecurityToken regardless of the parameters.

That may be a separate bug. It is relevant in this case; the required method is specifically not implemented in the KeyVaultSignatureProvider which is used with the KeyVaultSecurityKey. The method is implemented in other providers and works fine.

Note that this is related to JsonWebToken and the JsonWebTokenHandler, not JwtSecurityToken.

V0ldek commented 10 months ago

I'll open a separate one.

jennyf19 commented 10 months ago

I assume this repros on 7.0.0 as well?

dezfowler commented 10 months ago

I assume this repros on 7.0.0 as well?

@jennyf19 It does.

brentschmaltz commented 9 months ago

@jennyf19 we probably want to fix this.