Consider adding algorithm-agnostic X509Certificate2.GetPublicKey() and X509Certificate2.GetPrivateKey()

[kevinchalet]

A while ago, it was decided to remove these properties from the public contract to encourage people to use the strongly typed extensions: https://github.com/dotnet/corefx/issues/2449#issuecomment-129042445.

Though the motivation seems legitimate, this approach leads to very weird code in practice, specially in framework classes that don't need to deal with the concrete algorithm instance, as it ties them to algorithm-specific extensions:

• https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/KDev/src/Microsoft.IdentityModel.Tokens/X509SecurityKey.cs#L78
• https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/KDev/src/Microsoft.IdentityModel.Tokens/X509SecurityKey.cs#L102

Is there a chance to bring these properties back?

/cc @bartonjs @brentschmaltz

[bartonjs]

When we were building .NET 4.6 there was a compatibility concern about the PrivateKey property returning anything other than RSACryptoServiceProvider or DSACryptoServiceProvider; and so they don't. (Too much code assumes it can just blindly cast it over to RSACSP, and we're actually trying to remove all callers of these properties in netfx, since they don't work with CNG-keyed certificates.)

I note in your reference example class that you actually only support RSA; and if someone were to construct an X509SecurityKey with a DSA/DH/ECDH/future type of certificate that they'd get a much delayed failure.

And, ultimately, you're going to have to give this object to something that can do Sign/Verify/Encrypt/Decrypt/DeriveKey. Since AsymmetricAlgorithm has no useful members on it, these properties effectively just return object; and so have very little value.

(BTW: If your linked code runs on desktop, your #else doesn't work with CNG-backed private keys or RSA public keys whose Modulus value exceeds uint.MaxValue... you should call GetRSAPrivateKey instead :smile:)

[kevinchalet]

I note in your reference example class that you actually only support RSA; and if someone were to construct an X509SecurityKey with a DSA/DH/ECDH/future type of certificate that they'd get a much delayed failure.

Ideally, it shouldn't be bound to a particular algorithm as it's just a reference to a key.

IdentityModel itself only supports RSA certificates ATM when it comes to sign or verify, but developers using IdentityModel may want to implement a custom signature provider to support other algorithms. Without being able to retrieve the concrete algorithm instance from X509SecurityKey (due to GetRSAPrivateKey()/GetECDsaPrivateKey() being used), this scenario is sadly broken.

And, ultimately, you're going to have to give this object to something that can do Sign/Verify/Encrypt/Decrypt/DeriveKey. Since AsymmetricAlgorithm has no useful members on it, these properties effectively just return object; and so have very little value.

True. But this ticket doesn't aim at solving that at all, it's actually just about being able to flow the algorithm instance without having to rely on alg-specific extensions, that prevent full flexibility.

(BTW: If your linked code runs on desktop, your #else doesn't work with CNG-backed private keys or RSA public keys whose Modulus value exceeds uint.MaxValue... you should call GetRSAPrivateKey instead :smile:)

Interesting. Is GetRSAPrivateKey supported on .NET 4.5.1?

[bartonjs]

Interesting. Is GetRSAPrivateKey supported on .NET 4.5.1?

No, the first CNG certificate support (which is why those were added) came in .NET 4.6. (I could say the dread word "reflection" here)

And, also: net451 is out of support, net452 is the oldest still-supported version in the v4 family: https://blogs.msdn.microsoft.com/dotnet/2015/12/09/support-ending-for-the-net-framework-4-4-5-and-4-5-1/. That doesn't really help here, but I thought I'd point it out.

Quick run-down of the difference:

• PrivateKey property
  • CAPI Only
  • Fails if the private key is in a CNG KSP (hardware or software)
  • For RSA, fails if sizeof(the public modulus) > sizeof(DWORD)
  • For DSA, fails if the key is not FIPS 186-1 compliant.
  • RSA, DSA only.
  • Difficult to understand lifetime
  • Returns the same object every call
  • Despite the X509Certificate2 object caching the handle, it never calls Dispose; meaning this almost always gets finalized.
• PublicKey.Key property
  • CAPI Only
  • Doesn't care about the private key storage, can succeed when PrivateKey will fail
  • For RSA, fails if sizeof(the public modulus) > sizeof(DWORD)
  • For DSA, fails if the key is not FIPS 186-1 compliant.
  • RSA, DSA only.
  • Difficult to understand lifetime
  • Returns the same object every call
  • Despite the PublicKey object caching the handle (which was itself cached by the X509Certificate2), it never calls Dispose; meaning this almost always gets finalized.
• Get[Algorithm]PrivateKey method
  • CNG when able, CAPI as a fallback (e.g. legacy HSM with no CNG driver)
  • If something newer than CNG were to appear, it would take priority.
  • RSA large moduli are valid.
  • DSA is FIPS 186-3 compliant.
  • Currently has RSA, DSA, ECDSA variants.
  • And reduces the casting required, so hopefully people stop casting beyond the algorithm base class unless doing interop.
  • Caller lifetime
  • Every call returns a distinct object, so the caller can manage it meaningfully.
• Get[Algorithm]PublicKey method
  • Always CNG (CNG can read all public keys)
  • If something newer than CNG were to appear, this would change.
  • RSA large moduli are valid.
  • DSA is FIPS 186-3 compliant.
  • Currently has RSA, DSA, ECDSA variants.
  • And reduces the casting required, so hopefully people stop casting beyond the algorithm base class unless doing interop.
  • Caller lifetime
  • Every call returns a distinct object, so the caller can manage it meaningfully.

[kevinchalet]

Unfortunately, IdentityModel needs to target net451 as it's the minimal TFM used by ASP.NET Core, so reflection would be the only option (but that's another topic :smile:)

Thanks for the list, it's really interesting :clap:

Since re-introducing these properties in .NET Core and changing the concrete implementations they return on .NET Desktop to avoid breaking existing code making casts is not an option, would you consider adding new methods/extensions instead? (e.g AsymmetricAlgorithm GetPrivateKey()/AsymmetricAlgorithm GetPublicKey()).

They would work exactly like Get[Algorithm]PrivateKey/PublicKey but wouldn't be tied to a particular algorithm.

[bartonjs]

True. But this ticket doesn't aim at solving that all, it's actually just about being able to flow the algorithm instance without having to rely on alg-specific extensions, that prevent full flexibility.

I realized I had neglected this part and was working on a reply that said something very much like:

would you consider adding new methods/extensions instead? (e.g AsymmetricAlgorithm GetPrivateKey()/AsymmetricAlgorithm GetPublicKey()).

I, personally, don't find a lot of value in this; and in some ways it is a net46 anti-goal (because it requires consumers to cast, and we've already seen a propensity to cast too far). But, I acknowledge that is very idealist of me. But, I can acknowledge that middleware has a place, and if all you're going to do is null-coalesce the known algorithms, we may as well do that for you.

so reflection would be the only option (but that's another topic :smile:)

Hey, it's not so bad. I had to do it from inside mscorlib, where I couldn't even talk about System.Core (where the extension methods live). Your reflection'll be more straightforward than that :smile:.

[kevinchalet]

and if all you're going to do is null-coalesce the known algorithms, we may as well do that for you.

I promise :trollface:

In a perfect world, the platform-specific OpenSSL/CNG/CSP packages shouldn't be referenced at all from user code, which would "de facto" prevent the "abuse of casts" you're mentioning.

In the same world, we'd just have to deal with the base classes (RSA, ECDsa, DSA). We're not really living in an ideal world, but the upcoming RSA.Create(RSAParameters) and ECDsa.Export/Import should help make the base classes more useful than they are currently (well, at least for .NET Core and > .NET Desktop 4.6.3 :smile:)

Hey, it's not so bad. I had to do it from inside mscorlib, where I couldn't even talk about System.Core (where the extension methods live). Your reflection'll be more straightforward than that :smile:.

Yeah :smile: Alternatively, I guess IdentityModel could also have a >net46 TFM to avoid reflection when using recent .NET Framework bits.

/cc @tushargupta51

[RussKie]

I don't mean to hijack the issue, but our issue seems related.

In the .NET Fx (4.0-4.5.x, haven't tested in 4.6.x) the following code worked: X509Certificate2.PrivateKey as RSACryptoServiceProvider in .NETCore certificate.GetRSAPrivateKey() comes as System.Security.Cryptography.RSACng which can't be cast to RSACryptoServiceProvider.

For us it seems to be a major problem. We have certificate-based encryption performed by .NET 4.5 code:

            var rscp = certificate.PublicKey.Key as RSACryptoServiceProvider;
            if (rscp == null) { ... }
            var encryptedData = rscp.Encrypt(_base64Encoder.Decode(encKey.Key), false);
            return _base64Encoder.Encode(encryptedData);

On a receiving end the key would be decrypted as follows (works in .NET 4.0-4.5.x, haven't tested in 4.6.x):

            var rscp = certificate.PrivateKey as RSACryptoServiceProvider;
            if (rscp == null) { ... }
            var encryptedData = _base64Encoder.Decode(encryptedKey);
            byte[] symKey = rscp.Decrypt(encryptedData, false);

I can't seem to find a way to achieve the same functionality in .NETCore app as RSACng appears to provide a different mechanism for decryption: byte[] Decrypt(byte[], RSAEncryptionPadding). I've tried all possible paddings and nothing worked...

Is there a way forward for us?

[bartonjs]

@RussKie It's only RSACng some of the time. Some of the time it might be an RSACryptoServiceProvider, some of the time it might be an RSAOpenSsl, and some of the time it might be something else.The reason it was moved to a different member was to prevent crashing because of bad casts :smile:.

If your non-Core code is targeting .NET Framework 4.6 or higher only then you can happily write platform agnostic code:

var publicKey = certificate.GetRSAPublicKey();
if (publicKey == null) { ... }
var encryptedData = publicKey.Encrypt(
    _base64Encoder.Decode(encKey.Key),
     RSAEncryptionPadding.Pkcs1);
return _base64Encoder.Encode(encryptedData);

var privateKey = certificate.GetRSAPrivateKey();
if (privateKey == null) { ... }
var encryptedData = _base64Encoder.Decode(encryptedKey);
byte[] symKey = privateKey.Decrypt(encryptedData, RSAEncryptionPadding.Pkcs1);

There's no way to write RSA code which is compatible with both net45x and .NET Core 1.0; the dependency on RSACryptoServiceProvider meant it wasn't really cross-platform capable. If you still need to support net45x, my personal recommendation would be to conditionally compile in an adapter extension method class so you can start calling RSA in the 4.6 way.

[bartonjs]

As part of the netstandard2.0 project the legacy PrivateKey property will be returning; but it will maintain the "shared object, don't dispose" behavior of .NET Framework (though it won't return RSACryptoServiceProvider anymore, unless GetRSAPrivateKey would have).

[kevinchalet]

@bartonjs nice :clap:

FWIW, I really love the API convergence plan. Too bad other teams don't take it as seriously as yours (yeah IdentityModel guys, I'm looking at you...)

[RussKie]

IdentityModel APIs are messy...

[karelz]

We need API proposal - especially the name (we have to avoid collision with existing methods)

Idea from @bartonjs: GetAnyPublicKey() & GetAnyPrivateKey().

[kevinchalet]

GetAnyPublicKey() and GetAnyPrivateKey() sound a bit weird, as they seem to imply a certificate may contain multiple public keys/private keys (which is, AFAIK, not possible, at least with X.509 v3).

Since @bartonjs wants these API to return non-shared instances, why not something like GetDisposablePublicKey()/GetDisposablePrivateKey()?

[RussKie]

I find Disposable bit confusing - does it now imply the end-consumer of API has to dispose the key after they used it?

GetCertificatePublicKey / GetCertificatePrivateKey?

[kevinchalet]

does it now imply the end-consumer of API has to dispose the key after they used it?

Yes. That's how GetRSAPublicKey and GetRSAPrivateKey are supposed to be used: https://blogs.msdn.microsoft.com/dotnet/2015/07/20/announcing-net-framework-4-6/

[bartonjs]

@RussKie Well, they do return unique disposable objects, so... yes, the caller is responsible for managing the lifetime. That's already true of the Get[Algorithm]{Public|Private}Key() methods. (The PrivateKey property and PublicKey.Key property return shared objects, so the caller should not dispose them)

[bartonjs]

• GetCertPublicKey()
• GetAnyAlgPublicKey()
• GetAbstractPublicKey()
• GetPublicKeyObject()
• ...

The problem is the best name, GetPublicKey() is already taken by X509Certificate to return the byte[]. And while we could do public new virtual AsymmetricAlgorithm GetPublicKey() that's a breaking change for anyone trying to call the base class one on a X509Certificate2 object. And I'm sort of at a loss for words to go between Get and Public.

[RussKie]

Yes. That's how GetRSAPublicKey and GetRSAPrivateKey are supposed to be used: https://blogs.msdn.microsoft.com/dotnet/2015/07/20/announcing-net-framework-4-6/

Thanks for the clarification, but I can't seem to find where it is stated on the blog... Regardless, until @bartonjs clarified the lifetime difference here I wasn't sure how it was meant to be used correctly (and I am pretty particular about disposing of resources).

GetAlgorithAgnosticPublicKey?

[RussKie]

And I'm sort of at a loss for words to go between Get and Public.

The type of keys depend on the underlying certificate (the crypto algorithm to generate it), isn't it? So "Certificate" fits nicely in between :)

[kevinchalet]

@RussKie it's in the code sample under Cryptography Updates:

// 4.6:
public static byte[] SignDataPkcs1Sha256(X509Certificate2 cert, byte[] data)
{
    // GetRSAPrivateKey returns an object with an independent lifetime, so it should be
    // handled via a using statement.
    using (RSA rsa = cert.GetRSAPrivateKey())
    {
        // RSA now exposes SignData, and the hash algorithm parameter takes a strong type,
        // which allows for IntelliSense hints.
        return rsa.SignData(data, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);
    }
}

[RussKie]

Thanks, looks like my corporate proxy blocks some stuff and I can't see the code samples... Just tried it on the phone and indeed there is heaps more stuff I was oblivious to.

[bartonjs]

I sort of think cert.GetCertificatePrivateKey() (or cert.GetCertPrivateKey()) looks a bit weird, but I guess it's less weird if you name your variables only one letter :smile:.

@PinpointTownes: Thoughts on "Certificate" (or "Cert") as the "middle word"?

public static AsymmetricAlgorithm GetCertificatePrivateKey(this X509Certificate2 cert) { throw null; }
public static AsymmetricAlgorithm GetCertificatePublicKey(this X509Certificate2 cert) { throw null; }

[kevinchalet]

@PinpointTownes: Thoughts on "Certificate" (or "Cert") as the "middle word"?

Sounds fine.

Or if you don't like certificate, maybe something like GetPrivateKeyAlgorithm/GetPublicKeyAlgorithm?

[bartonjs]

maybe something like GetPrivateKeyAlgorithm/GetPublicKeyAlgorithm?

That returns an OID, right? :smile:

[bartonjs]

While writing up the proposal for this I realized there's a bit of a snag... ECC.

Certificates using id-ecPublicKey can be any of:

• ECDSA (this the only valid OID for ECDSA)
• ECDH (which can be disambiguated via id-ecdh)
• ECMQV (which can be disambiguated via id-ecMQV)

Other algorithms that RFC 5480 doesn't explicitly talk about:

• ECIES
• EdDSA
• ECQV

That makes it hard to know what to return for an unrestricted id-ecPublicKey. We might have to come up with some complicated "unknown ECC" holder type.

[kevinchalet]

That returns an OID, right? :smile:

Well, they return AsymmetricAlgorithm instances (but I agree it might be confusing) :trollface:

Concerning your last remark, aren't PublicKey.Key and PrivateKey already impacted by the same concern/issue? I took a brief look at the code, and it looks like both the Windows/Linux PALs assume that ECC always means ECDSA and not ECDH (the other algorithms don't have a correspond CLR type, so I suppose they would fall in the "not supported exception" case):

https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.PrivateKey.cs#L34

https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509CertificateReader.cs#L267

[bartonjs]

Yeah, mainly because we don't have ECDH in Core yet; or ECDH-from-a-cert in Framework (not that Framework will support ECDSA from PrivateKey or PublicKey.Key; so maybe we should just remove that case label and let them throw like netfx). Good catch, though...

[RussKie]

This may be a crazy idea, but I thought I'd share regardless

GetKeys(bool loadPrivateKey = false) which returns an object with both public and private (if available) keys as properties.

[bartonjs]

While I appreciate the complexities that not having something like GetAnyPublicKey() leaves on libraries that want to unify between a provided key and a certificate... the current design of ECDSA vs ECDH precludes creating one (we have to instantiate the key as one of the two classes, but don't know which kind you're expecting).

Even if we did add it, we're back at the classic problem that PrivateKey had: It would return AsymmetricAlgorithm, and people love casting too far (RSACryptoServiceProvider vs RSA... granted... our API from .NET Framework 2.0 made it impossible to do otherwise).

Lacking any clear path forward, and the issue having not had activity for about 6 years, I think it's time to just close it.

(In the meantime, my recommendation to libraries would be to just keep passing the certificate along as the "some sort of key" type... and if you get a raw key you can perhaps use the CertificateRequest API to make a self-signed cert and unify in the certificate direction.)