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

["Mirja Kuehlewind (IETF)" <[e-mail]>]

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

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…?

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.

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…?

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.

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

- 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?

- 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.“

- 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?

- 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 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 …?

- 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.

- The design notes (section 9) could actually be integrated in the security consideration; you can also have subsection in the security considerations…

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

Thanks!!!
Mirja