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

Refactor PaddingScheme into a trait #244

Merged
merged 5 commits into from
Jan 10, 2023

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Jan 6, 2023

As proposed in #226, splits up the PaddingScheme enum into four structs, named after the previous variants of the struct (adopting capitalization from the Rust API guidelines):

  • oaep::Oaep
  • pkcs1v15::{Pkcs1v15Encrypt, Pkcs1v15Sign}
  • pss::Pss

All of these are re-exported from the toplevel.

Each of these structs impls one or more of the following traits:

  • PaddingScheme: used for encryption
  • SignatureScheme: used for signing

The PaddingScheme constructors have been remapped as follows:

  • new_oaep => Oaep::new
  • new_oaep_with_label => Oaep::new_with_label
  • new_oaep_with_mgf_hash => Oaep::new_with_mgf_hash
  • new_oaep_with_mgf_hash_with_label => Oaep::new_with_mgf_hash_and_label
  • new_pkcs1v15_encrypt => Pkcs1v15Encrypt
  • new_pkcs1v15_sign => Pkcs1v15Sign::new
  • new_pkcs1v15_sign_raw => Pkcs1v15Sign::new_raw
  • new_pss => Pss::{new, new_blinded}
  • new_pss_with_salt => Pss::{new_with_salt new_blinded_with_salt}

@tarcieri
Copy link
Member Author

tarcieri commented Jan 6, 2023

cc @lumag

@tarcieri
Copy link
Member Author

tarcieri commented Jan 6, 2023

Note that there's various additional refactoring that could probably be performed, like getting rid of the inherent methods for these functionality and using the traits exclusively, or getting rid of the SignatureScheme trait and factoring the relevant code into SigningKey and VerifyingKey.

I deliberately didn't make any of those kinds of changes to make code review easier.

src/key.rs Outdated Show resolved Hide resolved
src/oaep.rs Outdated Show resolved Hide resolved
@dignifiedquire
Copy link
Member

I overall very much like this approach, it makes the API clearer to understand and reason about.

@tarcieri
Copy link
Member Author

tarcieri commented Jan 9, 2023

@dignifiedquire this should be good to go now

src/key.rs Outdated Show resolved Hide resolved
src/key.rs Outdated Show resolved Hide resolved
Splits up the `PaddingScheme` enum into four structs, named after the
previous variants of the struct (adopting capitalization from the Rust
API guidelines):

- `oaep::Oaep`
- `pkcs1v15::{Pkcs1v15Encrypt, Pkcs1v15Sign}`
- `pss::Pss`

All of these are re-exported from the toplevel.

Each of these structs impls one or more of the following traits:

- `PaddingScheme`: used for encryption
- `SignatureScheme`: used for signing

The `PaddingScheme` constructors have been remapped as follows:

- `new_oaep` => `Oaep::new`
- `new_oaep_with_label` => `Oaep::new_with_label`
- `new_oaep_with_mgf_hash` => `Oaep::new_with_mgf_hash`
- `new_oaep_with_mgf_hash_with_label` => `Oaep::new_with_mgf_hash_and_label`
- `new_pkcs1v15_encrypt` => `Pkcs1v15Encrypt`
- `new_pkcs1v15_sign` => `Pkcs1v15Sign::new`
- `new_pkcs1v15_sign_raw` => `Pkcs1v15Sign::new_raw`
- `new_pss` => `Pss::{new, new_blinded}`
- `new_pss_with_salt` => `Pss::{new_with_salt new_blinded_with_salt}`
@tarcieri tarcieri force-pushed the refactor-padding-scheme-into-trait branch from 69c9ba0 to dc47326 Compare January 10, 2023 16:45
Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

thank you, lgtm

@tarcieri tarcieri merged commit 35372d9 into master Jan 10, 2023
@tarcieri tarcieri deleted the refactor-padding-scheme-into-trait branch January 10, 2023 20:59
@tarcieri tarcieri mentioned this pull request Jan 16, 2023
sorah added a commit to sorah/needroleshere that referenced this pull request Nov 4, 2023
Resolve incompatibilities as follows:

- src/certificate.rs: RustCrypto/formats#771
- src/cmd/serve.rs: Move to FromRequestParts and use headers crate

- src/ecdsa_sha256.rs: RustCrypto/traits#1196
  and RustCrypto/signatures#574

- src/error.rs: Remove Duplicates

- src/identity.rs: Remove owned validation logic as it should be done at
    x509_cert::serial_number::SerialNumber<x509_cert::certificate::Rfc5280>

- src/sign.rs: RustCrypto/RSA#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

Successfully merging this pull request may close these issues.

2 participants