Skip to content

Conversation

@tarcieri
Copy link
Member

@tarcieri tarcieri commented Aug 7, 2020

Implements the changes made in RustCrypto/signatures#120.

These changes have the k256 always compute a recoverable::Signature via RecoverableSignPrimitive and use the blanket impl of SignPrimitive for RecoverableSignPrimitive to convert it to a regular k256::ecdsa::Signature in ecdsa::signer::Signer<Secp256k1>.

Ideally the k256::ecdsa::Signer newtype could go away and this could all be handled by ecdsa::signer::Signer via blanket and conditional impls, however I'm not sure how to prevent the impls of RandomizedSigner from overlapping in a generic context.

Implements the changes made in RustCrypto/signatures#120.

These changes have the `k256` always compute a `recoverable::Signature`
via `RecoverableSignPrimitive` and use the blanket impl of
`SignPrimitive` for `RecoverableSignPrimitive` to convert it to a
regular `k256::ecdsa::Signature` in `ecdsa::signer::Signer<Secp256k1>`.

Ideally the `k256::ecdsa::Signer` newtype could go away and this could
all be handled by `ecdsa::signer::Signer` via blanket and conditional
impls, however I'm not sure how to prevent the impls of
`RandomizedSigner` from overlapping in a generic context.
@tarcieri tarcieri requested a review from fjarri August 7, 2020 22:42
@tarcieri
Copy link
Member Author

tarcieri commented Aug 7, 2020

I tried moving this into the ecdsa crate, but as noted in the description, to prevent the two RandomizedSigner impls from overlapping, I'd need to ensure RecoverableSignPrimitive<C>::RecoverableSignature is not the same type as ecdsa::Signature<C>.

There are various ways to solve this. One potential approach is to use two different signer types with traits to convert between them. It'd also be possible using hacks like typenum uses but I'd prefer to avoid that (at least until it's natively supported by Rust).

@codecov-commenter
Copy link

Codecov Report

Merging #123 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #123   +/-   ##
=======================================
  Coverage   51.57%   51.57%           
=======================================
  Files          21       21           
  Lines        3488     3484    -4     
=======================================
- Hits         1799     1797    -2     
+ Misses       1689     1687    -2     
Impacted Files Coverage Δ
k256/src/ecdsa/normalize.rs 0.00% <0.00%> (ø)
k256/src/ecdsa/signer.rs 0.00% <0.00%> (ø)
k256/src/arithmetic/scalar/scalar_4x64.rs 75.82% <0.00%> (-0.74%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac9c80e...322c5d8. Read the comment docs.

(*self, false)
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: this PR incorporates #122 (although I needed to deref in the else clause).

@tarcieri
Copy link
Member Author

tarcieri commented Aug 7, 2020

Going to merge this as I believe it addresses the main problem we've been discussing in #119.

I think there are a lot of other ways to potentially go with this, either possibly hoisting the whole thing up into the ecdsa crate somehow, or if that isn't feasible, getting rid of RecoverableSignPrimitive and just having a private function at the core of both implementations which returns the recoverable::Id (and discards it in SignPrimitive). But for now, I consider this progress.

@tarcieri tarcieri merged commit 86845cf into master Aug 7, 2020
@tarcieri tarcieri deleted the k256/new-ecdsa-randomized-sign-primitive-impl branch August 7, 2020 23:22
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