serialize_der() regenerates the certificate

[jbg]

I am using rcgen to generate a certificate, then I'm sharing the fingerprint of the certificate with a peer, and then I'm using a library to communicate with that peer. The library requires me to provide the certificate & private key in PEM format.

I've been debugging for a while why the fingerprint I generate, like this:

let fingerprint = ring::digest::digest(&ring::digest::SHA256, &cert.serialize_der()?);

... does not match the fingerprint the peer expects to receive, nor the fingerprint OpenSSL reports when I supply it with the PEM-encoded certificate obtained with serialize_pem().

After reading the discussion in #28, and looking at the code, I think I understand what's going on — serialize_der() and serialize_pem() are not actually just serializing; they are in fact generating the certificate each time they are called, so the random components are different between the DER serialization and the PEM serialization.

Ideally the certificate would be generated when the Certificate struct is created, so that it can be repeatedly serialized — perhaps into different formats — without regenerating it each time.

If that's not possible, the functions should either be renamed or at least the mis-naming documented clearly to save the time of anyone else who encounters this.

Right now, from_params() is documented as "Generates a new certificate" and serialize_der() is documented as "Serializes the certificate to the binary DER format", when in reality from_params() just stores the params (and does keygen if needed), and serialize_der() etc actually do the certificate generation.

[est31]

Thanks for filing a bug about this. I haven't come around to file an issue about it, but wanted to find solutions for it.

Resolving this issue likely requires a breaking change, due to the serialize_der_with_signer function existing. I guess there need to be two steps: one step letting you choose how to sign something. And the result of that can then be exported in DER or in PEM format.

In the meantime, you can do it like rsa-irc.rs and build the der from the pem using the pem crate

[jeffparsons]

For the record, I recently got bitten by this while tinkering with certificate generation for tests. I was trying to figure out why there was a difference between serializing a certificate to PEM and serializing to DER. It turns out my client was ending up with one certificate and the server was ending up with another -- just because I was accidentally regenerating it, not because I was mishandling the DER formatted cert.

[est31]

I guess there need to be two steps: one step letting you choose how to sign something. And the result of that can then be exported in DER or in PEM format.

To expand on this, my proposal would be to remove serialize(_der)(_with_signer) etc functions in favour of serialize(_with_signer) functions that return a SerializedCertificate (name to be bikeshedded). Then SerializedCertificate has export_pem and export_der functions or something like that. Also gets rid of the combinatorial explosion problem.

[cpu]

I'm not sure about adding a third layer like that. I think the OP and I both agree on the idea that Certificate should do what the name implies and represent a constructed certificate. I think it should hold the der bytes for a full signed certificate in addition to the CertificateParams and KeyPair.

What about an API where:

• CertificateParams are constructed much like they are now.
• Certificate gains a der field to hold a signed serialized certificate, and der() and pem() fn's for returning those bytes in the correct format.
• Certificate::generate_self_signed(params: CertificateParams) -> Result<Self> creates a self-signed Certificate from CertificateParams, generating the Certificate keypair if not specified in params.
• Certificate::generate(params: CertificateParams, issuer: &Certificate) -> Result<Self> creates a certificate from CertificateParams that's signed by the issuer.
• Certificate::generate_csr(csr: CertificateSigningRequest, issuer: &Certificate) -> Result<Self> creates a certificate from the CSR CertificateParams that's signed by the issuer
• We remove Certificate::from_params and serialize_(der|pem)[with_signer], and we remove CertificateSigningRequest::serialize_(der|pem)[with_signer]. I think the private key serialization fns on Certificate can remain as-is.

[est31]

I actually like your approach better (outside of naming and some details).

We remove Certificate::from_params

a function like that is needed for importing of existing certificates. We could for example rename it to import_from_der and import_from_pem.

[cpu]

a function like that is needed for importing of existing certificates. We could for example rename it to import_from_der and import_from_pem.

Good point. Having Certificate::from_der(der: &[u8], key_pair: KeyPair) and the same for PEM seems reasonable 👍 We would remove the CertificateParams::from_ca_cert_pem and CertificateParams::from_ca_cert_der fns in that case.

[cpu]

I started working on the proposed API last night and I like the direction but wasn't able to implement the idea as described end-to-end.

Certificate::generate_csr(csr: CertificateSigningRequest, issuer: &Certificate) -> Result creates a certificate from the CSR CertificateParams that's signed by the issuer

One roadblock here: this isn't tenable with the Certificate holding KeyPair since the CertificateSigningRequest only has public key data.

IME it's more common to separate key generation and certificate generation. I think the Certificate type holding a private key is a little bit confusing. It means something that would otherwise be public data contains a secret (and so has to have a Zeroize impl and be handled carefully), and it introduces this asymmetry where in one case we want to generate a certificate without holding the subject's private key.

Perhaps Certificate::generate_self_signed, Certificate::generate, Certificate::from_der should be adapted to return Result<(Certificate, PrivateKey)> and Certificate::generate_csr would return just Result<Certificate>. In the case where the input CertificateParams specified a KeyPair, we can pass it through to the result. In the case where the params specify None we return the generated KeyPair along with the certificate.

[djc]

Perhaps Certificate::generate_self_signed, Certificate::generate, Certificate::from_der should be adapted to return Result<(Certificate, PrivateKey)> and Certificate::generate_csr would return just Result<Certificate>. In the case where the input CertificateParams specified a KeyPair, we can pass it through to the result. In the case where the params specify None we return the generated KeyPair along with the certificate.

This sounds good to me (does generate_csr() yield a Certificate type today -- feels like that should be a distinct type?).

[cpu]

(does generate_csr() yield a Certificate type today -- feels like that should be a distinct type?)

The equivalent API today is CertificateSigningRequest::serialize_der_with_signer and CertificateSigningRequest::serialize_pem_with_signer. They yield DER/PEM directly, not a Certificate representation, which is how they've side-stepped this problem.

I think that API is a little bit confusing because at face value you'd expect they would be serializing the CSR itself, not a certificate issued from the CSR. You serialize a CSR from a Certificate with Certificate::serialize_request_pem or Certificate::serialize_request_der.

[djc]

Ahh, so the difference is in serialize_*_with_signer() -> certificate vs serialize_request_* -> CSR.

[cpu]

Here's where I'm at with the refactor so far in case there's interest for early review: https://github.com/cpu/rcgen/tree/cpu-api-refactor-wip

Some warnings up front;

• I'm likely to rewrite history on that branch.
• It's lacking a rebase on main for the aws-lc-rs changes
• I haven't done my own typical self-review pass to catch issues with the commit history etc.
• I think it needs a pass to harmonize fn names. E.g. the KeyPair fns for serialization are serialize_pem/serialize_der but Certificate uses pem/der.

The last remaining change I wanted to do was try and harmonize the "from scratch" CertificateSigningRequest creation to match the new API, e.g. something like CertificateSigningRequest::generate(params: CertificateParams, key_pair: KeyPair) instead of the existing Certificate::serialize_request_der() and Certificate::serialize_request_pem. Some of the groundwork is there but the API change hasn't been implemented yet.

I'll try to push more on this branch soon, but it's something I've mostly been working on in evenings. I have more pressing rustls work to focus on during the day and updating all of the unit tests for each API change is somewhat time consuming.

[cpu]

The last remaining change I wanted to do was try and harmonize the "from scratch" CertificateSigningRequest creation to match the new API, e.g. something like CertificateSigningRequest::generate(params: CertificateParams, key_pair: KeyPair)

This ends up being fairly tricky if we want to support all of the existing use cases because CertificateParams isn't Clone (mostly due to potentially holding private key material that itself isn't Clone). I've tried a few potential rearrangements but haven't arrived at something that seems workable. I might punt on this part and open a PR with the rest of the work since it seems more straight forward and would resolve the immediate issue.

[est31]

Some ideas to impl Clone: one can put the private key material into an Arc or implement it manually by serializing, then deserializing again (and panicking if that doesn't work).

[cpu]

I'm not going to have time to invest in continuing the API refactor beyond the certificate issuance API in the near future so I've cleaned up my WIP branch for review: #205

The certificate issuance API is the most important w.r.t to the user confusion we see and so I think it makes sense to fix that without blocking on improvements to CertificateSigningRequest.

If folks disagree I'm happy to cede my branch to someone that has more time to push the remaining parts forward.