Use pki_types to improve the interoperability with the rustls ecosystem

[Tudyx]

Use pki_types to improve the interoperability with the rustls ecosystem and make types pretty clear, as suggested by #170 (comment).

To fully get rid of the pem dependency (by replacing it by rustls_pemfile), I think those further actions will be needed:

• add CertificateSigningRequestDer to pki_types
• add a Csr variant to rustls_pemfile::Item which is a newtype of CertificateSigningRequestDer
• add some encoding capability to rustls_pemfile, basically it would mean adding headers ("---- BEGIN ----") and handle well line ending.

Please let me know if I miss something and if you think this goes in the right direction.
I'm open to submit following PR for this.

[djc]

Thanks for working on this! I would propose doing a smaller increment which preserves the use of the pem crate for parsing (but still uses the pki-types for DER-containing types in the public API).

I also think it would be okay to add CertificateSigningRequestDer to pki-types, do you want to submit a PR for that?

What do you think?

[Tudyx]

Yes sure, let's do this step by step!
I've kept the dependency to the pem crate, and submit a PR to pki_types to add the CertificateSigningRequestDer type (rustls/pki-types#33)

[cpu]

ci / Validate external types appearing in public API (pull_request) Failing after 1m

To fix this task you'll need to update the cargo-check-external-types metadata to add rustls_pki_types to the allow-list:

rcgen/rcgen/Cargo.toml

Lines 47 to 51 in 747b5d8

	[package.metadata.cargo_check_external_types]
	allowed_external_types = [
	"time::offset_date_time::OffsetDateTime",
	"zeroize::Zeroize"
	]

The intent of pki-types is to be a stable API and so it's also ignored for this task in the other various rustls repos.

[Tudyx]

To fix this task you'll need to update the cargo-check-external-types metadata to add rustls_pki_types to the allow-list:

I see, thank you for the tip!

[cpu]

Thanks for picking this up. It'll be nice to get this in with the other breaking changes in main before a release 👍

I flagged some nits but the overall meat of the work seems great as-is.

[cpu]

We're going to cut a pki-types release with the CSR type: rustls/pki-types#34 If you want to take a Cargo patch and switch this branch over we can probably have that release published before this PR is done with review saving a follow-up for that.

[Tudyx]

I've used a patch while waiting for the release 👍

I wonder if CeritificateSigningRequest::from_der should take ownership or a reference to CeritificateSigningRequestDer

[cpu]

I've used a patch while waiting for the release 👍

pki-types v1.3.0 is now available.

[cpu]

@est31 Do you think you'll have bandwidth to review this soon? Should we continue to hold merging?

[est31]

It needs a rebase, right?

[est31]

I've always felt down when using these special types with crates like ring for example (the rust crypto ecosystem also uses them heavily), because you have to import an additional type to use the API, and maybe even an additional crate. Actually you don't but you need to first look up if there is a from impl, etc. You also need to figure out how to convert it into bytes again in the output. This makes interaction with such APIs way more expensive for developers.

At the least, we should make this interaction very clear in the docs, making sure that users know how to use rcgen without using the interop crate. I've pointed out all the locations in the comments.

It might indeed improve interopability with other libraries/crates that use these types, but I think that most users just want to convert from byte slices and back, because they get something from a network, or a file, or the other way round. So while improving the interopability with a minority of use cases, it hurts the interopability with the majority (or at least I believe). Vec and slices have conversions to the Bytes type for example, but I couldn't find anything about Bytes in the docs of PrivatePkcs8KeyDer. So you first have to convert the Bytes to a Vec or a slice and then to a PrivatePkcs8KeyDer.

The main advantage I see in DER specific types would be that it could take care of conversion to PEM, you'd just call a function .pem() on it. But when I look at the rustdocs of PrivatePkcs8KeyDer I don't see any explanation how to convert it to pem, nor is there an easy pem() that returns a string (or another wrapper).

Such types do have advantages as they carry more meaning than the byte slice type, and I'm definitely not advocating for adopting C where everything is a void* but one should be extremely careful to not stand in the way of users, and stuff like I/O is usually just byte slices.

[djc]

I've always felt down when using these special types with crates like ring for example (the rust crypto ecosystem also uses them heavily), because you have to import an additional type to use the API, and maybe even an additional crate. Actually you don't but you need to first look up if there is a from impl, etc. You also need to figure out how to convert it into bytes again in the output. This makes interaction with such APIs way more expensive for developers.

I do sympathize with these concerns, however in this case:

• I think it substantially helps users of different experience levels to be clear about PEM vs DER. Although rcgen already does a great job of making this clear in the function names, I think the type-level distinctions can help.
• It is very easy to convert a &[u8] or Vec<u8> to one of these new types.

(I myself recently got confused and passed DER into an API that expected a Vec<u8> containing PEM. This was an SDK wrapping tonic, which does clearly mark the underlying fields as PEM.)

The main advantage I see in DER specific types would be that it could take care of conversion to PEM, you'd just call a function .pem() on it. But when I look at the rustdocs of PrivatePkcs8KeyDer I don't see any explanation how to convert it to pem, nor is there an easy pem() that returns a string (or another wrapper).

Yes, there is no PEM encoding code in the rustls project. I think we might want to change that going forward, and it would definitely make the case for this stronger.

[est31]

I myself recently got confused and passed DER into an API that expected a Vec<u8> containing PEM. This was an SDK wrapping tonic, which does clearly mark the underlying fields as PEM.

Fair point, and I personally also had some confusing experiences where one Rust crypto library provided a key in one format and the other library was reading the key in the other format. In these instances, the chosen format has always been underdocumented (it's already over a year ago and I don't remember the exact details any more).

I think we can eventually merge this PR, if it documents the new API well, and by that I mean at each location where the new types are being used.

You are right that it's very easy to convert these types to/from byte buffers but it needs to be documented how, one shouldn't need the docs of the interface crate for that.

[Tudyx]

I've rebased on the latest main and add some comments as suggested in the review.

[est31]

good except the nits

[djc]

Thanks for working on this, I'll just merge this now and might follow up with a PR with some nits.

[cpu]

Nice work @Tudyx 👏