[aumetra]

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]

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]

@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]

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]

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]

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]

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.

[tarcieri]

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)

[tarcieri]

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

[tarcieri]

Nit: would suggest naming this file public_key.rs

[tarcieri]

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

[tarcieri]

Thank you!