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

Remove padding using new encryption API #246

Closed
wants to merge 6 commits into from

Conversation

lumag
Copy link
Contributor

@lumag lumag commented Jan 6, 2023

This is a reimplementation of RSA encryption/decryption API on top of #245 .

cc @tarcieri @newpavlov @dignifiedquire

lumag added 6 commits January 6, 2023 05:59
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>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Add traits following the signature design for encryption and decryption.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Drop encryption/decryption from the RsaPublicKey/RsaPrivateKey structs,
add new EncryptingKey and DecryptingKey structs implementing Encryptor /
Decryptor traits.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
The crate doesn't use anymore the PaddingScheme. Drop corresponding API.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
The function mgf_xor1() was replaced with mgf_xor1_digest(). Drop the
legacy function.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
@lumag
Copy link
Contributor Author

lumag commented Jan 6, 2023

This PR incorporates #245 instead of replacing it to let us discuss these two PRs separately.

@@ -13,7 +13,9 @@
//!
//! ## PKCS#1 v1.5 encryption
//! ```
//! use rsa::{PublicKey, RsaPrivateKey, RsaPublicKey, PaddingScheme};
//! use rsa::{RsaPrivateKey, RsaPublicKey};
//! use rsa::pkcs1v15::{DecryptingKey, EncryptingKey};
Copy link
Member

Choose a reason for hiding this comment

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

EncryptingKey reads quite odd, I would have expected it to be called EncryptionKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

@dignifiedquire
Copy link
Member

It's a neat API, but from a convenience perspective I am not a fan honestly. Having to create new types for single function calls just does not make for a simple to understand API, especially given that the key is not actually changed, it just stores some metadata. So I honestly think we should not remove the legacy API, or at least provide a better convenience API on top of this structure.

@tarcieri
Copy link
Member

tarcieri commented Jan 6, 2023

I do like this approach overall.

I worry there's going to be a lot of complaints and confusion for anyone attempting to upgrade who just wants to be able to call an inherent method on a key type.

If we do go with this I think it needs some documentation in the changelog or elsewhere about how to upgrade from the previous API.

@tarcieri
Copy link
Member

tarcieri commented Jan 6, 2023

Re: this vs #244, I think both would be interesting?

I think there are plenty of use cases for #244 where a given key might be used for any application with any padding mode, especially KMS/TPM-style applications where a given client might ask for a key operation involving any of them (i.e. both signing and encryption, depending on the client's whims).

However, if you know ahead of time what the key's usage will be, the API in this PR is great.

@tarcieri
Copy link
Member

@lumag if you can rebase, the encryptor/decryptor types seem interesting

@lumag
Copy link
Contributor Author

lumag commented Feb 7, 2023

Closing in favour of #259

@lumag lumag closed this Feb 7, 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.

3 participants