Skip to content

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

Merged
djc merged 15 commits into
rustls:mainfrom
cpu:cpu-62-issuance-api-rework
Jan 16, 2024
Merged

Rework Certificate issuance API, make DER/PEM serialization stable#205
djc merged 15 commits into
rustls:mainfrom
cpu:cpu-62-issuance-api-rework

Conversation

@cpu

@cpu cpu commented Jan 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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
@cpu cpu mentioned this pull request Jan 10, 2024
Closed
djc
djc approved these changes Jan 11, 2024
View reviewed changes

@djc djc left a comment

Copy link
Copy Markdown
Member

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!

Comment thread rcgen/src/lib.rs Outdated
Comment thread rcgen/src/lib.rs Outdated
Comment thread rcgen/src/lib.rs Outdated
Comment thread rcgen/src/lib.rs Outdated
Comment thread rcgen/src/lib.rs Outdated
Comment thread rcgen/src/lib.rs
Comment thread rcgen/src/lib.rs Outdated
Comment thread rcgen/src/lib.rs Outdated
@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
djc
djc approved these changes Jan 12, 2024
View reviewed changes
Comment thread rcgen/src/csr.rs
cpu added 15 commits January 12, 2024 14:25
@cpu
key_pair: fix typo
379266b
@cpu
lib: fix Certificate comment
68633ef 
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.
@cpu
lib: lift out key identifier calculation fn
33e9d1c 
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.
@cpu
lib: have AKI write fn take AKI value as arg
5cca7fe 
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`.
@cpu
lib: minimize serialize fn's inputs
ff25b4a 
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.
@cpu
lib: pull out helper for preparing key pair
6de8815 
This will make it easier to have more than one callsite that performs
on-demand key generation.
@cpu
lib: move serialization to Certificate construction
3f021a8 
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.
@cpu
lib: retain SPKI in Certificate
4890ca4 
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.
@cpu
lib: return CertifiedKey from generate
4db5232 
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.
@cpu
lib: add clarification for CSR serialization fns
1ccd0db 
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`.
@cpu
csr: move CSR->Cert gen to Certificate:from_request:
13991d2 
This unifies the creation of `Certificate`s to the `Certificate`
constructors instead of being spread across both `Certificate` and
`CertificateSigningRequest`.
@cpu
lib: CertificateSigningRequest -> CertificateSigningRequestParams
7c05f4c 
This rename better emphasizes the role the CSR type plays, and aligns it
with other `*Params` types that already exist in the API surface.
@cpu
lib: tune up Certificate/CertificateParams rustdocs
5980234 
A quick pass through the rustdoc strings, adding more detail and
clarifying text where appropriate.
@cpu
lib: unify CertificateParams impl blocks
1d2042a
@cpu
Cargo: version 0.12.0 -> 0.13.0
f3daf4b
@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
est31
est31 approved these changes Jan 16, 2024
View reviewed changes

@est31 est31 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks great!

@est31

est31 commented Jan 16, 2024

Copy link
Copy Markdown
Member

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
@cpu cpu mentioned this pull request Jan 16, 2024
Merged
@djc djc mentioned this pull request Jan 17, 2024
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Jan 23, 2024
@djc
External keys (#213)
b87fced 
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.
@djc djc mentioned this pull request Mar 8, 2024
Merged
@cpu cpu mentioned this pull request Jun 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djc djc djc approved these changes

@est31 est31 est31 approved these changes

Assignees

@cpu cpu

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

serialize_der() regenerates the certificate

3 participants

@cpu @est31 @djc