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

Should PaddingScheme be a trait instead of an enum? #226

Closed
tarcieri opened this issue Nov 15, 2022 · 10 comments
Closed

Should PaddingScheme be a trait instead of an enum? #226

tarcieri opened this issue Nov 15, 2022 · 10 comments

Comments

@tarcieri
Copy link
Member

Looking at issues like #215 it seems like requirements of certain variants are making the others somewhat harder to use.

The PKCS1v15Encrypt and PKCS1v15Sign have relatively minimal requirements with PKCS1v15Encrypt having no members whatsoever, whereas OAEP and PSS both need Box<dyn DynDigest + Send + Sync>.

Having a trait would allow the impls to live in e.g. pkcs1v15 and pss modules as well, whereas right now the pkcs1v15 module only exposes encryption-related functionality.

@newpavlov
Copy link
Member

Maybe it's better to completely remove PaddingScheme and instead introduce separate padding-specific methods on RsaPrivateKey?

@tarcieri
Copy link
Member Author

tarcieri commented Dec 1, 2022

@newpavlov both the public and private key need padding-related functionality. The public key needs to generate a padded message for encryption, and verify the padding with a signature, with the inverse true for private keys.

As it were the padding is already largely implemented as crate-private free functions. The real question is how to expose it in the public API.

@lumag
Copy link
Contributor

lumag commented Jan 6, 2023

I'd second @newpavlov here. I'd drop the PaddingScheme completely instead. For the sign/verify ops we have Signer / Verifer families which do not need PaddingScheme. So, I think, it would be logical to add Encryptor / Decryptor and drop the PS.

@tarcieri
Copy link
Member Author

tarcieri commented Jan 6, 2023

As I mentioned in #244, I'd like to get rid of the SignatureScheme trait I introduced there and move the relevant functionality to SigningKey and VerifyingKey.

I still think #244 is a reasonable first step towards splitting up the relevant functionality.

@lumag
Copy link
Contributor

lumag commented Jan 6, 2023

@tarcieri I'd start with something like #245

@tarcieri
Copy link
Member Author

tarcieri commented Jan 6, 2023

It's going to be a nightmare to rebase #244 on that 😢

It would've been better if you started with #244 as the base.

@lumag
Copy link
Contributor

lumag commented Jan 6, 2023

@tarcieri well, I branched off the trunk on purpose. IMHO there is no point in having SignatureScheme if we can drop it first.

@lumag
Copy link
Contributor

lumag commented Jan 6, 2023

@tarcieri @newpavlov @dignifiedquire Also I think that we should make encryption/decryption follow the Signer API. In other words, have a separate Encryptor / Decryptor traits, implement padding-specific keys and drop PaddingScheme completely.

@lumag
Copy link
Contributor

lumag commented Jan 6, 2023

I implemented my ideas as #246

@tarcieri
Copy link
Member Author

Resolved by #244

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

No branches or pull requests

3 participants