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

signature v2.0.0-pre #1141

Merged
merged 3 commits into from
Oct 29, 2022
Merged

signature v2.0.0-pre #1141

merged 3 commits into from
Oct 29, 2022

Conversation

tarcieri
Copy link
Member

This commit contains the first breaking changes to the signature crate made since its initial 1.0 stabilization. Planning for these changes occurred in #237.

The following changes have been made:

  1. The Signature trait has been renamed to SignatureEncoding. The Signature bound on *Signer and *Verifier traits has been removed, meaning there are no longer any mandatory trait bounds on the signature paramters at all.
  2. The AsRef<[u8]> bound formerly found on the Signature crate has been replaced with an associated Repr type on the new SignatureEncoding trait, inspired by the group::GroupEncoding trait. This means signature types no longer need to retain a serialized form, and can parse the bag-of-bytes representation to something more convenient, which is useful for e.g. batch verification.
  3. The std feature is no longer enabled by default, which matches the other RustCrypto/traits crates.
  4. The derive-preview and hazmat-preview features have been stabilized.
  5. The former Keypair trait has been renamed to KeypairRef, and the signature generic parameter removed. A new Keypair trait has been added which returns an owned instance of the associated VerifyingKey, and a blanket impl of Keypair for KeypairRef has been added. This addresses the issues described in signature: Keypair design problems #1124.

@tarcieri
Copy link
Member Author

cc @lumag @npmccallum @rozbb

This commit contains the first breaking changes to the `signature` crate
made since its initial 1.0 stabilization. Planning for these changes
occurred in #237.

The following changes have been made:

- The `Signature` trait has been renamed to `SignatureEncoding`. The
  `Signature` bound on `*Signer` and `*Verifier` traits has been
  removed, meaning there are no longer any mandatory trait bounds on the
  signature paramters at all.

- The `AsRef<[u8]>` bound formerly found on the `Signature` crate has
  been replaced with an associated `Repr` type, inspired by the
  `group::GroupEncoding` trait. This means signature types no longer
  need to retain a serialized form, and can parse the bag-of-bytes
  representation to something more convenient, which is useful for e.g.
  batch verification.

- The `std` feature is no longer enabled by default, which matches the
  other RustCrypto/traits crates.

- The `derive-preview` and `hazmat-preview` features have been
  stabilized.

- The former `Keypair` trait has been renamed to `KeypairRef`, and the
  signature generic parameter removed. A new `Keypair` trait has been
  added which returns an owned instance of the associated
  `VerifyingKey`, and a blanket impl of `Keypair` for `KeypairRef` has
  been added. This addresses the issues described in #1124.
@tarcieri tarcieri force-pushed the signature/2.0.0-pre branch from 73e4ef5 to 2c1b3bd Compare October 24, 2022 03:59
@npmccallum
Copy link

@tarcieri What about the problem of owned vs unowned Signatures? I really want zero-copy validation.

@tarcieri
Copy link
Member Author

@npmccallum removing the AsRef<[u8]> bound allows structured internal types for signatures. In the case of the rsa crate, that could be a num_bigint_dig::BigUint, which is needed to perform the modpow operation needed to verify the signature.

So this could eliminate the intermediate Vec which is currently used, allowing a straight path from a byte slice to a BigUint

@lumag
Copy link
Contributor

lumag commented Oct 24, 2022

I don't quite follow the design behind removing the Signature trait. Is it expected now that Signer/Verifier can work with any signature type? Or are we expected to define additional per signature-type traits and use such traits for Signers/Verifiers?
If the signature has several different representations, are we expected to implement several SignatureEncoding implementations for a single Signature?

Please excuse me if these questions sound stupid. I just can't grok the expected model.

@tarcieri
Copy link
Member Author

tarcieri commented Oct 24, 2022

@lumag I didn't spell out the exact rationale for that, so thanks for pointing it out.

The main argument for getting rid of it is simplifying the API and permitting more possible combinations, for example it becomes possible to use foreign types which don't necessarily implement the signature crate's traits.

It also really doesn't introduce any additional complexity to generic code, since you need to notate the current S: Signature bound anyway. That becomes S: SignatureEncoding with this change.

The only real disadvantage I can think of is it no longer "forces" users of the *Signer/*Verifier traits to impl Signature on the signature type, whereas you could rely on that before. It also communicated the expectation that the Signature trait needed to be impl'd on the signature type, whereas now the documentation will have to strongly recommend impl'ing the SignatureEncoding trait.

Can you think of any other disadvantages to removing it?

@tarcieri tarcieri closed this Oct 24, 2022
@tarcieri tarcieri reopened this Oct 24, 2022
@lumag
Copy link
Contributor

lumag commented Oct 24, 2022

I think it'd just complicate the users API. E.g. when having a generic function over the different signature types, I have the following arguments:

fn gen_sign_cert<S, SK, PK, F, PKF>(
    rng: &mut (impl CryptoRng + RngCore),
    serno: &[u8],
    subject: &RdnSequence,
    public_key: &PK,
    issuer: &RdnSequence,
    signing_key: &SK,
    get_pub_key: PKF,
    encode_sig: F,
) -> Result<Vec<u8>, Box<dyn Error>>
where       
    S: Signature,
    SK: RandomizedSigner<S>,
    PK: Verifier<S> + EncodePublicKey + KeyAlgorithmIdentifier,
    PKF: Fn(&SK) -> PK,
    F: Fn(&S) -> Vec<u8>,
{  ...... } 

The PKF in this case can be replaced with Keypair usage, but the F can not be easily replaced. I'm not 100% sure that it's a good idea to tie the signature with its ASN.1 representation. But strictly speaking I can not come up with the public key system where there are several formats for signatures (well, I have one tiny example: when using GOST TLS ciphersuites, in one of the messages the byteorder of the signature is inverted).
So, long things short, I'd either need a function connecting the S to SignatureEncoder (ugh) or just bind the S to be the SignatureEncoder for all the practical purposes.

@tarcieri
Copy link
Member Author

tarcieri commented Oct 24, 2022

@lumag in practice S: Signature should just change to S: SignatureEncoding. It shouldn't require any additional bounds. Also do you dislike the length of the name?

I'm not sure what the purpose of the other parameters is in your example? F seems redundant.

I'm not 100% sure that it's a good idea to tie the signature with its ASN.1 representation.

What do you think is doing that? The only crate that supports ASN.1 signatures at all is ecdsa, AFAIK, and it's very much a secondary API to the default fixed-sized signatures.

There is nothing ASN.1-specific in the signature crate whatsoever, and many signature formats don't have an ASN.1 representation.

But strictly speaking I can not come up with the public key system where there are several formats for signatures

ECDSA supports fixed-sized and ASN.1 DER signatures.

@lumag
Copy link
Contributor

lumag commented Oct 24, 2022

The F function converts from the Signature to the encoding of the signature suitable for the certificate. I'm crafting certificates on the fly, so I need a function to perform such conversion. s.to_der().as_bytes().to_vec() for ECDSA and |s| s.as_bytes().to_vec() for RSA. I'd have preferred a single API call, but for now I have to live with such closure.

@lumag
Copy link
Contributor

lumag commented Oct 24, 2022

@tarcieri So, as ecdsa example demonstrates, having S: SignatureEncoding would mean that for ecdsa we'd have to implement SigningKey<...> impl Singer<Asn1EcdsaSignatureEncoding> and SigningKey<...> impl Singer<FixedLengthEcdsaSignatureEncoding>. Correct?

@tarcieri
Copy link
Member Author

tarcieri commented Oct 24, 2022

@lumag both the signature types already exist:

It would be possible to add Signer/Verifier impls that operate on ecdsa::der::Signature directly, if that's the reason for the indirection with F.

It would also be possible to add a to_vec method (or thereabouts) on SignatureEncoding if the alloc feature is enabled.

@lumag
Copy link
Contributor

lumag commented Oct 24, 2022

@tarcieri I know :-)
Currenty it was quite easy, Signer works with ecdsa::Signature (I viewed it as an abstract, mathematical R+S signature), which is later converted to ecdsa::der::Signature.

Use `package` on the dependency import instead
Since signatures all encode to a "bag of bytes" of some form, choosing a
common representation is useful when abstracting over a number of
different signature systems.

These helpers make it easy to use either a `Vec<u8>` or `Box<[u8]>` as
that common type.
@tarcieri
Copy link
Member Author

@lumag f6ffa5c adds alloc-gated methods for converting to Vec<u8> or Box<[u8]>

@tarcieri tarcieri changed the title [WIP] signature v2.0.0-pre signature v2.0.0-pre Oct 29, 2022
@tarcieri tarcieri marked this pull request as ready for review October 29, 2022 20:46
@tarcieri tarcieri merged commit 36aa416 into master Oct 29, 2022
@tarcieri tarcieri deleted the signature/2.0.0-pre branch October 29, 2022 20:47
@tarcieri tarcieri added the signature Digital signature crate label Oct 29, 2022
@tarcieri
Copy link
Member Author

@lumag there's one big reason I've encountered porting various code not to bound on SignatureEncoding.

ASN.1 signatures can impl SignatureEncoding using a Self::Repr of Vec<u8>, conditionally gated on cfg(feature = "alloc").

Without the bounds, this means that these signature types can still be used with the *Signer/*Verifier traits if they're otherwise properly heapless, as ecdsa::der::Signature happens to be.

tarcieri added a commit to RustCrypto/signatures that referenced this pull request Oct 30, 2022
Implements the proposed breaking changes to the `signature` crate from
RustCrypto/traits#1141

Most notably the `Signature` trait has been replaced with a
`SignatureEncoding` trait which permits an internally structured
signature representation.
tarcieri added a commit to RustCrypto/signatures that referenced this pull request Oct 30, 2022
Implements the proposed breaking changes to the `signature` crate from
RustCrypto/traits#1141

Most notably the `Signature` trait has been replaced with a
`SignatureEncoding` trait which permits an internally structured
signature representation.
tarcieri added a commit to RustCrypto/signatures that referenced this pull request Oct 30, 2022
Implements the proposed breaking changes to the `signature` crate from
RustCrypto/traits#1141

Most notably the `Signature` trait has been replaced with a
`SignatureEncoding` trait which permits an internally structured
signature representation.
Copy link
Contributor

@rozbb rozbb left a comment

Choose a reason for hiding this comment

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

Sorry it took so long. Nothing here is a big deal. Would like to know some of the reasoning though because I might wanna copy these techniques over to kem later

use alloc::{boxed::Box, vec::Vec};

/// Support for decoding/encoding signatures as bytes.
pub trait SignatureEncoding:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the main argument here that there's no inherent need for someone with access to an S: Signer to be able to (de)serialize the resulting signatures? I'd buy that argument. Are there examples in the wild?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is largely a reaction (perhaps an overreaction) to mandatory trait bounds around the Signature trait in the v1.0 signature crate being too restrictive and preventing some things that would otherwise be valid. Although debatably the AsRef<[u8]> one was too onerous.

As far as a real-world example of something eliminating the bounds enables, the main thing is probably a heapless core implementation with an alloc-dependent SignatureEncoding impl:

https://github.com/RustCrypto/signatures/pull/562/files#diff-23f3fe37b15706957a041307578d2433204986a9eb42dc8d7c4126dcaf9266f1R226-R239

In this case, it's a variable-width ASN.1 DER-encoded type, which knows how to serialize itself into a buffer on heapless platforms, but when the heap is available, it's nice to be able to serialize it into a heap-backed structure (in this case Box<[u8]>)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. In the above case, couldn't SignatureEncoding be implemented without the feature gate? The def simply won't contain the to_vec part if alloc isn't set. Or is the trait simply not useful in that case (for this particular crate)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The SignatureEncoding::Repr in that example is an alloc::boxed::Box<[u8]>, so no, it cannot be defined without the alloc feature enabled.

signature/src/encoding.rs Show resolved Hide resolved
/// Signing keypair with an associated verifying key.
///
/// This represents a type which holds both a signing key and a verifying key.
pub trait Keypair<S: Signature>: AsRef<Self::VerifyingKey> {
pub trait Keypair {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another maybe silly question: how is this intended to be used? It seems that the only thing Keypari is good for is getting a verification key. But you can't even do anything with that verification key, because it's not necessarily part of or an input to a Verifier. So when does this trait get used?

Copy link
Member Author

@tarcieri tarcieri Oct 31, 2022

Choose a reason for hiding this comment

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

Keypair originally started out with mandatory bounds for Signer and Verifier. We removed these because there are other possible traits Keypair could be used in conjunction with, like DigestSigner/DigestVerifier, PrehashSigner/PrehashVerifier, as well as RandomizedSigner/RandomizedDigestSigner/RandomizedPrehashSigner (and these all have further async equivalents in async-signature). See #1107.

So, like with SignatureEncoding, the idea is to provide more flexible traits that don't mandate bounds but are intended to be composed.

In the case of Keypair, the trait is intended only as a way to obtain a verifying key from a signing key.

See also #1124 /cc @lumag

@tarcieri tarcieri mentioned this pull request Oct 31, 2022
tarcieri added a commit to RustCrypto/signatures that referenced this pull request Oct 31, 2022
Implements the proposed breaking changes to the `signature` crate from
RustCrypto/traits#1141

Most notably the `Signature` trait has been replaced with a
`SignatureEncoding` trait which permits an internally structured
signature representation.
tarcieri added a commit to RustCrypto/signatures that referenced this pull request Oct 31, 2022
Implements the proposed breaking changes to the `signature` crate from
RustCrypto/traits#1141

Most notably the `Signature` trait has been replaced with a
`SignatureEncoding` trait which permits an internally structured
signature representation.
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Nov 5, 2022
This (unpublished) prerelease of the `ecdsa` crate impl's the
prospective `signature` v2 API from: RustCrypto/traits#1141
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Nov 5, 2022
This (unpublished) prerelease of the `ecdsa` crate impl's the
prospective `signature` v2 API from: RustCrypto/traits#1141
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Nov 5, 2022
This (unpublished) prerelease of the `ecdsa` crate impl's the
prospective `signature` v2 API from: RustCrypto/traits#1141
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this pull request Nov 6, 2022
This (unpublished) prerelease of the `ecdsa` crate impl's the
prospective `signature` v2 API from: RustCrypto/traits#1141
tarcieri added a commit to dalek-cryptography/ed25519-dalek that referenced this pull request Nov 7, 2022
Bumps the `ed25519` crate to the v2.0.0-pre.0 prerelease.

This version notably uses the `signature` crate's v2 API:

RustCrypto/traits#1141
tarcieri added a commit to dalek-cryptography/ed25519-dalek that referenced this pull request Nov 20, 2022
Bumps the `ed25519` crate to the v2.0.0-pre.0 prerelease.

This version notably uses the `signature` crate's v2 API:

RustCrypto/traits#1141
tarcieri added a commit to dalek-cryptography/ed25519-dalek that referenced this pull request Nov 20, 2022
Bumps the `ed25519` crate to the v2.0.0-pre.0 prerelease.

This version notably uses the `signature` crate's v2 API:

RustCrypto/traits#1141
tarcieri added a commit to dalek-cryptography/ed25519-dalek that referenced this pull request Nov 21, 2022
Bumps the `ed25519` crate to the v2.0.0-pre.0 prerelease.

This version notably uses the `signature` crate's v2 API:

RustCrypto/traits#1141
tarcieri added a commit to dalek-cryptography/ed25519-dalek that referenced this pull request Nov 21, 2022
Bumps the `ed25519` crate to the v2.0.0-pre.0 prerelease.

This version notably uses the `signature` crate's v2 API:

RustCrypto/traits#1141
rozbb pushed a commit to dalek-cryptography/ed25519-dalek that referenced this pull request Nov 21, 2022
Bumps the `ed25519` crate to the v2.0.0-pre.0 prerelease.

This version notably uses the `signature` crate's v2 API:

RustCrypto/traits#1141
This was referenced Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
signature Digital signature crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants