[Gen-art] Genart last call review of draft-ietf-tcpinc-tcpcrypt-07

[Dale Worley <[e-mail]>]

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.

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]