Skip to content

Rework Certificate issuance API, make DER/PEM serialization stable #205

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

Merged
merged 15 commits into from
Jan 16, 2024

Conversation

cpu
Copy link
Member

@cpu cpu commented Jan 10, 2024

Previously a Certificate was a container for CertificateParams and a KeyPair, most commonly created from a CertificateParams instance. Serializing the Certificate (either as self signed with serialize_pem or serialize_der, or signed by an issuer with serialize_pem_with_signer or serialize_der_with_signer) would issue a certificate and produce the serialized form in one operation. The net result is that if a user wanted both DER and PEM serializations they would likely call serialize_der(_with_signer) and then serialize_pem(_with_signer) and mistakenly end up with the encoding of two distinct certificates, not the PEM and DER encoding of the same cert. Since the KeyPair contains a private key this API design also meant that the Certificate type had to be handled with care, and Zeroized.

This branch reworks the issuance API and Certificate type to better match user expectation: Certificate is only public material and represents an issued certificate that can be serialized in a stable manner in DER or PEM encoding.

I recommend reviewing this commit-by-commit, but here is a summary of the most notable API changes:

  • Certificate::from_params and Certificate::serialize_der and Certificate::serialize_pem for issuing a self-signed certificate are replaced with Certificate::generate_self_signed() and calling der or pem on the result.
  • Certificate::from_params and Certificate::serialize_der_with_signer and Certificate::serialize_pem_with_signer for issuing a certificate signed by another certificate are replaced with Certificate::generate() and calling der or pem on the result.
  • CertificateSigningRequest::serialize_der_with_signer and CertificateSigningRequest::serialize_pem_with_signer for issuing a certificate from a CSR are replaced with Certificate::from_request and calling der or pem on the result. The CertificateSigningRequest type is renamed to CertificateSigningRequestParams to better emphasize its role and match the other *Params types that already exist.
  • Since we now calculate the DER encoding of the certificate at Certificate construction time, the pem and der fns are now infallible.
  • Since Certificate no longer holds KeyPair, the generate fns now expect a &KeyPair argument for the signer when issuing a certificate signed by another certificate.
  • The generation fns now return a CertifiedKey that contains both a Certificate and a KeyPair. For params that specify a compatible KeyPair it is passed through in the CertifiedKey as-is. For params without a KeyPair a newly generated KeyPair is used.

In the future we should look at harmonizing the creation of CertificateSigningRequest and CertificateRevocationList to better match this updated API. Unfortunately I don't have time to handle that at the moment. Since this API surface is relatively niche compared to the Certificate issuance flow it felt valuable to resolve #62 without blocking on this future work.

Resolves #62

@cpu cpu self-assigned this Jan 10, 2024
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes the API much better, nice!

@cpu cpu force-pushed the cpu-62-issuance-api-rework branch 2 times, most recently from 796b5d7 to f55ac1c Compare January 11, 2024 22:54
cpu added 15 commits January 12, 2024 14:25
The `Certificate` type as it exists at this commit has no signature, and
it isn't self-signed. It _may_ be turned into a self signed certificate
by calling `serialize_der()` or `serialize_pem()`, or it may be turned
into a certificate signed by an issuer! Simplify the comment before more
substantial refactoring.
This commit associates deriving a key identifier with the
`KeyIdMethod` enum. This will make it easier to calculate a key
identifier with only the public key in hand as opposed to the full
parameters.

Along the way the rustdocs associated with `KeyIdMethod` are tuned up
for clarity/specificity.
Rather than have the `write_x509_authority_key_identifier` fn take
a `Certificate` as input in order to calculate the AKI value, have the
caller find the AKI and pass it to the write fn for writing. This
simplifies generating a certificate with the issuer's `KeyPair`, and
not a `Certificate`.
This commit updates the various `serialize_*` fns to take
more targeted inputs as opposed to a `Certificate`. This will make it
easier to perform subsequent refactors.
This will make it easier to have more than one callsite that performs
on-demand key generation.
This commit removes `Certificate::from_params`, replacing it with
`Certificate::generate_self_signed`. The `serialize_x` fns are replaced
with `pem()` and `der()` accessors. To issue a certificate signed by an
issuer, a new `Certificate::generate` constructor can be used.

Unit tests, and the rustls-cert-gen utility, are updated accordingly.
In preparation for removing the `KeyPair` from `Certificate` we need to
hold on to the subject public key info for use when generating key
identifiers.
Rather than store `KeyPair` in `Certificate`, take `KeyPair` as
a parameter when required for signing and return `KeyPair` from the
generate methods as a new `CertifiedKey` type. This will allow unifying
the API around the idea that a `Certificate` is public material.

Tests and the cert gen utility are updated accordingly.
This commit updates the rustdoc comments on
`Certificate::serialize_request_der` and
`Certificate::serialize_request_pem`, emphasizing that they generate
and sign CSRs for each invocation. The purpose of the `subject_key`
argument is emphasized.

Along the way, fix an inaccurate comment in
`CertificateParams::write_request` - we're writing the _subject_
distinguished name, not the issuer. The `Name` field in the syntax in
RFC 2986 is `subject`.
This unifies the creation of `Certificate`s to the `Certificate`
constructors instead of being spread across both `Certificate` and
`CertificateSigningRequest`.
This rename better emphasizes the role the CSR type plays, and aligns it
with other `*Params` types that already exist in the API surface.
A quick pass through the rustdoc strings, adding more detail and
clarifying text where appropriate.
@cpu cpu force-pushed the cpu-62-issuance-api-rework branch from f55ac1c to f3daf4b Compare January 12, 2024 20:02
@cpu cpu requested a review from est31 January 15, 2024 14:30
Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

@est31
Copy link
Member

est31 commented Jan 16, 2024

I also really like the CertifiedKey struct. Maybe the name is not ideal but we can always change it in a followup PR.

I'm also thinking about a struct CertifiedKeyRef<'a,'b> { pub cert: &'a Certificate, pub key: &'b KeyPair }, which can be used in the cases where references for both the key and the cert are wanted, and a conversion function like impl CertifiedKey { fn ref(&self) -> CertifiedKeyRef<'_, '_> {...}} . I think that's also best for a followup.

@djc djc added this pull request to the merge queue Jan 16, 2024
Merged via the queue into rustls:main with commit 30489d7 Jan 16, 2024
@cpu cpu deleted the cpu-62-issuance-api-rework branch January 16, 2024 14:36
@djc djc mentioned this pull request Jan 17, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 23, 2024
As an alternative to #212, this goes further in the direction of #205,
removing `alg` and `key_pair` from `CertificateParams` and requiring the
caller to pass in a reference to them when signing. This seems to have a
number of nice properties:

* `alg` is derived from the passed in `KeyPair`, so key algorithm
mismatch errors can no longer occur
* No need for passing in a signing algorithm when parsing from
pre-existing parameters or key pairs
* Should make it easy to support long-lived (remote) key pairs

The main downside as far as I can see is that the top-level API gets a
bit more complicated, because generating a `KeyPair` must now be done by
the caller, and for now we force them to pick a signing algorithm. I
think we might mitigate this by adding a no-argument constructor like
`generate_default()` (or use `generate()` for the no-argument variant
and `generate_for()` for the variant that requires an argument).

Generally, this feels like a clear improvement in the API's design to
me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

serialize_der() regenerates the certificate
3 participants