Use pki_types to improve the interoperability with the rustls ecosystem by Tudyx · Pull Request #223 · rustls/rcgen

Tudyx · 2024-02-11T12:41:46Z

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 · 2024-02-12T10:56:17Z

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 · 2024-02-12T21:25:52Z

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 · 2024-02-12T21:33:21Z

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 · 2024-02-12T21:50:34Z

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.

Cargo.toml

rcgen/Cargo.toml

rcgen/examples/rsa-irc.rs

rcgen/src/key_pair.rs

rcgen/src/lib.rs

rcgen/tests/botan.rs

rcgen/tests/webpki.rs

cpu · 2024-02-12T22:33:50Z

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 · 2024-02-13T07:47:48Z

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

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

rcgen/src/key_pair.rs

rcgen/src/lib.rs

rcgen/src/key_pair.rs

cpu · 2024-02-13T15:37:08Z

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

pki-types v1.3.0 is now available.

cpu · 2024-02-26T16:10:36Z

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

est31 · 2024-02-27T00:21:51Z

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.

rcgen/src/key_pair.rs

rcgen/src/csr.rs

rcgen/src/key_pair.rs

rcgen/src/lib.rs

djc · 2024-02-28T13:28:32Z

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 · 2024-03-02T01:05:12Z

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 · 2024-03-13T18:10:46Z

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

est31

good except the nits

rcgen/src/certificate.rs

rcgen/src/csr.rs

rcgen/src/key_pair.rs

djc · 2024-03-14T08:03:41Z

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

cpu · 2024-03-14T13:22:16Z

Nice work @Tudyx 👏

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch from c85822e to 4e3028a Compare February 12, 2024 19:41

Tudyx mentioned this pull request Feb 12, 2024

Add CertificateSigningRequestDer type rustls/pki-types#33

Merged

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch from 4e3028a to 782ac0f Compare February 12, 2024 21:49

cpu approved these changes Feb 12, 2024

View reviewed changes

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch from 782ac0f to 8d9acc6 Compare February 12, 2024 22:26

cpu approved these changes Feb 12, 2024

View reviewed changes

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch 2 times, most recently from f24d833 to adf9b31 Compare February 13, 2024 07:31

djc reviewed Feb 13, 2024

View reviewed changes

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

rcgen/src/lib.rs Outdated Show resolved Hide resolved

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch from adf9b31 to a172806 Compare February 13, 2024 11:36

djc approved these changes Feb 13, 2024

View reviewed changes

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

cpu requested a review from est31 February 13, 2024 15:37

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch 4 times, most recently from 78aa5d4 to 8d762ed Compare February 14, 2024 18:28

Tudyx mentioned this pull request Feb 14, 2024

add certificate signing request rustls/pemfile#37

Merged

cpu mentioned this pull request Feb 21, 2024

Support CryptoProvider like plugin mechanism #228

Open

est31 requested changes Feb 27, 2024

View reviewed changes

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch 2 times, most recently from db6b565 to 9daeee2 Compare February 28, 2024 12:11

djc mentioned this pull request Mar 7, 2024

0.13.0 release planning #236

Closed

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch from 9daeee2 to cc39754 Compare March 12, 2024 07:52

djc mentioned this pull request Mar 13, 2024

Support more private key formats when using aws_lc_rs feature #242

Merged

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch from cc39754 to 0c5da82 Compare March 13, 2024 18:09

est31 approved these changes Mar 14, 2024

View reviewed changes

rcgen/src/certificate.rs Outdated Show resolved Hide resolved

rcgen/src/csr.rs Outdated Show resolved Hide resolved

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

rcgen/src/key_pair.rs Outdated Show resolved Hide resolved

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch 2 times, most recently from 67833ef to 4ed17c7 Compare March 14, 2024 07:24

use pki types to represent der encoded struct

657c615

Tudyx force-pushed the faciliate_interoperability_with_the_rustls_ecosystem branch from 4ed17c7 to 657c615 Compare March 14, 2024 07:36

djc added this pull request to the merge queue Mar 14, 2024

Merged via the queue into rustls:main with commit 02de032 Mar 14, 2024

djc mentioned this pull request Mar 15, 2024

Follow-up from pki-types conversion #246

Merged

Conversation

Tudyx commented Feb 11, 2024

Uh oh!

djc commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tudyx commented Feb 12, 2024

Uh oh!

cpu commented Feb 12, 2024

Uh oh!

Tudyx commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpu commented Feb 12, 2024

Uh oh!

Tudyx commented Feb 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpu commented Feb 13, 2024

Uh oh!

cpu commented Feb 26, 2024

Uh oh!

est31 commented Feb 27, 2024

Uh oh!

est31 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc commented Feb 28, 2024

Uh oh!

est31 commented Mar 2, 2024

Uh oh!

Tudyx commented Mar 13, 2024

Uh oh!

est31 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc commented Mar 14, 2024

Uh oh!

cpu commented Mar 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

djc commented Feb 12, 2024 •

edited

Loading

Tudyx commented Feb 12, 2024 •

edited

Loading