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

Make PrecomputedValues public #221

Merged
merged 5 commits into from
Nov 11, 2022
Merged

Conversation

BrettMayson
Copy link
Contributor

I need access to these values in order to store a custom private key format, this is preventing me from switching away from openssl

@tarcieri
Copy link
Member

tarcieri commented Nov 8, 2022

Is there a particular reason why you want to store these values instead of computing them on-the-fly?

The trend in RSA private key format design has been to store fewer precomputed values rather than more. Modern key formats store only d and can compute everything else from that and the public key.

Also: are you actually using multiprime RSA? (re: crt_values)

@BrettMayson
Copy link
Contributor Author

I need to store a custom format that I have no control over, which requires these values to be stored

@tarcieri
Copy link
Member

tarcieri commented Nov 9, 2022

Do you actually need to support multiprime RSA?

It would probably be worth keeping that out of the public API, especially as it currently has an odd definition owing to PKCS#1 also needing the CRT coefficient for two primes, so the current vector stores "additional CRT coefficients" beyond that one IIUC.

@newpavlov
Copy link
Member

I don't think we should make the fields public. If you only need to read pre-computed values, then a bunch of getters should do the job. If you need to restore the values from serialized data, then we would need to think about validation.

@BrettMayson
Copy link
Contributor Author

BrettMayson commented Nov 10, 2022

Also: are you actually using multiprime RSA? (re: crt_values)

No, just dp, dq and qinv

I can modify this PR to have getters for those 3 values in a few days

@tarcieri
Copy link
Member

In that case it sounds like you should just add read-only getter methods for those three

@tarcieri tarcieri merged commit b06a5ce into RustCrypto:master Nov 11, 2022
@tarcieri tarcieri mentioned this pull request Nov 15, 2022
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.

4 participants