search

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

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

CryptoProviderFactory.IsEcdsaAlgorithmSupported() doesn't ensure algorithms are compatible with the curves used by ECDSA keys #488

Closed kevinchalet closed 6 years ago

kevinchalet commented 8 years ago

IsEcdsaAlgorithmSupported returns true if the algorithm corresponds to ES256, ES384 or ES512 but doesn't ensure the curve type of the ECDSA key is compatible with the specified algorithm.

ES256 -> P-256 (= ECCurve.NamedCurves.nistP256) ES384 -> P-384 (= ECCurve.NamedCurves.nistP384) ES512 -> P-521 (= ECCurve.NamedCurves.nistP521)

/cc @brentschmaltz @polita

brentschmaltz commented 7 years ago

moving to 5.1.4

kevinchalet commented 6 years ago

@mafurman why was this thread closed? I don't see in the PR diff any change that would fix the API mentioned in this ticket.

brentschmaltz commented 6 years ago

@PinpointTownes @mafurman Yes, we need a check between 'algorithm' and 'CRV'. I would argue this is not a IsEcdsaAlgorithmSupported function. As this describes what the runtime is capable of. The check you are looking for is matching CRV with Algorithm. I think this is covered with the combination of GetKeySize and ValidateECDSAKeySize.

kevinchalet commented 6 years ago

Let me remind you what the requirement is, in case you'd have missed it (well, this ticket was opened almost 2 years ago so...):

I need CryptoProviderFactory.IsSupportedAlgorithm(string algorithm, SecurityKey key) to return false if key is an ECDSA key AND algorithm an ECDSA algorithm that uses a curve different than the one associated with key.

brentschmaltz commented 6 years ago

We will add tests and API if necessary, to make sure that contract is enforced.