Skip to content

Conversation

@joegoldman2
Copy link
Contributor

@joegoldman2 joegoldman2 commented Oct 21, 2023

Fixes #1943.
Fixes #2217.

@jaanclaeys
Copy link

I applaud the fix, but ...

I think we need a better solution going forward in the future. I am pretty sure we will see more algorithms in the future on certificates (Dilithium e.g.)

Problem is probably here with X509Certificate2 and the extension methods. Trying each private key method might be a bit of a performance hit.
Having an informed constructor might help here ? Or even different subclasses X509RsaSecurityKey / X509EcdsaSecurityKey, or fix this in X509Certificate2 itself ?

@joegoldman2
Copy link
Contributor Author

I don't think that creating sub-classes is the right solution. In many cases, the algorithm of the key in the certificate is not known in advance and X509SecurityKey is helping to abstract it.
If the algorithm is known, you can simply do:

X509Certificate2 certificate = ...;
ECDsa ecdsa = certificate.GetECDsaPrivateKey();
ECDsaSecurityKey key = new ECDsaSecurityKey(ecdsa);

And no need for any new X509EcdsaSecurityKey or equivalent.

Otherwise, I agree that the current fix isn't as future-proof as we'd like, but I think it's a good first fix before perhaps a deeper refactoring.

@joegoldman2 joegoldman2 requested a review from a team as a code owner April 19, 2024 15:41
@keegan-caruso
Copy link
Contributor

Similar comment to #2379, can we add tests for this?

@joegoldman2
Copy link
Contributor Author

Similar comment to #2379, can we add tests for this?

Can you please check the initial comment of this PR?

@keegan-caruso
Copy link
Contributor

I can commit an EC cert if that is what you would like to do.

I think in-memory certs would also be fine here if we can use them. One less thing to suppress for test data.

@joegoldman2
Copy link
Contributor Author

I generated a self-signed cert this way:

$cert = New-SelfSignedCertificate -DnsName "CN=KeyStoreTestCertificate" -KeyAlgorithm ECDSA_nistP256 -KeyExportPolicy Exportable -CertStoreLocation Cert:\CurrentUser\My
$cert | Export-PfxCertificate -FilePath "Certificate.pfx" -Password (ConvertTo-SecureString -String "abcd" -Force -AsPlainText)
$pfxBytes = [System.IO.File]::ReadAllBytes("Certificate.pfx")
$base64Cert = [System.Convert]::ToBase64String($pfxBytes)

And added a unit test.

@joegoldman2 joegoldman2 changed the title Add ECDsa support in X509SecurityKey Add ECDsa support in X509SecurityKey and JsonWebKeyConverter.ConvertFromX509SecurityKey Jul 2, 2024
@joegoldman2
Copy link
Contributor Author

The unit tests are not passing yet. I'm running into a issue that I'm not sure to understand and I opened a discussion in the runtime repo: dotnet/runtime#104294.

@joegoldman2
Copy link
Contributor Author

I updated the unit tests not to use the self-signed certificate generated by me but an existing EC key + CertificateRequest to generate the certificate in memory. All unit tests are passing now.

The parameter representAsRsaKey exposed by JsonWebKeyConverter.ConvertFromX509SecurityKey should be renamed as now ECDsa is supported. I'm open to suggestions.

Otherwise, this PR is ready to be reviewed.

This was referenced Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

IdentityModel8x Future breaking issues/features for IdentityModel 8x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] JsonWebKeyConverter.ConvertFromX509SecurityKey lies about the Key Type [Bug] X509SecurityKey X509SigningCredentials are missing ECDsa

8 participants