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

dsa: Add initial DSA implementation #471

Merged
merged 29 commits into from
May 16, 2022
Merged

Conversation

aumetra
Copy link
Contributor

@aumetra aumetra commented May 9, 2022

This PR adds an initial implementation of DSA (see #8)

The following things work when tested against OpenSSL:

  • The generated keys are valid and can be imported and exported from/to their DER/PEM representation
  • Signatures generated by this library can be successfully verified
  • Signatures can be imported and exported from/into their DER representation
  • Signatures generated by OpenSSL can be successfully imported and verified

Things that need to be looked at:

  • Additional test coverage (especially verification of OpenSSL generated signatures (edit: added with commit 40e7fd2))
  • Code structure (is the current file structure fine or should it be changed?)
  • signature compatibility (the crate itself isn't deeply integrated with signature and only offers compatibility wrappers (see the compat.rs file) Should those traits be integrated more directly?) (edit: The signature crate has been integrated more tightly with commit 7123e0f)

@tarcieri
Copy link
Member

tarcieri commented May 9, 2022

Thank you for contributing this!

I'll try to do a more thorough review when I have some time. Just glancing through quickly I don't see any immediate concerns.

@aumetra
Copy link
Contributor Author

aumetra commented May 10, 2022

@tarcieri No problem, take your time!

Out of interest, would it be okay to use one of the signature preview APIs? In particular RandomizedDigestSigner.
This trait would fit pretty well for the current implementation and would allow us to get rid of most compatibility wrappers

@tarcieri
Copy link
Member

Yes, it's fine to use the digest feature of signature. Just make sure to lock to the minor version.

The ecdsa crate also does this if you'd like to use it as an example.

@tarcieri
Copy link
Member

One thing I thought I might mention before I can do a more thorough review is RFC6979 deterministic signature support.

The rfc6979 crate is designed with the intention of being potentially applicable to DSA as well as ECDSA, although some breaking changes might be needed. Fortunately, it's not a part of the ecdsa crate's public API so we're free to make such breaking changes, it's just the ecdsa crate will need to be updated too.

HmacDrbg is part of its public API so you're also free to just use that if not the generate_k function which is presently coupled to crypto-bigint.

@aumetra
Copy link
Contributor Author

aumetra commented May 11, 2022

The test for the deterministic signatures is pretty messy and doesn't include all test vectors but should be good enough for a start
(I definitely plan on revisiting and cleaning this up later)

@tarcieri
Copy link
Member

A few more notes...

You appear to be running into one of the problems with the current signature trait, which is the lack of a separation between signature types and their encodings (i.e. the AsRef<[u8]> bound).

There's some discussion of that on this tracking issue: RustCrypto/traits#237

Unfortunately ASN.1 DER-encoded signatures are one of the reasons that the AsRef<[u8]> bound exists: to support variable-sized signatures. And beyond that, the proposed solution in that issue doesn't actually provide any solution for that case. It may require splitting things into FixedSignature versus VariableWidthSignature traits, or something to that effect.

The way both the ecdsa crate and ed25519-dalek handle this is by defining methods to parse the signature from bytes into higher-level types, while keeping the signature itself represented as bytes.

ecdsa::der::Signature determines the range within an ASN.1 document where the r and s components are located, and can parse them into byte slices.

ed25519-dalek has an InternalSignature type similar to the one you have in this PR to represent the parsed form, but the external form is just the "bag of bytes" representation.

Anyway, all that said this is a use case to definitely keep in mind when thinking about a hypothetical signature v2.0.

dsa/README.md Outdated Show resolved Hide resolved
dsa/tests/privatekey.rs Outdated Show resolved Hide resolved
@aumetra aumetra marked this pull request as ready for review May 15, 2022 15:22
@aumetra aumetra changed the title WIP: dsa: Add initial DSA implementation dsa: Add initial DSA implementation May 15, 2022
dsa/src/lib.rs Outdated
Comment on lines 2 to 3
#![forbid(missing_docs, unsafe_code)]
#![deny(rust_2018_idioms)]
Copy link
Member

Choose a reason for hiding this comment

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

We generally use warn rather than deny, and enforce code is warning-free in CI

Suggested change
#![forbid(missing_docs, unsafe_code)]
#![deny(rust_2018_idioms)]
#![forbid(unsafe_code)]
#![warn(missing_docs, rust_2018_idioms)]

(unsafe_code is the one exception in crates where we don't want any)

@@ -0,0 +1,200 @@
//!
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we generally call these modules private_key.rs which follows a camel case -> snake case conversion

@@ -0,0 +1,145 @@
//!
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would suggest naming this file public_key.rs

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

This is looking pretty close to an MVP. Just a few nits.

@aumetra aumetra requested a review from tarcieri May 16, 2022 17:41
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Thank you!

@tarcieri tarcieri merged commit 373e4fe into RustCrypto:master May 16, 2022
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.

2 participants