Thanks for the thorough review, Dale! So that others can see how the draft will be changed to address your comments, I'll respond to them inline below. Dale Worley wrote: > Reviewer: Dale Worley > Review result: Ready with Nits > > 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 > <https://wiki.tools.ietf.org/area/gen/wiki/GenArtfaq>. > > 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. Yes, underscores are used to mark terms that are being introduced for the first time and defined. And quotation marks are used where logical names (generally mathematical values or protocol field names) are embedded in normal text, in order that they can be more easily parsed as identifiers instead of English. (I'm going through now and making this slightly more consistent than it was.) Note that our method of composing this document (mmark2rfc) produces XML output (which we submit alongside the text version of each draft) that can be used to set off these identifiers in a different typeface if the format supports it. For example, the xml2rfc tool produces HTML output with these identifiers in a "typewriter" face ... but I don't know what is the future of such formats in the IETF. The quotation marks that are produced for plain text output look a little busy, but do seem to achieve the goal of disambiguation. We're absolutely open to suggestions here. > > 2. Section 1 should be updated to use the language of BCP 14 (RFC 8174) > section 2. Thanks, done! > > 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. Thanks for pointing this out; it's true that there is some confusing conflation between "key-agreement schemes" and TEPs. I've gone through and used something like "negotiated TEP" in a couple places where the document said that a parameter depended on the "negotiated key-agreement scheme", and also added various phrases to make clear that the TEP dictates all the parameters. I've fixed the first paragraph of "3.2 Protocol negotiation"; you'll find that some edit-requests below where you refer to that paragraph. Lastly, section "5. Key agreement schemes" now begins like this: The TEP negotiated via TCP-ENO indicates the use of one of the key- agreement schemes named in Table 2. For example, "TCPCRYPT_ECDHE_P256" names the tcpcrypt protocol with key-agreement scheme ECDHE-P256 and the associated length parameters below. All the TEPs specified in this document require the use of HKDF- Expand-SHA256 as the CPRF, and these lengths for nonces and session keys: N_A_LEN: 32 bytes N_B_LEN: 32 bytes K_LEN: 32 bytes If future documents assign additional TEPs for use with tcpcrypt, they may specify different values for the lengths above. Note that the minimum session ID length required by TCP-ENO, together with the way tcpcrypt constructs session IDs, implies that "K_LEN" must have length at least 32 bytes. > > 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. I'm not sure what you mean here, as tables 2 and 3 probably need to be in "IANA considerations", which immediately follows the sections most closely related to those tables. > > 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? Yes (in section 5.1): Each TEP MUST define a session ID that is computable by both endpoints and uniquely identifies each encrypted TCP connection. Implementations MUST expose the session ID to applications via an API extension. The API extension MUST return an error when no session ID is available because ENO has failed to negotiate encryption or because no connection is yet established. > > 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. I'll explain this design choice under separate cover, when I have a moment ... > > 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.) I suppose you're right, that would be a clever economy. However, if it has been some time since you last sent data and your security policy is not to encrypt any data under sufficiently-stale keys, then it's good to have the ability to go ahead with the new key immediately. > > * 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. Dang, yeah ... I prefer "Capitalize only first letter" for readability, but the several RFCs I just peeked at do all caps, so we'll assimilate. > > 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. > > -- Yep, thanks ... I've gone with something very similar: 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 produces 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 CPRF when parameterized by key K and the constant 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. Good points, both; I've changed that paragraph to: The Extract and CPRF functions used by the tcpcrypt variants defined in this document are the Extract and Expand functions of HKDF [RFC5869], which is built on HMAC [RFC2104]. These are defined as follows in terms of the function "HMAC-Hash(key, value)" for a negotiated "Hash" function such as SHA-256; the symbol | denotes concatenation, and the counter concatenated to the right of CONST occupies a single octet. > > 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?) Oh, good catch ... actually in RFC 5869 it addresses this with the input constraint "L <= 255*HashLen". So I've added a line to the bottom of our HKDF figure: HKDF-Extract(salt, IKM) -> PRK PRK = HMAC-Hash(salt, IKM) HKDF-Expand(PRK, CONST, L) -> OKM T(0) = empty string (zero length) T(1) = HMAC-Hash(PRK, T(0) | CONST | 0x01) T(2) = HMAC-Hash(PRK, T(1) | CONST | 0x02) T(3) = HMAC-Hash(PRK, T(2) | CONST | 0x03) ... OKM = first L octets of T(1) | T(2) | T(3) | ... where L < 255*OutputLength(Hash) > > 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. Right ... I've changed this paragraph so that it leaves "key-agreement scheme" as a shorthand for all the associated parameters (because the ECDH and KDF steps could be considered two parts of key-agreement), but enumerates those explicitly in a later sentence: 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. TCP-ENO negotiates the use of a particular TCP encryption protocol or _TEP_ by including protocol identifiers in ENO suboptions. This document associates four TEP identifiers with the tcpcrypt protocol, as listed in Table 2. Each identifier indicates the use of a particular key-agreement scheme, with an associated CPRF and length parameters. Future standards may associate additional TEP identifiers with tcpcrypt, following the assignment policy specified by TCP-ENO. > > -- > > 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. Okay. The TCP-ENO document uses "host A" and "host B" too, so I've gone with: TCP-ENO uses the terms "host A" and "host B" to identify each end of a connection uniquely, and this document employs those terms in the same way. > > -- > > 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. We really mean "every ENO suboption", so I've changed this sentence to "An ENO suboption includes a flag `v` ..." > > 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...". I've changed this to, "an identifier derived from a session secret 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... Yeah ... at least in this document we don't name private keys, as they are never transmitted. > > 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. > Right. I'll use, "The process of advancing through the series of master keys is described in section 3.8." > 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. Yes, the hairy bit is that there is a series of session secrets `s[i]` that you walk through by session resumption, and within each session there are master keys `mk[j]` that you walk through by re-keying. We once *doubly* indexed the keys. But you've inspired me to try the middle way, which is to make the session index implicit for keys but always explicitly give the "generation number", `j`. I think this works reasonably well and is less mysterious. > > 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.) Good catch. I've changed these paragraphs to: Finally, each master key "mk[j]" is used to generate keys for authenticated encryption: k_ab[j] = CPRF(mk[j], CONST_KEY_A, ae_keylen) k_ba[j] = CPRF(mk[j], CONST_KEY_B, ae_keylen) In the first session derived from fresh key-agreement, keys "k_ab[j]" are used by host A to encrypt and host B to decrypt, while keys "k_ba[j]" are used by host B to encrypt and host A to decrypt. In a resumed session, as described more thoroughly below in Section 3.5, each host uses the keys in the same way as it did in the original session, regardless of its role in the current session: for example, if a host played role "A" in the first session, it will use keys "k_ab[j]" to encrypt in each derived session. > > 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) > -- Looks great, I've adopted this language almost verbatim. > > 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, Good idea; I've used: If a passive opener receives a resumption suboption containing an identifier-half it recognizes as being derived from a session secret that it has cached, it SHOULD (with exceptions specified below) agree to resume from the cached session by sending its own resumption suboption, which will contain the other half of the identifier. > > -- > > 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 > ... > Okay, I've put the noun before the pronoun, but had to leave "with a particular TEP" in order to make clear that you can request "same TEP but fresh key-exchange". If the passive opener does not agree to resumption with a particular TEP, it may either request fresh key exchange by responding with a non-resumption suboption using the same TEP, or else respond to any other received suboption. > -- > > 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. Good questions. I've changed this paragraph to: Implementations that cache session secrets MUST provide a means for applications to control that caching. In particular, when an application requests a new TCP connection, it must be able to specify that during the connection no session secrets will be cached and all resumption requests will be ignored in favor of fresh key exchange. And for an established connection, an application must be able to cause any cache state that was used in or resulted from establishing the connection to be flushed. > > 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. The phrase "concurrent re-key operation" is perhaps heavy-handed. It refers to the process of sending "re-key" and awaiting the "re-key" flag in response, after which the two hosts have the same generation number. The idea is that you shouldn't re-key again (i.e., "concurrently" with the first request) until your peer has responded in kind, which it is required to do as soon as it receives your message. And if there has been no response -- whether your primary intention was to advance the key schedule or to probe for liveness -- then you have your answer about liveness and there should be no need to perform a following keep-alive. So ... I *think* ... the paragraph works as written. > > 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]| > +--------+---------+---------+---...---+-----------+ Duh, of course. And bless you for drawing out the ascii for me! > > 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. Great, yes. > > 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] > > The idea is that, instead of specifying a particular set of authentication schemes as part of this protocol (as TLS does for example), applications can use arbitrary means to compare session IDs and thus achieve communication security. As examples, a public-key infrastructure can be used to sign session IDs; or session IDs can be compared out-of-band and even after-the-fact in order to audit the security of past communications. This property is shared with any protocol that uses TCP-ENO, and should probably be expounded on a little more clearly there. It looks like we streamlined things a little too far. Thanks again for the very close reading of this document, Dale. You turned up a bunch of misleading wording and even a couple outright mistakes. daniel