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.

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

Conversation

lumag
Copy link
Contributor

@lumag lumag commented Aug 25, 2022

Quoting Tony Arcieri:

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.

Signed-off-by: Dmitry Baryshkov [email protected]

Quoting Tony Arcieri:

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

Signed-off-by: Dmitry Baryshkov <[email protected]>
@tarcieri
Copy link
Member

I'd suggest keeping the existing methods as-is, but renaming them to note they accept a prehashed message digest rather than an unhashed message, e.g. sign_prehash_with_rng and verify_prehash.

You can then implement traits like DigestSigner/Signer using that API. You can look at the ecdsa crate as an example:

https://github.com/RustCrypto/signatures/blob/a440df1/ecdsa/src/sign.rs#L175-L202

@lumag
Copy link
Contributor Author

lumag commented Aug 25, 2022

@tarcieri this PR changes only new SignerKey/VerifierKey implementations, the old API remains intact. I can change the existing RsaPublicKey/RsaPrivateKey API (and maybe internal API) to follow the naming you have suggested if that's your inent.

@tarcieri
Copy link
Member

Let me back up and leave some line notes.

}
}

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

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

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.

@lumag lumag closed this Aug 30, 2022
@lumag lumag deleted the digest-rsa branch October 8, 2022 19:19
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.

3 participants