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

Implement Signer/Verifier/Signature interfaces for the RSA signatures #174

Merged
merged 5 commits into from Aug 19, 2022

Conversation

lumag
Copy link
Contributor

@lumag lumag commented Jul 31, 2022

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 lumag force-pushed the rsa-signer branch 3 times, most recently from e211bef to 76200c9 Compare July 31, 2022 22:26
@tarcieri
Copy link
Member

tarcieri commented Jul 31, 2022

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:

...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>,
    [...]
}

@lumag
Copy link
Contributor Author

lumag commented Aug 1, 2022

...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>); ?

@tarcieri
Copy link
Member

tarcieri commented Aug 7, 2022

@lumag yep!

Move DummyRng to the separate module to allow it to be used from
PaddingScheme module.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
@lumag
Copy link
Contributor Author

lumag commented Aug 13, 2022

@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.

@tarcieri
Copy link
Member

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.

@lumag lumag force-pushed the rsa-signer branch 4 times, most recently from f1e596b to 0e507d5 Compare August 14, 2022 10:00
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Add tests for pkcs1v15 and pss signature verification functions to check
that verifying invalid signatures returns an error.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
@lumag
Copy link
Contributor Author

lumag commented Aug 14, 2022

@tarcieri pushed next iteration following your comments.

src/pkcs1v15.rs Outdated Show resolved Hide resolved
src/pkcs1v15.rs Outdated Show resolved Hide resolved
src/pss.rs Outdated Show resolved Hide resolved
src/signature.rs Outdated Show resolved Hide resolved
src/pkcs1v15.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

Looking better!

@lumag lumag force-pushed the rsa-signer branch 2 times, most recently from 52ad0a5 to cea23c7 Compare August 14, 2022 18:33
@lumag
Copy link
Contributor Author

lumag commented Aug 14, 2022

@tarcieri done

Cargo.toml Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

Looks mostly good to me now.

@lumag can you update the PR description?

Implement Signature, Signer and Verifier traits from signature crate.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
@lumag
Copy link
Contributor Author

lumag commented Aug 14, 2022

@tarcieri updated the description

@glaeqen
Copy link

glaeqen commented Aug 17, 2022

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
Copy link

glaeqen commented Aug 17, 2022

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.

@tarcieri
Copy link
Member

@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.

@lumag
Copy link
Contributor Author

lumag commented Aug 17, 2022

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.

@glaeqen
Copy link

glaeqen commented Aug 17, 2022

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.

@tarcieri
Copy link
Member

tarcieri commented Aug 17, 2022

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.

@lumag
Copy link
Contributor Author

lumag commented Aug 19, 2022

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

@tarcieri
Copy link
Member

@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

@lumag
Copy link
Contributor Author

lumag commented Aug 19, 2022

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

@tarcieri
Copy link
Member

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 tarcieri merged commit 40242fb into RustCrypto:master Aug 19, 2022
@sandhose
Copy link
Contributor

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

@lumag
Copy link
Contributor Author

lumag commented Aug 25, 2022

@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).

@tarcieri
Copy link
Member

tarcieri commented Aug 25, 2022

@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.

@lumag
Copy link
Contributor Author

lumag commented Aug 25, 2022

@sandhose #178

@tarcieri tarcieri mentioned this pull request Oct 10, 2022
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.

None yet

4 participants