Skip to content

rsa: pass unhashed data to SignerKey/VerifierKey #178

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

rsa: pass unhashed data to SignerKey/VerifierKey #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 signing_key = SigningKey::new_with_hash(private_key, Hash::SHA2_256, Box::new(Sha256::new()));
//! let verifying_key: VerifyingKey = (&signing_key).into();
//!
//! // Sign
//! let data = b"hello world";
//! let digest = Sha256::digest(data).to_vec();
//! let signature = signing_key.sign_with_rng(&mut rng, &digest);
//! let signature = signing_key.sign_with_rng(&mut rng, data);
//! assert_ne!(signature.as_bytes(), data);
//!
//! // Verify
//! verifying_key.verify(&digest, &signature).expect("failed to verify");
//! verifying_key.verify(data, &signature).expect("failed to verify");
//! ```
//!
//! Using PSS signatures
Expand All @@ -87,12 +86,11 @@
//!
//! // Sign
//! let data = b"hello world";
//! let digest = Sha256::digest(data).to_vec();
//! let signature = signing_key.sign_with_rng(&mut rng, &digest);
//! let signature = signing_key.sign_with_rng(&mut rng, data);
//! assert_ne!(signature.as_bytes(), data);
//!
//! // Verify
//! verifying_key.verify(&digest, &signature).expect("failed to verify");
//! verifying_key.verify(data, &signature).expect("failed to verify");
//! ```
//!
//! ## PKCS#1 RSA Key Encoding
Expand Down
58 changes: 40 additions & 18 deletions src/pkcs1v15.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use alloc::boxed::Box;
use alloc::vec;
use alloc::vec::Vec;
use core::fmt::{Debug, Display, Formatter, LowerHex, UpperHex};
use digest::DynDigest;
use rand_core::{CryptoRng, RngCore};
use signature::{RandomizedSigner, Signature as SignSignature, Signer, Verifier};
use subtle::{Choice, ConditionallySelectable, ConstantTimeEq};
Expand Down Expand Up @@ -288,6 +290,7 @@ fn non_zero_random_bytes<R: RngCore + CryptoRng>(rng: &mut R, data: &mut [u8]) {
pub struct SigningKey {
inner: RsaPrivateKey,
hash: Option<Hash>,
digest: Box<dyn DynDigest>,
}

impl SigningKey {
Expand All @@ -299,24 +302,33 @@ impl SigningKey {
self.hash
}

pub fn new(key: RsaPrivateKey) -> Self {
pub(crate) fn digest(&self) -> Box<dyn DynDigest> {
self.digest.box_clone()
}

pub fn new(key: RsaPrivateKey, digest: Box<dyn DynDigest>) -> Self {
Self {
inner: key,
hash: None,
digest,
}
}

pub fn new_with_hash(key: RsaPrivateKey, hash: Hash) -> Self {
pub fn new_with_hash(key: RsaPrivateKey, hash: Hash, digest: Box<dyn DynDigest>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

This seems problematic for a number of reasons.

You now have hash and digest parameters. "Hash" and "digest" are synonyms and in the examples you're passing effectively the same thing, it's just an enum variant versus a Box-ed digest instance. This isn't DRY.

I think you should pick one of hash or digest. With hash, you can pick the concrete Digest to use.

Why are you using Box? It erases the digest type, reducing type safety. If you choose to make everything use digest, I'd suggest introducing a generic parameter. That would be a more invasive change, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought for using the generic parameter quite a while, while working on Signer/Verifier support. And in the end I decided against it. I wanted to have just two key types for all RSA signing keys (the PKCS1 v1.5 and PSS ones). In fact making the SigningKey/VerifyingKey accept the message rather than pre-hashed value just begs for that from my point of view. This way the all PKCS1 (and all PSS keys) parsed from the SPKI are type-compatible and can be stored and handled in a uniform way. One has to handle two major cases: PKCS1v1.5 vs PSS.

An alterntive of course would be to parse the SPKI.parameters in the app and to perform all the matching inside the app, handling the keys in the generic way in separate arms.

BTW: as we are talking about it, should I also add the Digest argument to the RSA Signature traits?
BTW2: I generally feel that rsa::Hash is misplaced or misimplemented. It either should go to pkcs1 crate or be reimplemented as a trait, which is implemented for different digest types. Or, maybe even better, I can split the Pkcs1v15 signer into padded and unpadded structs/traits/implementations.

Copy link
Member

Choose a reason for hiding this comment

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

At the very least, there shouldn't be both hash and digest parameters.

Copy link
Member

Choose a reason for hiding this comment

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

BTW: as we are talking about it, should I also add the Digest argument to the RSA Signature traits?

Which traits are you referring to?

Are you talking about adding another Box<Digest>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tarcieri nah, about adding pss::Signature<D: Digest> to supplement pss::SignerKey<D: Digest>.

Copy link
Member

@tarcieri tarcieri Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, you're talking about the signature struct?

That's something I've considered but have not done yet with the ecdsa crate. The problem with parameterizing by a D: Digest (as opposed to an enum like Hash) is it complicates interoperability with things like hardware accelerators which may provide their own implementations of specific algorithms.

It's a tricky problem. Perhaps a trait could be used to associate a Hash constant with a particular Digest implementation.

Copy link
Contributor

@sandhose sandhose Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

On a quick note: I'm working on refactoring some JWT-related stuff in my code, and found that having the hash as a generic parameter helps, especially with allowing to impl<D: Digest> From<rsa::RsaPublicKey> for rsa::pss::VerifyingKey<D>, etc.
I ended up building a wrapper in the meantime: https://github.com/matrix-org/matrix-authentication-service/blob/b6e92fb8f7ef81a61dcdee28d12e710743c88860/crates/jose/src/jwa/rsa.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sandhose ack, this is on my todo list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sandhose I've pushed the #179, which implements the generified version of these structs + an RFC for further improvement. I'm going to close this PR.

Self {
inner: key,
hash: Some(hash),
digest,
}
}
}

impl Signer<Signature> for SigningKey {
fn try_sign(&self, digest: &[u8]) -> signature::Result<Signature> {
sign::<DummyRng, _>(None, &self.inner, self.hash.as_ref(), digest)
fn try_sign(&self, msg: &[u8]) -> signature::Result<Signature> {
let mut digest = self.digest.box_clone();
digest.update(msg);
let hashed = digest.finalize_reset();
sign::<DummyRng, _>(None, &self.inner, self.hash.as_ref(), &hashed)
.map(|v| v.into())
.map_err(|e| e.into())
}
Expand All @@ -326,9 +338,12 @@ impl RandomizedSigner<Signature> for SigningKey {
fn try_sign_with_rng(
&self,
mut rng: impl CryptoRng + RngCore,
digest: &[u8],
msg: &[u8],
) -> signature::Result<Signature> {
sign(Some(&mut rng), &self.inner, self.hash.as_ref(), digest)
let mut digest = self.digest.box_clone();
digest.update(msg);
let hashed = digest.finalize_reset();
sign(Some(&mut rng), &self.inner, self.hash.as_ref(), &hashed)
.map(|v| v.into())
.map_err(|e| e.into())
}
Expand All @@ -337,20 +352,23 @@ impl RandomizedSigner<Signature> for SigningKey {
pub struct VerifyingKey {
inner: RsaPublicKey,
hash: Option<Hash>,
digest: Box<dyn DynDigest>,
}

impl VerifyingKey {
pub fn new(key: RsaPublicKey) -> Self {
pub fn new(key: RsaPublicKey, digest: Box<dyn DynDigest>) -> Self {
Self {
inner: key,
hash: None,
digest,
}
}

pub fn new_with_hash(key: RsaPublicKey, hash: Hash) -> Self {
pub fn new_with_hash(key: RsaPublicKey, hash: Hash, digest: Box<dyn DynDigest>) -> Self {
Self {
inner: key,
hash: Some(hash),
digest,
}
}
}
Expand All @@ -360,6 +378,7 @@ impl From<SigningKey> for VerifyingKey {
Self {
inner: key.key().into(),
hash: key.hash(),
digest: key.digest(),
}
}
}
Expand All @@ -369,13 +388,17 @@ impl From<&SigningKey> for VerifyingKey {
Self {
inner: key.key().into(),
hash: key.hash(),
digest: key.digest(),
}
}
}

impl Verifier<Signature> for VerifyingKey {
fn verify(&self, msg: &[u8], signature: &Signature) -> signature::Result<()> {
verify(&self.inner, self.hash.as_ref(), msg, signature.as_ref()).map_err(|e| e.into())
let mut digest = self.digest.box_clone();
digest.update(msg);
let hashed = digest.finalize_reset();
verify(&self.inner, self.hash.as_ref(), &hashed, signature.as_ref()).map_err(|e| e.into())
}
}

Expand Down Expand Up @@ -526,17 +549,18 @@ mod tests {
),
)];

let signing_key = SigningKey::new_with_hash(priv_key, Hash::SHA1);
let signing_key = SigningKey::new_with_hash(priv_key, Hash::SHA1, Box::new(Sha1::new()));

for (text, expected) in &tests {
let digest = Sha1::digest(text.as_bytes()).to_vec();

let out = signing_key.sign(&digest);
let out = signing_key.sign(text.as_bytes());
assert_ne!(out.as_ref(), text.as_bytes());
assert_ne!(out.as_ref(), digest);
assert_eq!(out.as_ref(), expected);

let mut rng = ChaCha8Rng::from_seed([42; 32]);
let out2 = signing_key.sign_with_rng(&mut rng, &digest);
let out2 = signing_key.sign_with_rng(&mut rng, text.as_bytes());
assert_eq!(out2.as_ref(), expected);
}
}
Expand Down Expand Up @@ -606,12 +630,10 @@ mod tests {
),
];
let pub_key: RsaPublicKey = priv_key.into();
let verifying_key = VerifyingKey::new_with_hash(pub_key, Hash::SHA1);
let verifying_key = VerifyingKey::new_with_hash(pub_key, Hash::SHA1, Box::new(Sha1::new()));

for (text, sig, expected) in &tests {
let digest = Sha1::digest(text.as_bytes()).to_vec();

let result = verifying_key.verify(&digest, &Signature::from_bytes(sig).unwrap());
let result = verifying_key.verify(text.as_bytes(), &Signature::from_bytes(sig).unwrap());
match expected {
true => result.expect("failed to verify"),
false => {
Expand Down Expand Up @@ -642,10 +664,10 @@ mod tests {
#[test]
fn test_unpadded_signature_signer() {
let msg = b"Thu Dec 19 18:06:16 EST 2013\n";
let expected_sig = Base64::decode_vec("pX4DR8azytjdQ1rtUiC040FjkepuQut5q2ZFX1pTjBrOVKNjgsCDyiJDGZTCNoh9qpXYbhl7iEym30BWWwuiZg==").unwrap();
let expected_sig = Base64::decode_vec("F8rxGUnrRLYr9nTWrYMZYk3Y0msVzfl9daWt32AZHJNCVENOWUS17OwcFawgmYhyJZDG3leTT6S5QZLaozun/A==").unwrap();
let priv_key = get_private_key();

let signing_key = SigningKey::new(priv_key);
let signing_key = SigningKey::new(priv_key, Box::new(Sha1::new()));
let sig = signing_key.sign(msg);
assert_eq!(sig.as_ref(), expected_sig);

Expand Down
38 changes: 22 additions & 16 deletions src/pss.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,15 +358,18 @@ impl RandomizedSigner<Signature> for SigningKey {
fn try_sign_with_rng(
&self,
mut rng: impl CryptoRng + RngCore,
digest: &[u8],
msg: &[u8],
) -> signature::Result<Signature> {
let mut digest = self.digest.box_clone();
digest.update(msg);
let hashed = digest.finalize_reset();
sign_int(
&mut rng,
false,
&self.inner,
digest,
&hashed,
self.salt_len,
self.digest.box_clone().as_mut(),
digest.as_mut(),
)
.map(|v| v.into())
.map_err(|e| e.into())
Expand Down Expand Up @@ -401,15 +404,18 @@ impl RandomizedSigner<Signature> for BlindedSigningKey {
fn try_sign_with_rng(
&self,
mut rng: impl CryptoRng + RngCore,
digest: &[u8],
msg: &[u8],
) -> signature::Result<Signature> {
let mut digest = self.digest.box_clone();
digest.update(msg);
let hashed = digest.finalize_reset();
sign_int(
&mut rng,
true,
&self.inner,
digest,
&hashed,
self.salt_len,
self.digest.box_clone().as_mut(),
digest.as_mut(),
)
.map(|v| v.into())
.map_err(|e| e.into())
Expand Down Expand Up @@ -468,11 +474,14 @@ impl From<&BlindedSigningKey> for VerifyingKey {

impl Verifier<Signature> for VerifyingKey {
fn verify(&self, msg: &[u8], signature: &Signature) -> signature::Result<()> {
let mut digest = self.digest.box_clone();
digest.update(msg);
let hashed = digest.finalize_reset();
verify(
&self.inner,
msg,
&hashed,
signature.as_ref(),
self.digest.box_clone().as_mut(),
digest.as_mut(),
)
.map_err(|e| e.into())
}
Expand Down Expand Up @@ -578,8 +587,7 @@ mod test {
let verifying_key: VerifyingKey = VerifyingKey::new(pub_key, Box::new(Sha1::new()));

for (text, sig, expected) in &tests {
let digest = Sha1::digest(text.as_bytes()).to_vec();
let result = verifying_key.verify(&digest, &Signature::from_bytes(sig).unwrap());
let result = verifying_key.verify(text.as_bytes(), &Signature::from_bytes(sig).unwrap());
match expected {
true => result.expect("failed to verify"),
false => {
Expand Down Expand Up @@ -627,10 +635,9 @@ mod test {
let verifying_key: VerifyingKey = (&signing_key).into();

for test in &tests {
let digest = Sha1::digest(test.as_bytes()).to_vec();
let sig = signing_key.sign_with_rng(&mut rng, &digest);
let sig = signing_key.sign_with_rng(&mut rng, test.as_bytes());
verifying_key
.verify(&digest, &sig)
.verify(test.as_bytes(), &sig)
.expect("failed to verify");
}
}
Expand All @@ -645,10 +652,9 @@ mod test {
let verifying_key: VerifyingKey = (&signing_key).into();

for test in &tests {
let digest = Sha1::digest(test.as_bytes()).to_vec();
let sig = signing_key.sign_with_rng(&mut rng, &digest);
let sig = signing_key.sign_with_rng(&mut rng, test.as_bytes());
verifying_key
.verify(&digest, &sig)
.verify(test.as_bytes(), &sig)
.expect("failed to verify");
}
}
Expand Down