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

PKCS#8 support #224

Merged
merged 1 commit into from
Dec 13, 2022
Merged

PKCS#8 support #224

merged 1 commit into from
Dec 13, 2022

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Nov 20, 2022

NOTE: depends on #222 and #223 which need to be merged first.

Adds optional integration with ed25519::pkcs8 with support for decoding/encoding Keypair from/to PKCS#8-encoded documents.

TODO: support for decoding/encoding PublicKey from/to SPKI documents. Added, good to go!

@tarcieri tarcieri force-pushed the pkcs8 branch 3 times, most recently from 6297bc1 to fff3b56 Compare November 21, 2022 15:03
@tarcieri
Copy link
Contributor Author

This is feature-complete now but still needs docs

@tarcieri tarcieri force-pushed the pkcs8 branch 2 times, most recently from 4f264f9 to a0d1bdc Compare November 25, 2022 20:59
@tarcieri tarcieri changed the title [WIP] PKCS#8 support PKCS#8 support Nov 25, 2022
@tarcieri tarcieri marked this pull request as ready for review November 25, 2022 21:00
@tarcieri tarcieri requested a review from rozbb November 25, 2022 21:00
@tarcieri
Copy link
Contributor Author

This should hopefully be good to go now.

pkcs8 has an MSRV of 1.57, so I bumped to that unilaterally for now.

Cargo.toml Outdated Show resolved Hide resolved
@tarcieri tarcieri changed the base branch from main to release/2.0 November 25, 2022 21:07
Copy link
Contributor

@rozbb rozbb left a comment

Choose a reason for hiding this comment

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

Looks great! Left some small comments

src/keypair.rs Outdated Show resolved Hide resolved
src/keypair.rs Show resolved Hide resolved
@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 7, 2022

Had to rebase so I guess I can go ahead and squash

@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 7, 2022

@rozbb this is green again

@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 8, 2022

Re: 5eab15d, IMO having extern crate anywhere than the toplevel lib.rs is a bit weird since it's a crate global linking directive.

I swear there used to be a warning/lint for it but perhaps I'm mistaken

@tarcieri tarcieri force-pushed the pkcs8 branch 3 times, most recently from 7693dfb to 1a6f50a Compare December 10, 2022 17:51
@tarcieri
Copy link
Contributor Author

Ran rustfmt over it and the only problems were with the newly-added PKCS#8 code, so this is also rustfmt clean.

@rozbb ready for re-review

Copy link
Contributor

@rozbb rozbb left a comment

Choose a reason for hiding this comment

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

Left minor comments. Also since dalek-cryptography/curve25519-dalek#470 merged, this line needs to be changed to use Scalar::ZERO:

while s_candidate == Scalar::zero() {

EDIT: Fixed

src/keypair.rs Show resolved Hide resolved
src/public.rs Show resolved Hide resolved
@pinkforest
Copy link
Contributor

pinkforest commented Dec 13, 2022

@rozbb - Fixed current tests failing here in #239

@rozbb
Copy link
Contributor

rozbb commented Dec 13, 2022

I thought I was going crazy. I have pages and pages of errors when testing locally. Thankfully this shows up in the CI.

Probably something related to the warning:

warning: Patch `ed25519 v2.0.0-pre.1 (https://github.com/RustCrypto/signatures.git#fd78bf79)`
    was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

@pinkforest
Copy link
Contributor

pinkforest commented Dec 13, 2022

I can confirm that most of the build errors go away by bumping to .1 as I've .pathc'd

EDIT: Also curve25519-dalek [dependency] needs to be patched to .pre.3 - updated .patch: #224 (comment)

Just leaves one batch related error after:

error[E0432]: unresolved import `rand::thread_rng`
  --> src/batch.rs:28:5
   |
28 | use rand::thread_rng;
   |     ^^^^^^^^^^^^^^^^ no `thread_rng` in the root

Which is fixed in

Cargo.toml Outdated

[patch.crates-io]
curve25519-dalek = { git = "https://github.com/dalek-cryptography/curve25519-dalek.git", branch = "release/4.0" }
ed25519 = { git = "https://github.com/RustCrypto/signatures.git"}
ed25519 = { git = "https://github.com/RustCrypto/signatures.git" }
Copy link
Contributor

@pinkforest pinkforest Dec 13, 2022

Choose a reason for hiding this comment

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

Also @rozbb the curve25519-dalek [dependencies] has to be .patched as well to pre.3 so:

diff --git a/Cargo.toml b/Cargo.toml
index f87f430..971328e 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -24,8 +24,8 @@ rustdoc-args = ["--cfg", "docsrs"]
 features = ["nightly", "batch", "pkcs8"]
 
 [dependencies]
-curve25519-dalek = { version = "=4.0.0-pre.2", default-features = false, features = ["digest", "rand_core"] }
-ed25519 = { version = "=2.0.0-pre.0", default-features = false }
+curve25519-dalek = { version = "=4.0.0-pre.3", default-features = false, features = ["digest", "rand_core"] }
+ed25519 = { version = "=2.0.0-pre.1", default-features = false }
 merlin = { version = "3", default-features = false, optional = true }
 rand = { version = "0.8", default-features = false, optional = true }
 rand_core = { version = "0.6", default-features = false, optional = true }

Problem with [patch.crates-io] here was that it needs to match the .patched manifest version exactly. if the patch target e.g. git branch Cargo.toml does not match the asked depdendency - it will give out that warning and does nothing leading to that error - unless intended as eventual "patch kill switch"

In this case release/4.0 curve25519-dalek and ed25519 repo branch Cargo.toml's got updated and then it didn't match the asked version and rendered .patch void that was asking pre.0 for ed25519 and pre.2 for curve25519.

This is why it is also best to avoid cargo publish anything with [patch.crates-io] that mutable target git ref without locking to immutable branch / commit ref. Usually it's best to just override dependency to directly target a git instead if this is required as .patch only gives a warning (sometimes this is feasible but rarely) instead of error.

@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 13, 2022

@pinkforest [patch.crates-io] works just fine if Cargo.lock is checked in, which stores the upstream git SHA1 hash that was used at the time, and won't update it unless cargo update [-p <package>] was used explicitly.

I think the advice not to check in Cargo.lock for libraries is a bit dated now that dependabot exists, and without it builds aren't reproducible or bisectable.

@tarcieri tarcieri force-pushed the pkcs8 branch 2 times, most recently from 9ac0fbb to 2595abd Compare December 13, 2022 13:32
Adds optional integration with `ed25519::pkcs8` with support for
decoding/encoding `Keypair` from/to PKCS#8-encoded documents as well as
`PublicKey` from/to SPKI-encoded documents.

Includes test vectors generated for the `ed25519` crate from:

https://github.com/RustCrypto/signatures/tree/master/ed25519/tests/examples

[badges]
travis-ci = { repository = "dalek-cryptography/ed25519-dalek", branch = "master"}

[package.metadata.docs.rs]
# Disabled for now since this is borked; tracking https://github.com/rust-lang/docs.rs/issues/302
# rustdoc-args = ["--html-in-header", ".cargo/registry/src/github.com-1ecc6299db9ec823/curve25519-dalek-0.13.2/rustdoc-include-katex-header.html"]
features = ["nightly", "batch"]
rustdoc-args = ["--cfg", "docsrs"]
features = ["nightly", "batch", "pkcs8"]
Copy link
Contributor

Choose a reason for hiding this comment

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

nightly doesn't exist anymore, but we can get that in another PR

@rozbb rozbb merged commit 55620dc into release/2.0 Dec 13, 2022
@tarcieri tarcieri deleted the pkcs8 branch January 5, 2023 20:09
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.

3 participants