Rustls-cert-gen support basic parameters #185

tbro · 2023-10-31T21:05:06Z

This commit adds basic functionality for rustls-cerg-gen crate. A
small wrapper library has been added in order to organize code into
small modules and to provide a simple API that can easily be updated
as new functionality is added in the future. There are some rough
edges, some missing documentation for example. But I hope to get the
broader structure approved before refining the documentation. Several
parameters have been added with the idea of supporting a broad range
of use-cases and test expectations around design and
maintainability. The easiest way to view currently supported options
is with cargo run -- --help.

Closes #175

codecov · 2023-10-31T21:12:41Z

Codecov Report

Merging #185 (faaee63) into main (0318d2f) will increase coverage by 3.93%.
The diff coverage is 79.85%.

❗ Current head faaee63 differs from pull request most recent head 201f34a. Consider uploading reports for the commit 201f34a to get more accurate results

@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
+ Coverage   72.86%   76.80%   +3.93%     
==========================================
  Files           7       13       +6     
  Lines        1861     2259     +398     
==========================================
+ Hits         1356     1735     +379     
- Misses        505      524      +19

Files	Coverage Δ
rustls-cert-gen/src/cert/params.rs	`96.59% <96.59%> (ø)`
rustls-cert-gen/src/args.rs	`79.16% <79.16%> (ø)`
rustls-cert-gen/src/cert/entity.rs	`90.21% <90.21%> (ø)`
rustls-cert-gen/src/cert/ca.rs	`72.72% <72.72%> (ø)`
rustls-cert-gen/src/cert/mod.rs	`0.00% <0.00%> (ø)`
rustls-cert-gen/src/cert/signature.rs	`84.72% <84.72%> (ø)`
rustls-cert-gen/src/main.rs	`0.00% <0.00%> (ø)`

... and 3 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

est31

this is a partial review. I'm happy with trying out non-clap options for CLI arg parsing, we can always switch to clap later.

rustls-cert-gen/README.md

est31 · 2023-11-01T12:54:28Z

rustls-cert-gen/src/args.rs

+	pub output: PathBuf,
+	/// Persist ca signing-key to `output` dir
+	#[bpaf(long, switch)]
+	pub insecure: bool,


I'm not sure about calling this mode "insecure" or not doing it by default (IMO it should by default emit all files, including the signing key)

I am happy to change it, but I would like to share my thought process first. I think the signing key is only useful for "advanced" usage. Namely, as input to another tool (or a possible future iteration of this one) to sign more certificates. Someone looking for a quick way to generate certificates for a TLS connection won't not need it. I think such a person might also be less likely to have a complete grasp of the why and wherefore of all the files. They might also be less informed about secure practices for key management and the implications of leaking the signing key of their ad-hoc certificate-authority. That's how I came to naming the switch insecure. So the idea was to make it easy for those that want something easy and provide a path to more advanced usage for those that know what they are doing.

I don't like the name here because it doesn't feel like persisting a private key to disk is inherently "insecure". Unless we have functionality for generating new certificates that are signed with an existing on-disk CA private key, I'm not sure it makes sense to persist the CA private key to disk because it'll be relatively hard to use for anything. Adding such functionality (to sign with a persisted CA private key) makes sense to me and probably shouldn't be too hard, but also probably doesn't need to be in this PR.

IMO it makes sense to have the key around even if such a functionality doesn't exist yet, as then the key is around once we add that functionality, instead of being lost.

And yeah, full agree, persisting a private key to disk is not inherently insecure. This is a tool in the end, and yeah if you deploy the CA into a root store then the private key becomes very powerful. But that's up to the users.

OK --secure switch was removed in latest commit and all files will always be persisted.

est31 · 2023-11-01T12:56:02Z

rustls-cert-gen/src/args.rs

+fn parse_sans(hosts: Vec<String>) -> Vec<SanType> {
+	hosts
+		.into_iter()
+		.map(|s| s.to_string())


This isn't needed if you make parse_san take a &str instead of String.

In that case I think we would need something like .iter().map(|s|parse_san(s)). The function call is needed to get the &str (I think). But it turns out that s.to_string() was unnecessary even without changing the signature. So I just removed that line in the latest commit.

cpu · 2023-11-06T17:55:10Z

Just leaving a comment to say I haven't forgotten this PR. I'll try and give it a review pass in the next day or two. Thanks!

cpu

Thanks @tbro. I had a fair amount of feedback, but I don't think any of it is particularly complex. LMK what you think.

rcgen/Cargo.toml

rustls-cert-gen/README.md

rustls-cert-gen/src/cert/params.rs

rustls-cert-gen/src/cert/mod.rs

rustls-cert-gen/src/lib.rs

rustls-cert-gen/src/cert/entity.rs

tbro · 2023-11-08T00:18:39Z

Thanks @tbro. I had a fair amount of feedback, but I don't think any of it is particularly complex. LMK what you think.

Thanks for being thorough. Unless I missed something, I've taken all of your suggestions except one. I don't think we can simply derive Clone and Debug for CertficateBuilder (was BuildParams). But if you'd like to pursue another route we can consider more options.

tbro · 2023-11-08T00:25:31Z

Thanks for being thorough. Unless I missed something, I've taken all of your suggestions except one. I don't think we can simply derive Clone and Debug for CertficateBuilder (was BuildParams). But if you'd like to pursue another route we can consider more options.

Double checking and I definitely did miss a few comments, I should be able to address the rest tomorrow.

djc · 2023-11-09T09:45:05Z

Thanks for working on this!

IMO it would be good to clean up the commit history a bit (or split into separate PRs) to make this easier to review.

In particular, here are some logical chunks of changes I'm seeing:

Change original CLI to be an example
Introduce new rustls-cert-gen crate
Add README for rustls-cert-gen

I'm surprised cargo fmt isn't introducing empty lines between methods in all the new code, I think that's the more idiomatic style.

I also think right now the module sizes in rustls-cert-gen are so small as to make the code harder to follow. In particular, IMO the contents of the args module should just be inlined into main.rs, same for the contents of lib.rs. I'd concatenate all the modules in cert into a single file which I guess might be called cert.rs.

Personally I'm not fond of Result type aliases and I think we should just write it out, which is much more obvious when reviewing. Similarly I've found that type aliases in general are kind of annoying and it might be better to add a newtype wrapper if we want to use Box<dyn Error> as the type in the CLI crate (which otherwise certainly seems a decent choice). It might make sense to just use anyhow directly, which I think is geared for this use case directly.

tbro · 2023-11-09T16:09:29Z

IMO it would be good to clean up the commit history a bit (or split into separate PRs) to make this easier to review.

In particular, here are some logical chunks of changes I'm seeing:
* Change original CLI to be an example

* Introduce new rustls-cert-gen crate

* Add README for rustls-cert-gen

I can easily squash the history, so I'll start with that. While I generally agree that having small PRs is easier to review, I think this might be an exception to that rule. Sure, we can have a couple of tiny PRs like those you mention, but there is still going to be a huge one with all the "beef". Also, I think it is fairly common to initialize new crates with a README, so splitting that off doesn't make much sense to me. Also moving the former main.rs to examples is also very much a part of this. I'm happy to do those splits if the team really wants it, but I'll wait for more push back (push forward?).

I also think right now the module sizes in rustls-cert-gen are so small as to make the code harder to follow. In particular, IMO the contents of the args module should just be inlined into main.rs, same for the contents of lib.rs. I'd concatenate all the modules in cert into a single file which I guess might be called cert.rs.

I disagree that having a few very large files is easier to follow than having smaller dedicated modules. Possibly is related to choice of IDE? I have certainly seen both styles employed by different crates. But I am happy to conform to the preferred style of this team, which I take to be what you have outlined here.

Personally I'm not fond of Result type aliases and I think we should just write it out, which is much more obvious when reviewing. Similarly I've found that type aliases in general are kind of annoying and it might be better to add a newtype wrapper if we want to use Box<dyn Error> as the type in the CLI crate (which otherwise certainly seems a decent choice). It might make sense to just use anyhow directly, which I think is geared for this use case directly.

Given these choices, I would reach for anyhow. Does anyone disagree with this?

Add basic functionality for rustls-cert-gen This commit adds basic functionality for rustls-cerg-gen crate. A small wrapper library has been added in order to organize code into small modules and to provide a simple API that can easily be updated as new functionality is added in the future. There are some rough edges, some missing documentation for example. But I hope to get the broader structure approved before refining the documentation. Serval parameters have been added with the idea of supporting a broad range of use-cases and test expectations around design and maintainability. The easiest way to view currently supported options is with `cargo run -- --help`. Closes rustls#175

tbro · 2023-11-09T16:39:40Z

I'm surprised cargo fmt isn't introducing empty lines between methods in all the new code, I think that's the more idiomatic style.

There is blank_lines_lower_bound but that puts a new line even between every line within a method.

split off from rustls#185

Adds a README.md to `rustls-cert-gen`. It was split off from rustls#185.

This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow` has also been added as a dependency to replace the `Result` alias. Closes rustls#175

split off from rustls#185 and make some minor changes. * remove inconsistent use of `sys::fs` * remove `&` when on file writes * remove clippy declaration at top of example

This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow` has also been added as a dependency to replace the `Result` alias. Closes rustls#175 includes review fixes such as: * remove top-level rsa dependency * inline parse_san * Check for presence of EKU before pushing. * Replace `struct Signature` struct w/ `enum KeypairAlgorithm` * update some doc strings

This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow` has also been added as a dependency to replace the `Result` alias. Closes rustls#175 includes review fixes such as: * remove top-level rsa dependency * inline parse_san * Check for presence of EKU before pushing. * Replace `struct Signature` struct w/ `enum KeypairAlgorithm` * update some doc strings * make EndEntity and Ca public so they appear in the docs

This takes what was formerly `rcgen/src/main.rs` and moves it to the examples folder as `simple.rs`. It was split off from #185 Co-authored-by: tbro <[email protected]>

This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow` has also been added as a dependency to replace the `Result` alias. Closes rustls#175 includes review fixes such as: * remove top-level rsa dependency * inline parse_san * Check for presence of EKU before pushing. * Replace `struct Signature` struct w/ `enum KeypairAlgorithm` * update some doc strings * make EndEntity and Ca public so they appear in the docs

cpu · 2023-12-05T21:50:50Z

@tbro It feels like we should close this and focus on #190 - do you agree?

tbro · 2023-12-05T22:10:50Z

@tbro It feels like we should close this and focus on #190 - do you agree?

agreed 👍

This is basically rustls#185 minus rustls#188 and rustls#189. The structure also differs as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow` has also been added as a dependency to replace the `Result` alias. Closes rustls#175 includes review fixes such as: * remove top-level rsa dependency * inline parse_san * Check for presence of EKU before pushing. * Replace `struct Signature` struct w/ `enum KeypairAlgorithm` * update some doc strings * make EndEntity and Ca public so they appear in the docs * additional test cases

This is basically #185 minus #188 and #189. The structure also differs as sub modules have been inlined in `main.rs` and `cert.rs`. `anyhow` has also been added as a dependency to replace the `Result` alias. Closes #175 --------- Co-authored-by: tbro <[email protected]>

tbro changed the title ~~Rustls cerg gen support params~~ Rustls-cert-gen support basic parameters Oct 31, 2023

est31 requested changes Nov 1, 2023

View reviewed changes

est31 mentioned this pull request Nov 1, 2023

codecov bot #186

Closed

tbro requested review from djc and est31 November 2, 2023 18:30

tbro force-pushed the rustls-cerg-gen-support-params branch from faaee63 to a28ad7e Compare November 6, 2023 17:45

cpu reviewed Nov 7, 2023

View reviewed changes

tbro requested a review from cpu November 8, 2023 17:39

tbro force-pushed the rustls-cerg-gen-support-params branch from 96dd493 to 871d554 Compare November 9, 2023 16:17

tbro added a commit to tbro/rcgen that referenced this pull request Nov 13, 2023

new simple example for rcgen

334e03c

split off from rustls#185

tbro mentioned this pull request Nov 13, 2023

new simple example for rcgen #188

Merged

tbro added a commit to tbro/rcgen that referenced this pull request Nov 13, 2023

Add a README.md to rustls-cert-gen

a2657aa

Adds a README.md to `rustls-cert-gen`. It was split off from rustls#185.

tbro mentioned this pull request Nov 13, 2023

Add a README.md to rustls-cert-gen #189

Closed

tbro mentioned this pull request Nov 13, 2023

A functional rustls-cert-gen with basic parameters. #190

Merged

tbro added a commit to tbro/rcgen that referenced this pull request Nov 13, 2023

new simple example for rcgen

1f3fd1e

split off from rustls#185 and make some minor changes. * remove inconsistent use of `sys::fs` * remove `&` when on file writes * remove clippy declaration at top of example

cpu closed this Dec 5, 2023

tbro deleted the rustls-cerg-gen-support-params branch December 5, 2023 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustls-cert-gen support basic parameters #185

Rustls-cert-gen support basic parameters #185

tbro commented Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

est31 left a comment

est31 Nov 1, 2023

tbro Nov 1, 2023

djc Nov 1, 2023

est31 Nov 1, 2023

tbro Nov 2, 2023

est31 Nov 1, 2023

tbro Nov 1, 2023

cpu commented Nov 6, 2023

cpu left a comment

tbro commented Nov 8, 2023

tbro commented Nov 8, 2023

djc commented Nov 9, 2023

tbro commented Nov 9, 2023

tbro commented Nov 9, 2023

cpu commented Dec 5, 2023

tbro commented Dec 5, 2023

Rustls-cert-gen support basic parameters #185

Rustls-cert-gen support basic parameters #185

Conversation

tbro commented Oct 31, 2023 • edited Loading

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

est31 left a comment

Choose a reason for hiding this comment

est31 Nov 1, 2023

Choose a reason for hiding this comment

tbro Nov 1, 2023

Choose a reason for hiding this comment

djc Nov 1, 2023

Choose a reason for hiding this comment

est31 Nov 1, 2023

Choose a reason for hiding this comment

tbro Nov 2, 2023

Choose a reason for hiding this comment

est31 Nov 1, 2023

Choose a reason for hiding this comment

tbro Nov 1, 2023

Choose a reason for hiding this comment

cpu commented Nov 6, 2023

cpu left a comment

Choose a reason for hiding this comment

tbro commented Nov 8, 2023

tbro commented Nov 8, 2023

djc commented Nov 9, 2023

tbro commented Nov 9, 2023

tbro commented Nov 9, 2023

cpu commented Dec 5, 2023

tbro commented Dec 5, 2023

tbro commented Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading