Conversation
No objections. IMO this is the right way. It is similar to the ECX algorithms. |
doc/man7/EVP_KEM-MLKEM.pod
Outdated
| EVP_KEM-MLKEM, | ||
| EVP_KEM-ML-KEM |
There was a problem hiding this comment.
IMO, this should look like this:
| EVP_KEM-MLKEM, | |
| EVP_KEM-ML-KEM | |
| EVP_KEM-ML-KEM-768 |
The name after the EVP_KEM- prefix should be the name as it is fetched from the provider, i.e. the names as listed in the PROV_NAMES_MLKEM768 define. It is not possible to "fetch" an algorithm called "ML-KEM" or "MLKEM" so we should not list them here. We can extend the list to include "ML-KEM-512" and "ML-KEM-768" as we implement them
There was a problem hiding this comment.
ACK. See below. Let me play with the doc-nits script how to bring your logical argument (that I agree with) and that check in line...
There was a problem hiding this comment.
Now changed adding all possibly key types -- but had to leave "generic" EVP_KEM-MLKEM to satisfy make doc-nits.
doc/man7/EVP_PKEY-MLKEM.pod
Outdated
| EVP_PKEY-MLKEM, | ||
| EVP_PKEY-ML-KEM, | ||
| EVP_KEYMGMT-ML-KEM, | ||
| EVP_PKEY-ML-KEM-768 |
There was a problem hiding this comment.
Again the only valid algorithm name here is ML-KEM-768. You can't "fetch" anything else.
There was a problem hiding this comment.
Well, the name "EVP_PKEY-MLKEM" I only added after a test complained that it needs adding. The next two to allow the links from default provider to work -- exactly to avoid having to change more than 2 files as and when -512 and -1024 get added as per my initial comment.
There was a problem hiding this comment.
Changed equivalent as for PKEY above.
There was a problem hiding this comment.
You could try adding this somewhere:
=for openssl generic
(the empty lines surrounding it are significant)
doc/man7/OSSL_PROVIDER-default.pod
Outdated
|
|
||
| =item EC, see L<EVP_KEM-EC(7)> | ||
|
|
||
| =item ML-KEM-768, see L<EVP_KEM-ML-KEM(7)> |
There was a problem hiding this comment.
| =item ML-KEM-768, see L<EVP_KEM-ML-KEM(7)> | |
| =item ML-KEM-768, see L<EVP_KEM-ML-KEM-768(7)> |
There was a problem hiding this comment.
OK -- so basically the statement is that all these files need updating as -512 and -1024 get added?
doc/man7/OSSL_PROVIDER-default.pod
Outdated
|
|
||
| =item SM2, see L<EVP_KEYMGMT-SM2(7)> | ||
|
|
||
| =item ML-KEM-768, see L<EVP_KEYMGMT-ML-KEM(7)> |
There was a problem hiding this comment.
| =item ML-KEM-768, see L<EVP_KEYMGMT-ML-KEM(7)> | |
| =item ML-KEM-768, see L<EVP_KEYMGMT-ML-KEM-768(7)> |
There was a problem hiding this comment.
Changed to all specific key types in 5b7aad1
doc/man7/provider-keymgmt.pod
Outdated
| L<EVP_PKEY-X25519(7)>, L<EVP_PKEY-X448(7)>, L<EVP_PKEY-ED25519(7)>, | ||
| L<EVP_PKEY-ED448(7)>, L<EVP_PKEY-EC(7)>, L<EVP_PKEY-RSA(7)>, | ||
| L<EVP_PKEY-DSA(7)>, L<EVP_PKEY-DH(7)> | ||
| L<EVP_PKEY-DSA(7)>, L<EVP_PKEY-DH(7)>, L<EVP_PKEY-MLKEM(7)> |
There was a problem hiding this comment.
| L<EVP_PKEY-DSA(7)>, L<EVP_PKEY-DH(7)>, L<EVP_PKEY-MLKEM(7)> | |
| L<EVP_PKEY-DSA(7)>, L<EVP_PKEY-DH(7)>, L<EVP_PKEY-ML-KEM-768(7)> |
There was a problem hiding this comment.
Changed to all specific key types in 5b7aad1
doc/man3/EVP_PKEY_decapsulate.pod
Outdated
| L<EVP_PKEY_CTX_new_from_pkey(3)>, | ||
| L<EVP_PKEY_encapsulate(3)>, | ||
| L<EVP_KEM-RSA(7)>, L<EVP_KEM-X25519(7)>, L<EVP_KEM-EC(7)> | ||
| L<EVP_KEM-RSA(7)>, L<EVP_KEM-X25519(7)>, L<EVP_KEM-EC(7)>, L<EVP_KEM-MLKEM(7)> |
There was a problem hiding this comment.
| L<EVP_KEM-RSA(7)>, L<EVP_KEM-X25519(7)>, L<EVP_KEM-EC(7)>, L<EVP_KEM-MLKEM(7)> | |
| L<EVP_KEM-RSA(7)>, L<EVP_KEM-X25519(7)>, L<EVP_KEM-EC(7)>, L<EVP_KEM-ML-KEM-768(7)> |
There was a problem hiding this comment.
Changed to all specific key types in 5b7aad1
doc/man3/EVP_PKEY_encapsulate.pod
Outdated
| L<EVP_PKEY_CTX_new_from_pkey(3)>, | ||
| L<EVP_PKEY_decapsulate(3)>, | ||
| L<EVP_KEM-RSA(7)>, L<EVP_KEM-X25519(7)>, L<EVP_KEM-EC(7)> | ||
| L<EVP_KEM-RSA(7)>, L<EVP_KEM-X25519(7)>, L<EVP_KEM-EC(7)>, L<EVP_KEM-MLKEM(7)> |
There was a problem hiding this comment.
| L<EVP_KEM-RSA(7)>, L<EVP_KEM-X25519(7)>, L<EVP_KEM-EC(7)>, L<EVP_KEM-MLKEM(7)> | |
| L<EVP_KEM-RSA(7)>, L<EVP_KEM-X25519(7)>, L<EVP_KEM-EC(7)>, L<EVP_KEM-ML-KEM-768(7)> |
There was a problem hiding this comment.
Changed to all specific key types in 5b7aad1
|
|
||
| The OpenSSL ML-KEM encapsulation mechanism can only be modified by | ||
| setting randomness during encapsulation facilitating testing as per | ||
| FIPS 203, section 6.2, algorithm 17. |
There was a problem hiding this comment.
This wording is a little unclear to me what this actually does. Can we clarify it?
There was a problem hiding this comment.
Encapsulation normally consumes 32 bytes of RNG entropy, which is hashed to produce other random values within the encapsulation process. Specifying this explicitly, instead of generating it randomly, make encasulation fully deterministic.
|
|
||
| In addition to the common parameters that all keytypes should support (see | ||
| L<provider-keymgmt(7)/Common parameters>), the implementation of these keytypes | ||
| support the following. |
There was a problem hiding this comment.
Are these gettable, settable or both?
There was a problem hiding this comment.
Mostly gettable. What's settable are private or public keys, the seed for key generation and the entropy for encapsulation.
There was a problem hiding this comment.
We should whether they are gettable/settable in the documentation
doc/man7/EVP_PKEY-MLKEM.pod
Outdated
| These implementations support the associated key, containing the public | ||
| key I<pub> and the private key I<priv>. |
| ML-KEM internally requires the generation of a keypair using a random value (seed). | ||
| This optional parameter can be used to set the value prior to key pair generation | ||
| or for retrieval of the random value created internally after keypair generation. | ||
| According to FIPS 203, section 3.3, this parameter should only be used for | ||
| test purposes and be treated with the same care as private key material. | ||
| The length of the seed is 64 bytes. |
There was a problem hiding this comment.
See above comment. The code DOES NOT currently allow retrieval of the seed when randomly generated.
There was a problem hiding this comment.
fixed in latest commit.
|
|
||
| In addition to the common parameters that all keytypes should support (see | ||
| L<provider-keymgmt(7)/Common parameters>), the implementation of these keytypes | ||
| support the following. |
There was a problem hiding this comment.
Mostly gettable. What's settable are private or public keys, the seed for key generation and the entropy for encapsulation.
|
|
||
| =item "pub" (B<OSSL_PKEY_PARAM_PUB_KEY>) <octet string> | ||
|
|
||
| The public key value. |
There was a problem hiding this comment.
This is its standard wire form encoding as the vector t and the matrix seed rho.
| Used for getting and setting the encoding of a public key. Public keys are | ||
| expected be encoded in a format as defined by FIPS 203. |
There was a problem hiding this comment.
If I got the discussion right this morning, you'll be adding logic that needs to be described here anyway, so I leave "generic" as-is for now such as to close out this PR.
There was a problem hiding this comment.
Actually, at this point setting the public key is not possible if any key is already set, it can only be set for a key that is still under construction.
|
I've pulled my IKME changes into a separate PR (#26082). |
Reviewed, looks OK, but please rebase. The provider changes will otherwise conflict. |
| Used for getting and setting the encoding of a public key. Public keys are | ||
| expected be encoded in a format as defined by FIPS 203. |
There was a problem hiding this comment.
If I got the discussion right this morning, you'll be adding logic that needs to be described here anyway, so I leave "generic" as-is for now such as to close out this PR.
|
@vdukhovni @mattcaswell I went through all feedback I felt possible to act upon now, commented on those that are still WIP (i.e., topic for a new iteration of documentation as agreed-upon this morning) and thus would like to suggest another review/merge for now so @vavroch2010 can clean out the sprint. |
mattcaswell
left a comment
There was a problem hiding this comment.
Mostly good. Some minor tweaks to address and I could approve this.
doc/designs/mlkem.md
Outdated
| ML-KEM makes extensive use of SHA3 primitives, SHA3-256, SHA3-512, SHAKE256 and SHAKE128. | ||
| To improve ML-KEM execution performance the EVP handles for these are pre-fetched during ML-KEM | ||
| key initialisation and stored in an MLKEM_CTX object. | ||
| These then used in key generation, encapsulation and decapsulation. |
doc/designs/mlkem.md
Outdated
| Keys can therefore be generated as "usual" by way of the EVP functions | ||
| EVP_PKEY_generate() and EVP_PKEY_Q_keygen(). | ||
|
|
||
| The "seed format" can be selected by either setting the OSSL_PARAM value |
There was a problem hiding this comment.
| The "seed format" can be selected by either setting the OSSL_PARAM value | |
| The "seed format" can be selected by setting the OSSL_PARAM value |
doc/man7/EVP_KEM-MLKEM.pod
Outdated
|
|
||
| =over 4 | ||
|
|
||
| =item "entropy" (B<OSSL_KEM_PARAM_MLKEM_ENC_ENTROPY>)<UTF8 string> |
There was a problem hiding this comment.
We should specify whether this is gettable or settable
|
|
||
| In addition to the common parameters that all keytypes should support (see | ||
| L<provider-keymgmt(7)/Common parameters>), the implementation of these keytypes | ||
| support the following. |
There was a problem hiding this comment.
We should whether they are gettable/settable in the documentation
Thanks @mattcaswell . All comments incorporated in latest commit. |
doc/designs/mlkem.md
Outdated
| ML-KEM makes extensive use of SHA3 primitives, SHA3-256, SHA3-512, SHAKE256 and SHAKE128. | ||
| To improve ML-KEM execution performance the EVP handles for these are pre-fetched during ML-KEM | ||
| key initialisation and stored in an MLKEM_CTX object. | ||
| These then used in key generation, encapsulation and decapsulation. |
There was a problem hiding this comment.
| These then used in key generation, encapsulation and decapsulation. | |
| These are then used in key generation, encapsulation and decapsulation. |
| NIST guidance, only for testing. If the seed version is retrieved from a | ||
| normal key generation operation, it shall be subject to the same level of | ||
| protection given to private key material. |
There was a problem hiding this comment.
| NIST guidance, only for testing. If the seed version is retrieved from a | |
| normal key generation operation, it shall be subject to the same level of | |
| protection given to private key material. | |
| NIST guidance, only for testing. |
doc/man7/EVP_KEM-MLKEM.pod
Outdated
|
|
||
| =over 4 | ||
|
|
||
| =item "entropy" (B<OSSL_KEM_PARAM_MLKEM_ENC_ENTROPY>)<UTF8 string> |
|
|
||
| The OpenSSL ML-KEM encapsulation mechanism can only be modified by | ||
| setting randomness during encapsulation facilitating testing as per | ||
| FIPS 203, section 6.2, algorithm 17. |
There was a problem hiding this comment.
Encapsulation normally consumes 32 bytes of RNG entropy, which is hashed to produce other random values within the encapsulation process. Specifying this explicitly, instead of generating it randomly, make encasulation fully deterministic.
|
|
||
| =back | ||
|
|
||
| This can be set when using EVP_PKEY_encapsulate_init(). |
| Used for getting and setting the encoding of a public key. Public keys are | ||
| expected be encoded in a format as defined by FIPS 203. |
There was a problem hiding this comment.
Actually, at this point setting the public key is not possible if any key is already set, it can only be set for a key that is still under construction.
|
@vdukhovni I see the merit of some of your change requests (some had already been incorporated prior to your feedback) in light of all your improvements. However, I understood our agreement from last week to close this out based on the then-current code base and for @vavroch2010 to create a new tracking issue for another sprint and a consequent new documentation PR after the dust has settled (after #26054 has landed) as the stated goal/limitation was to not shift this PR into another sprint. I now see two mutually exclusive ways forward:
I'll wait with further work until a decision has been made on whether 1) or 2) "rules". My preference is 2) ("process should not trump progress") but it is not what has been agreed last week. |
@baentsch - I don't see my comments about specifying all params as either gettable or settable as having been addressed? |
@mattcaswell I am of the opinion that it has (or that no further documentation is warranted: Or do you also expect params to be documented that are default ones, i.e., OSSL_PKEY_PARAM_PUB_KEY, OSSL_PKEY_PARAM_PRIV_KEY, OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY, OSSL_PKEY_PARAM_BITS, OSSL_PKEY_PARAM_SECURITY_BITS, OSSL_PKEY_PARAM_MAX_SIZE ? The one MLKEM PKEY specific one, "OSSL_PKEY_PARAM_MLKEM_SEED" is documented along this line and reads
Or do you prefer a specific additional statement along the lines "This parameter can only be set." or even (additionally?) "This parameter cannot be retrieved."? The MLKEM KEM "OSSL_KEM_PARAM_MLKEM_ENC_ENTROPY" param does have such very explicit statement even though upon re-reading that seems a bit "too much"/"in your face" in my eyes. Just let me know what you prefer and/or is the norm for other docs and I'll adapt. |
Something like that would be helpful I think. It is implied that it can only be set, but only by absence of anything being said about getting. Nothing is said about the getability/setability of OSSL_PKEY_PARAM_PUB_KEY, OSSL_PKEY_PARAM_PRIV_KEY - so I think we should be more explicit here. |
|
I have a bunch of minor tweaks to existing docs with mostly breadcrumb comments about specific places to mention MLKEM in the future, and a couple of places where it was already possible before this PR lands. Should I hold on to these, or push them to this PR? |
Explicit statement added.
Usage reference added. |
mattcaswell
left a comment
There was a problem hiding this comment.
@vdukhovni please reconfirm
|
This pull request is ready to merge |
|
Squashed and merged to the feature branch. Thank you. |
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#26037)
Fixes openssl/project#1008.
Open question: Objections to putting all documentation for all ML-KEM algorithms in one file? References to other variants should be simply added to the 2 new core files doc/man7/EVP_KEM-MLKEM.pod and as and doc/man7/EVP_PKEY-MLKEM.pod when #26006 is resolved.
One new ToDo created referenced in this PR: #26036