Re: [tcpinc] AD review of draft-ietf-tcpinc-tcpcrypt

[Daniel B Giffin <[e-mail]>]

Hi Mirja (et al.),

Thanks for your detailed review!  I've got a new
version of the draft ready that I think addresses what
you've brought up.  I'll detail the changes below (with some
of the new language inline) before submitting so you have
another chance to respond if necessary.

Mirja Kuehlewind wrote:
> Hi all,
> 
> thanks again for this also very well-written document and shepherd write-up. I still remember a earlier version I’ve read that was more paper-like, so thanks a lot for all the editorial (and technical) work you did here!
> 
> Again, I’m afraid we need a few minor updates before we can go to IETF last call. Nothing big, mostly processing:
> 
> 1) As indicated by the nits check, this document has the wrong disclaimer. I think that really should be fixed before we call in Last call…
> 
> https://www.ietf.org/tools/idnits?url=https://www.ietf.org/archive/id/draft-ietf-tcpinc-tcpcrypt-06.txt

Oh thanks, I've fixed that.

> 2) Again IANA: 
>    - This is probably rather a comment for tcp-eno but I just realized this now: Probably tcpcrypt should register itself in the "TCP encryption protocol identifiers“ created by the tcp-eno draft...
>    - Is the "tcpcrypt AEAD parameter“ registry a sub registry under the "Transmission Control Protocol (TCP) Parameters“ registry and what’s the registration policy for this new registry? I guess it could be good to also add a column in the registry for the respective RFCs that specify the algorithms…?

Ok, I've tried to fix up the IANA section as follows.  (Note
that the TCP-ENO document includes these same tcpcrypt
values, as well as one for TCP-Use-TLS, in its listing of
"initial values" for the table.)

  7.  IANA considerations

     Tcpcrypt's TEP identifiers will need to be incorporated in IANA's
     "TCP encryption protocol identifiers" registry under the
     "Transmission Control Protocol (TCP) Parameters" registry, as in the
     following table.  The various key-agreement schemes used by these
     tcpcrypt variants are defined in Section 5.

               +-------+---------------------------+-----------+
               | Value | Meaning                   | Reference |
               +-------+---------------------------+-----------+
               | 0x21  | TCPCRYPT_ECDHE_P256       | [RFC-TBD] |
               | 0x22  | TCPCRYPT_ECDHE_P521       | [RFC-TBD] |
               | 0x23  | TCPCRYPT_ECDHE_Curve25519 | [RFC-TBD] |
               | 0x24  | TCPCRYPT_ECDHE_Curve448   | [RFC-TBD] |
               +-------+---------------------------+-----------+

                Table 2: TEP identifiers for use with tcpcrypt

     In Section 4.1, this document defines "sym-cipher" specifiers for
     which IANA is to maintain a new "tcpcrypt AEAD Algorithm" registry
     under the "Transmission Control Protocol (TCP) Parameters" registry,
     with initial values as given in the following table.  The AEAD
     algorithms named there are defined in Section 6.  Future assignments
     are to be made under the "RFC Required" policy detailed in [RFC8126],
     relying on early allocation [RFC7120] to facilitate testing before an
     RFC is finalized.

          +-------+------------------------+------------+-----------+
          | Value | AEAD Algorithm         | Key Length | Reference |
          +-------+------------------------+------------+-----------+
          | 0x01  | AEAD_AES_128_GCM       | 16 bytes   | [RFC-TBD] |
          | 0x02  | AEAD_AES_256_GCM       | 32 bytes   | [RFC-TBD] |
          | 0x10  | AEAD_CHACHA20_POLY1305 | 32 bytes   | [RFC-TBD] |
          +-------+------------------------+------------+-----------+

      Table 3: Authenticated-encryption algorithms corresponding to sym-
                cipher specifiers in Init1 and Init2 messages.


> 3) Appendix are usually not normativ, therefore Table 3 should be moved somewhere in the body of the document; probably a new subsection in section 4.

Ok great, I've moved the table of constants to section
"4. Encodings" as you suggest.

> 4) One technical comment/question:
> I think the following sentence in section 3.6 is actually not well enough defined:
> "In the latter case, the implementation MUST
>    either ignore the frame or abort the connection;“
>    - What does ignore in this case mean? This sounds like you ack the TCP segment but just don’t deliver the data… I guess that is not what you meant to say because that would break the TCP stream…? I guess you mean drop the TCP segment, right? In this case it’s probably useful to say somewhere that first the frame has to be decrypted and only if the decryption succeeds and ACK can be sent.
>    - Similar question for abort: Does this mean you send a TCP RST?
>    - Maybe it'd better to say that in general if encryption fails the TCP segment MUST be dropped. Failed decryption attempts SHOULD be logged and the connection MAY be reset if failure rate is high…?

Okay yes, I've slightly modified that text to use the
clearer "drop segment" as you suggest:

			 In the latter case, the implementation MUST
   either drop the TCP segment(s) containing the frame or abort the
   connection; but if it aborts, the implementation MUST raise an error
   condition distinct from the end-of-file condition.

I think the option to abort needs to be retained, as this is
likely to be the most convenient behavior for implementors
and for users (and doesn't make a tcpcrypt-protected
connection any more brittle than a plain one).  Still, the
"drop" option is there in case an implementation is able to
get some amount of DOS-resistance out of it.

As for describing the abort behavior precisely, it feels
safer simply to use the expression "abort the connection"
from RFC793 instead of trying to summarize its semantics
here.

> Please fix/address this and resubmit! Given I have reviewed the whole document in detail I have some more, mostly editorial comments below. Please have a look at these as well and update where needed, given we anyway need another version:
> 
> - Again (as noted with tcp-eno) there should be no references in the abstract. Maybe just remove the reference there and add a sentence (or half-sentence) on tcp-eno in the intro with the reference.

Ok thanks; the intro cites ENO so I've just removed the
reference in the abstract.

> - In section 3.1 PRK and OKM are not extended here.

Okay yes, I will define PRK/OKM before using them.

> - section 3.3:
> „However, the segment containing the
>    last byte of an "Init1" or "Init2" message SHOULD have TCP's PSH bit
>    set.“
>    - PSH should be extended: s/TCP's PSH bit/TCP's push flag (PSH)/
>    - Why does this need to be set? And why is that a SHOULD?

Okay, I've changed that sentence to:

  However, the segment containing the last byte of an
  "Init1" or "Init2" message MUST have TCP's push flag (PSH)
  set.

And yes, we agreed to change it to MUST: this prevents the
tcpcrypt handshake from getting hung up in a buffer
somewhere, and there's no good reason not to do it.

> 
> - also section 3.3:
> „"eno-transcript" is the protocol-
>    negotiation transcript defined in TCP-ENO“
> MAYBE
> ""eno-transcript" is the protocol-
>    negotiation transcript defined for TCP-ENO in section X of [draft-ietf-tcpinc-tcpeno], containing the two full tcp-eno option send in the SYN and SYN-ACK.“

Okay, that paragraph now reads:

   Above, "|" denotes concatenation; "eno-transcript" is the protocol-
   negotiation transcript defined in Section 4.8 of
   [I-D.ietf-tcpinc-tcpeno]; and "Init1" and "Init2" are the transmitted
   encodings of the messages described in Section 4.1.

> - Maybe s/3.5.  Session caching/3.5.  Session resumption/
>   Resumption is at least to me the more commonly used term, or was this on purpose named differently?

Good idea, thanks -- I've gone through and named this
facility "session resumption" throughout.  (In the details
of storing session-related data, we still use "session
caching" to describe particular actions.)

> - section 3.5
> "A host MUST NOT resume with a session secret if it has ever
>    successfully negotiated resumption in the past,...“
> Was this meant to mean „previous resumption attempt(s) failed“? Because if I read it like there was a least one successful resumption performed already, that’s not true for the first resumption attempt and therefore I could never use it…

I'm not quite sure I understand what was confusing about
that wording, but I've adjusted it as follows to make sure
that "with the same secret" cannot be missed:

   A host MUST NOT resume with a session secret if it has ever
   successfully negotiated resumption in the past with the same secret,
   in either role.

Does this help at all?

> - I’m not sure about the term _application frames_ in section 3.6. That can be confusing as tcpcrypt is part of the transport. Maybe just call it _frame_ or maybe _encryption frame_ or _record frame_ or _tcpcrypt frame_ or …?

I think you're right.  I never really liked that term.  I've
changed this term to "encryption frame" throughout (with
occasional abbreviation to "frame" where the context is
clear).

> - section 3.7
>    - It’s a bit confusing that FINp and URGp are explained quite extensively in this section while the frame format is specified later. Probably it's enough to say here that the FIN flag and the urgent pointer (URG) of the TCP header are protected (and the segment must be dropped if the protected values differ from the values in the TCP header). 
>    - I also think this sentence is not quite correct:
>     "When "FINp" is set, it indicates that the sender will send no more
>    application data after this frame.“
>    When the FIN flag in the TCP header is set it indicates the sender will send no more payload data, however, the FINp is only the protected version of the FIN flag. It’s it’s a minor wording difference but it good to note that the TCP header information still define the normative behavior and also tcpcrypt does it to decide if the segment is value or not which cases a drop before the TCP precessing even starts.

Yes, good points.  I've changed that language as follows in
order to make it clearer that we aren't interfering with TCP
semantics apart from defining when invalid segments must be
dropped:

   A sender MUST set the "FINp" bit on the last frame it sends in the
   connection (unless it aborts the connection), and MUST set the TCP
   FIN flag on the last TCP segment used to transmit that frame.  A
   sender MUST NOT set the TCP FIN flag except on the last segment of a
   frame with "FINp" set.

   Once a receiving host has received all segments containing a frame
   and has successfully decrypted the frame, it MUST verify that the TCP
   FIN flag in the last of these segments matches the "FINp" bit in the
   frame; if they differ, the receiving host MUST either drop the
   segments or abort the connection and raise an error condition
   distinct from the end-of-file condition.

As for where this text should live, I agree it feels a
little scattered to have it separate from where the format
is defined, but now that the document has a major separation
between the semantics in "3. Encryption protocol" and the
formats in "4. Encodings", it seems best to stick with that
arrangement.
 
> - The design notes (section 9) could actually be integrated in the security consideration; you can also have subsection in the security considerations…

Good idea, I've done that.

> - Any again, do you want to note Andrea especially in the acknowledgement? This has been done in previous drafts, so that nothing uncommon.

Thanks for the thought.  We've discussed it and decided
Andrea would probably be most glad simply to have his
name on the work.
 
daniel