Hi Yaron, thank you for the review.
Our responses are inline below and the PR addressing your review is
https://github.com/lamps-wg/cms-kyber/pull/29
FixedDocument: draft-ietf-lamps-cms-kyber Title: Use of ML-KEM in the Cryptographic Message Syntax (CMS) Reviewer: Yaron Sheffer Review result: Has Nits This is a straightforward application of ML-KEM to the KEM framework previously defined for CMS. The document is simple and clear. Abstract: typo - parameters sets
Fixed with your suggestionIntroduction: "Prior to standardization, the algorithm was known as Kyber. ML-KEM and Kyber are not compatible." These two sentences do not combine well. How about: "ML-KEM is a standardized and incompatible version of the older Kyber algorithm"
Section 1.2 is part of the Introduction, and so it is introducing ML-KEM. Section 2 defines the use of ML-KEM in CMS. So I don't think your suggestions for clarification would be appropriate in section 1.2, since it applies to CMS. The second paragraph of Section 2 refers to (Section 2 of) RFC 9629 for the general processing of a KEM with KEMRecipientInfo. Do you think that is sufficient, or should that Section 2 text be amended with your clarification above?1.2: the FIPS document provides a better explanation of the whole process and seems to be required reading for those new to KEM usage. A bit of extra clarification here would help, e.g. "ss is used by the originator and separately by the recipient to generate a KEK. The KEK is then used to wrap (encrypt) a CMS content encryption key." Alternatively, refer the reader to RFC 9629 for an overview of the process.
1.2: the text that introduces the 3 functions is strange. It state that ML-KEM provides 3 functions, and those correspond to the ones defined in FIPS 203. But isn't this the authoritative document for ML-KEM itself? You might have wanted to say, This document defines 3 functions...
FIPS 203 is the authoritative document on the ML-KEM algorithm. I-D.ietf-lamps-kyber-certificates is the authoritative document on encoding ML-KEM in PKI certs and CRLs. This document (draft-ietf-lamps-cms-kyber) is the authoritative document on using ML-KEM in CMS and it references those two other documents.
There was some redundancy where general KeyGen()/Encapsulate()/Decapsulate() functions were defined, then repeated in reference to ML-KEM. In the PR we've collapsed that redundancy for each function into "This is what the function does and this is where
it's defined for ML-KEM," which hopefully makes the text less strange.
OLD:
Like all KEM algorithms, ML-KEM provides three functions: KeyGen(),
Encapsulate(), and Decapsulate().
<general text about those functions>
The KEM functions defined above correspond to the following functions
in [FIPS203]:
NEW (full text here in PR):
All KEM algorithms provide three functions: KeyGen(), Encapsulate(),
and Decapsulate().
The following summarizes these three functions for the ML-KEM
algorithm, referencing corresponding functions in [FIPS203]:
<function description and mapping to FIPS 203>
An editorial question: you "deep link" to specific algorithm numbers in FIPS 203. Is this legit? Can FIPS publish a new version of the document and mix up the numbers?
When FIPS publishes a new version of a document they call it 203-1 or 203a or something. So there is only one FIPS 203 and I think we are safe.
Fixed. We were also using "receiver," which has also been changed to "responder." And we've made the other side consistent, changing "sender" to "originator".Terminology: you sometimes use "recipient" and sometimes "responder".
2.1: "ukm" sounds like a useless piece of complexity - even some complexity if you don't implement it. Given that this is a profile of RFC 9629, couldn't you remove it?
Having implemented an algorithm-agnostic RFC 9629 and then this draft using RFC 9629, I think it would add more complications if we specify that an originator MUST NOT include the ukm field in the KEMRecipientInfo structure when ML-KEM is used, because it is an algorithm-specific restriction on the general RFC 9629 KDF calculation. Such a restriction would have made sense for the ukm in KeyAgreeRecipientInfo because that ukm is passed to specific key agreement algorithms, some of which take a ukm and some of which don't. But since KEMRecipientInfo's ukm is used at a higher level than the specific KEM algorithm I don't think removing it makes sense.
ukm also provides a way for the application to bind extra context information or out-of-band pre-shared key material, so we think we should continue to allow it.
2.3, as far as I can tell, you are more strict than RFC 9629 in saying that only keyEncipherment must be asserted. I'm not familiar enough with real-life CMS, but this may be difficult to implement.
RFC 9629 gives the minimal requirements for using a KEM, which is the keyEncipherment bit. We were repeating the restrictions from I-D.ietf-lamps-kyber-certificates (LAMPS discussed this several years ago and decided that by convention, all KEMs will use the keyEncipherment key usage). Our text for section 2.3 had been based on a stripped down version of the same section from RFC 5990/9690 (RSA-KEM in CMS), however RSA-KEM is more complicated because it uses an RSA public key, which could be used for all sorts of purposes. The similar section from RFC 8418 (x25519/x448 in CMS) would be more appropriate. So I propose replacing the text from our draft with modified text from RFC 8418, deferring all ML-KEM certificate conventions to I-D.ietf-lamps-kyber-certificates.
OLD:
The conventions specified in this section augment [RFC5280].
| RFC EDITOR: Please replace the following reference to
| [I-D.ietf-lamps-kyber-certificates] with a reference to the
| published RFC.
A recipient who employs the ML-KEM algorithm with a certificate MUST
identify the public key in the certificate using the id-alg-ml-kem-
512, id-alg-ml-kem-768, or id-alg-ml-kem-1024 object identifiers
following the conventions specified in
[I-D.ietf-lamps-kyber-certificates].
In particular, the key usage certificate extension MUST only contain
keyEncipherment (Section 4.2.1.3 of [RFC5280]).
NEW:
| RFC EDITOR: Please replace the following reference to
| [I-D.ietf-lamps-kyber-certificates] with a reference to the
| published RFC.
RFC 5280 [RFC5280] specifies the profile for using X.509 Certificates
in Internet applications. A recipient static public key is needed
for ML-KEM, and the originator obtains that public key from the
recipient's certificate. The conventions for carrying ML-KEM public
keys are specified in [I-D.ietf-lamps-kyber-certificates].
Fixed2.4: typo - When the one.
Fixed3. The hashAlgs OID is listed, but it's not used anywhere in the document.
4. In the paragraph, "Implementations MUST protect the ML-KEM private key", please point out which keys are ephemeral and mention that they must be wiped after use.
Proposal:
OLD:
Implementations MUST protect the ML-KEM private key, the key-
encryption key, the content-encryption key, message-authentication
key, and the content-authenticated-encryption key.
NEW:
Implementations MUST protect the ML-KEM private key, the key-
encryption key, the content-encryption key, message-authentication
key, and the content-authenticated-encryption key. Of these keys,
all but the private key are ephemeral and MUST be wiped after use.
It is. In CMS the public key comes in a certificate. I haven't found other CMS RFCs that go into details on how a certificate keeps a public key safe, presumably leaving it up to RFC 5280. So I don't think there's anything that would need to be added to this draft which just profiles the use of a particular public key algorithm in CMS.Also, integrity of the public key is almost as important.
The proposed changes to Section 1.3 above make it more clear that the public key is obtained from a certificate (the previous text implied that *if* you're using a certificate then restrictions apply, but in CMS a certificate is the only way to obtain the public key). Is that sufficient?And in general, the document never points out that the public key must be protected by the entity that runs KeyGen, e.g. by wrapping the public key in a certificate.
Table 2: the caption says "sizes" but the column is named Secret.
It is the shared secret size. We'll update the table and column titles to make it clearer.
Regards,
the authors
-- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
