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

ecdsa: refactor signature types into Signature + asn1::Document #98

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

tarcieri
Copy link
Member

The previous implementation had Asn1Signature and FixedSignature types which sat side-by-side as equals.

However, that's annoying, because it means we need to do various work in duplicate for both signature types.

It's also annoying because there's no "one true signature type" to reach for.

The original motivations for doing this were a few different things:

  • Initial implementation lacked transcoding necessitating the split
  • Avoids unnecessary transcoding, which could preserve ASN.1 structures

Now that we have bidirectional transcoding between the formats, and particularly one which should always serialize to "strict DER", it seems like it's probably worth it to pay the transcoding costs and "standardize" on the previous FixedSignature format as the blessed ecdsa::Signature type.

This allows factoring everything ASN.1 related (besides a few helper methods and a From-impl on Signature) into the ecdsa::asn1 module which everything else aside just feels cleaner than before.

This also means downstream ECDSA provider crates don't need to worry about ASN.1 at all (unless they're e.g. parsing it from HSM/KMS output) and can focus exclusively on the ecdsa::Signature type.

The previous implementation had `Asn1Signature` and `FixedSignature`
types which sat side-by-side as equals.

However, that's annoying, because it means we need to do various work in
duplicate for both signature types.

It's also annoying because there's no "one true signature type" to reach
for.

The original motivations for doing this were a few different things:

- Initial implementation lacked transcoding necessitating the split
- Avoids unnecessary transcoding, which could preserve ASN.1 structures

Now that we have bidirectional transcoding between the formats, and
particularly one which should always serialize to "strict DER", it
seems like it's probably worth it to pay the transcoding costs and
"standardize" on the previous `FixedSignature` format as the blessed
`ecdsa::Signature` type.

This allows factoring everything ASN.1 related (besides a few helper
methods and a From-impl on Signature) into the `ecdsa::asn1` module
which everything else aside just feels cleaner than before.

This also means downstream ECDSA provider crates don't need to worry
about ASN.1 at all (unless they're e.g. parsing it from HSM/KMS output)
and can focus exclusively on the `ecdsa::Signature` type.
@codecov-commenter
Copy link

Codecov Report

Merging #98 into master will increase coverage by 54.44%.
The diff coverage is 70.50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #98       +/-   ##
===========================================
+ Coverage    0.00%   54.44%   +54.44%     
===========================================
  Files           4        3        -1     
  Lines         179      180        +1     
===========================================
+ Hits            0       98       +98     
+ Misses        179       82       -97     
Impacted Files Coverage Δ
ecdsa/src/lib.rs 61.76% <61.76%> (ø)
ecdsa/src/asn1.rs 73.33% <73.33%> (ø)

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 4c3cf22...9d8f3c5. Read the comment docs.

@tarcieri tarcieri merged commit bc5a705 into master Jul 14, 2020
@tarcieri tarcieri deleted the ecdsa/refactor-signature-types branch July 14, 2020 01:09
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Jul 14, 2020
The equivalents of these types used to live in the `ecdsa` crate, but
were removed in this PR:

RustCrypto/signatures#96

The goal of that PR was to reverse the previous relationship where the
`ecdsa` crate depended on the `k256`/`p256`/`p384` crates, and instead
have the curve implementation crates consume the `ecdsa` crate as an
(optional) dependency.

It makes each curve implementation a one-stop-shop for everything
related to that curve, while allowing the ECDSA crate to provide some
common functionality like ASN.1 (de)serialization, in addition to
allowing it to export "primitive" traits which can be used with the
goal of a reusable high-level ECDSA implementation which is generic over
elliptic curves.

This commit ports over equivalent types that were removed in
`RustCrypto/signatures#96`, but also incorporates these changes:

RustCrypto/signatures#98

Where the `ecdsa` crate previously had `Asn1Signature` and
`FixedSignature` types generic over a curve, the PR above refactored it
to make the "fixed" form the preferred `Signature` type, and refactoring
ASN.1 DER support into an `ecdsa::asn1::Document` type.

The nice advantage of that approach is it means the curve
implementations no longer need to worry about an `Asn1Signature` type
and can focus on `ecdsa::Signature` as the type they need to support.
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Jul 14, 2020
The equivalents of these types used to live in the `ecdsa` crate, but
were removed in this PR:

RustCrypto/signatures#96

The goal of that PR was to reverse the previous relationship where the
`ecdsa` crate depended on the `k256`/`p256`/`p384` crates, and instead
have the curve implementation crates consume the `ecdsa` crate as an
(optional) dependency.

It makes each curve implementation a one-stop-shop for everything
related to that curve, while allowing the ECDSA crate to provide some
common functionality like ASN.1 (de)serialization, in addition to
allowing it to export "primitive" traits which can be used with the
goal of a reusable high-level ECDSA implementation which is generic over
elliptic curves.

This commit ports over equivalent types that were removed in
`RustCrypto/signatures#96`, but also incorporates these changes:

RustCrypto/signatures#98

Where the `ecdsa` crate previously had `Asn1Signature` and
`FixedSignature` types generic over a curve, the PR above refactored it
to make the "fixed" form the preferred `Signature` type, and refactoring
ASN.1 DER support into an `ecdsa::asn1::Document` type.

The nice advantage of that approach is it means the curve
implementations no longer need to worry about an `Asn1Signature` type
and can focus on `ecdsa::Signature` as the type they need to support.
@tarcieri tarcieri mentioned this pull request Aug 11, 2020
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