RSA icon indicating copy to clipboard operation
RSA copied to clipboard

Implement Signer/Verifier/Signature interfaces for the RSA signatures

Open lumag opened this issue 3 years ago • 10 comments

Refactor the rsa crate to use the API defined by the signature crate. This adds pss and pkcs1v15 modules, each of them providing Signature, Verifier and Signer/RandomizedSigner implementations.

lumag avatar Jul 31 '22 22:07 lumag

As far as the structure goes, I'd suggest doing something similar to the https://github.com/rustcrypto/elliptic-curves crates, which define the following types inside of modules named after the signature algorithm:

  • Signature
  • SigningKey
  • VerifyingKey

See the following modules as examples:

  • https://docs.rs/k256/latest/k256/ecdsa/index.html
  • https://docs.rs/k256/latest/k256/schnorr/index.html

...then impl From<RsaPrivateKey> for SigningKey and impl From<&RsaPrivateKey> for SigningKey (and so on)

For PSS, parameters like the MGF digest can be generic parameters, e.g.

pub struct SigningKey<Mgf: Digest> {
    inner: RsaPrivateKey,
    mgf: PhantomData<Mgf>,
    [...]
}

tarcieri avatar Jul 31 '22 22:07 tarcieri

...then impl From<RsaPrivateKey> for SigningKey and impl From<&RsaPrivateKey> for SigningKey (and so on)

For PSS, parameters like the MGF digest can be generic parameters, e.g.

pub struct SigningKey<Mgf: Digest> {
    inner: RsaPrivateKey,
    mgf: PhantomData<Mgf>,
    [...]
}

What about the salt_len? Is it fine to set it via fn pss::SigningKey::set_salt_len(&self, Option<usize>); ?

lumag avatar Aug 01 '22 04:08 lumag

@lumag yep!

tarcieri avatar Aug 07 '22 21:08 tarcieri

@tarcieri I have mostly finished the implementation of Signer/Verifier implementations. However I now have an issue with the PSS Signer. The trait passes self as non-mutable object, while the signing function uses salt rng as mutable.

lumag avatar Aug 13 '22 07:08 lumag

The trait passes self as non-mutable object, while the signing function uses salt rng as mutable.

RandomizedSigner/RandomizedDigestSigner accept the RNG as an explicit parameter so you don't have to shove it in self.

tarcieri avatar Aug 13 '22 13:08 tarcieri

@tarcieri pushed next iteration following your comments.

lumag avatar Aug 14 '22 10:08 lumag

Looking better!

tarcieri avatar Aug 14 '22 12:08 tarcieri

@tarcieri done

lumag avatar Aug 14 '22 18:08 lumag

Looks mostly good to me now.

@lumag can you update the PR description?

tarcieri avatar Aug 14 '22 19:08 tarcieri

@tarcieri updated the description

lumag avatar Aug 14 '22 19:08 lumag

Is it necessary to couple Signature type with std/alloc Vec? Maybe having Signature<T: AsRef<[u8]>> { bytes: T } would give more freedom, especially when reusing a type to implement a signature verifier on some embedded allocless platform. I'm not sure how feasible linking against RSA crate without alloc really is anyway, maybe pss::Signature or any other <specific>::Signature could be kept in some other lightweight crate that could provide some convenience aliases for specific platforms that would use Vec as is?

pub struct Signature<T: AsRef<[u8]>> {
    bytes: T
}
pub mod std_alloc {
    pub type Signature = super::Signature<alloc::vec::Vec<u8>>;
}
// proper re-exports depending on `std` feature?

glaeqen avatar Aug 17 '22 19:08 glaeqen

I realized that Signature has a hard requirement to be constructible from arbitrary slices (signature::Signature::from_bytes). That might make this exercise a little bit harder.

glaeqen avatar Aug 17 '22 19:08 glaeqen

@vccggorski the rsa crate has a hard dependency on liballoc for the BigUint representation (in num-bigint-dig). So the crate lacks heapless support in general.

There's been some discussion of heapless support using stack-allocated bigints, e.g. via the crypto-bigint crate.

I realized that Signature has a hard requirement to be constructible from arbitrary slices

The constructor is fallible, so that is not a problem at all. The ecdsa and ed25519 crates both provide stack-allocated types which impl Signature.

tarcieri avatar Aug 17 '22 19:08 tarcieri

Initially I had a separate alloc-less Signature implementation using on-stack array. However after noticing that the rest of the code (both pss and pkcs1) heavily depend on alloc, I dropped this implementation. If at some point the RSA crates moves towards alloc-less implementation of bigint, then both Signature and dynamic digests code can be changed.

lumag avatar Aug 17 '22 19:08 lumag

Also by reading the code I got a little bit confused because my understanding of a difference between DigestVerifier/Signer and Verifier/Signer is that former operates on a hash of a message whereas latter needs to generate a hash/digest first. This is why you could derive Verifier out of DigestVerifier if your Signature is a PreHashedSignature and knows what is its associated hashing method. Here, pss::VerifyingKey implements just Verifier and assumes that a message is a hash. Sorry if I'm confusing things, I'm still trying to wrap my head around these concepts.

glaeqen avatar Aug 17 '22 19:08 glaeqen

The Digest* traits are more flexible in that they allow you to pass alternative hash functions, or alternative implementations of hash functions (e.g. wrapper for a hardware accelerator).

The ecdsa crate impls both sets of traits.

tarcieri avatar Aug 17 '22 21:08 tarcieri

I did not see a particular value of implementing the DigestSigner traits, especially given their description. However I can implement them if required.

lumag avatar Aug 19 '22 09:08 lumag

@lumag the nice thing would be using a hardware accelerator for hashing, but given this crate isn't particularly embedded-friendly to begin with I think leaving that out would be fine for an MVP

tarcieri avatar Aug 19 '22 12:08 tarcieri

@tarcieri ok, I will add them to my queue to take a look after sorting out the From<&SK> story.

lumag avatar Aug 19 '22 13:08 lumag

I'm going to go ahead and merge this.

@lumag feel free to follow up with Digest* impls if you'd like, although it seems like there are additional features you're working on which are more important.

tarcieri avatar Aug 19 '22 17:08 tarcieri

Regarding the Digest* traits, doesn't that mean that the Signer/Verifier traits should operate on the message directly (and therefore do the hashing themselves) instead of expecting a hashed input? That doesn't seem to be the case here

sandhose avatar Aug 25 '22 12:08 sandhose

@sandhose @tarcieri Well, I followed the previous design of signing the pre-hashed messages, since this looks to me the way how the RSA signatures usually work. However I'm fine with changing that to use raw messages and hashing them during the sign procedure. The only question is about the raw PKCS1 v1.5 signatures (which do not have the ASN.1 wrapping).

lumag avatar Aug 25 '22 13:08 lumag

@sandhose good catch! The Signer and Verifier traits should prehash the message.

@lumag it would probably be good to retain inherent methods which operate on the raw bytes of a message digest, then implement traits like DigestSigner and Signer in terms of the inherent methods.

Generally working directly with raw digests is an antipattern. It's much less of a problem with RSA than it is with e.g. ECDSA or Schnorr though, where it can lead to existential forgeries.

tarcieri avatar Aug 25 '22 14:08 tarcieri

@sandhose https://github.com/RustCrypto/RSA/pull/178

lumag avatar Aug 25 '22 21:08 lumag