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

feat: drop old signing/verification API #245

Closed
wants to merge 1 commit into from

Conversation

lumag
Copy link
Contributor

@lumag lumag commented Jan 6, 2023

The signing and verification is now implemented using the generic Signature and *Signer / *Verifier traits. Drop old API proprietary to the RSA crate.

Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

The signing and verification is now implemented using the generic
Signature and *Signer / *Verifier traits. Drop old API proprietary to
the RSA crate.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
@@ -41,17 +42,25 @@ fn bench_rsa_2048_pkcsv1_decrypt(b: &mut Bencher) {
#[bench]
fn bench_rsa_2048_pkcsv1_sign_blinded(b: &mut Bencher) {
let priv_key = get_key();
let signing_key = rsa::pkcs1v15::SigningKey::<Sha256>::new_with_prefix(priv_key);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a regression in API usability: having to create a newtype struct to call a single function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dignifiedquire why? The signing_key is a long term object, which can be stored and further used. This is more or less what we have for other PK types. Also I think we have all API in place to work with these keys as a first-class objects (generate, read, write, etc). The only thing that prevents me from thinking about turning RsaPublicKey and RsaPrivateKey into create-private structs is the existence of S+E keys.

Copy link
Member

Choose a reason for hiding this comment

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

Because now I am forced to decide on the padding + hashing when storing the key. Most of my use cases are that these are dynamically chosen parameters, so I need to store the key itself, and apply the different algorithms depending on runtime inputs.

Copy link
Member

Choose a reason for hiding this comment

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

This is more or less what we have for other PK types

Yes, and I think it is not great there either, if you have dynamic cases.

Copy link
Member

Choose a reason for hiding this comment

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

as an example, the code I would have to write now will look sth like this

match padding {
  "pkcs" => {
    let key = rsa::pkcs1v15::SigningKey::<Sha256>::new_with_prefix(priv_key);
    key.sign()
  }
  ...
}

Copy link
Member

Choose a reason for hiding this comment

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

@lumag I think it's somewhat more common with RSA due to legacy reasons for keys to be used in multiple roles, even across encryption and signing.

See for example TLS < 1.3 using RSA keys for both key encipherment and digital signatures as part of a handshake.

Which algorithm is used depends on a combination of server-side configuration and the capabilities of the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Or the (rare) GPG S+E keys.
So, how do we want to proceed? Do we drop these two PRs (perfectly fine with me) or would you like for me to improve them? Let's probably decide on this one first.

@tarcieri
Copy link
Member

Seems like we're keeping this API for now

@tarcieri tarcieri closed this Jan 21, 2023
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

3 participants