I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair. Please treat these comments just like any other last call comments. For more information, please see the FAQ at . Document: review-draft-ietf-tcpinc-tcpcrypt-07 Reviewer: Dale R. Worley Review Date: 2017-10-18 IETF LC End Date: 2017-10-19 IESG Telechat date: 2017-10-26 Summary: This draft is basically ready for publication, but has nits that should be addressed before publication. * Major/global items: 1. The construction _phrase_ is used in many places. The construction "phrase" is also used frequently. It's not clear if these have specific semantics, though _..._ seems to be used for defining instances of a term, and "..." seems to be used around mathematical notation. What syntax(es) are intended to be used in the RFC, and with what meaning(s)? The RFC Editor can probably make recommendations here. 2. Section 1 should be updated to use the language of BCP 14 (RFC 8174) section 2. 3. The term "key agreement scheme" doesn't seem to be used consistently. In a narrow sense, it seems to be used for the initial phases of the encryption. In a broad sense, it seems to be used for the set of algorithm selections, key lengths, and magic numbers that are used by the tcpcrypt algorithm, a set identified by a particular TEP identifier. The two can be confused, because it seems that only a few items in the set can be varied using the 4 defined TEP identifiers. But I reflexively assume that all of these parameters can be varied within the overall scheme of "tcpcrypt". Is it the intention that the TEP identifier *only* specifies the key agreement scheme in the narrow sense, and we are *committing* to never varying the other parameters? Or are we taking the more natural path that the TEP identifier specifies all of these parameters, but the currently defined values all specify the same values for all but one parameter? In either case, we need to make the overall scheme clear early on and use the terminology consistently. 4. The positioning of the tables seems to be poor relative to the sections which refer to them. Presumably the RFC Editor will clean that up. 5. Does draft-ietf-tcpinc-tcpeno require that the application can query the stack to find out whether encryption was established vs. the connection has fallen back to being unencrypted? 6. It might be worth adjusting the rules for how the A and B roles are carried forward during session resumption. Of course, each host should compute the resumption identifier that it expects to receive based on the role it had in the previous session. But it's not clear to me why a host that used k_ab for encryption (i.e., had the A role) in the previous session must also use k_ab for encryption in the resumed session, since the two sequences of k_ab/k_ba are generated from the different session keys of the two sessions. If you made the choice of k_ab/k_ba be dependent on the A/B roles established by TCP-ENO for *this* session, it seems like the specification of the protocol would be a bit simpler. 7. In the encryption frame, it seems to me that the (unencrypted) control byte could be eliminated and the rekey flag put into the (encrypted) flags byte, if we define that rekey=1 means that rekeying takes effect on the *next* frame rather than the current one. However, that would eliminate the 7 reserved unencrypted flags the frame format now has, which might be useful in the future. (I suspect that the usefulness of an unencrypted field in the frame is something that cryptographers understand but I don't.) * Minor/editorial items: Table of Contents 11.1. Normative References . . . . . . . . . . . . . . . . . . 24 11.2. Informative References . . . . . . . . . . . . . . . . . 25 Authors' Addresses . . . . . . . . . . . . . . . . . . . . . . . 25 The names of these three sections aren't capitalized like those of other section. 3.1. Cryptographic algorithms o A _collision-resistant pseudo-random function (CPRF)_ is used to generate multiple cryptographic keys from a pseudo-random key, typically the output of the extract function. The CPRF is defined to produce an arbitrary amount of Output Keying Material (OKM), and we use the notation CPRF(K, CONST, L) to designate the first L bytes of the OKM produced by the pseudo-random function identified by key K on CONST. It is unclear what "the pseudo-random function identified by key K" means, as only three functions have been identified to this point, and none of them seem to have identifiers. It sounds like CPRF is defined to produce an endless stream of OKM based on two inputs, K and CONST -- T(1) | T(2) | T(3) | ... -- and CPRF(K, CONST, L) is the first L bytes of the stream. If so, it seems to me that it would be clearer to say it in those terms: o A _collision-resistant pseudo-random function (CPRF)_ is used to generate multiple cryptographic keys from a pseudo-random key. The CPRF produces an endless stream of Output Keying Material (OKM), and we use the notation CPRF(K, CONST, L) to designate the first L bytes of the OKM produced by the CRPF when keyed with K and CONST. -- The Extract and CPRF functions used by default are the Extract and Expand functions of HKDF [RFC5869]. These functions don't have these roles "by default", but rather, these functions are specified for these roles by the four defined TEP identifiers, and indeed, there is no way to specify that other functions are to be used in these roles. It seems more sensible to say something like The Extract and CPRF functions used with the tcpcrypt variants defined in this document are the Extract and Expand functions of HKDF [RFC5869]. Since you expand on what is in RFC 5869, it might be worth providing a reference for HMAC-Hash to RFC 2104. It doesn't seem to be stated here or in RFC 5869 that the value of the counter in the calculation of T(n) is n reduced modulo 256 -- there's no statement that after 0xFF is used to generate T(255), T(256) is generated using 0x00. (Should that be specified here or put in an erratum to RFC 5869?) 3.2. Protocol negotiation Tcpcrypt depends on TCP-ENO [I-D.ietf-tcpinc-tcpeno] to negotiate whether encryption will be enabled for a connection, and also which key agreement scheme to use. This doesn't really classify things correctly. It should be something like Tcpcrypt depends on TCP-ENO [I-D.ietf-tcpinc-tcpeno] to negotiate that encryption will be enabled for a connection, that tcpcrypt will be used, and which cryptographic algorithms and parameters tcpcrypt will use. -- This document adopts the terms "host A" and "host B" to identify each end of a connection uniquely, following TCP-ENO's designation. You don't actually say that this document's use of A and B matches the A and B roles assigned by TCP-ENO. If you mean it to, say This document uses the terms "host A" and "host B" to identify the hosts that TCP-ENO designates as the A role and B role. -- ENO suboptions include a flag "v" ... Might be better to phrase it "The ENO suboptions ..." to connect with the negotiation described in the preceding paragraph. In order to propose session resumption (described further below) with a particular TEP, a host sends a variable-length suboption containing the TEP identifier, the flag "v = 1", and an identifier for a session previously negotiated with the same host and the same TEP. Probably better to say "an identifier derived from a session previously negotiated...". 3.3. Key exchange o "PK_A", "PK_B": ephemeral public keys for hosts A and B, respectively. The use of "PK" for a public key seems to be poorly mnemonic, as it is also the acronym of "private key". There ought to be standard (and distinct!) abbreviations for these phrases, but I can't find any... The particular master key in use is advanced as described in Section 3.8. Presumably, "The first master key used is mk[0], and use advances to successive master keys as described in section 3.8." -- we have a series of master keys, so the keys are numbers, and so a key *itself* cannot "advance", what advances is something which selects/uses one of the series of master keys. You probably want to index k_ab and k_ba by the index of the mk they are generated from: k_ab[i] = CPRF(mk[i], CONST_KEY_A, ae_keylen) k_ba[i] = CPRF(mk[i], CONST_KEY_B, ae_keylen) and similarly for all uses of k_ab and k_ba. 3.4. Session ID As required, a tcpcrypt session ID begins with the negotiated TEP identifier along with the "v" bit as transmitted by host B. The remainder of the ID is derived from the session secret, as follows: session_id[i] = TEP-byte | CPRF(ss[i], CONST_SESSID, K_LEN) This might be better phrased As required, a tcpcrypt session ID begins with the byte transmitted by host B that contained the negotiated TEP identifier along with the "v" bit. The remainder of the ID is derived from the session secret for this session, ss: session_id = TEP-byte | CPRF(ss, CONST_SESSID, K_LEN) Exactly how you describe the TEP-byte depends on the terminology established in draft-ietf-tcpinc-tcpeno, but it seems that that draft doesn't define a term for "the byte that carries v and the TEP identifier". Finally, each master key "mk" is used to generate keys for authenticated encryption for the "A" and "B" roles. Key "k_ab" is used by host A to encrypt and host B to decrypt, while "k_ba" is used by host B to encrypt and host A to decrypt. k_ab = CPRF(mk, CONST_KEY_A, ae_keylen) k_ba = CPRF(mk, CONST_KEY_B, ae_keylen) Though this needs to be written more carefully: Which key is used by each host is not determined by its A/B role in *this* connection, but by the role it had in the first session in the resumption-sequence of which this session is a part. See the second-to-last paragraph in section 3.5. (You may want to introduce terms for those two roles.) 3.5. Session resumption When two hosts have already negotiated session secret "ss[i-1]", they can establish a new connection without public-key operations using "ss[i]". A host signals willingness to resume with a particular session secret by sending a SYN segment with a resumption suboption: that is, an ENO suboption containing the negotiated TEP identifier from the original session and part of an identifier for the session. The resumption identifier is calculated from a session secret "ss[i]" as follows: resume[i] = CPRF(ss[i], CONST_RESUME, 18) I don't like the phrasing here because it depends on ss[i] being within a larger sequence of session secrets without ever describing it as such. I don't think you mean "negotiated TEP identifier from the original session [when PRK was computed]". You might mean "negotiated TEP identifier from the previous session", but it seems from later paragraphs, you mean "negotiated TEP identifier of the new session", because the later paragraphs seem to show v = 1 as mandatory, which is true of the new session but not necessarily true of the previous session. Paragraph 1 mentions "an identifier for the session" but paragraph 2 says "The resumption identifier". I think you want to phrase this paragraph something like this: When two hosts have already negotiated a session with a particular session secret, they can establish a new connection without public-key operations using the next session secret in the sequence derived from the original PRK. A host signals willingness to resume with a particular new session secret by sending a SYN segment with a resumption suboption: that is, an ENO suboption whose value is the negotiated TEP identifier of the session concatenated with half of the "resumption identifier" for the session. The resumption identifier is calculated from a session secret "ss" as follows: resume = CPRF(ss, CONST_RESUME, 18) -- If a passive opener recognizes the identifier-half in a resumption suboption it has received and knows "ss[i]" It seems like "and knows ss[i]" is redundant. This could be more clearly stated: If a passive opener recognizes the identifier-half as being derived from a session secret and PRK that it has cached, -- If it does not agree to resumption with a particular TEP It's best not to start a paragraph with "it" as a subject. And what is the significance of "with a particular TEP"? It seems better to say If the passive opener does not agree to resumption, it may either ... -- Implementations that perform session caching MUST provide a means for applications to control session caching, including flushing cached session secrets associated with an ESTABLISHED connection or disabling the use of caching for a particular connection. What is "session caching"? What is the significance of the term "ESTABLISHED"? And "disabling the use of caching" seems to be ambiguous -- does it mean that nothing will be read from the cache (session resumption will not be accepted for this session) or that nothing will be written to the cache (no later session can be a resumption of this session)? I suspect this paragraph hasn't been updated from using the terminology of an earlier version. 3.8. Re-keying A host SHOULD NOT initiate more than one concurrent re-key operation if it has no data to send; that is, it should not initiate re-keying with an empty encryption frame more than once while its record of the remote generation number is less than its own. I think you meant "consecutive" here instead of "concurrent". But that still isn't the rule you want, since a host may have to perform two consecutive keepalives without sending any data between them. I'm not sure how you want to state this condition. Perhaps something like A host SHOULD NOT initiate a re-key operation if it has sent no data since the last re-key operation unless sufficient time has passed to require a keep-alive as described in Section 3.9. 4.1. Key exchange messages 8 +--------+-------+-------+---...---+-------+ |nciphers|sym- |sym- | |sym- | | =K+1 |cipher0|cipher1| |cipherK| +--------+-------+-------+---...---+-------+ Generally when a sequence is 0-indexed, you would identify the count (nciphers) as "K" and the items as "sym-cipher0" through "sym-cipherK-1". Or probably better, "sym-cipher[0]" through "sym-cipher[K-1]", giving 8 +--------+---------+---------+---...---+-----------+ |nciphers|sym- |sym- | |sym- | | =K |cipher[0]|cipher[1]| |cipher[K-1]| +--------+---------+---------+---...---+-----------+ When sending "Init1", implementations of this protocol MUST omit the field "ignored"; that is, they must construct the message such that its end, as determined by "message_len", coincides with the end of the field "PK_A". Maybe better to say Implementations of this protocol MUST construct "Init1" with the field "ignored" of zero length. Ditto for Init2. 8. Security considerations If it can be established that the session IDs computed at each end of the connection match, then tcpcrypt guarantees that no man-in-the-middle attacks occurred unless the attacker has broken the underlying cryptographic primitives (e.g., ECDH). A proof of this property for an earlier version of the protocol has been published [tcpcrypt]. Is there a known/defined/standard way to perform such a comparison? If this is valuable enough to be mentioned, it seems like tcpcrypt should incorporate a way of doing it. [END]