Bring back SecurityKey.IsSupportedAlgorithm

[kevinchalet]

When fixing ASOS to use the newest IM bits, I had to replace SecurityKey.IsSupportedAlgorithm by the new syntax.

key.IsSupportedAlgorithm(SecurityAlgorithms.RsaSha256Signature)

->

key.CryptoProviderFactory.IsSupportedAlgorithm(SecurityAlgorithms.RsaSha256Signature)

Unfortunately, it doesn't work at all because the key itself is not flowed up to CryptoProviderFactory.IsSupportedAlgorithm so it always returns false even if the key is a RSA key.

You now have to use this extremely weird and error-prone syntax:

key.CryptoProviderFactory.IsSupportedAlgorithm(SecurityAlgorithms.RsaSha256Signature, key)

@brentschmaltz @tushargupta51 @polita any chance we can reintroduce SecurityKey.IsSupportedAlgorithm for RTM?

[brentschmaltz]

SecurityKey.IsSupportedAlgorithm(x) just calls SecurityKey.CryptoProviderFactory.IsSupportedAlgorithm(x, this).

We felt it was just redundant and wanted to highlight that CryptoProviderFactory is the object that knows that there is support for the (algorithm, key) pair.

It is something we could add back, but not for RTM.

[kevinchalet]

SecurityKey.IsSupportedAlgorithm(x) just calls SecurityKey.CryptoProviderFactory.IsSupportedAlgorithm(x, this).

Seems much better than this syntax, IMHO:

key.CryptoProviderFactory.IsSupportedAlgorithm(SecurityAlgorithms.RsaSha256Signature, key)

Looks like it's another limitation I'll have to work around. Kinda tired of that, TBH.

[brentschmaltz]

Syntactically we could go either way. So we went smaller, it's easy to add. Very hard to remove. But is it really limiting? To be able to serve up a crypto operator with confidence the key and algorithm must be presented.

Are you in favor of such methods on SecurityKey:

GetSignatureProvider(algorithm) GetAlgorithm (Asymmetric / Symmetric) IsSupportedAlgorithm(x) GetPublicJson GetPublicXml

[kevinchalet]

But is it really limiting?

It's not a blocking issue. I just wasted 1/2 hour trying to understand why it was not working. The syntax really sucks, IMHO.

Are you in favor of such methods on SecurityKey:

Except GetPublicJson and GetPublicXml, yes, I am :smile:

[brentschmaltz]

The syntax really sucks, IMHO.

Your opinion matters, so we should start thinking about the api's. What led to our change was we were concerned that it would be hard to put in a single FIPS compliant SHA256 HashAlgorithm that both OIDCProtocolValidator would use for c_hash and JWTHandler for tokens. Our previous OM made that hard.

I think the conversation we had along the lines:

if SecurityToken.IsSupported(RSA256) then an RSA algorithm is available.

Is a good place to start. I suppose we could use something like SecurityKey.GetAlgorithm("RSA256"); Which would return null if not supported.

I am starting to think along the lines that the OIDCProtocolValidator should validate the OIDC response including the id_token basics. In effect add ISecurityTokenValidator to its capabilities this would reduce the number of things the user has to think about.

[brentschmaltz]

@kevinchalet i originally pushed back on this, but it would be as simple as calling: SecurityKey.CryptoProviderFactory.IsSupportedAlgorithm(algorithm, this).

If you still care about this, i will get it in. If NOT, we should close it.

[kevinchalet]

I ended up using SecurityKey.CryptoProviderFactory.IsSupportedAlgorithm(algorithm, this), but given how confusing it is to have SecurityKey.CryptoProviderFactory.IsSupportedAlgorithm(algorithm) and SecurityKey.CryptoProviderFactory.IsSupportedAlgorithm(algorithm, this) behaving differently, I still think having a SecurityKey.IsSupportedAlgorithm(algorithm) would be a very welcome addition.

[kevinchalet]

But yeah, the internal implementation would be as simple as

public virtual bool IsSupportedAlgorithm(string algorithm)
{
    if (string.IsNullOrEmpty(algorithm))
    {
        throw new ArgumentException("The algorithm cannot be null or empty.", nameof(algorithm));
    }

    return CryptoProviderFactory.IsSupportedAlgorithm(algorithm, this);
}

[brentschmaltz]

@kevinchalet I'll get it in....

[brentschmaltz]

@kevinchalet https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/1472

[kevinchalet]

@brentschmaltz thanks! I can confirm the new API works like a charm 🎉