Key generation and signer interface

[lukehinds]

Much like the go counterpart in sigstore/sigstore, implement key generation and a signer interface. This should provide the following:

1. Generate of a key pair using a provided alg (rsa, ecdsa)
2. public key derive from private key
3. Signing interface
4. Serialisation (handle PEM / DER en/decode etc).

[lukehinds]

Will keep this open to track Ed25519 and RSA modules

• [x] ecdsa
• [x] Ed25519
• [x] RSA
• [x] examples/key_interface (or however you would like to name)
  • [x] Generate a key pair
  • [x] Sign data
  • [x] Verify signature
  • [x] Export public key in PEM format
  • [x] Export public key in DER format
  • [x] Export private key in PEM format
  • [x] Export encrypted private key in PEM format
  • [x] Export private key in DER format
  • [x] Import public key in PEM format as CosignVerificationKey
  • [x] Import public key in DER format as CosignVerificationKey
  • [x] Import private key in PEM format as KeyPair/SigStoreSigner
  • [x] Import encrypted private key in PEM format as KeyPair/SigStoreSigner
  • [x] Import private key in DER format as KeyPair/SigStoreSigner

Many thank @Xynnn007 for taking this on!

[Xynnn007]

@lukehinds Hi, ed25519 has been supported in https://github.com/sigstore/sigstore-rs/pull/87/commits/8314f8f7867e0e80c2e2ca0470a6897f859ae93c

[lukehinds]

hi @Xynnn007 , I must have misunderstood and was going off this comment https://github.com/sigstore/sigstore-rs/pull/87#issuecomment-1200745101

[lukehinds]

Just going to add one more @Xynnn007 , some examples (for users) would be very useful. See examples in the root folder.

[Xynnn007]

Also, now CosignVerificationKey has the similar semantics with the SigStoreSigner's public key. I'd prefer to add the following for ease

• Into trait for SigStoreSigner or KeyPair
• From trait for CosignVerificationKey If ok, can add them into the tracking list :-)

[lukehinds]

sounds good @Xynnn007

[Xynnn007]

There might need some refactoring for the key_interface, to deal with the following problems:

• Now when users use the signing_key module, they must import the crates into the Cargo.toml, like p256, p384, etc. I'd prefer users do not need to care about the underlying Elliptic Curves. They can directly use sigstore crate without import any other crates to do all the key related operations, thus we need to do some wrapping.
• Now we do not have a convenient call routine for a user to import a private key and convert it into a SigStoreSigner. Also SigStoreSigner is somehow overlapped functionally with Signer trait object. I'd like to combine them, and give a more easy way for users to convert a PEM/DER encoded private key into an object who can sign.

[lkatalin]

@Xynnn007 Strongly agree about not having users import extra crates, thank you!

[Xynnn007]

@Xynnn007 Strongly agree about not having users import extra crates, thank you!

Yea, in a sense CosignVerificationKey struct has the same issue that when use this we need to import ring crate. How about refactor it into an enum, the enum values will be the same as crypto::signing_key::SigningScheme, s.t.

pub enum SigningScheme {
    // TODO: RSA schemes
    ECDSA_P256_SHA256_ASN1,
    ECDSA_P384_SHA384_ASN1,
    ED25519,
}

Before refactored, the interface looks like the following (what we use now):

use ring::signature;
use sigstore::crypto::verification_key::CosignVerificationKey;

let _vk: CosignVerificationKey = CosignVerificationKey::from_der(der_data: &[u8], verification_algorithm: &'static dyn signature::VerificationAlgorithm)?;
let _vk: CosignVerificationKey = CosignVerificationKey::from_pem(pem_data: &[u8], signature_digest_algorithm: SignatureDigestAlgorithm)?;

I prefer to change it into the following:

use sigstore::crypto::verification_key::CosignVerificationKeyWrapper;
use sigstore::crypto::signing_key::SigningScheme;

// CosignVerificationKeyWrapper is enum wrapping CosignVerificationKey inside
let _vk: CosignVerificationKeyWrapper = SigningScheme::ECDSA_P256_SHA256_ASN1.verification_key_from_der(der_data: &[u8]);
let _vk: CosignVerificationKeyWrapper = SigningScheme::ECDSA_P256_SHA256_ASN1.verification_key_from_pem(pem_data: &[u8]);

Avoid import the external ring crate. Certainly the CosignVerificationKeyWrapper can be other names, like VerificationKey for short.

How do you think about this? @lukehinds @flavio @lkatalin

[flavio]

I used ring back in the days because there was no native support of p384 inside of "RustCrypto ecosystem". This recently changed, hence I'm fine dropping ring support.

As for the proposed API change, I like end users to not worry about which format the key they are importing might be. They just need to know if the data is encoded using PEM or not, and then call CosignVerificationKey::from_pem or CosignVerificationKey::from_der.

You are not proposing to drop CosignVerificationKey and this API, are you?

[Xynnn007]

As for the proposed API change, I like end users to not worry about which format the key they are importing might be. They just need to know if the data is encoded using PEM or not, and then call CosignVerificationKey::from_pem or CosignVerificationKey::from_der.

Partially agree with that.

What a user may use about CosignVerificationKey is from_pem/der() and key.verify(). The two interfaces do not care about the key type.

But as the name CosignVerificationKey shows, this may be something like a public key. And that public key + digest algorithm + other parameters (like padding alg in RSA-based signing)= Verifier

Here Verifier can have an API verify(sig).

You are not proposing to drop CosignVerificationKey and this API, are you?

I'm not proposing to drop the function of CosignVerificationKey. It is very useful, but need some refactoring. IMO there may be three layers for the verification key abstraction:

• InnerPublicKey struct of different type of keys, implemented by generic type.
• PublicKey enum that wraps different kinds of public keys : with from_pem/der, to_pem/der API. These API's matches what you've recommended no signing schemes are involved. And they may also have API like try_to_ed25519_verifier(), try_to_ecdsa_verifier(digest_alg),try_to_rsa_verifier(padding, digest_alg)
• SigStoreVerifier: enum wraps PublicKey and concrete signing scheme inside. It may have APIs like from_ed25519_public_key(key), from_rsa_public_key(key, padding, digest_alg), from_ecdsa_public_key(key, digest_alg)

The first layer abstraction is to use generic RustCrypto ecosystem of ecdsa (curve as generic parameter). The second layer abstraction is to avoid import other crates, e.g., enum different cruves for ecdsa. The third layer abstraction is to overcome the gap from public key to a specific signing algorithm public key, such as rsa public key -> rsa public key + Sha256 + PKCS1.5 padding

If we agree here, I'd like to do the changes of this function, including drop ring and use RustCrypto ecosystem as you've mentioned, because it is related to signing_key module closely.

[lkatalin]

Without having delved into the existing code too deeply, I think this makes sense in terms of the APIs and separation between PublicKey vs. Verifier. Can you say more about why the InnerPublicKey struct is necessary? Could whatever that is doing be done with a trait on the PublicKey instead? I would assume the key types themselves are defined somewhere in RustCrypto so we don't need to re-implement them(?) but I have mostly worked with the openssl crate so I'm not sure what's different.

[Xynnn007]

Can you say more about why the InnerPublicKey struct is necessary? Could whatever that is doing be done with a trait on the PublicKey instead? I would assume the key types themselves are defined somewhere in RustCrypto so we don't need to re-implement them(?) but I have mostly worked with the openssl crate so I'm not sure what's different.

The RustCrypto is implemented highly generic. Take ecdsa's VerificationKey for example, it only defines the abstract interfaces with generic parameter, and the concrete implementation is in the crates like p256, p384. I tried to find the defined key types as you'd mentioned, but unfortunately failed.

Back to the reason of InnerPublicKey. There are two:

• Avoid duplicated ecdsa key enum.
• Different function implementing PublicKey -> Verifier has different parameters.

Let me clarify:

Avoid duplicated ecdsa key enum

The public key used to verify a signature in ecdsa is defined as ecdsa::VerifyingKey<C> in RustCrypto and here C is a generic parameter for different curves.

If we use a PublicKey to directly warp it, it will look like

// case 1
enum PublicKey {
    EcdsaP256(ecdsa::VerifyingKey<p256::NistP256>)
    EcdsaP384(ecdsa::VerifyingKey<p384::NistP384>)
   //...
}

Rather than the following that using a generic parameter

enum PublicKey {
    // Here should be named as Ec, for it is only a key 
    Ecdsa(ecdsa::VerifyingKey<C>)
   //...
}

Because any definition with generic parameter is decided when compiled, and we need to add extra crates when coding. Maybe we prefer something like

enum PublicKey {
    Ecdsa(...)
    Ed25519(...)
    Rsa(...)
   //...
}

Besides, if there more curves, it may seem redundant, like when we implement to_verifier function like

impl PublicKey {
    pub fn to_verifier(&self, ...) -> Result<Verifier> {
        match self {
            EcdsaP256PublicKey(inner) => inner.to_verifier(...),
            EcdsaP384PublicKey(inner) => inner.to_verifier(...),
            EcdsaK256PublicKey(inner) => inner.to_verifier(...),
        }
    }
}

the XXX(inner) => XXX is all same for the same type of public key.

Different function implementing PublicKey -> Verifier has different parameters.

This is the most important reason. If we use InnerPublicKey to wrap, it may look like

pub enum InnerEcdsaPublickey {
    P256(ecdsa::VerifyingKey<p256::NistP256>),
    P384(ecdsa::VerifyingKey<p384::NistP384>),
...
}

And for InnerEcdsaPublickey, we can define to_verifier(&self, digest: SignatureDigestAlgorithm) For InnerRsaPublickey, to_verifier(&self, digest: SignatureDigestAlgorithm, padding: PaddingScheme) They are functions with different parameters.

Now then use PublicKey to wrap InnerEcdsaPublickey, InnerRsaPublickey, InnerEd25519Publickey to provide a uniform interface like from_pem/der(). When need to convert it into Verifier, PublicKey can firstly degenerate to InnerPublicKey then call relative to_verify()

If we do not use InnerPublicKey but directly use PublicKey enum to wrap them, like the following

enum PublicKey {
    EcdsaP256(..),
    Rsa(..)
   //...
}

The function to_verifier(&self, ...) is hard to define. May look like this

fn to_verifier(&self, Option<SignatureDigestAlgorithm>, Option<PaddingScheme>) {
    match self {
        EcdsaP256 => // use SignatureDigestAlgorithm,
        Rsa=> // use SignatureDigestAlgorithm and PaddingScheme,
    }
}

I would assume the key types themselves are defined somewhere in RustCrypto so we don't need to re-implement them(?) but I have mostly worked with the openssl crate so I'm not sure what's different.

I think it is not re-implementing, but wrap the generic definitions using enums.

[flavio]

I'm not proposing to drop the function of CosignVerificationKey. It is very useful, but need some refactoring. IMO there may be three layers for the verification key abstraction: [...] If we agree here, I'd like to do the changes of this function, including drop ring and use RustCrypto ecosystem as you've mentioned, because it is related to signing_key module closely.

Fine with me. I'm happy to remove the ring dependency. Let's just ensure the final refactoring will not degrade the developer experience for the consumers of the crate (translated: let's not require them to have a major on crypto algorithms).

[lkatalin]

@Xynnn007 Thanks for the detailed explanation. I see the problem you point out with the different number of parameters to turn the different key types into a Verifier. What you propose sounds fine as far as I can tell, especially if the names of the wrappers, etc., are clear and short.

[Xynnn007]

Fine with me. I'm happy to remove the ring dependency. Let's just ensure the final refactoring will not degrade the developer experience for the consumers of the crate (translated: let's not require them to have a major on crypto algorithms).

What I wanted might degrade the developer experience in a sense. Let me firstly propose a new pr to remove ring first, while keeping the original API as possible. Then we can do further refactoring if needed.