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.

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

Jump to bottom

pkcs1v15: use AssociatedOID for getting the RSA prefix #183

Merged
merged 4 commits into from
Sep 16, 2022
Merged

pkcs1v15: use AssociatedOID for getting the RSA prefix #183

merged 4 commits into from
Sep 16, 2022

Conversation

lumag
Copy link
Contributor

@lumag lumag commented Sep 9, 2022

Drop internal implementation of AssociatedHash and use AssociatedOID
trait to get the OID corresponding to the Digest and to format the ASN.1
prefix.

@lumag
Copy link
Contributor Author

lumag commented Sep 9, 2022

This depends on RustCrypto/traits#1098 and RustCrypto/hashes#405 to be merged first.

@lumag lumag mentioned this pull request Sep 9, 2022
Merged
@lumag lumag force-pushed the rsa-oid branch 4 times, most recently from 4dcee07 to 6c62f69 Compare September 12, 2022 15:03
@tarcieri tarcieri mentioned this pull request Sep 13, 2022
Merged
@lumag lumag marked this pull request as ready for review September 16, 2022 02:13
newpavlov
newpavlov reviewed Sep 16, 2022
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
newpavlov
newpavlov reviewed Sep 16, 2022
View reviewed changes
src/hash.rs Show resolved Hide resolved
@lumag
pkcs1v15: Also test SHA1 and SHA3-256 prefixes
fa18521 
Add tests using RSA-SHA1 and RSA-SHA3-256 signature schemes.

Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
Cargo.toml: bump versions
5617d9c 
Bump digest, sha1 and sha2 crates versions to resolve the
OID/AssociatedOId implementations.

Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
pkcs1v15: use AssociatedOID for getting the RSA prefix
cdb5c2e 
Drop internal implementation of AssociatedHash and use AssociatedOID
trait to get the OID corresponding to the Digest and to format the ASN.1
prefix.

Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
Get rid of rsa::Hash
53a085a 
Fully replace rsa::Hash with AssociatedOid usage.

Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
Copy link
Contributor Author

lumag commented Sep 16, 2022 •
edited
Loading

@tarcieri @newpavlov Note, this pull request contains breaking change, it drops rsa::Hash. If you wish I can pull this commit out and leave only non-breaking changs.

@tarcieri
Copy link
Member

@lumag breaking changes are fine as there are already ones merged (master is currently v0.7.0-pre)

@newpavlov
Copy link
Member

It would be nice to update the changelog. It's easier to edit "unreleased" section, than to collect all changes introduced since previous release.

@tarcieri
Copy link
Member

@newpavlov unfortunately there is no changelog! (#151)

But I'm happy to add one and document all of the changes in this (upcoming) release.

@tarcieri
Copy link
Member

I'm going to go ahead and merge this as there are several people asking for an RC.

Can wait for feedback from @dignifiedquire before cutting a final release. And in the meantime, I can get a changelog added.

@tarcieri tarcieri merged commit 92ef4c8 into RustCrypto:master Sep 16, 2022
@newpavlov
Copy link
Member

unfortunately there is no changelog

Ah, true. I think the next breaking release is a good opportunity to add one then.

@dignifiedquire
Copy link
Member

Uhm, I am not sure I can support this, but from first glance this means I can't change the hash function at runtime, which is functionality I need. Or am I missing something?

dignifiedquire
dignifiedquire reviewed Sep 18, 2022
View reviewed changes
src/pkcs1v15.rs
),
false,
),
];
let pub_key: RsaPublicKey = priv_key.into();
let verifying_key = VerifyingKey::<Sha256>::new_with_prefix(pub_key);
let verifying_key = VerifyingKey::<Sha1>::new_with_prefix(pub_key);
Copy link
Member

Choose a reason for hiding this comment

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

why did these get changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a revert of previous commit which changed Sha1 to Sha256.

@lumag
Copy link
Contributor Author

lumag commented Sep 18, 2022

Uhm, I am not sure I can support this, but from first glance this means I can't change the hash function at runtime, which is functionality I need. Or am I missing something?

Unfortunately changing the hash function at runtime doesn't match the SigningKey model. You can have several signing keys at once.

@tarcieri
Copy link
Member

tarcieri commented Sep 18, 2022 •
edited
Loading

@dignifiedquire we could ensure the low-level APIs (defined in terms of RsaPrivateKey/RsaPublicKey) operate in terms of DynDigest and accept an ObjectIdentifier parameter where necessary. Does that work?

Re: SigningKey, using (Randomized)DigestSigner would at least reduce the part that needs to be monomorphized to the signing function itself, which would call into a common implementation once the digest has been computed.

An enum could be used to select the concrete Digest to use when invoking (Randomized)DigestSigner, although that could potentially be defined to represent a specific ciphersuite in a downstream crate, rather than in the rsa crate itself.

@lumag
Copy link
Contributor Author

lumag commented Sep 18, 2022

@dignifiedquire The SigningKey expects to get clear text, not the pre-hashed data. DigestSigningKey accepts Digest instance. I can implement the hazmat's interfaces to be able to accept prehashed texts. Or the old API is still accessible.

@dignifiedquire
Copy link
Member

This is the code that I need to support: https://github.com/rpgp/rpgp/blob/master/src/crypto/rsa.rs#L67-L82
The hash function is supplied dynamically with a list of supported hashes here: https://github.com/rpgp/rpgp/blob/master/src/crypto/hash.rs#L45

@dignifiedquire
Copy link
Member

I can implement the hazmat's interfaces to be able to accept prehashed texts

This specifically needs to support prehashed input. The linked api usage is 80% of the reason that I originally wrote this library. 😅

@lumag
Copy link
Contributor Author

lumag commented Sep 21, 2022

@dignifiedquire ack, I see the issue. I'll send a pull request in one of the forthcoming days.

@dignifiedquire
Copy link
Member

thanks @lumag!

@lumag
Copy link
Contributor Author

lumag commented Sep 21, 2022

@dignifiedquire I've sketched rpgp/rpgp#193. However it might be easier to just restore the rsa::Hash struct and use old API as you have been using it up to now.

@lumag lumag deleted the rsa-oid branch October 8, 2022 19:18
@tarcieri tarcieri mentioned this pull request Oct 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dignifiedquire dignifiedquire dignifiedquire left review comments

@newpavlov newpavlov newpavlov left review comments

@tarcieri tarcieri tarcieri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lumag @tarcieri @newpavlov @dignifiedquire