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: implement Signer and Verifier using SHA-256 as default #559

Merged
merged 1 commit into from
Oct 29, 2022

Conversation

cobratbq
Copy link
Contributor

In follow-up of discussion in #520: a minimal implementation for Signer and Verifier using SHA-256 for digests. The issue discusses possible options of introducing the OID, however this is not part of this implementation.

@cobratbq cobratbq force-pushed the implement-signer-verifier branch from 213426d to 2549009 Compare October 27, 2022 15:14
@cobratbq cobratbq marked this pull request as ready for review October 27, 2022 15:23
@lumag
Copy link
Contributor

lumag commented Oct 27, 2022

I suppose that it might be better to add a <D: Digest> generic arg.

@cobratbq
Copy link
Contributor Author

I suppose that it might be better to add a <D: Digest> generic arg.

You are right. I investigated this earlier. There are cascading effects involved, due to currently the generic type being completely absent. I do not want to go there now for its structural changes. It involves some deriving of hash functions from component lengths, for the cases of reading signatures from byte-sequences.

Regardless, those changes are orthogonal in nature, so they need not block this contribution if you are interested.

Incidentally, how do you cope with backwards-incompatible changes? Such as those discussed in #520, and as you mentioned just now.

@tarcieri
Copy link
Member

Incidentally, how do you cope with backwards-incompatible changes? Such as those discussed in #520, and as you mentioned just now.

We can bump the minor version, as the crate is still 0.x

That said, you should be able to add a generic digest type parameter with a default of e.g. sha2::Sha256 in a backwards-compatible way.

@cobratbq
Copy link
Contributor Author

@tarcieri

That said, you should be able to add a generic digest type parameter with a default of e.g. sha2::Sha256 in a backwards-compatible way.

Just checking: you are referring to the use of Sha256 as digest D for DigestSigner etc. right? (That is, not for deterministically signing the prehash according to RFC 6979.)

@tarcieri
Copy link
Member

@cobratbq that would be for controlling the digest function used to prehash the message, not for RFC6979 specifically (which is an opaque, internal detail from a user-facing perspective)

@lumag
Copy link
Contributor

lumag commented Oct 28, 2022

@cobratbq I was thinking about Signer and Verifier rather than DigestSigner / DigestVerifier.

@cobratbq
Copy link
Contributor Author

I do not get how you see this being done: Signer and Verifier effectively rely on DigestSigner and DigestVerifier a-like logic.

  • If the generic is introduced in Signature (which I think you do not suggest) then it impacts every aspect of the Signature impl.
  • If I modify Signer and Verifier themselves, I would need to update signature which is at version 1.6.4.

@tarcieri
Copy link
Member

tarcieri commented Oct 28, 2022

@cobratbq again, you an add a D: Digest parameter to the SigningKey/VerifyingKey types (not any of the traits)

@lumag
Copy link
Contributor

lumag commented Oct 28, 2022

@cobratbq I took the liberty and sketched a quick & dirty implementation at #560.

@lumag
Copy link
Contributor

lumag commented Oct 28, 2022

Ok, and as we can see, the quick-and-dirty doesn't seem to work.

@tarcieri tarcieri changed the title dsa: implement Signer and Verifier using SHA-256 as default (#520) dsa: implement Signer and Verifier using SHA-256 as default Oct 29, 2022
@tarcieri tarcieri changed the title dsa: implement Signer and Verifier using SHA-256 as default dsa: implement Signer and Verifier using SHA-256 as default Oct 29, 2022
@tarcieri tarcieri merged commit 4ed617c into RustCrypto:master Oct 29, 2022
@tarcieri
Copy link
Member

This is fine for now.

I'm going to release this, and then work on the proposed changes for signature v2.0. See: RustCrypto/traits#1141

@cobratbq if you'd like to propose any breaking changes to the traits, this will be the only opportunity in the immediate future to do so.

@tarcieri tarcieri mentioned this pull request Oct 29, 2022
@cobratbq cobratbq deleted the implement-signer-verifier branch November 1, 2022 15:43
@cobratbq
Copy link
Contributor Author

cobratbq commented Nov 1, 2022

@tarcieri

@cobratbq if you'd like to propose any breaking changes to the traits, this will be the only opportunity in the immediate future to do so.

I don't have anything meaningful right now. My exploration brought me into type-safety-complications, and -- as you know -- these things can be solved in a variety of ways.

One thing to point out for consideration: the ECDSA Signature type have the Digest as generic type for the signature, while you also discussed doing this differently. Is this something you need to standardize? (I don't feel confident enough to propose something myself. However, changing the approach means changing all the ..Signer and ..Verifier interfaces, so now is the time to reconsider.)

@tarcieri
Copy link
Member

tarcieri commented Nov 1, 2022

The ecdsa crate pretty much exclusively relies on PrehashSignature::Digest for the digest type. This keeps the digest pretty much completely out of the way.

What we might consider is redesigning DigestSigner/DigestVerifier, especially now that PrehashSigner/PrehashVerifier are available.

It'd be good to open an issue with the problems you'd like to see solved.

@tarcieri
Copy link
Member

tarcieri commented Nov 2, 2022

@cobratbq ideally what I think would be nice as a pattern for tying the digest algorithm to the signature at the type level is leveraging const generics ala something like:

pub struct Signature<const DIGEST_OID: ObjectIdentifier = sha2::Sha256::OID> { ... }

This allows specifying what algorithm should be used without tying it to a concrete implementation, which makes it possible to plug in things like wrappers for hardware cryptographic accelerators so long as they claim to support the correct OID for a particular algorithm.

You can then bound on e.g. D: Digest + AssociatedOid<OID = sha2::Sha256::OID>

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