rcgen

rcgen copied to clipboard

Motivation #274, based off https://github.com/rustls/rcgen/pull/269#issuecomment-2079282218

Still required before merge:

• [ ] Finalise the API requirements
• [ ] Implement it all
• [ ] Documentation comments
• [ ] Changelog - This will require a major release!

Questions

rustls_cert_gen::Ca?

We could either keep it as a wrapper type of rcgen::Issuer or phase it out completely.

If we were gonna phase it out I would do it with the following changes:

• [ ] Drop rustls_cert_gen::Ca in favor of rcgen::Issuer
• [ ] Add a conversion from rcgen::Issuer to rcgen::CertifiedKey using From or to_certified_key method.
• [ ] Move rcgen::PemCertifiedKey::write to rcgen::CertifiedKey::write_pem (replacing Ca::serialize_pem().write with Issuer::to_certified_key().write_pem(), alternatively we could also just have Issuer::write_pem)
• [ ] Drop rustls_cert_gen::PemCertifiedKey in favor of rcgen::CertifiedKey (which is fine because the method is now write_pem not write)
• [ ] Remove rustls_cert_gen::EndEntity::serialize_pem and provide conversion from rustls_cert_gen::EndEntity to rcgen::CertifiedKey using From or to_certified_key method.
• [ ] Should rustls_cert_gen::EndEntity just be rcgen::CertifiedKey or is that too far?

KeyPair and Clone?

I have removed the lifetime from Issuer so it holds owned values, as I personally think this is the best call and it was implied by @djc's message linked above.

This does raise an interesting concern, how should CertificateParams::self_signed construct an rcgen::Issuer as it takes in &KeyPair and we would require an owned KeyPair.

Options:

• [ ] Implement Clone publically for KeyPair and make CertificateParams::self_signed take KeyPair
• [ ] Implement Clone privately for KeyPair and use it
• [ ] Make Issuer hold a Cow<'a, KeyPair> and provide an Issuer::to_owned method which allocates the data and returns Issuer<'static>.
• [ ] Make Issuer hold a lifetime to the KeyPair

Option 4 has the major downside that it makes parsing around an Issuer really difficult due to it always having a lifetime to some buffer. I know in my personal use of rcgen I load the issuer on startup and give it around to Tokio tasks, and it would be nice to avoid leaking memory as would be required with this option.

rcgen::Issuer vs rcgen::CertifiedKey?

Aren't these basically the same type.

rustls_cert_gen::Ca being either a wrapper or replaced by rcgen::Issuer means we need to provide a way to go from an Issuer to Certificate which we will use to replace/reimplement the existing rustls_cert_gen::Ca::cert method.

Given this, I think it seems logical that in code Issuer could be:

pub struct CertifiedKey {
    pub cert: Certificate,
    pub key_pair: KeyPair,
}

which is rcgen::CertifiedKey.

I could be missing something but if they are the same type I feel like unifying them into one makes sense and simplifies the public API.

TODO

The from_ca_cert_*() constructors from CertificateParams move to Issuer, and we see if we can address the documented caveats on those methods.

Gotta do this as @djc noted in the linked message.

Questions

rustls_cert_gen::Ca?

We could either keep it as a wrapper type of rcgen::Issuer or phase it out completely.

If we were gonna phase it out I would do it with the following changes:

* [ ]  Drop `rustls_cert_gen::Ca` in favor of `rcgen::Issuer`

* [ ]  Add a conversion from `rcgen::Issuer` to `rcgen::CertifiedKey` using `From` or `to_certified_key` method.

* [ ]  Move `rcgen::PemCertifiedKey::write` to `rcgen::CertifiedKey::write_pem` (replacing `Ca::serialize_pem().write` with `Issuer::to_certified_key().write_pem()`, alternatively we could also just have `Issuer::write_pem`)

* [ ]  Drop `rustls_cert_gen::PemCertifiedKey` in favor of `rcgen::CertifiedKey` (which is fine because the method is now `write_pem` not `write`)

* [ ]  Remove `rustls_cert_gen::EndEntity::serialize_pem` and provide conversion from `rustls_cert_gen::EndEntity` to `rcgen::CertifiedKey` using `From` or `to_certified_key` method.

* [ ]  Should `rustls_cert_gen::EndEntity` just be `rcgen::CertifiedKey` or is that too far?

I generally think we should focus on getting the rcgen API first before we think much about the rustls-cert-gen API -- in the end for rustls-cert-gen, it's really only the CLI that matters. That said, it probably makes sense to replace Ca with Issuer.

KeyPair and Clone?

I have removed the lifetime from Issuer so it holds owned values, as I personally think this is the best call and it was implied by @djc's message linked above.

This does raise an interesting concern, how should CertificateParams::self_signed construct an rcgen::Issuer as it takes in &KeyPair and we would require an owned KeyPair.

Options:

* [ ]  Implement `Clone` publically for `KeyPair` and make `CertificateParams::self_signed` take `KeyPair`

* [ ]  Implement `Clone` privately for `KeyPair` and use it

* [ ]  Make `Issuer` hold a `Cow<'a, KeyPair>` and provide an `Issuer::to_owned` method which allocates the data and returns `Issuer<'static>`.

* [ ]  Make `Issuer` hold a lifetime to the `KeyPair`

I think we should start to have a type system-level difference between the notion of self-signed certificates in general and CA-level certificates (which have some extra metadata). As such, I think perhaps CertificateParams::self_signed() is the exception and it should not take an Issuer, instead only taking the &KeyPair it takes today.

TODO

The from_ca_cert_*() constructors from CertificateParams move to Issuer, and we see if we can address the documented caveats on those methods.

Gotta do this as @djc noted in the linked message.

Moving the from_ca_cert_*() constructors is important. I also think we'll want to stop storing the params in Certificate and take &self as a reference in signed_by() methods.