Unbundle params from output types

[djc]

This is an alternative direction to #326. From here, I think we can add a nice Issuer builder API.

[est31]

I like this one better than #326, but not sure if it has some negative consequences if the params are not available for Certificate.

[djc]

I like this one better than #326, but not sure if it has some negative consequences if the params are not available for Certificate.

The caller could always bundle up the params and the Certificate if they need both, right?

[djc]

@est31 sorry, I just pushed a bunch more stuff -- will stop here and let you take another look.

[blheatwole]

I set my project to use rcgen = { version = "0.13", features = ["aws_lc_rs"], git = "https://github.com/rustls/rcgen", branch="issuer"} which put me at 9e88850.

After a few updates I was able to successfully sign using a mock Key Vault. I'm working on confirming with the real Azure Key Vault.

[cpu]

I'll give this a review by EOW.

[cpu]

Thanks, this is a nice improvement.

One of the two CI failures will be fixed with a rebase to pick up the version bump in main. The other is a real doctest issue:

---- rcgen\src\crl.rs - crl::CertificateRevocationList (line 23) stdout ----
error[E0407]: method `public_key` is not a member of trait `SigningKey`
 --> rcgen\src\crl.rs:31:3
  |
9 |   fn public_key(&self) -> &[u8] { &self.public_key }
  |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not a member of trait `SigningKey`

error[E0407]: method `algorithm` is not a member of trait `SigningKey`
  --> rcgen\src\crl.rs:33:3
   |
11 |   fn algorithm(&self) -> &'static SignatureAlgorithm { &PKCS_ED25519 }
   |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not a member of trait `SigningKey`

It seems like the doctest in question is more complicated than it needs to be in order to work around builds without crypto enabled.

WDYT about simplifying that example to avoid all the key pair trait stuff and just configuring the docs build to have the required feature? IMO the example is better when it focuses on the CRL bits and avoids a dummy remote key pair.

[est31]

IMO the example is better when it focuses on the CRL bits and avoids a dummy remote key pair.

do we have examples with dummy remote key pairs? We should at least have some example.

[djc]

IMO the example is better when it focuses on the CRL bits and avoids a dummy remote key pair.

do we have examples with dummy remote key pairs? We should at least have some example.

I kept the existing example which demonstrates both, I think?

[est31]

I kept the existing example which demonstrates both, I think?

That's fine, @cpu was suggesting to remove that usage from the crl example.

In any case, wondering about the reasons behind the inverting of the KeyPair/SigningKey hierarchy: why do we do this?

I chose the old hierarchy because felt to me that it occurs less often that you want to use a RemoteKeyPair than if you want to use one with the whole key pair available. So I felt that users shouldn't be punished by extra complexity that a trait introduces (you need to find types compatible with that trait).

I'm not saying we shouldn't do it, but want to hear arguments :).

[est31]

From here, I think we can add a nice Issuer builder API.

@djc what do you envision for that API? just that I know which direction we are walking in.

[djc]

I chose the old hierarchy because felt to me that it occurs less often that you want to use a RemoteKeyPair than if you want to use one with the whole key pair available. So I felt that users shouldn't be punished by extra complexity that a trait introduces (you need to find types compatible with that trait).

I'm not saying we shouldn't do it, but want to hear arguments :).

Basically, I think that it simplies the code. A bunch of public API unnecessarily took a &KeyPair directly which actually just needs the signing (and public key) capabilities. By making KeyPair implement SigningKey I feel that the simplicity of the external API is mostly intact (most usages should keep working), while function signatures can directly express their requirements for &impl PublicKeyData and &impl SigningKey.

From here, I think we can add a nice Issuer builder API.

@djc what do you envision for that API? just that I know which direction we are walking in.

I'm envisioning something like Issuer::from_cert_pem(&pem)?.with_key(&ca_key) or Issuer::builder().with_distinguished_name(DnType::OrganizationName, "foo").with_usage(KeyUsagePurpose::DigitalSignature).with_key(KeyPair::generate()?). I'd like to work towards a direction where CertificateParams, CertificateSigningRequestParams and CertificateRevocationListParams (or derivatives of these) implement a ToDer trait such that we can have an Issuer::sign(subject: &impl ToDer) (I have a private branch that moves in this direction). This would drop the CertificateParams::from_ca_cert*() methods.

[cpu]

IMO the example is better when it focuses on the CRL bits and avoids a dummy remote key pair.

do we have examples with dummy remote key pairs? We should at least have some example.

I kept the existing example which demonstrates both, I think?

That's fine, @cpu was suggesting to remove that usage from the crl example.

Yeah exactly, I think it's best to try to keep it one concept per example. If we don't have a separate example showing implementing the trait then I'd be 👍 to adding that with the bits removed from the CRL example.

I can take a crack at this as a follow-up if folks agree on the direction. No need to block this PR since it's a pre-existing example.

[djc]

@est31 I'll await your acknowledgement of my last comment before merging, let me know. 👍

[est31]

By making KeyPair implement SigningKey I feel that the simplicity of the external API is mostly intact (most usages should keep working)

It still works but if you try to write new code that uses the library, each trait is another point of friction: you need to look up implementations of that trait and which one is good for you. It's better to be economic with traits, and the old hierarchy ensured that only people who really needed the trait needed to learn about it (most people don't). Still, I think the change is good overall as it gives a little bit more flexibility (esp wrt stuff like Box vs Rc vs Arc, although you can always just put everything into Box<Arc<_>>).

I'd like to work towards a direction where CertificateParams, CertificateSigningRequestParams and CertificateRevocationListParams (or derivatives of these) implement a ToDer trait such that we can have an Issuer::sign(subject: &impl ToDer) (I have a private branch that moves in this direction).

The name would probably need to be adjusted, i.e. to CanSignDer, as simple strings of course can be converted to Der too but we don't want to expose that, we should only allow signing of stuff that is specified in the RFCs.

As I've explained above, I'm not a fan of even more traits in the API surface :). We should make sure that the main recommended API usage for signing doesn't involve traits, but maybe goes via inherent functions on the params types (at least most of them, we can be economic here).

[est31]

fine with merging this one

[djc]

By making KeyPair implement SigningKey I feel that the simplicity of the external API is mostly intact (most usages should keep working)

It still works but if you try to write new code that uses the library, each trait is another point of friction: you need to look up implementations of that trait and which one is good for you. It's better to be economic with traits, and the old hierarchy ensured that only people who really needed the trait needed to learn about it (most people don't). Still, I think the change is good overall as it gives a little bit more flexibility (esp wrt stuff like Box vs Rc vs Arc, although you can always just put everything into Box<Arc<_>>).

In this case, because rcgen itself provides a single implementation of the trait, that seems like a clear way forward for users who want to stick with the simple path.

I'd like to work towards a direction where CertificateParams, CertificateSigningRequestParams and CertificateRevocationListParams (or derivatives of these) implement a ToDer trait such that we can have an Issuer::sign(subject: &impl ToDer) (I have a private branch that moves in this direction).

The name would probably need to be adjusted, i.e. to CanSignDer, as simple strings of course can be converted to Der too but we don't want to expose that, we should only allow signing of stuff that is specified in the RFCs.

As I've explained above, I'm not a fan of even more traits in the API surface :). We should make sure that the main recommended API usage for signing doesn't involve traits, but maybe goes via inherent functions on the params types (at least most of them, we can be economic here).

Sure, let's discuss in future PRs how to shape this API.