@mattcaswell - I am a bit stuck with how to plumb this in..

It looks nearly like the normal rsa encrypt except the thing to encrypt is an internally generated random secret - not a passed in input..
The return from the encrypt (it is called generate) is the ciphertext and the generated secret.

The decrypt (recover) is just a straight decrypt (with a length check) to recover the secret..

The operations use RSA oeap.

So I am wondering if I should have a generate and recover function that I just add to the rsa_enc dispatch table and then have EVP_PKEY_generate_init(), EVP_PKEY_generate() & EVP_PKEY_recover_init(), EVP_PKEY_recover() API's..

I have tried to create a separate function table and then dont quite know how to select it..
I could set a parameter to flag sve mode and then just use the encrypt/decrypt API's instead..

I believe it would be better to drop support for this in 3.0 or, if we really need it in, we should do the proper thing and treat RSA SVE as a KEM.

KEM as an abstract operation has 3 fundamental components:

• keygen: KG() --> (privk, pubk)
  • generate a key pair consisting of a private and a public part
• encapsulate: Enc(pubk) --> (C, W)
  • a symmetric cipher key (W) is generated and encapsulated as a ciphertext C under the public key pubk.
  • this is a single operation returning at the same time both C and W
  • in some cases, one can implement this composing two separate asym and sym ciphers, but for the general case we need this to be a one-shot operation (and if I am interpreting the RSA SVE requirements for FIPS this is the case here as well).
  • one can think of KEM as abstracting a whole lot of operations inside of Enc: e.g., asym encryption + nonce generation + KDF step + authenticated encryption to ensure tampering is detected, etc. In a proper KEM design all the properties and knobs of each of these primitives are tuned, and proofs provided, so that the end result is not a sum of the potential pitfalls of the underlying blocks, but a robust chain where every ring has the required set of guarantees for the given threat model and equivalent bits of security
• decapsulate: Dec(privk, C) --> W
  • a symmetric cipher key (W) is decapsulated from the ciphertext C using the secret key privk
  • W could take a special rejection value in case C was tampered with, the used privk did not match the original pubk, etc.

We would need:

• to define a new abstract operation for KEM to support encapsulate/decapsulate
• ensure that the keymgmt infrastructure allows enough flexibility to accomodate KEM keys
• enrich the internal plumbing to be able to route the new operation to provider algorithms
• test, test, test, test
• introduce a user friendly EVP_KEM API (or similar, namimg is always a source for debate) to expose the new operations to applications without having to dig into the details of the provider API
• test moar

"Abusing" the asym cipher operation in the way the draft currently does might work as a special handling for RSA SVE, but is a hack and I am afraid we would have to pay the cost of supporting this hack in the future, exactly as we are paying the price of merging SM2 too close to the beta of 1.1.1 and having to deal with a lot of ugliness now (and for many years to come).

I believe it would be better to drop support for this in 3.0 or, if we really need it in, we should do the proper thing and treat RSA SVE as a KEM.

As previously stated - it is a fundamental requirement of SP800-56B - so it needs to go in.

I think the approach needed is a minimal interface that can be expanded later using set_params to do other operations if required..

So a KEM looks like this..

(from https://lists.w3.org/Archives/Public/public-xmlsec/2009May/att-0032/Key_Encapsulation.pdf)

1. Generate a random integer z (0 ≤z≤n-1) z = RandomInteger (0, n-1)
2. Encrypt the random integer z using the recipient's public key (n,e) c = ze mod n
3. Derive a key-encrypting key KEK of length kekLen bytes from z using the underlying key derivation function KEK = KDF (z, kekLen)
4. Wrap the keying data K with the key-encrypting key KEK using the underlying key-wrapping scheme to obtain wrapped keying data WK WK = Wrap (KEK, K)
5. Concatenate the ciphertext C and the wrapped keying data WK to obtain the encrypted keying data EK EK = C || WK 6. Output the encrypted keying data EK
   .....
   To be useful it would output (EK, z) which is similiar to outputting (c, z)
   ....

For now I only need steps 1 & 2 because that is what is required for Sp800-56b.
e.g:

Party U                                                                                               PartyV
(PubKeyU, PrivKeyU)                                                                         (PubKeyV, PrivKeyV)
Obtain party V’s public key-establishment              ← PubKeyV
                                                                                   PubKeyU →   Obtain party U’s public key-establishment key
(ZU, CU) = RSASVE.GENERATE(PubKeyV)               CU→              ZU =  RSASVE.RECOVER(PrivKeyV, CU)
 ZV =  RSASVE.RECOVER(PrivKeyU, CV)                 ←CV              (ZV, CV) = RSASVE.GENERATE(PubKeyU)
Z = ZU || ZV                                                                                      Z = ZU || ZV
Compute DerivedKeyingMaterial and destroy Z                             Compute DerivedKeyingMaterial and destroy Z

Hopefully you can see that a KDF + WRAP is not needed here..

NOTES:
(1) There is a section titled
'9.3 Hybrid Key-Transport Methods' - But that just says to use AES key wrapping on the KEK..
(2) Previous version of SP800-56B specifically mentioned using KEM, but it has all been removed in the latest standard.

Being able to add a KDF and wrap step can be done later iff they are needed (This could be done with set_params() OR another API..)..

I believe it would be better to drop support for this in 3.0 or, if we really need it in, we should do the proper thing and treat RSA SVE as a KEM.

As previously stated - it is a fundamental requirement of SP800-56B - so it needs to go in.

Why SP800-56B must be supported and claimed by the OpenSSL 3.0 FIPS module? There are other FIPS compliant key establishment methods supported (ephemeral DH and ECDH namely). The SP800-56B is unusable by TLS anyway.

56A and 56B were part of the specs that we said we support in the initial design..
It is not just about TLS.

I nearly have a implementation taking into account @romen's comments..

The thing I initially did was overly complicated... I for some reason thought it was using OEAP.. it doesnt, so it is a simple RSA operation..

Is this still draft, or ready for review? I note we are missing documentation for the new EVP_ASYM_WRAP_* functions. Probably also needs a make update

Is this still draft, or ready for review? I note we are missing documentation for the new EVP_ASYM_WRAP_* functions. Probably also needs a make update

I just sent a email in relation to this PR.. Feedback on the API's and the passed parameters in particular would be welcome..

I would prefer to not do documentation until people are happy with the direction of this PR.

I haven't played with it, and so far only looked at the code, but so far this looks very good to me!

Thanks @slontis for taking in my feedback!

I have two notes so far:

• I see we generate RSA keys and then use them with wrap/unwrap assuming you will get RSASVE, but I don't see a change in the rsa keymgmt of RSA to return "RSASVE" on a queryopname, how are they glued together?
• Following up from above. So far we have assumed the convention that 1 key type (e.g RSA) means only one implementation for a given operation (in this case wrap). With this PR we are implicitly making RSASVE the default KEM for RSA (and changing this in the future would be a breaking change). Are we committed to this or would it be better to have a separate RSASVE keymgmt (supporting only wrap/unwrap) and generate a RSASVE key rather than a RSA key?

As a side note, and trying to avoid yet another naming war by disclaiming that I am not particularly against wrap/unwrap, I wonder if KEM would be less surprising for external future users and if we are diverging from existing patterns by having

• EVP_PKEY_wrap_init
• EVP_PKEY_wrap_encapsulate
• EVP_PKEY_unwrap_init
• EVP_PKEY_unwrap_decapsulate

instead of

• EVP_PKEY_wrap_encapsulate_init
• EVP_PKEY_wrap_encapsulate
• EVP_PKEY_wrap_decapsulate_init
• EVP_PKEY_wrap_decapsulate

(I would prefer EVP_PKEY_KEM_*)

* I see we generate RSA keys and then use them with wrap/unwrap assuming you will get RSASVE, but I don't see a change in the rsa keymgmt of RSA to return "RSASVE" on a queryopname, how are they glued together?

I have done this by passing the algorithm to the init (so that it fetches the correct algorithm without having to use the default query) e.g.
EVP_PKEY_wrap_init(sctx, "RSASVE");

hmm yes my rsasve_init() assumes the thing it is being passed is a RSA*..
It seems quite horrible that we need a separate keymanager for everything..
I guess it could just be a nearly identical table with a different query + encoders..
Looks like I need to change to what you suggest..

(I would prefer EVP_PKEY_KEM_*)

Does this name really apply to the RSASVE?

If we need to allow a user to supply their own symmetric key what would be the best way of supplying this info..
A separate API function (or via set_params)?

Taken out of draft now that it has been documented.
Updated to use KEM instead of wrap for the name.
NOTE I have not made a new keymanager for RSASVE.. I have just added an alias..

• I see we generate RSA keys and then use them with wrap/unwrap assuming you will get RSASVE, but I don't see a change in the rsa keymgmt of RSA to return "RSASVE" on a queryopname, how are they glued together?

I have done this by passing the algorithm to the init (so that it fetches the correct algorithm without having to use the default query) e.g.
EVP_PKEY_wrap_init(sctx, "RSASVE");

Are there other operations where we do this? Also this would probably also require a propquery arg?

hmm yes my rsasve_init() assumes the thing it is being passed is a RSA*..

This might be an issue, also it opens a door for an RSA key coming from a provider and an RSASVE implementation coming from another provider, and no key conversion inbetween, probably resulting in all kinds of segfaults!

It seems quite horrible that we need a separate keymanager for everything..

To avoid adding a separate RSASVE keymgmt we could make RSASVE the default KEM for RSA, and let RSAFOO or RSABAR in OpenSSL 3.42 deal with adding the new keymgmts for them.
But, this would mean that for external providers, from 3.0 forward, for eternity, the RSA KEM operation must be RSASVE.
To me it seems a bit late in the release a process to take such a drastic decision, without proper consideration of what the cost of it will be for the future.

So if we don't embed RSASVE in the RSA keymgmt, and we stick with the convention we decided upon of only one operation implementation for key type, it looks like adding a new keymgmt for a new operation is inevitable..

I guess it could just be a nearly identical table with a different query + encoders..

... but as you suggest actual code duplication should be minimal.

Let's assume there are alternative KEM implementations built on top of RSA (which is not a fantasy scenario, given that there is an exponential number of alternative combinations of symm ciphers, kdf, aead modes for wrapping and knobs to play with): if FIPS already mandated 2 alternative ones and we already were implementing both of them now, having separate keymgmts would be the only way and probably we wouldn't be even discussing it!

(I would prefer EVP_PKEY_KEM_*)

Does this name really apply to the RSASVE?

In my limited knowledge and under the duck test I ran, it seems to apply: as a black box it definitely seems to behave as one.

Under the hood it might not come with all the bells and whistles of other academic results with full-fledged security proofs (I am not saying that it does not, just that I did not do enough research to confirm nor deny it), but its API does quack like a KEM API.

If we need to allow a user to supply their own symmetric key what would be the best way of supplying this info..
A separate API function (or via set_params)?

This is an antipattern with KEM, I see the lure of it for testing purposes, but exposing such a mechanism to users opens many potential cans of worms (not the ones you would use for bait, more like the ones from Arrakis).

Still an experimental external provider that is building its implementation and needs to access the guts could temporarily expose a settable parameter to do this.
So I think we are providing enough flexibility there, but I would refrain from exposing such a thing to our users in our builtin providers.

For the same reason, while it is ok if a provider decides to offer such a functionality to users through EVP_PKEY_CTX_set_params() I wouldn't encourage such task by adding a new dedicated function in the API.

Rebased..
ping

I applied these 2 patches on top of 95a2bdc4e8b884bd118f442becd8d21364c1325a

From 44169cd4065de2caec06405c02b98779b2726979 Mon Sep 17 00:00:00 2001
From: Nicola Tuveri <[email protected]>
Date: Tue, 8 Sep 2020 13:18:34 +0300
Subject: [PATCH 1/2] [test][rsasve] Use TEST_int_eq instead of TEST_true

The newly added documentation states that

- `EVP_PKEY_KEM_encapsulate_init`
- `EVP_PKEY_KEM_encapsulate`
- `EVP_PKEY_KEM_decapsulate_init`
- `EVP_PKEY_KEM_decapsulate`

Should return 1 for success, 0 or negative for failure (and explicitly
-2 if the operation is not supported).

This commit replace TEST_true(foo()) with TEST_int_eq(foo(), 1) to test
for success of these functions within the rsasve_gen_recover() test.
---
 test/evp_libctx_test.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/test/evp_libctx_test.c b/test/evp_libctx_test.c
index d73f6eeaef..f56a2e60c6 100644
--- a/test/evp_libctx_test.c
+++ b/test/evp_libctx_test.c
@@ -485,25 +485,27 @@ static int rsasve_gen_recover(void)
     if (!TEST_ptr(sctx = EVP_PKEY_CTX_new_from_pkey(libctx, pub, NULL)))
         goto done;
 
-    if (!TEST_true(EVP_PKEY_KEM_encapsulate_init(sctx)))
+    if (!TEST_int_eq(EVP_PKEY_KEM_encapsulate_init(sctx), 1))
         goto done;
-    if (!TEST_true(EVP_PKEY_KEM_encapsulate(sctx, NULL, &ctlen, NULL,
-                                            &secretlen)))
+    if (!TEST_int_eq(EVP_PKEY_KEM_encapsulate(sctx, NULL, &ctlen, NULL,
+                                              &secretlen), 1))
         goto done;
     if (!TEST_int_eq(ctlen, secretlen)
         || !TEST_int_eq(ctlen, 2048 / 8))
         goto done;
-    if (!TEST_true(EVP_PKEY_KEM_encapsulate(sctx, ct, &ctlen, secret,
-                                            &secretlen)))
+    if (!TEST_int_eq(EVP_PKEY_KEM_encapsulate(sctx, ct, &ctlen, secret,
+                                              &secretlen), 1))
         goto done;
 
     if (!TEST_ptr(rctx = EVP_PKEY_CTX_new_from_pkey(libctx, priv, NULL)))
         goto done;
-    if (!TEST_true(EVP_PKEY_KEM_decapsulate_init(rctx)))
+    if (!TEST_int_eq(EVP_PKEY_KEM_decapsulate_init(rctx), 1))
         goto done;
-    if (!TEST_true(EVP_PKEY_KEM_decapsulate(rctx, NULL, &unwraplen, ct, ctlen)))
+    if (!TEST_int_eq(EVP_PKEY_KEM_decapsulate(rctx, NULL, &unwraplen,
+                                              ct, ctlen), 1))
         goto done;
-    if (!TEST_true(EVP_PKEY_KEM_decapsulate(rctx, unwrap, &unwraplen, ct, ctlen)))
+    if (!TEST_int_eq(EVP_PKEY_KEM_decapsulate(rctx, unwrap, &unwraplen,
+                     ct, ctlen), 1))
         goto done;
     if (!TEST_mem_eq(unwrap, unwraplen, secret, secretlen))
         goto done;
-- 
2.27.0

From 36f23f47cb1a5fdb86b24ebeedac11df55400c96 Mon Sep 17 00:00:00 2001
From: Nicola Tuveri <[email protected]>
Date: Tue, 8 Sep 2020 13:33:25 +0300
Subject: [PATCH 2/2] [test] plain RSA keys should not support EVP_PKEY_KEM
 operations

This adds negative testing to ensure that a plain RSA key does not
default to RSASVE if used in the context of the `EVP_PKEY_KEM_*`
operations.

Making RSASVE the default is not wrong per se, but we don't want to
commit to this behavior now without further consideration: in a future
release we might want to assign a default KEM algorithm for RSA keys and
we don't want to be held back by accidentally creating a default.
---
 test/evp_libctx_test.c | 59 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/test/evp_libctx_test.c b/test/evp_libctx_test.c
index f56a2e60c6..f75f977dd1 100644
--- a/test/evp_libctx_test.c
+++ b/test/evp_libctx_test.c
@@ -518,6 +518,64 @@ done:
     return ret;
 }
 
+/*
+ * This test replicates 1:1 rsasve_gen_recover() changing RSASVE to RSA as the
+ * algorithm name to fetch, and expecting failures instead of successes.
+ *
+ * We want to ensure at this point that a "plain RSA" key does not support KEM
+ * operations (defaulting to RSASVE due to aliasing).
+ *
+ * This is because later on, in a future release, we might want to assign an
+ * explicit default KEM algorithm for "plain RSA" keys, depending on
+ * standardization results, and we shouldn't be held back by the accidental
+ * creation of defaults.
+ */
+static int rsasve_gen_recover_negative(void)
+{
+    int ret = 0;
+    EVP_PKEY *pub = NULL;
+    EVP_PKEY *priv = NULL;
+    EVP_PKEY_CTX *sctx = NULL, *rctx = NULL;
+    unsigned char secret[256] = { 0, };
+    unsigned char ct[256] = { 0, };
+    unsigned char unwrap[256] = { 0, };
+    size_t ctlen = 0, unwraplen = 0, secretlen = 0;
+    const char *alg = "RSA";
+
+    if (!TEST_true(rsa_keygen(alg, 2048, &pub, &priv)))
+        goto done;
+
+    if (!TEST_ptr(sctx = EVP_PKEY_CTX_new_from_pkey(libctx, pub, NULL)))
+        goto done;
+
+    if (!TEST_int_eq(EVP_PKEY_KEM_encapsulate_init(sctx), -2))
+        goto done;
+    if (!TEST_int_lt(EVP_PKEY_KEM_encapsulate(sctx, NULL, &ctlen, NULL,
+                                              &secretlen), 1))
+        goto done;
+    if (!TEST_int_lt(EVP_PKEY_KEM_encapsulate(sctx, ct, &ctlen, secret,
+                                              &secretlen), 1))
+        goto done;
+
+    if (!TEST_ptr(rctx = EVP_PKEY_CTX_new_from_pkey(libctx, priv, NULL)))
+        goto done;
+    if (!TEST_int_eq(EVP_PKEY_KEM_decapsulate_init(rctx), -2))
+        goto done;
+    if (!TEST_int_lt(EVP_PKEY_KEM_decapsulate(rctx, NULL, &unwraplen,
+                                              ct, ctlen), 1))
+        goto done;
+    if (!TEST_int_lt(EVP_PKEY_KEM_decapsulate(rctx, unwrap, &unwraplen,
+                     ct, ctlen), 1))
+        goto done;
+    ret = 1;
+done:
+    EVP_PKEY_free(pub);
+    EVP_PKEY_free(priv);
+    EVP_PKEY_CTX_free(rctx);
+    EVP_PKEY_CTX_free(sctx);
+    return ret;
+}
+
 int setup_tests(void)
 {
     const char *prov_name = "default";
@@ -573,6 +631,7 @@ int setup_tests(void)
     ADD_ALL_TESTS(test_cipher_reinit, sk_OPENSSL_CSTRING_num(cipher_names));
     ADD_ALL_TESTS(test_cipher_reinit_partialupdate,
                   sk_OPENSSL_CSTRING_num(cipher_names));
+    ADD_TEST(rsasve_gen_recover_negative);
     ADD_TEST(rsasve_gen_recover);
     return 1;
 }
-- 
2.27.0

This results in the following failures:

30-test_evp_libctx.t .. 1/?
    # ERROR: (int) 'EVP_PKEY_KEM_encapsulate_init(sctx) == -2' failed @ test/evp_libctx_test.c:551
    # [1] compared to [-2]
    not ok 6 - rsasve_gen_recover_negative
# ------------------------------------------------------------------------------
    # ERROR: (int) 'EVP_PKEY_KEM_encapsulate(sctx, ct, &ctlen, secret, &secretlen) == 1' failed @ test/evp_libctx_test.c:497
    # [256] compared to [1]
    not ok 7 - rsasve_gen_recover
# ------------------------------------------------------------------------------
../../util/wrap.pl ../../test/evp_libctx_test -config ../../test/fips.cnf -provider fips => 1
not ok 2 - running fips evp_libctx_test
# ------------------------------------------------------------------------------
    # ERROR: (int) 'EVP_PKEY_KEM_encapsulate_init(sctx) == -2' failed @ test/evp_libctx_test.c:551
    # [1] compared to [-2]
    not ok 6 - rsasve_gen_recover_negative
# ------------------------------------------------------------------------------
    # ERROR: (int) 'EVP_PKEY_KEM_encapsulate(sctx, ct, &ctlen, secret, &secretlen) == 1' failed @ test/evp_libctx_test.c:497
    # [256] compared to [1]
    not ok 7 - rsasve_gen_recover
# ------------------------------------------------------------------------------
../../util/wrap.pl ../../test/evp_libctx_test -config ../../test/default-and-legacy.cnf => 1
not ok 3 - running default-and-legacy evp_libctx_test
30-test_evp_libctx.t .. 3/? ----------------------------------------------------
#   Failed test 'running default-and-legacy evp_libctx_test'
#   at test/recipes/30-test_evp_libctx.t line 44.
30-test_evp_libctx.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/3 subtests

Test Summary Report
-------------------
30-test_evp_libctx.t (Wstat: 512 Tests: 3 Failed: 2)
  Failed tests:  2-3
  Non-zero exit status: 2
Files=1, Tests=3,  1 wallclock secs ( 0.04 usr  0.01 sys +  1.10 cusr  0.02 csys =  1.17 CPU)
Result: FAIL

Also notice that, without applying the 2 patches above, replacing alg = "RSASVE" with "RSA" results in the original version of the test passing without issues.

Ok should be reviewable now..

rebased again

@romen - are you planning to review this PR?

@romen I hope you are not too upset that I rebased to collapse the commits (effectively removing your commits).
The various commits changed the functionality too much to leave them as they were (even your tests got mostly replaced).
I have credited you in the commit - if you would like me to update what it says I would be happy to do so.
Considering that you made a significant contribution to this PR.

The rebase did not affect the content - but it did modify the commit message.

@romen I hope you are not too upset that I rebased to collapse the commits (effectively removing your commits).
The various commits changed the functionality too much to leave them as they were (even your tests got mostly replaced).

Not at all!

I have credited you in the commit - if you would like me to update what it says I would be happy to do so.
Considering that you made a significant contribution to this PR.

In the past I have used this style to credit co-authors, as it is understood by github and other tools.

https://docs.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors

I am not asking you to change it, it's just a note in case you were not aware of this tag!

https://docs.github.com/en/github/committing-changes-to-your-project/creating-a-commit-with-multiple-authors

I think this is a good idea to follow, generally speaking. Perhaps we should make it policy...

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

Unfortunately the travis failures look relevant.

Just a simple make update to regenerate libcrypto.num was required. No other changes were made.

Thanks... Merged to master.