Update RSA signature traits implementations #179

lumag · 2022-08-30T08:07:42Z

Change Signer/Verifier implementations to accept raw messages rather than pre-hashed data
Implement DigestSigner/Verifier traits
Get rid of DynDigest bounds for the RSASSA-PSS implmentation
Sort out specifying Hash to the pkcs1v15::SigningKey<D>::new_with_hash, which duplicates information passed in <D>
~~Use passed digest object for PSS calculations rather than newly created Digest?~~

Change the SigningKey and VerifiyingKey implementations accept raw message rather than pre-hashed message. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

lumag · 2022-08-30T08:29:49Z

RFC:

DynDigest in PSS implementations. I did not want to touch internal implementation (yet), which uses DynDigest internally. The easiest way to drop the bound is to implement internal dynamic DynDigest wrapper around passed D: Digest type. More proper would be to change/duplicate emsa_pss_encode and emsa_pss_verify into generic functions. Which path should I implement?
Hash duplicates D type argument. I have thought about adding new trait and adding a type boundary to the Pkcs1v15 type argument. It should probably go to the pkcs1 crate. However implementing it would bring in at least sha1/sha2/sha3 dependencies.
```
pub trait DigestWithOID: Digest {
    fn get_oid() -> &'static [u8];
}
```
Another approach might be to change new_with_hash argument to accept OID/prefix directly. But then we are just changing the form of information duplication instead of removing it.
Regarding using passed digest. It is closely tied to the first point. Plus I was not sure if it is okay to reuse the external struct.

sandhose · 2022-08-30T08:43:44Z

DynDigest in PSS implementations. I did not want to touch internal implementation (yet), which uses DynDigest internally. The easiest way to drop the bound is to implement internal dynamic DynDigest wrapper around passed D: Digest type. More proper would be to change/duplicate emsa_pss_encode and emsa_pss_verify into generic functions. Which path should I implement?

Shouldn't dyn DynDigest implement Digest? If it's the case, I think you could have emsa_pss_encode be emsa_pss_encode<D: Digest>(..., digest: &mut D), and still use it with a &dyn DynDigest/Box<dyn DynDigest>

Hash duplicates D type argument. I have thought about adding new trait and adding a type boundary to the Pkcs1v15 type argument. It should probably go to the pkcs1 crate. However implementing it would bring in at least sha1/sha2/sha3 dependencies.
pub trait DigestWithOID: Digest {
    fn get_oid() -> &'static [u8];
}
Another approach might be to change new_with_hash argument to accept OID/prefix directly. But then we are just changing the form of information duplication instead of removing it.

Looks like there is const_oid::AssociatedOid trait for that: https://docs.rs/const-oid/latest/const_oid/trait.AssociatedOid.html
Not 100% sure it's the right thing, but it's implemented for elliptic curves. If it is, it's then a matter of implementing them in the various digest crates

lumag · 2022-08-30T09:04:56Z

DynDigest in PSS implementations. I did not want to touch internal implementation (yet), which uses DynDigest internally. The easiest way to drop the bound is to implement internal dynamic DynDigest wrapper around passed D: Digest type. More proper would be to change/duplicate emsa_pss_encode and emsa_pss_verify into generic functions. Which path should I implement?

Shouldn't dyn DynDigest implement Digest? If it's the case, I think you could have emsa_pss_encode be emsa_pss_encode<D: Digest>(..., digest: &mut D), and still use it with a &dyn DynDigest/Box<dyn DynDigest>

No, there are minor differences there, so Digest and DynDigest are not directly interchangeable. However. Let me check if it would be possible to wrap it using Fn.

Hash duplicates D type argument. I have thought about adding new trait and adding a type boundary to the Pkcs1v15 type argument. It should probably go to the pkcs1 crate. However implementing it would bring in at least sha1/sha2/sha3 dependencies.
pub trait DigestWithOID: Digest {
    fn get_oid() -> &'static [u8];
}
Another approach might be to change new_with_hash argument to accept OID/prefix directly. But then we are just changing the form of information duplication instead of removing it.
Looks like there is const_oid::AssociatedOid trait for that: https://docs.rs/const-oid/latest/const_oid/trait.AssociatedOid.html Not 100% sure it's the right thing, but it's implemented for elliptic curves. If it is, it's then a matter of implementing them in the various digest crates

This looks like what I was looking for. Let's look it using it (and implementing it in various digest crates) would be fine with @tarcieri .

newpavlov · 2022-08-30T14:46:32Z

Shouldn't dyn DynDigest implement Digest?

It's not possible. The main difference is that Digest returns hash results as arrays, which makes it non-object-safe, while DynDigest works around it by returning hash results as boxed slices. Imagine code which tries to simultaneously work with both SHA-1 and SHA-256.

It's an architecture decision: you should decide whether you rely on dynamic dispatch (i.e. DynDigest and Box<[u8]>) or on monomorphization (i.e. D: Digest and [u8; D::OutputSize]). I don't think it makes much sense to mix them in the same code.

lumag · 2022-09-02T19:55:10Z

@tarcieri @dignifiedquire your opinion/comments?

Cargo.toml

tarcieri · 2022-09-04T19:07:08Z

src/lib.rs

@@ -58,17 +58,16 @@
 //!
 //! let bits = 2048;
 //! let private_key = RsaPrivateKey::new(&mut rng, bits).expect("failed to generate a key");
-//! let signing_key = SigningKey::new_with_hash(private_key, Hash::SHA2_256);
-//! let verifying_key: VerifyingKey = (&signing_key).into();
+//! let signing_key = SigningKey::<Sha256>::new_with_hash(private_key, Hash::SHA2_256);


SigningKey::<Sha256>::new_with_hash(private_key, Hash::SHA2_256);

...still repeats Sha256 and Hash::SHA2_256.

This is a bullet 3 in the RFC in the first comment. @sandhose suggested implementing const_oid::AssociatedOID for the Digest implementations instead of having a separate Hash class. Is that ok with you?

Change the SigningKey and VerifiyingKey implementations accept raw message rather than pre-hashed message. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Implement the experimental (preview) DigestSigner and DigestVerifier traits for the PKCS1v15 structs. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Implement the experimental (preview) RandomizedDigestSigner and DigestVerifier traits for the PSS structs. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

tarcieri · 2022-09-05T18:02:59Z

@lumag using a trait to provide the linkage between a generic D: Digest and the current usages of Hash is a good idea.

Will AssociatedOid actually work for this use case though? Aren't the OIDs that matter here RSA-specific (e.g. MGF)?

If you use AssociatedOid, the relevant OIDs will need to be added to types defined in the sha2 crate. And if we do that, they need to be the generic OIDs that aren't predicated around RSA-specific use cases, e.g. id-sha256 (2.16.840.1.101.3.4.2.1)

lumag · 2022-09-05T18:09:36Z

@tarcieri no, the OIDs are generic enough. Consider e.g. Hash::SHA2_256:

Hash::SHA2_256 => &[
                0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02,
                0x01, 0x05, 0x00, 0x04, 0x20,
            ],

This translates to:

  0  49: SEQUENCE {
  2  13:   SEQUENCE {
  4   9:     OBJECT IDENTIFIER sha-256 (2 16 840 1 101 3 4 2 1)
 15   0:     NULL
       :     }
 17  32:   OCTET STRING

tarcieri · 2022-09-05T18:13:23Z

Ok, that would work, but you (or someone else) will need to add the impls upstream to sha2

lumag · 2022-09-05T18:14:10Z

I'll do that, I'll need to test it anyway.

lumag · 2022-09-05T18:24:44Z

@tarcieri @sandhose my bad. The AssociatedOid doesn't work out of box for Sha*, since they are CoreWrappers. Rustc wouldn't allow implementing the trait (only traits defined in the current crate can be implemented for arbitrary types). I'll check if adding DynAssociatedOid trait (which provides a function rather than a const) would help.

tarcieri · 2022-09-05T19:16:57Z

@lumag maybe for now you can add a trait with an associated constant of the existing Hash type?

Using OIDs seems interesting but yes that's a complication. Semi-related issue: RustCrypto/traits#1069. If we encapsulated each hash algorithm in a newtype, it would solve this particular problem.

lumag · 2022-09-05T19:19:29Z

This will add dependencies from the rsa crate on the sha1/sha2/sha3 crates. Is it fine for now?

tarcieri · 2022-09-05T19:26:02Z

@lumag maybe optional dependencies with sha2 enabled by default?

lumag · 2022-09-05T20:42:41Z

@tarcieri done

src/hash.rs

There is an information duplication when the keys are expected to generate signatures over the ASN.1-prefixed data: SigningKey::<Sha256>::new_with_hash(private_key, Hash::SHA2_256); Remove this duplication by adding the AssociatedHash trait and implementing it for sha1/sha2/sha3 types. Note, this trait should morph into DynAssociatedOid trait and be implemented directly for the corresponding digest implementations. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

tarcieri · 2022-09-06T14:29:56Z

src/hash.rs

+/* FIXME: This trait should be refactored into per-digest implementations returning OID */
+pub trait AssociatedHash {
+    const HASH: Hash;
+}


This trait could probably use a doc comment, even if the intent is to remove/replace it

tarcieri · 2022-09-07T17:42:43Z

Going to go ahead and merge this as it's hopefully uncontroversial additive changes to the new signature features.

@lumag seems we have a path forward on replacing Hash with OIDs if you'd like to attempt that as a followup: RustCrypto/traits#1098

lumag · 2022-09-07T18:55:14Z

@tarcieri thanks!

Make pkcs1v15::Signing/VerifyingKey accept raw message

2fc5e79

Change the SigningKey and VerifiyingKey implementations accept raw message rather than pre-hashed message. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

lumag mentioned this pull request Aug 30, 2022

rsa: pass unhashed data to SignerKey/VerifierKey #178

Closed

tarcieri reviewed Sep 4, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

tarcieri reviewed Sep 4, 2022

View reviewed changes

lumag force-pushed the rsa-signer branch from 4af8b5a to 70bb260 Compare September 5, 2022 16:56

lumag added 3 commits September 5, 2022 21:01

Make pss::Signing/VerifyingKey accept raw message

a4a36bb

Change the SigningKey and VerifiyingKey implementations accept raw message rather than pre-hashed message. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Implement DigestSigner/Verifier for PKCS1v15 structs

b496923

Implement the experimental (preview) DigestSigner and DigestVerifier traits for the PKCS1v15 structs. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Implement RandomizedDigestSigner, DigestVerifier for PSS structs

4750052

Implement the experimental (preview) RandomizedDigestSigner and DigestVerifier traits for the PSS structs. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

lumag force-pushed the rsa-signer branch from 70bb260 to 4750052 Compare September 5, 2022 18:02

lumag force-pushed the rsa-signer branch from 09391ca to c9e4068 Compare September 5, 2022 20:42

lumag force-pushed the rsa-signer branch from c9e4068 to 1a349be Compare September 5, 2022 20:45

lumag marked this pull request as ready for review September 5, 2022 20:45

tarcieri reviewed Sep 5, 2022

View reviewed changes

src/hash.rs Outdated Show resolved Hide resolved

lumag force-pushed the rsa-signer branch from 1a349be to 841f744 Compare September 5, 2022 20:51

tarcieri reviewed Sep 6, 2022

View reviewed changes

tarcieri approved these changes Sep 6, 2022

View reviewed changes

tarcieri requested a review from dignifiedquire September 6, 2022 14:31

tarcieri mentioned this pull request Sep 7, 2022

digest: type name readability regression RustCrypto/traits#1069

Open

tarcieri merged commit d68e273 into RustCrypto:master Sep 7, 2022

lumag deleted the rsa-signer branch September 7, 2022 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update RSA signature traits implementations #179

Update RSA signature traits implementations #179

lumag commented Aug 30, 2022 •

edited

lumag commented Aug 30, 2022

sandhose commented Aug 30, 2022

lumag commented Aug 30, 2022

newpavlov commented Aug 30, 2022 •

edited

lumag commented Sep 2, 2022

tarcieri Sep 4, 2022

lumag Sep 5, 2022 •

edited

tarcieri commented Sep 5, 2022

lumag commented Sep 5, 2022

tarcieri commented Sep 5, 2022

lumag commented Sep 5, 2022

lumag commented Sep 5, 2022

tarcieri commented Sep 5, 2022 •

edited

lumag commented Sep 5, 2022

tarcieri commented Sep 5, 2022

lumag commented Sep 5, 2022

tarcieri Sep 6, 2022

tarcieri commented Sep 7, 2022

lumag commented Sep 7, 2022

Update RSA signature traits implementations #179

Update RSA signature traits implementations #179

Conversation

lumag commented Aug 30, 2022 • edited

lumag commented Aug 30, 2022

sandhose commented Aug 30, 2022

lumag commented Aug 30, 2022

newpavlov commented Aug 30, 2022 • edited

lumag commented Sep 2, 2022

tarcieri Sep 4, 2022

Choose a reason for hiding this comment

lumag Sep 5, 2022 • edited

Choose a reason for hiding this comment

tarcieri commented Sep 5, 2022

lumag commented Sep 5, 2022

tarcieri commented Sep 5, 2022

lumag commented Sep 5, 2022

lumag commented Sep 5, 2022

tarcieri commented Sep 5, 2022 • edited

lumag commented Sep 5, 2022

tarcieri commented Sep 5, 2022

lumag commented Sep 5, 2022

tarcieri Sep 6, 2022

Choose a reason for hiding this comment

tarcieri commented Sep 7, 2022

lumag commented Sep 7, 2022

lumag commented Aug 30, 2022 •

edited

newpavlov commented Aug 30, 2022 •

edited

lumag Sep 5, 2022 •

edited

tarcieri commented Sep 5, 2022 •

edited