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

[Mirja Kühlewind <[e-mail]>]

Hi Daniel,

thanks for the edits. A few minor comments below. Those parts I removed look 
all good!


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

The assignment should only be made in one of the documents and I think this 
one it the right one. So that should be removed from draft-ietf-tcpinc-tcpeno.

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

So in RFC793, if I recognize correctly, abort is rather something like a 
silent close. Is this what you want, or would it make sense to send a reset?


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

No, what I meant is maybe to say this:

"A host MUST NOT resume with a session secret if it has already failed
     to negotiated resumption in the past with the same secret,
     in either role."

Is that still what you wanted to say? If so, I think that's more 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.


I would rather state this the other way around:

"If the TCP FIN is set (on the last segment), the sender MUST set the "FINp" 
bit on the last frame that is send in that segment (unless it aborts the 
connection). A sender MUST NOT set the "FINp" bit except when the TCP FIN 
flag is set on the segment carrying that frame."

I know that sounds a bit confusing and maybe you can phrase it better. The 
point is you should not re-define (normatively) when the TCP FIN flag must be 
set, as this is defined in RFC793 and it is not good practice to define 
things twice in different documents.

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

Okay!

Thanks,
Mirja