Skip to content

Export / import RSA to / from provider#10190

Closed
levitte wants to merge 22 commits intoopenssl:masterfrom
levitte:export-import-rsa
Closed

Export / import RSA to / from provider#10190
levitte wants to merge 22 commits intoopenssl:masterfrom
levitte:export-import-rsa

Conversation

@levitte
Copy link
Member

@levitte levitte commented Oct 15, 2019

This lays the foundation for transferring RSA keys to / from providers.

It goes with the idea of using multiple OSSL_PARAM items with the same key to pass multiple primes, CRT exponents and CRT coefficients, and in doing so, simplifies it by making p and q part of the primes (they are the first two), dP and dQ part of the CRT exponents, and qInv part of the CRT coefficients. Extra RSA functionality is added to support this.

  • documentation is added or updated (for the extra RSA functionality)
  • tests are added or updated (for the extra RSA functionality)

@levitte levitte added the branch: master Applies to master branch label Oct 15, 2019
@levitte
Copy link
Member Author

levitte commented Oct 15, 2019

The reason I'm making this a separate PR is because the same kind of work exists both in an upcoming key generation branch I'm working on and in @mattcaswell's #10152, and that part of the work should be synchronised, at least with regards to export/import.

@slontis
Copy link
Member

slontis commented Oct 15, 2019

Keep in mind that in fips mode multiprimes are not supported.
see the #ifdef FIPS_MODE in rsa files.
e.g: rsa_builtin_keygen

@levitte
Copy link
Member Author

levitte commented Oct 15, 2019

Sure, but that will be for key generation in the provider, no? Or should the import functions do some extra checks? But in that case, shouldn't the RSA functionality simply refuse working with multi-primes when FIPS_MODE is defined, going so far as disabling RSA_set0_multi_prime_params() completely? That's not what happens now.

@levitte
Copy link
Member Author

levitte commented Oct 15, 2019

Fixing the multiprime issue with FIPS is for another PR, methinks.

@levitte
Copy link
Member Author

levitte commented Oct 16, 2019

I added an internal keymgmt tester, test/keymgmt_internal_test.c, which tests that pushing around an RSA key through two providers (thereby exercising both import and export of key data) works as intended (it retrieves the numbers from the last provider instance and compares them to the original numbers).

@levitte levitte marked this pull request as ready for review October 16, 2019 22:44
@levitte levitte changed the title WIP: Export / import RSA to / from provider Export / import RSA to / from provider Oct 16, 2019
@levitte
Copy link
Member Author

levitte commented Oct 16, 2019

Not WIP anymore

@levitte
Copy link
Member Author

levitte commented Oct 17, 2019

Github has lost its mind... I have rebased, and yet... Let's see if a kick helps

@levitte levitte closed this Oct 17, 2019
@levitte levitte reopened this Oct 17, 2019
@levitte
Copy link
Member Author

levitte commented Oct 17, 2019

Let's see if a kick helps

Yup, it did

@levitte
Copy link
Member Author

levitte commented Oct 17, 2019

Most concerns answered, #10190 (comment) unresolved, and rebased.

@levitte
Copy link
Member Author

levitte commented Oct 17, 2019

Rebased on a fresher master. That's a moving target!

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Approved assuming the CHANGES entry is removed as suggested below.

the first of them, with the "extra" of everything in the multi-prime
RSA case. These functions handle everything via stacks of BIGNUMs
for dynamic processing.
[Richard Levitte]
Copy link
Member

Choose a reason for hiding this comment

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

This CHANGES entry no longer seems relevant since the functions aren't public any more. I think it should be deleted.

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Oct 17, 2019
levitte added a commit that referenced this pull request Oct 17, 2019
…ers.

rsa_set0_all_params() is used to set all the primes, exponents and
coefficients.  rsa_get0_all_params() is used to get all the primes,
exponents and coefficients.

"All" includes p, q, dP, dQ and qInv without making them separate.

All arrays of numbers are implemented as stacks to make dynamic use
easier.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10190)
levitte added a commit that referenced this pull request Oct 17, 2019
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10190)
levitte added a commit that referenced this pull request Oct 17, 2019
This tests diverse internal KEYMGMT features.  The current existing
test checks that evp_keymgmt_export_to_provider() passes the key data
correctly through two instances of the default provider, and that the
resulting numbers at the end match the initial numbers.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10190)
levitte added a commit that referenced this pull request Oct 17, 2019
It may be that the OSSL_PARAM array we used for getting parameter
values for a key had a few too many entries.  These are detected by
their return_size == 0.  Before making second export call, we prune
away these items so we only ask for parameters that exist.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10190)
@levitte
Copy link
Member Author

levitte commented Oct 17, 2019

Merged. Pfew! Thanks for the thorough review!

c3a4fa4 Added internal functions for easy getting and setting all RSA parameters.
29be602 New RSA keymgmt implementation to handle import / export of RSA keys
5a02d13 test/keymgmt_internal_test.c: New test of keymgmt internals
651101e evp_keymgmt_export_to_provider(): adjust OSSL_PARAM array for transfer

@richsalz
Copy link
Contributor

To make sure I understand: in the non-multiprime case you have several stacks of one entry.
In the multi-prime case you have to iterate through all the stacks in parallel?

@levitte
Copy link
Member Author

levitte commented Oct 18, 2019

In the 2-prime case, the factors or primes stack has two numbers (commonly known as p and q), the exps stack has two numbers has two numbers (commonly known as dP or dmp1, and dQ or dmq1), and the coeffs stack has one number (commonly known as qInv or iqmp). Whether you are building a 2-prime or a n-prime key, processing them in parallel is more practical because of the way the RSA structure is constructed (which, BTW, is directly related to the ASN.1 spec)

@richsalz
Copy link
Contributor

thanks. dees coeffs have to be a stack?

@levitte
Copy link
Member Author

levitte commented Oct 18, 2019

It's more practical that way, for where those functions are used. For n-primes, there's more than one, and I see very little benefit in special-casing 2-prime keys.

@richsalz
Copy link
Contributor

I missed the "for n-primes" part. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants