Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Enhancement: ECDiffieHellman X509 certificate management gaps #413

Open
CodeBlanch opened this issue Nov 30, 2019 · 10 comments
Open

API Enhancement: ECDiffieHellman X509 certificate management gaps #413

CodeBlanch opened this issue Nov 30, 2019 · 10 comments

Comments

@CodeBlanch
Copy link

@CodeBlanch CodeBlanch commented Nov 30, 2019

I have an ECC (ECDH_P256) certificate issued by Apple for Apple Pay processing flagged with these key usages:

image

There isn't much I can do with this certificate in .NET. ECDsaCertificateExtensions (GetECDsaPublicKey or GetECDsaPrivateKey) won't load it because it's used for deriving key material, not signing. I have to pinvoke into CryptAcquireCertificatePrivateKey to make use of it. Very heavy lifting! It seems like there should be an ECDiffieHellmanCertificateExtensions class for loading the public/private keys as ECDiffieHellman AsymmetricAlgorithm instances and doing things like calling CopyWithPrivateKey to join issued cert(s) to stored private keys. CertificateRequest has similar issues trying to self-sign or sign with CA a request using ECDiffieHellman keys, only RSA & ECDsa are supported there.

Suggested API additions would be:

namespace System.Security.Cryptography.X509Certificates {
    // New class
    public static class ECDiffieHellmanCertificateExtensions {
        public static X509Certificate2 CopyWithPrivateKey(this X509Certificate2 certificate, ECDiffieHellman privateKey);
        public static ECDiffieHellman GetECDiffieHellmanPrivateKey(this X509Certificate2 certificate);
        public static ECDiffieHellman GetECDiffieHellmanPublicKey(this X509Certificate2 certificate);
    }

    // Existing class
    public sealed class PublicKey {
        public bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
        public byte[] ExportSubjectPublicKeyInfo();
        public static void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
        public PublicKey(AsymmetricAlgorithm key);
    }
}
@vcsjones

This comment has been minimized.

Copy link
Member

@vcsjones vcsjones commented Dec 3, 2019

There isn't really a difference between an ECDH and ECDSA certificate beyond the key usage, they are both normally id-ecPublicKey keys in the AlgorithmIdentifier.

You should be able to use GetECDsaPublicKey and use that to smuggle out the ECParameters. Something like:

using var ecdsa = cert.GetECDsaPublicKey();
using var ecdh = ECDiffieHellman.Create(ecdsa.ExportParameters(false));

If you need the private key, you can use GetECDsaPrivateKey and use true for ExportParameters.

Where this wouldn't work is if your private key is non-exportable.

I'm not suggesting the API proposal isn't a good idea, just that it might be simpler to work around.

@vcsjones

This comment has been minimized.

Copy link
Member

@vcsjones vcsjones commented Dec 4, 2019

You should be able to use GetECDsaPublicKey and use that to smuggle out the ECParameters.

Hm, no that doesn't work if the cert has a keyUsage extension, which your certificate does. Apologies for the misdirection.

I tried implementing your proposal here as a proof of concept1 (works on Windows, probably needs work on *nix): https://github.com/dotnet/runtime/compare/master...vcsjones:ecdh-x509?expand=1

I think the proposal is doable, though I wonder if it would make more sense for GetECDiffieHellmanPublicKey to return a ECDiffieHellmanPublicKey instead of ECDiffieHellman.

1: Missing unit tests, code probably doesn't conform to style guidelines, needs work on non-Windows OSes.

/cc @bartonjs

@CodeBlanch

This comment has been minimized.

Copy link
Author

@CodeBlanch CodeBlanch commented Dec 4, 2019

@vcsjones Correct it doesn't work for my cert (really Apple's cert) because ECDsaCertificateExtensions.HasECDsaKeyUsage comes back false which means GetECDsaPublicKey and/or GetECDsaPrivateKey both return null.

Alternative ideas to get it working: What if we made HasECDsaKeyUsage less strict or allowed people to override/supply the predicate used to qualify the cert?

PS: When I first looked at the code I was expecting to see ECDsa and ECDiffieHellman sharing some kind of base class. If that was the case it might have been easier to retrieve off the certificate in a more general way and then pass the generic EC key into the specific algorithm using it (ECDH or ECDsa).

@bartonjs

This comment has been minimized.

Copy link
Member

@bartonjs bartonjs commented Dec 4, 2019

ECDH can be both id-ecdh and id-ecPublicKey, but the former can't be ECDSA. I have no objection to adding the extension methods against X509Certificate2, but CertificateRequest is an eensey bit harder since ECDH can't self-sign (being other than a signature algorithm).

Instead of adding ECDH to CertificateRequest it probably makes sense to a) add Import/ExportSubjectPublicKeyInfo to the PublicKey class, and b) perhaps make a PublicKey(AsymmetricAlgorithm) constructor (which, if nothing else, just logically does ImportSubjectPublicKeyInfo(key.ExportSubjectPublicKeyInfo()).

Then you can use the CertificateRequest(X500DistinguishedName subjectName, PublicKey publicKey, HashAlgorithmName hashAlgorithm) ctor and it's "very" clear that self-signing won't work.

@bartonjs

This comment has been minimized.

Copy link
Member

@bartonjs bartonjs commented Dec 4, 2019

I think the proposal is doable, though I wonder if it would make more sense for GetECDiffieHellmanPublicKey to return a ECDiffieHellmanPublicKey instead of ECDiffieHellman.

Logically? Yes. I could be convinced by a) it's the correct answer or b) return ECDiffieHellman because ECDH can make ECDHPK, but the other way around doesn't work.

PS: When I first looked at the code I was expecting to see ECDsa and ECDiffieHellman sharing some kind of base class. If that was the case...

Yeah, I also find it frustrating. But it's hard to change things that released in 2007.

@vcsjones

This comment has been minimized.

Copy link
Member

@vcsjones vcsjones commented Dec 4, 2019

return ECDiffieHellman because ECDH can make ECDHPK

That also makes sense, and it looks like ECDHPK doesn't have {Import,Export}SubjectPublicKeyInfo and such that are useful. Although, perhaps that isn't good reasoning and instead ECDHPK should have those methods in a separate proposal.

@boraozgen

This comment has been minimized.

Copy link

@boraozgen boraozgen commented Dec 6, 2019

I have a similar issue regarding ECDH and X509Certificate2. As far as I searched, there is no way to use the private key of a X509Certificate2 to instanciate a ECDiffieHellmanCng, unless the private key is "plaintext exportable" and using the following proposed workaround:

You should be able to use GetECDsaPublicKey and use that to smuggle out the ECParameters. Something like:

using var ecdsa = cert.GetECDsaPublicKey();
using var ecdh = ECDiffieHellman.Create(ecdsa.ExportParameters(false));

If you need the private key, you can use GetECDsaPrivateKey and use true for ExportParameters.

Where this wouldn't work is if your private key is non-exportable.

However this does not work if the certificate is only "exportable", which is the case when importing a certificate from a PKCS#12 file with the X509KeyStorageFlags.Exportable parameter.

I am currently using the "clrsecurity" extension to retrieve the private key as CngKey using the GetCngPrivateKey() method and instanciate the ECDH with it (this works even if the private key is not exportable). I believe that this API is not desirable (as discussed in this issue), however I hope this feature is added to the standard .NET framework, e.g. with ECDiffieHellmanCertificateExtensions as proposed.

@vcsjones

This comment has been minimized.

Copy link
Member

@vcsjones vcsjones commented Dec 10, 2019

It sounds like the API proposal looks something like this then?

namespace System.Security.Cryptography.X509Certificates {
    // New class
    public static class ECDiffieHellmanCertificateExtensions {
        public static X509Certificate2 CopyWithPrivateKey(this X509Certificate2 certificate, ECDiffieHellman privateKey);
        public static ECDiffieHellman GetECDiffieHellmanPrivateKey(this X509Certificate2 certificate);
        public static ECDiffieHellman GetECDiffieHellmanPublicKey(this X509Certificate2 certificate);
    }

    // Existing class
    public sealed class PublicKey {
        public bool TryExportSubjectPublicKeyInfo(Span<byte> destination, out int bytesWritten);
        public byte[] ExportSubjectPublicKeyInfo();
        public static void ImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, out int bytesRead);
        public PublicKey(AsymmetricAlgorithm key);
    }
}

CertificateRequest already has a constructor that takes PublicKey.

@bartonjs

This comment has been minimized.

Copy link
Member

@bartonjs bartonjs commented Dec 10, 2019

PublicKey currently doesn't have any (direct) post-creation mutability; so I think the import would be a static From.

public static PublicKey FromSubjectPublicKeyInfo(...);
@vcsjones

This comment has been minimized.

Copy link
Member

@vcsjones vcsjones commented Dec 10, 2019

@bartonjs Oops, good point and updated. It doesn't have a default constructor either, so it would be difficult to get an instance to import into anyway.

@bartonjs bartonjs added this to the 5.0 milestone Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.