Skip to content
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

Add example of generating a cert chain #174

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

tbro
Copy link
Contributor

@tbro tbro commented Oct 9, 2023

Generate two certificates and sign the second with the first. Addresses #79 and #89.

@tbro tbro force-pushed the sign-leaf-with-ca-example branch 2 times, most recently from 6089be4 to c5e20fe Compare October 9, 2023 23:34
@est31 est31 requested a review from cpu October 9, 2023 23:36
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.

It seems unfortunate that this doesn't show off the use of a CSR (mainly in order to show off how this can be done with the CA's key sitting in another process).

(I'll just link my old example here again: https://github.com/djc/sign-cert-remote/blob/main/src/main.rs.)

Cargo.toml Outdated Show resolved Hide resolved
examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
@tbro
Copy link
Contributor Author

tbro commented Oct 10, 2023

It seems unfortunate that this doesn't show off the use of a CSR (mainly in order to show off how this can be done with the CA's key sitting in another process).

(I'll just link my old example here again: https://github.com/djc/sign-cert-remote/blob/main/src/main.rs.)

I didn't see this example before. What I have done for now is add a comment to the top of the example that links to your more complete example. The only advantage I see of the one in this PR is simplicity. So it seems reasonable to me to have this very simple example here but have the obvious link to the more complete example. If your preference is to copy your example in to this repo as an example, I can replace the example already in the PR with that one. I can also see that this PR might be superfluous given that the more complete example already exists.

The rest of the comments on the PR have been addressed.

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I'm interested in moving rcgen towards generating more realistic and standards compliant certificates by default (in particular, following the guidance set by the CA/Browser forum since it dictates the direction of the most important TLS ecosystem).

I haven't started on updating any rcgen defaults to follow this model, but in the meantime I think making the examples do so would be a good first step. As the defaults in the library improve the example code can be trimmed down.

As a result of this larger goal most of my feedback is related to ways to make the example produce more complaint certificates. The base code looks good otherwise :-)

Cargo.toml Outdated Show resolved Hide resolved
examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
@tbro
Copy link
Contributor Author

tbro commented Oct 10, 2023

@cpu I have added the parameters to comply with standards. @djc I basically copied the same structure from your other example in order to bring in the CSR logic. Unfortunately, serialize_request_pem panics with the extra parameters added in the latest commit. So I have removed serialization of the indirect certificate. I can think a bit more about this later, but for now I've pushed the changes that resolve the rest of the comments.

@cpu
Copy link
Member

cpu commented Oct 10, 2023

Thanks!

Unfortunately, serialize_request_pem panics with the extra parameters added in the latest commit.

I think that will be resolved w/ #164 - right now not every extension that can be created from CertificateParams can be written to a CSR. There's code in main rejecting KU, EKU, and AKI when serializing a CSR from params.

So I have removed serialization of the indirect certificate.

That makes sense to me. We can extend the example when the CSR limitation is addressed by #164 (or another PR).

@tbro tbro requested review from cpu and djc October 10, 2023 19:58
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks for the iterations,

tbro requested review from cpu and djc 32 minutes ago

Here's some initial feedback, but I think the build failures in CI are also true positives to work through.

examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
Comment on lines 21 to 24
println!("ca certificate:");
let pem = ca.certificate.serialize_pem().unwrap();

println!("{}", pem);
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback here RE: re-arranging for one println!.

If you keep the separate println!'s I think this second instance should use an in-line var like println!("{pem}") for consistency with L19.

I also think ca_cert_pem might be a better name for the var as opposed to pem.

examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
Comment on lines 75 to 77
params
.subject_alt_names
.push(rcgen::SanType::DnsName(name.into()));
Copy link
Member

Choose a reason for hiding this comment

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

Because you gave the same nam to the CertificateParams constructor this is actually adding a second instance of the same domain name to the SAN extension. We'll want to avoid that. Probably deleting this explicit subject_alt_names.push is the best bet 👍

examples/sign-leaf-with-ca.rs Outdated Show resolved Hide resolved
@tbro
Copy link
Contributor Author

tbro commented Oct 10, 2023

Here's some initial feedback, but I think the build failures in CI are also true positives to work through.

I addressed your comments and builds seems to be passing now.

@est31
Copy link
Member

est31 commented Oct 10, 2023

Sorry for getting so late to the discussion. First a general point: I must say that I disagree with the opinion of @cpu to generally move rcgen to conform by CA/Browser forum rules. Yes, they are made by the professionals in the CA industry, those who issue certificates as a paid service. And it is valuable to have these rules apply to most TLS certificates you use to connect to random web services.

But it feels to me that most users of rcgen are not the "professionals", so they don't really need to conform by these rules. And in my opinion, the professional's rules are quite harmful for users.

  • The created certificates are now way bigger than they used to be, causing bloat. This means you cannot paste them as easily in the text form and are more likely to move them around as files.
  • Any additional complexity makes it hard to customize the certificates to the use case needed by the user of rcgen.
  • The worst part is probably not allowing certificates which last thousands of years. In many settings you don't really want to do key rotation or certificate renewal, and have no plans set up for that. I've heard many people reject all of TLS as a technology because of this strong enforcement of certificate renewal. In rcgen's use cases, often you have not really planned key rotation or certificate renewal. You deploy some device in the field, a certificate becomes invalid, and then boom, the device doesn't work any more.

To me, it feels that the CA/Browser forum rules are mostly relevant if you are either a member of that forum, or if you otherwise want to create certificates that are trusted by browsers by default. I don't think most users of rcgen are/want to do either, and I built rcgen to serve their needs (which was also my need).

The big browsers don't enforce most of these rules, at least for locally trusted certificates. For trusted-by-default certificates, it's a different thing, which is again fine and a good idea. But locally trusted certificates don't need this info, you know already that they have local trust.

I am in favour of adding a "CA/Browser Forum compliance" mode which enables the appropriate extensions for various scenarios, it could e.g. be different constructors of CertificateParams. But I don't think rcgen should output certificates that expire less than 1 year after issuance by default. That's way more btw than what chrome requires for (newly issued) globally trusted certificates, so it would really just be something that ticks a random checkbox somewhere while disrupting users in the field.


Now to the specific point. I think it's fine to have one or two examples that create CA/Browser Forum compliant certificates, just to show users who need it (they should be in a minority) how it's done. But I don't think we should use it in all examples, or even a majority.

@cpu
Copy link
Member

cpu commented Oct 11, 2023

First a general point: I must say that I disagree with the opinion of @cpu to generally move rcgen to conform by CA/Browser forum rules.

@est31 Fair, but I don't want to side-track this PR arguing my perspective. It sounds like it would be an uphill battle in either case.

But it feels to me that most users of rcgen are not the "professionals"

For what its worth I think this is a strong argument for why opinionated defaults are in order.

Now to the specific point. I think it's fine to have one or two examples that create CA/Browser Forum compliant certificates, just to show users who need it (they should be in a minority) how it's done. But I don't think we should use it in all examples, or even a majority.

It sounds like the changes I suggested are fine for the example at hand? If not I think it would be helpful to the OP if you leave some specific suggestions about what needs to change to land this example.

@djc
Copy link
Member

djc commented Oct 11, 2023

I do think I agree that getting this example to go through the CA/B hoops might not be worth it/increases scope too much for this PR, especially if we're planning to offer an API in the library that handles these details for you (so that we can then remove the code in this example again).

In general, it would be great if we can facilitate all three use cases:

  • Test certificates (simple, long-lived)
  • Custom deployment (diverse concerns, but making long-lived certificates easy)
  • CA/B-compliant certificates

Which seems feasible enough to me? For example, we could offer CA/B forum defaults, but under a specific API (like CertificateParams::web_server() or some such).

@cpu
Copy link
Member

cpu commented Oct 11, 2023

getting this example to go through the CA/B hoops might not be worth it/increases scope too much for this PR

The changes I requested have already been made and don't seem to increase the scope by much to me. It's a handful of small changes.

I think we should defer discussion of future API changes to a separate issue and focus on providing actionable feedback to the contributor. If there are parts of the work that are out of scope, let's suggest changes to bring it in-scope.

@est31
Copy link
Member

est31 commented Oct 11, 2023

Sorry for the off-topic discussion. I am approving this PR.

@djc
Copy link
Member

djc commented Oct 11, 2023

getting this example to go through the CA/B hoops might not be worth it/increases scope too much for this PR

The changes I requested have already been made and don't seem to increase the scope by much to me. It's a handful of small changes.

I think we should defer discussion of future API changes to a separate issue and focus on providing actionable feedback to the contributor. If there are parts of the work that are out of scope, let's suggest changes to bring it in-scope.

Fair enough, sorry!

@tbro tbro requested a review from cpu October 11, 2023 15:55
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thank you! Sorry that some of my feedback ended up being controversial 😓

Generate two certificates and sign the second with the first.
@cpu cpu force-pushed the sign-leaf-with-ca-example branch from a75cbc3 to aa2caf9 Compare October 11, 2023 16:57
@cpu
Copy link
Member

cpu commented Oct 11, 2023

cpu force-pushed the sign-leaf-with-ca-example branch from a75cbc3 to aa2caf9

Squashed the review commits so the merge queue rebase merge will be tidy.

@cpu cpu enabled auto-merge October 11, 2023 16:58
@cpu cpu added this pull request to the merge queue Oct 11, 2023
Merged via the queue into rustls:main with commit eb3c261 Oct 11, 2023
15 checks passed
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.

4 participants