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

tbro · 2023-11-13T20:01:04Z

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

cpu

Thanks! Here's another round of feedback

Cargo.toml

rustls-cert-gen/README.md

rustls-cert-gen/src/cert.rs

rustls-cert-gen/src/main.rs

tbro · 2023-11-14T22:39:35Z

Thanks! Here's another round of feedback

I think I have addressed everything in that round.

cpu · 2023-11-14T22:43:41Z

Thank you! I will do another pass in the next day or two.

cpu

Sorry for the delay getting this another review. I only have small nits to flag. I think there's likely good future work (like supporting a way to override the validity period), but I'm leaning towards trying to land this ASAP and iterating from there.

rustls-cert-gen/src/cert.rs

rustls-cert-gen/src/lib.rs

rustls-cert-gen/src/cert.rs

rustls-cert-gen/src/lib.rs

rustls-cert-gen/src/main.rs

tbro · 2023-12-06T15:56:08Z

Sorry for the delay getting this another review. I only have small nits to flag. I think there's likely good future work (like supporting a way to override the validity period), but I'm leaning towards trying to land this ASAP and iterating from there.

Yes I also thought of the validity periods. I'm sure there are a few more features that could be added. But my preference is also to finish this first bit. Then it should be much easier to review smaller future PRs that target specific features/enhancements.

tbro · 2023-12-06T16:25:20Z

I believe latest review comments have been addressed.

cpu

Thanks!

djc

I have a few minor nits (hope these are not contradicting any earlier feedback), this looks good to me!

djc · 2023-12-11T10:07:23Z

rcgen/Cargo.toml

 pem = { workspace = true, optional = true }
 time = { version = "0.3.6", default-features = false }
-x509-parser = { version = "0.15", features = ["verify"], optional = true }
+x509-parser = { workspace = true, features = ["verify"], optional = true }


Personally I would prefer specifying all dependency versions at the workspace level, because otherwise it's not very clear when adding a dependency where to look.

djc · 2023-12-11T10:08:46Z

rustls-cert-gen/src/cert.rs

+		params.distinguished_name = DistinguishedName::new();
+		Self { params }
+	}
+	/// Set signature algorithm (instead of default).


Please add empty lines between all the methods.

djc · 2023-12-11T10:09:34Z

rustls-cert-gen/src/cert.rs

+	}
+}
+
+/// Builder to configure TLS [CertificateParams] to be finalized


I think usually we link names with both backticks and brackets, like [`CertificateParams`].

djc · 2023-12-11T10:10:20Z

rustls-cert-gen/src/cert.rs

+		self.params.key_pair = Some(keypair);
+		Ok(self)
+	}
+	/// Set options for Ca Certificates


Should probably be [`Ca`], let's not capitalize "Certificates" here?

djc · 2023-12-11T10:12:47Z

rustls-cert-gen/src/cert.rs

+use std::{fmt, fs::File, io, path::Path};
+
+#[derive(Debug, Clone)]
+/// PEM serialized Certificate and PEM serialized corresponding private key


In terms of item ordering I think it would make more sense to keep PemCertifiedKey closer to EndEntity and Certificate, also keeping EndEntityBuilder and CaBuilder together.

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

cpu · 2023-12-14T18:59:45Z

exclude rustls-cert-gen from semver check

Why is this necessary - because there's no initial published verison? Can you add context motivating the change to the commit message?

`semver-checks` fail in CI because there is no published version of `rustls-cert-gen`. excluding it seems like the simplest fix for the moment. other workarounds can be found here: https://github.com/obi1kenobi/cargo-semver-checks#does-the-crate-im-checking-have-to-be-published-on-cratesio

tbro · 2023-12-14T19:20:32Z

exclude rustls-cert-gen from semver check

Why is this necessary - because there's no initial published verison? Can you add context motivating the change to the commit message?

Yes I'm working around CI failure due to there no published version of rustls-cert-gen. I have updated commit message as requested.

cpu · 2023-12-14T19:22:23Z

Makes sense, thank you!

…ic-functionality

Recent updates to `rcgen` resulted in an error on key generation. This address that by mapping ring's `Error::Unspecified` to `rcgen::Error::RingUnspecified`.

…ic-functionality

Specify previous versions of the dependencies to support a minimum rustc version of `1.69` and pass CI checks.

tbro · 2023-12-19T20:19:50Z

Hello everyone. Since we have 3 approvals can we go ahead and merge this? Pretty much every merge into main is causing conflicts requiring manual conflict resolution. In some cases changes to code. I have left these resolutions as separate commits above with clear messages, so they can easily be reviewed. I am happy to squash them all together given your go ahead. I fear if this PR is left sitting around the burden will become too much to keep up with. I understand that there are a few comments remaining above, but they are self-described as "minor nits". I suggest we move those to a separate issue . Feel free to assign it to me.

cpu · 2023-12-20T17:53:19Z

Hello everyone. Since we have 3 approvals can we go ahead and merge this?

That sounds good to me, I've been waiting for the branch to build green in CI again and it looks like that's been addressed. Thanks!

@djc @est31 Any reservations about merging this?

est31 · 2023-12-20T21:50:32Z

I'm fine with merging!

est31 · 2023-12-20T21:50:52Z

(and thanks @tbro for your contribution)

cpu · 2023-12-20T21:53:49Z

I'm fine with merging!

Cool, thanks for chiming in. I'm going to put it in the queue. Talking to Djc OOB I don't think he'll have a chance to look at this. If he does later we can tackle feedback in follow-up PRs.

(and thanks @tbro for your contribution)

+1!

cpu reviewed Nov 13, 2023

View reviewed changes

tbro force-pushed the rustls-cert-gen-basic-functionality branch 3 times, most recently from a8b1b9e to 34c6919 Compare November 14, 2023 22:29

tbro force-pushed the rustls-cert-gen-basic-functionality branch 2 times, most recently from 42fc2a5 to 2ab513c Compare December 4, 2023 20:13

cpu reviewed Dec 5, 2023

View reviewed changes

cpu requested a review from est31 December 5, 2023 21:49

This was referenced Dec 5, 2023

Rustls-cert-gen support basic parameters #185

Closed

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

Closed

tbro force-pushed the rustls-cert-gen-basic-functionality branch from 608ddd4 to 690fa6b Compare December 6, 2023 15:51

cpu approved these changes Dec 6, 2023

View reviewed changes

This was referenced Dec 8, 2023

Add PreSpecified(Vec<u8>) option to KeyIdMethod. #197

Merged

0.12.0 release planning #199

Closed

est31 approved these changes Dec 9, 2023

View reviewed changes

djc reviewed Dec 11, 2023

View reviewed changes

djc approved these changes Dec 11, 2023

View reviewed changes

tbro force-pushed the rustls-cert-gen-basic-functionality branch from 690fa6b to 6cb4b30 Compare December 11, 2023 17:11

tbro force-pushed the rustls-cert-gen-basic-functionality branch from de651d5 to 46efa11 Compare December 14, 2023 19:19

tbro added 2 commits December 14, 2023 13:39

Merge remote-tracking branch 'upstream/main' into rustls-cert-gen-bas…

0d5e158

…ic-functionality

fix dependencies that were updated upstream

1f69bb7

tbro added 4 commits December 14, 2023 13:51

update Cargo.lock

7413250

map ring error to rcgen::Error

61846c9

Recent updates to `rcgen` resulted in an error on key generation. This address that by mapping ring's `Error::Unspecified` to `rcgen::Error::RingUnspecified`.

Merge remote-tracking branch 'upstream/main' into rustls-cert-gen-bas…

f633504

…ic-functionality

downgrade anstyle and ignore deep dependencies

0c8dbaa

Specify previous versions of the dependencies to support a minimum rustc version of `1.69` and pass CI checks.

cpu added this pull request to the merge queue Dec 20, 2023

Merged via the queue into rustls:main with commit 6637060 Dec 20, 2023
20 checks passed

tbro deleted the rustls-cert-gen-basic-functionality branch December 21, 2023 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tbro commented Nov 13, 2023

cpu left a comment

tbro commented Nov 14, 2023

cpu commented Nov 14, 2023

cpu left a comment

tbro commented Dec 6, 2023

tbro commented Dec 6, 2023

cpu left a comment

djc left a comment

djc Dec 11, 2023

djc Dec 11, 2023

djc Dec 11, 2023

djc Dec 11, 2023

djc Dec 11, 2023

cpu commented Dec 14, 2023 •

edited

Loading

tbro commented Dec 14, 2023

cpu commented Dec 14, 2023

tbro commented Dec 19, 2023 •

edited

Loading

cpu commented Dec 20, 2023

est31 commented Dec 20, 2023

est31 commented Dec 20, 2023

cpu commented Dec 20, 2023

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

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

Conversation

tbro commented Nov 13, 2023

cpu left a comment

Choose a reason for hiding this comment

tbro commented Nov 14, 2023

cpu commented Nov 14, 2023

cpu left a comment

Choose a reason for hiding this comment

tbro commented Dec 6, 2023

tbro commented Dec 6, 2023

cpu left a comment

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

djc Dec 11, 2023

Choose a reason for hiding this comment

djc Dec 11, 2023

Choose a reason for hiding this comment

djc Dec 11, 2023

Choose a reason for hiding this comment

djc Dec 11, 2023

Choose a reason for hiding this comment

djc Dec 11, 2023

Choose a reason for hiding this comment

cpu commented Dec 14, 2023 • edited Loading

tbro commented Dec 14, 2023

cpu commented Dec 14, 2023

tbro commented Dec 19, 2023 • edited Loading

cpu commented Dec 20, 2023

est31 commented Dec 20, 2023

est31 commented Dec 20, 2023

cpu commented Dec 20, 2023

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

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

cpu commented Dec 14, 2023 •

edited

Loading

tbro commented Dec 19, 2023 •

edited

Loading