Skip to content

Implement Keypair trait for the RSA keys #190

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
Closed
wants to merge 6 commits into from
Closed

Implement Keypair trait for the RSA keys #190

wants to merge 6 commits into from

Conversation

lumag
Copy link
Contributor

@lumag lumag commented Sep 16, 2022

This is rather intrusive implementation. It reworks the way RsaPrivateKey is organized. Unfortunately it also meant that default Serialize an Deserilialize derivations provide a list of tokens that are not backwards-compatible. Thus I had to implement those traits manually.

@tarcieri
Copy link
Member

@lumag I'm confused why you need to change RsaPublicKey/RsaPrivateKey at all

@lumag
Copy link
Contributor Author

lumag commented Sep 18, 2022

@lumag I'm confused why you need to change RsaPublicKey/RsaPrivateKey at all

@tarcieri because I have to embed a VerifyingKey as a field into the SigningKey. Otherwise as_ref() will fail with the returning a reference for temp variable error. And putting VerifyingKey next to RsaPrivateKey results in information duplication. So...I ended up with RsaPrivateKeyParts,

@tarcieri tarcieri mentioned this pull request Sep 19, 2022
Closed
@tarcieri
Copy link
Member

@lumag I opened a tracking issue for problems with Keypair and suggested a possible alternative: RustCrypto/traits#1124

@lumag
Copy link
Contributor Author

lumag commented Sep 24, 2022

@tarcieri as a side note:
While this PR is intrusive, it removes the required usage of RsaPrivateKey, making all SigningKey implementations first class citizens. Also note, that ECDSA signing keys also incorporate VerifyingKey as a field.

@tarcieri tarcieri mentioned this pull request Oct 5, 2022
Closed
2 tasks
lumag added 4 commits October 11, 2022 15:33
@lumag
Mark internal functions as private
5ef5131 
The functions internal::decrypt(), internal::blind() and
internal::unblind() are not used outside of the internal.rs. Stop
marking them as public.

Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
Define PrivateKeyParts trait
7c88331 
Define the PrivateKeyParts trait used to represent the RSA Private key
interface.

Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
Split the private key components to a separate struct
d3499e9 
Split private key components to a separate struct to allow reusing them
for the SigningKey imlementations.

Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
Split generic key parts into the separate module
21874b4 
Signed-off-by: Dmitry Baryshkov <[email protected]>
lumag added 2 commits October 23, 2022 02:04
@lumag
Implement AsRef for SigningKey structs
99c43c1 
Implement AsRef for SigningKey and BlindedSigningKey structures
returning corresponding VerifyingKey.

Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
Implement Keypair traits for SigningKeys
e46b618 
Signed-off-by: Dmitry Baryshkov <[email protected]>
@lumag
Copy link
Contributor Author

lumag commented Oct 22, 2022

I have rebased this PR on top of RSA 0.7.0. Note, I had to disable AsRef<RsaPrivateKey> for SigningKey<D> implementations. Since RsaPrivateKey is not a field of SigningKey, I don't see a way to implement AsRef

@tarcieri
Copy link
Member

tarcieri commented Oct 24, 2022 •
edited
Loading

@lumag PTAL at the Keypair changes to the signature crate here:

https://github.com/RustCrypto/traits/pull/1141/files#diff-1eb80c5452df3ad684ce36b4bfb4590a600978c10f0135bcba713ca18617d9bc

It makes the AsRef support an optional optimization, and changes the default Keypair trait to return the public key by value.

This is a proposed breaking change which I think would help simplify this use case.

@lumag lumag closed this Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lumag @tarcieri