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

Use pkcs1 and pkcs8 crates; MSRV 1.51+ #104

Merged
merged 1 commit into from Jul 26, 2021
Merged

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Jul 24, 2021

Closes #75

Switches the rsa crate to use the pkcs1 and pkcs8 crates from https://github.com/rustcrypto/utils

Rationale

  • Interoperability: PKCS#8 in particular is algorithm agnostic, and the pkcs8 crate provides the following traits which can be used to abstract over the algorithm used by a particular key (NOTE: this PR does not yet impl these traits, but probably should)
  • Security: an upcoming ACM CCS paper claims to be able to extract RSA private keys from SGX using PEM parsing sidechannels alone. The new pem-rfc7468 crate implements a constant time PEM parser which leverages the base64ct crate for constant-time encoding/decoding.
  • Parsimony: by switching to crates from https://github.com/rustcrypto/utils a user of the rsa crate who is also leveraging any of RustCrypto's elliptic curve crates such as p256, p384, or k256 will have fewer overall dependencies.
  • PKCS#8 Encryption: the pkcs8 crate has support for ENCRYPTED PRIVATE KEYs using the pkcs5 crate. With this it can encrypt any PKCS#8 key under a password with scrypt-based key derivation and AES-CBC encryption (unfortunately PKCS#8 has no support for AEADs, alas)

Notes

This is the first time I've attempted to use the pkcs1 crate as it's brand new, so this integration is effectively taking it out for a test drive.

One of the key parts of format encoding/decoding with RSA when implementing PKCS#1 and PKCS#8 is that the latter is effectively a wrapper for the former in modern use. So therefore it seems like there should be a first-class integration between the pkcs1 and pkcs8 crates, namely I think the pkcs1 crate should support an optional dependency on pkcs8.

While the pkcs8 crate has some nice traits like the aforementioned FromPrivateKey/ToPrivateKey, the pkcs1 crate does not. The rsa crate defines some traits for this (e.g. PrivateKeyEncoding, PrivateKeyPemEncoding) but I really feel like the traits should get hoisted up into the pkcs1 crate.

If that were to happen, I think that the integration with the rsa crate could just be for PKCS#1 DER encoding/decoding. Concerns like PKCS#8 and PEM could be hoisted up into traits defined by the pkcs1 crate.

@tarcieri tarcieri marked this pull request as draft July 24, 2021 19:12
@tarcieri
Copy link
Member Author

tarcieri commented Jul 24, 2021

Sidebar:

I noticed the rsa crate does the following:

#![cfg_attr(not(test), no_std)]

An alternative to this is to have a separate integration tests directory i.e. tests/ ala here:

https://github.com/RustCrypto/utils/tree/master/pkcs8/tests

Integration tests are linked separately from the crate you're developing and automatically get std, even if the crate in src/ is #[no_std], so you don't need to conditionally link std for testing.

There are a few other items marked #[cfg(test)] and I think using integration tests like that helps keep the main src/lib.rs clean.

Also I think it'd be nice to standardize on a set of test vectors. The ones in the pkcs8 crate were generated by myself using recent versions of OpenSSL and I've reused them with the other crates in the utils repo as well as the https://github.com/rustcrypto/elliptic-curves repo.

Edit: split out the encoding tests into tests/ and switched to the same test vectors as the pkcs1 and pkcs8 crates.

@newpavlov
Copy link
Member

An alternative to this is to have a separate integration tests directory i.e. tests/ ala here:

It's not always possible to use integration tests, e.g. if tests are for private internals of a module. But apart from that I agree that we should prefer the tests/ folder if possible. Also a more common approach for handling optional std dependency is:

#![no_std]

#[cfg(features = "std")]
extern crate std;

We can modify it to use #[cfg(any(features = "std", test))].

src/pss.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member Author

It's not always possible to use integration tests, e.g. if tests are for private internals of a module.

Well yeah, I'd call those unit tests 😉

@tarcieri
Copy link
Member Author

Note: I'm going to add a set of traits to the pkcs1 crate for encoding/decoding keys similar to the traits in the pkcs8 crate:

RustCrypto/utils#540

From there I'd like to add an optional pkcs8 crate to the pkcs1 crate, and a blanket impl of the pkcs8 key traits for types which impl the ones from the pkcs1 crate. That will make PKCS#8 support reusable for any types which impl the PKCS#1 traits.

@tarcieri tarcieri force-pushed the use-pkcs1-and-pkcs8-crates branch 2 times, most recently from 9013ba0 to 1785ca0 Compare July 25, 2021 18:24
@tarcieri tarcieri changed the title [WIP] Use pkcs1 and pkcs8 crates [WIP] Use pkcs1 and pkcs8 crates; MSRV 1.51+ Jul 25, 2021
@tarcieri
Copy link
Member Author

Note that this is going to need an MSRV bump to 1.51, so maybe it would make sense to merge #92 first, cut another release, and then merge this after cutting the next release

@tarcieri tarcieri force-pushed the use-pkcs1-and-pkcs8-crates branch 4 times, most recently from 9ddc2ee to 35ea859 Compare July 25, 2021 20:54
@tarcieri tarcieri changed the title [WIP] Use pkcs1 and pkcs8 crates; MSRV 1.51+ Use pkcs1 and pkcs8 crates; MSRV 1.51+ Jul 25, 2021
@tarcieri tarcieri marked this pull request as ready for review July 25, 2021 21:03
@tarcieri
Copy link
Member Author

tarcieri commented Jul 25, 2021

Removing WIP/draft.

Aside from some rustdoc on how to use the traits from the pkcs1/pkcs8 crates I'd say this is good to go.

It's now extensively tested with all of the RSA vectors from the pkcs1 and pkcs8 crates, to ensure they parse correctly and round trip as both DER and PEM.

I'd suggest merging #92 (quick fix for PKCS#8) and #105 (rustfmt) first, as re: the former it doesn't need an MSRV bump and re: the latter this PR has rustfmt applied and with it some irrelevant changes it'd be nice to get taken care of in a rustfmt-specific PR. Edit: #92 and #105 are both merged.

@tarcieri tarcieri force-pushed the use-pkcs1-and-pkcs8-crates branch 10 times, most recently from 1f7e7f3 to 9ef9526 Compare July 26, 2021 00:08
@tarcieri tarcieri force-pushed the use-pkcs1-and-pkcs8-crates branch 5 times, most recently from accc4b3 to 4261a06 Compare July 26, 2021 00:42
@tarcieri
Copy link
Member Author

Calling this finished for now. I've added some preliminary documentation, although it's not quite as extensive as the documentation it's replacing. I can add more if desired.

Cargo.toml Outdated
digest = { version = "0.9.0", default-features = false }
pkcs1 = { version = "0.2.2", default-features = false }
pkcs8 = { version = "0.7.4", default-features = false }

[dependencies.zeroize]
version = "=1.3" # zeroize 1.4 is MSRV 1.51+
Copy link
Member

Choose a reason for hiding this comment

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

if we bump MSRV to 1.51, lets update this as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Bumped MSRV in 2f2a673.

I went with a less restrictive >=1, <1.5 requirement to ensure rsa can be used alongside crates which are locking =1.3. Otherwise it wouldn't be possible to use both /cc @newpavlov

src/encode.rs Show resolved Hide resolved
This was referenced Jul 26, 2021
Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

Awesome work, thank you! I love the smell of delted code and more constant time options even more.

@dignifiedquire
Copy link
Member

Released 0.4.1, so feel free to rebase & merge when you are ready.

This is an initial attempt to switch the `rsa` crate to use the `pkcs1`
and `pkcs8` crates from https://github.com/rustcrypto/utils
@tarcieri
Copy link
Member Author

tarcieri commented Jul 26, 2021

Rebased and squashed

@tarcieri tarcieri merged commit f251dee into master Jul 26, 2021
@tarcieri tarcieri deleted the use-pkcs1-and-pkcs8-crates branch July 26, 2021 20:57
@tarcieri tarcieri mentioned this pull request Jul 26, 2021
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.

pkcs8 crate
3 participants