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

Add easy way to create a certificate from a multi-PEM or cert-PEM + key-PEM #31944

Closed
bartonjs opened this issue Feb 7, 2020 · 33 comments · Fixed by #38280
Closed

Add easy way to create a certificate from a multi-PEM or cert-PEM + key-PEM #31944

bartonjs opened this issue Feb 7, 2020 · 33 comments · Fixed by #38280
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@bartonjs
Copy link
Member

bartonjs commented Feb 7, 2020

Something like

namespace System.Security.Cryptography.X509Certificates {
    partial class X509Certificate2 {
        public static X509Certificate2 CreateFromPemFile(string certPemFilePath, string keyPemFilePath = default);
        public static X509Certificate2 CreateFromEncryptedPemFile(string certPemFilePath, ReadOnlySpan<char> password, string keyPemFilePath = default);
        public static X509Certificate2 CreateFromEncryptedPemFile(string certPemFilePath, ReadOnlySpan<byte> passwordBytes, string keyPemFilePath = default);

        public static X509Certificate2 CreateFromPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<char> password);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<byte> passwordBytes);
    }

    partial class X509Certificate2Collection {
        public void ImportFromPemFile(string certPemFilePath);
        public void ImportFromPem(ReadOnlySpan<char> certPem);
    }
}

If no keyPemFile is specified, certPemFile is searched for both the cert and the key.

certPemFile probably should be "loads the first CERTIFICATE" entry from it; but if there are popular Unix-ish utilities that read multi-PEMs backwards, or "whichever one matched a private key", then we can consider a different behavior.

The keyPemFile is only allowed to specify one of the possible private key formats.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Feb 7, 2020
@bartonjs bartonjs added this to the 5.0 milestone Feb 7, 2020
@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Feb 7, 2020
@los93sol
Copy link

los93sol commented Feb 7, 2020

If this supported openssl encrypted key files from pem that would be great as well. Hopefully people don't have unprotected pem files laying around with keys in them :)

Exporting to a pem object would be nice as well, from X509Certificate2 it seems like it would be pretty trivial to give back and object with the cert pem and the key pem, but when you have an X509CertificateCollection with intermediate/root certs in it things are quite a bit more difficult, maybe a collection with the chain certs as pem, then the leaf and key as properties....just tossing some ideas around since I have a TON of code to deal with certs in various formats

@rynowak
Copy link
Member

rynowak commented Feb 8, 2020

just tossing some ideas around since I have a TON of code to deal with certs in various formats

Can you share some more about what kinds of things you commonly need to do? Code samples are great 👍

@rynowak
Copy link
Member

rynowak commented Feb 8, 2020

@bartonjs - another example I've come across where .pem files are in use: https://github.com/kubernetes-client/csharp/blob/master/src/KubernetesClient/CertUtils.cs

@damienbod
Copy link

@vcsjones
Copy link
Member

vcsjones commented Feb 8, 2020

A lot of PEM bundles I run in to are basically

leaf
intermediate1
IntermediateN
private-key-for-leaf

Where the private key can appear at either the end of the bundle or immediately after the leaf, if it is present at all.

Is there any big value in getting the intermediates out as well in a single go? Perhaps return an array of X509Certificate2 in the order of the aggregate and match the private key up to the right certificate? Or some tuple of X509Certificate2 (Leaf) and X509Certificate2Collection (Additional certs)?

@bartonjs
Copy link
Member Author

bartonjs commented Feb 8, 2020

I think the accelerator on X509Certificate2 would be just like the X509Certificate2 ctors, it produces just one cert. X509Certificate2Collection could add a new instance ImportFromPem method and/or static CreateFromPem; and could do a semi-complex instance method to match additional keyfiles against existing certs... if that seems useful.

@bgrainger
Copy link
Contributor

Can you share some more about what kinds of things you commonly need to do?

MySqlConnector needs to load certificates to establish a (possibly mutually-authenticated) secure connection to a server.

@vcsjones
Copy link
Member

vcsjones commented Apr 10, 2020

@bartonjs What about something like this?

namespace System.Security.Cryptography.X509Certificates {
    partial class X509Certificate2 {
        public static X509Certificate2 CreateFromPemFile(string certPemFilePath, string keyPemFilePath = default);
        public static X509Certificate2 CreateFromEncryptedPemFile(string certPemFilePath, ReadOnlySpan<char> password, string keyPemFilePath = default);
        public static X509Certificate2 CreateFromEncryptedPemFile(string certPemFilePath, ReadOnlySpan<byte> passwordBytes, string keyPemFilePath = default);

        public static X509Certificate2 CreateFromPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<char> password);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<byte> passwordBytes);
    }

    partial class X509Certificate2Collection {
        public void ImportFromPemFile(string certPemFilePath);
        public void ImportFromPem(ReadOnlySpan<char> certPem);
    }
}

@bartonjs
Copy link
Member Author

@vcsjones Seems like a reasonable starting point.

Probably wants some symmetric API on X509Certificate2Collection for loading a chain/chain+key. I'm not sure if it needs to be able to support private keys from extra files, or if that's too far off the main scenario and it'll just opportunistically load them from within the same multipem.

@vcsjones
Copy link
Member

vcsjones commented Apr 10, 2020

@bartonjs makes sense, updated the above.

not sure if it needs to be able to support private keys from extra files,

My opinion is that since a collection already expects the file to be a PEM aggregate, I think it's reasonable to expect the certificates and any keys to be in the same path. However I'm not sure about the passwords, so I didn't add those APIs. If they were there, would be just assume each private key is encrypted with the same password?

@bgrainger
Copy link
Contributor

@vcsjones Is X509Certificate2Collection.CreateFromPemFile supposed to return a X509Certificate2Collection? (The proposal above says just X509Certificate2.)

@vcsjones
Copy link
Member

@bgrainger

Is X509Certificate2Collection.CreateFromPemFile supposed to return a X509Certificate2Collection

Yes. Fixed, and thank you.

@bartonjs
Copy link
Member Author

I'm fine with leaving the encrypted version off of the collection until there's a clear scenario for it.

Currently the collection type uses Import methods to load from files. I don't think there's a strong reason to be import for everything except multipem... so probably

partial class X509Certificate2Collection
{
    public void ImportFromPemFile(string certPemFilePath) { throw null; }
}

@vcsjones
Copy link
Member

vcsjones commented Apr 11, 2020

@bartonjs makes sense. I updated my suggestion. Since AsymmetricAlgorithm has similar functionality that takes content instead of paths, do we want to consider additional APIs that take content as well? Basically...

namespace System.Security.Cryptography.X509Certificates {
    partial class X509Certificate2 {
        public static X509Certificate2 CreateFromPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<char> password);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<byte> passwordBytes);
    }

    partial class X509Certificate2Collection {
        public void ImportFromPem(ReadOnlySpan<char> certPem);
    }
}

If the cert and key are in the same PEM aggregate, they can just pass the same ROS to both certPem and keyPem.

@bartonjs
Copy link
Member Author

@vcsjones CreateFromPem as a sister API (with higher testability) seems fine; though I expect the main scenario for this one to be from-file.

@vcsjones
Copy link
Member

Updated.

I expect the main scenario for this one to be from-file.

Makes sense; agreed.

@davidfowl
Copy link
Member

Where's the final API proposal here?

@vcsjones
Copy link
Member

vcsjones commented May 5, 2020

@davidfowl barring and additional feedback from @bartonjs, #31944 (comment) is the most-up-to-date proposal. I think it is ready for review at this point.

@davidfowl
Copy link
Member

I think we need to support paths as well as content.

@bartonjs
Copy link
Member Author

bartonjs commented May 5, 2020

I think we need to support paths as well as content.

What do you mean by that?

@vcsjones
Copy link
Member

vcsjones commented May 5, 2020

@davidfowl the linked comment has both path and content overloads.

@davidfowl
Copy link
Member

I’m blind

@NinoFloris
Copy link
Contributor

My opinion is that since a collection already expects the file to be a PEM aggregate, I think it's reasonable to expect the certificates and any keys to be in the same path

@vcsjones this is something that's rarely the case. If only this practice could have originated from good linux practices where you apply different acls on key and chain. So what I see most often is full cert chain file and key file.

As an example, this is straight from a certbot letsencrypt/live cert directory:

$cat README
This directory contains your keys and certificates.

`privkey.pem`  : the private key for your certificate.
`fullchain.pem`: the certificate file used in most server software.
`chain.pem`    : used for OCSP stapling in Nginx >=1.3.7.
`cert.pem`     : will break many server configurations, and should not be used
                 without reading further documentation (see link below).

WARNING: DO NOT MOVE OR RENAME THESE FILES!
         Certbot expects these files to remain in this location in order
         to function properly!

We recommend not moving these files. For more information, see the Certbot
User Guide at https://certbot.eff.org/docs/using.html#where-are-my-certificates

@vcsjones
Copy link
Member

@NinoFloris I'm not entirely sure what you're suggesting here; I agree with everything you said.

Using the let's encrypt as an example, I would expect the developer use the API proposal as:

var certWithKey = X509Certificate2.CreateFromPemFile("cert.pem", "privkey.pem");
var additionalCerts = new X509Certificate2Collection();
additionalCerts.ImportFromPemFile("chain.pem");

That comment you quoted was specifically around X509Certificate2Collection, that is to say most of the time you don't have a random bag of certificates in one file and a random bag of private keys in another file that need to get matched up.

@NinoFloris
Copy link
Contributor

NinoFloris commented May 13, 2020

Great, I was suggesting that it could be valuable to be able to do those steps in one go.

ImportFromPemFile("fullchain.pem", "privkey.pem")

This would output say 3 certs where one has a private key attached (in the collection).

I do see how that api is a bit awkward (what to do when I have private keys for more certs) but it does serve the 90% case, accommodating more keys would be a fairly simple change.

I initially missed ImportFromPemFile was not static, so your proposed mutable design does alleviate my previous concerns, replaced by concerns around mutability and certificates. If it's expected this collection will be requested by Kestrel or YARP for TLS and OCSP stapling etc I strongly lean towards an immutable design.

To elaborate, I never deal with the cert.pem, its often useless for webservers. In the case of nginx, to do OCSP stapling chain.pem is requested. Though this could potentially be reliably derived from fullchain (maybe there are some complex scenarios where that doesn't work, I'm no CA expert) which could make it one operation on just two files.

@bartonjs
Copy link
Member Author

I strongly lean towards an immutable design.

X509Certificate2Collection already has Import which will read from a PKCS#7 or PFX (or a single cert) in a mutating manner, the ImportFromPem methods are just modelling things the same way, but with the behavioral differences that are desired for the pem-concat,

@vcsjones
Copy link
Member

vcsjones commented Jun 5, 2020

@bartonjs I (optimistically) just finished implementing a chunk of this, and it left me with one question on API design: should there be overloads / optional param that takes X509KeyStorageFlags so that folks can use choose how they want to persist the private key and set exportability? Or is that making it complicated?

Basically:

namespace System.Security.Cryptography.X509Certificates {
    partial class X509Certificate2 {
        public static X509Certificate2 CreateFromPemFile(string certPemFilePath, string keyPemFilePath = default, X509KeyStorageFlags keyStorageFlags = default);
        public static X509Certificate2 CreateFromEncryptedPemFile(string certPemFilePath, ReadOnlySpan<char> password, string keyPemFilePath = default, X509KeyStorageFlags keyStorageFlags = default);
        public static X509Certificate2 CreateFromEncryptedPemFile(string certPemFilePath, ReadOnlySpan<byte> passwordBytes, string keyPemFilePath = default, X509KeyStorageFlags keyStorageFlags = default);

        public static X509Certificate2 CreateFromPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, X509KeyStorageFlags keyStorageFlags = default);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<char> password, X509KeyStorageFlags keyStorageFlags = default);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<byte> passwordBytes, X509KeyStorageFlags keyStorageFlags = default);
    }

    partial class X509Certificate2Collection {
        public void ImportFromPemFile(string certPemFilePath, X509KeyStorageFlags keyStorageFlags = default);
        public void ImportFromPem(ReadOnlySpan<char> certPem, X509KeyStorageFlags keyStorageFlags = default);
    }
}

@bartonjs
Copy link
Member Author

bartonjs commented Jun 5, 2020

My instinct is to say that we only do ephemeral imports; if anyone needs non-exportable persistent they can just export it as a PFX and reimport that. But, on the other hand, it's sort of hard to add the X509KeyStorageFlags later, since the defaults aren't the same for PFX and PEM. @GrabYourPitchforks do you have thoughts that you are willing to share? 😄.

@bartonjs
Copy link
Member Author

bartonjs commented Jun 5, 2020

@davidfowl For the scenarios you're looking at with Kestrel/YARP, is this a sufficient loader pattern?

public static X509Certificate2Collection LoadCertbotCerts(string livePath)
{
    X509Certificate2 leaf = X509Certificate2.CreateFromPemFile(
        Path.Combine(livePath, "cert.pem"),
        Path.Combine(livePath, "privkey.pem"));

    X509Certificate2Collection chain = new X509Certificate2Collection(cert);
    chain.ImportFromPemFile(Path.Combine(livePath, "chain.pem"));
    
    // Now has the leaf cert at [0] and the intermediate at [1], doesn't have the root.
    return chain;
}

X509Certificate2Collection everythingINeed =
    LoadCertbotCerts("/etc/letsencrypt/live/example.org");

Having the collection import support key matching requires a whole lot more work and a whole lot of failure cases, and I'd really like to avoid having a simple-looking method that requires pages and pages of documentation of what it does with unmatched keys, what happens if the same cert is specified twice and a key matches it, are multiple keys even supported, et cetera, at such a low level.

@davidfowl
Copy link
Member

That seems reasonable as long as we can make SSLStream take X509Certificate2Collection @wfurt ?

@wfurt
Copy link
Member

wfurt commented Jun 6, 2020

Would then be no function to read key with password or do we just miss convenience function to do both? Former would force people to have unencrypted keys on the disk AFAIK.
It may be worth of defaulting key path to certificate path,

Could one read PEM from string? e.g. cases when PEM blob is stored in database?
I'm wondering if we just could/should covert PEM -> PFX. (and threat then sane afterward)

We can make X509Certificate2Collection work on Unix for sure @davidfowl. I'm still not 100% sure about Windows.

@bartonjs
Copy link
Member Author

bartonjs commented Jun 6, 2020

Would then be no function to read key with password or do we just miss convenience function to do both?

public static X509Certificate2 CreateFromEncryptedPemFile(string certPemFilePath, ReadOnlySpan<char> password, string keyPemFilePath = default);

Former would force people to have unencrypted keys on the disk AFAIK.

That's how most keys are left on disk... unless it's an attended operation there's enough data to recover the key (e.g. the hard-coded password). But my sample used passwordless because that's what CertBot does.

Could one read PEM from string?

public static X509Certificate2 CreateFromPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem);

(and variants)

I'm wondering if we just could/should covert PEM -> PFX. (and threat then sa[m]e afterward)

If someone wants a PFX they can certainly do that. I don't see a reason to tie that in here. This is a pure load, PFX loads have side effects on macOS and Windows. I don't see reasons to tie them together other than the fact that it's required for SslStream on Windows -- but that seems like something that the Kestrel loader can do for Windows, and other callers can do when they think it's appropriate to do so (pretty much every other usage of certs in .NET works fine with ephemeral keys, including SslStream on other OSes).

@bartonjs bartonjs added the blocking Marks issues that we want to fast track in order to unblock other important work label Jun 16, 2020
@bartonjs
Copy link
Member Author

bartonjs commented Jun 18, 2020

Video

Approved without the byte-based password inputs.

namespace System.Security.Cryptography.X509Certificates {
    partial class X509Certificate2 {
        public static X509Certificate2 CreateFromPemFile(string certPemFilePath, string keyPemFilePath = default);
        public static X509Certificate2 CreateFromEncryptedPemFile(string certPemFilePath, ReadOnlySpan<char> password, string keyPemFilePath = default);

        public static X509Certificate2 CreateFromPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem);
        public static X509Certificate2 CreateFromEncryptedPem(ReadOnlySpan<char> certPem, ReadOnlySpan<char> keyPem, ReadOnlySpan<char> password);
    }

    partial class X509Certificate2Collection {
        public void ImportFromPemFile(string certPemFilePath);
        public void ImportFromPem(ReadOnlySpan<char> certPem);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels Jun 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants