Re: [tcpinc] AD review of tcp-eno

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

Hi David,

thanks for your quick reply!

See below.

> Am 27.07.2017 um 22:29 schrieb David Mazieres <[e-mail]>:
> 
> "Mirja Kuehlewind (IETF)" <[e-mail]> writes:
> 
>> I’m afraid we need a new revision before IETF Last Call because the
>> RFC should only specify the actually TCP option and not the
>> experimental one. So you can simply remove the experimental one in
>> Figure 1, 2 and 3 (as well as sec 4.8) and only note in the IANA
>> section that this experimental assignment also exists, as it is
>> already done.
> 
> Thanks for the feedback.  Just to confirm, is this still what we should
> do after the discussion?  

Yes, in any case we should not have both in the RFC. Given we are fostering deployment with the current version but just don’t have enough deployment experience yet to go for standards track, we will request the option number with IESG Approval. So please remove the experimental option from the spec and just note it in the IANA section.

> Or are there other things we need to add (like
> Joe's suggestion not to send both option types)?

That would be good, too; also in the IANA section probably.


Just to confirm this. All comments below are simply comments that came to my mind as I reviewed the whole document. The only other thing I really like to have sorted out before we go to IETF Last call, is the question which IANA policy is the correctly. All other of these changes are not required to move the document forward. However, as we update anyway, it’s probably good to add those things that help clarification. But that’s on you to decide! 

> 
>> 1) I would recommend to switch the last to paragraphs in section 4.5;
>> I think that helps understanding.
> 
> Good idea, but requires a little wordsmithing.  Here is the proposed new
> language:
> 
>        A passive opener (which is always host B) sees the remote host's
>        SYN segment before constructing its own SYN-ACK segment.  Hence,
>        a passive opener SHOULD include only one TEP identifier in
>        SYN-ACK segments and SHOULD ensure this TEP identifier is valid.
>        However, simultaneous open or implementation considerations can
>        prevent host B from offering only one TEP.
> 
>        To accommodate scenarios in which host B sends multiple TEP
>        identifiers in the SYN-ACK segment, the _negotiated TEP_ is
>        defined as the last valid TEP identifier in host B's SYN-form
>        ENO option.  This definition means host B specifies TEP
>        suboptions in order of increasing priority, while host A does
>        not influence TEP priority.

Looks good!

> 
>> 2) I’m wondering if it would make sense to specify at the beginning of
>> section 4.7 another requirement that TEPs SHOULD only specify the use
>> of SYN data if there is some a-priori knowledge that the other end is
>> likely to support tcp-eno and the tep (e.g. due to previous successful
>> connections aka session resumption, or application knowledge).
> 
> This may be true for SYN-only segments, but doesn't necessarily hold for
> SYN-ACK.  To my mind, the requirement that "To avoid unexpected
> connection aborts, ENO implementations MUST disable the use of data in
> SYN-only segments by default" is both stronger and more specific than
> saying only with a-priori knowledge.  Essentially we are saying the
> application has to request it.

Okay, your are actually right that what I phrased above is probably rather something to notice for the application that will use a TEP than a requirement for the TEP itself.

> So for now I haven't done anything with this feedback.  Let me know if
> you have a more specific suggestion.

If you want to and if you think that would help clarity, you can notice that an application that has a-priori knowledge about the other end could be a good use case.

> 
>> 3) Sec 4.8 does not really specify what to do with the transcript. I
>> guess that’s TEP specific but it sounds like there should be a
>> requirement stated that a TEP has to check this or abort the
>> connection…?
> 
> What to do with the transcript is highly TEP specific, but since every
> TEP will need to reference a transcript to comply with the requirements
> of section 5.1, it seemed simpler and less error prone to define one
> explicitly as part of TCP-ENO.  The connection doesn't need to be
> aborted, but the session IDs won't match.  Basically the transcript is
> required for the following point in section 5.1:
> 
>  The session ID MUST depend in a collision-resistant way on all of
>  the following (meaning it is computationally infeasible to produce
>  collisions of the session ID derivation function unless all of the
>  following quantities are identical):
>    ...
>    * The negotiation transcript specified in 4.8.
> 
> So for now I haven't done anything, but am open to more suggestions if
> you think this still needs to be fixed.

Okay, you are right that is actually covered. So it’s more an editorial issue because at the time you are reading section 4.8, you basically have no idea why this is needed (beside that it logically makes sense to have it). Maybe add a forward reference?

> 
>> 4) sec 5:
>> s/TEPs MUST protect TCP data streams with authenticated encryption./TEPs MUST protect TCP data streams with unauthenticated encryption./ ? Or do you mean must support both, authenticated and unauthenticated?
> 
> As David Black pointed out, the language was correct but maybe
> confusing.  Here is the new proposed language:
> 
>  TEPs MUST protect TCP data streams with authenticated encryption.
>  (Note "authenticated encryption" designates the REQUIRED form
>  encryption algorithm [@RFC5116]; it does not imply any actual
>  endpoint authentication.)

Thanks. That was probably basically just me being not familiar enough with the terminology but adding this explanation certainly doesn’t hurt.

> 
>> 5) section 6: Maybe
>> OLD
>> "Figure 9 shows a three-way handshake with a successful TCP-ENO
>>  negotiation.  The two sides agree to follow the TEP identified by
>>  suboption Y.“
>> NEW
>> "Figure 9 shows a three-way handshake with a successful TCP-ENO
>>  negotiation. Host A includes two ENO suboptions with TEP identifiers X and Y. 
>>  The two sides agree to follow the TEP identified by suboption Y.“ 
> 
> Fixed.
> 
>> 6) Also looking at the example in Figure 11, I have to say that the
>> spec does not clearly state anywhere that the first ACK has to have a
>> ENO option (it’s only mention implicitly somewhere). Maybe that can be
>> spelled out more clearly (in section 4.6)?
> 
> This was the intent of the first sentence of section 4.6:
> 
>        A host employing TCP-ENO for a connection MUST include an ENO
>        option in every TCP segment sent until either encryption is
>        disabled or the host receives a non-SYN segment.

Yes, this is covered. However, this is was not fully obvious to me when I was reading it, even though it’s the only logical way to read this. I guess it wouldn’t hurt to make it slight more clear, e.g. by saying

"A host employing TCP-ENO for a connection MUST include a (non-SYN) ENO
      option in every TCP segment sent until either encryption is
      disabled or the host receives a non-SYN segment, starting this the 
      first ACK of the TCP 3-way handshake.“

Does that make sense?


> 
> Rather than fiddle with 4.6, I propose clarifying section 6 as follows:
> 
>        (1) A -> B:  SYN      ENO<X,Y>
>        (2) B -> A:  SYN-ACK  ENO<b=1,X> [ENO stripped by middlebox]
>        (3) A -> B:  ACK
>        [rest of connection unencrypted legacy TCP]
> 
>        Figure 11: Failed TCP-ENO negotiation because of network filtering
> 
>        Figure 11 Shows another handshake with a failed encryption
>        negotiation. In this case, the passive opener B receives an ENO
>        option from A and replies. However, the reverse network path
>        from B to A strips ENO options. Hence, A does not receive an ENO
>        option from B, disables ENO, and does not include a non-SYN-form
>        ENO option in segment 3 when ACKing B's SYN. Had A not disabled
>        encryption, Section 4.6 would have required it to include a
>        non-SYN ENO option in segment 3. The omission of this option
>        informs B that encryption negotiation has failed, after which
>        the two hosts proceed with unencrypted TCP.

Also good!

> 
>> 7) Also based on this example. I guess it’s also possible that a
>> middlebox does not strip the options in the SYN packets but only in
>> the ACK. In this case host A would send encrypted data and host B
>> would think they are not encrypted and deliver garbage to the
>> application. I’m not sure how to avoid or even detect such a case but
>> maybe it’s still good to mention this somewhere…?
> 
> I propose addressing this in section 8, as follows:
> 
>        Once TCP-ENO is deployed, we will be in a better position to
>        gather data on two types of failure:
>        ...
>        2. Middleboxes causing TCP-ENO connections to fail
>           completely. This can happen if middleboxes perform deep
>           packet inspection and start dropping segments that
>           unexpectedly contain ciphertext, or if middleboxes
>           inconsistently strip ENO options from packets in the same
>           direction.

I would even mention the case, where SYN Options are passed but non-SYN options are not, separately because in this case both hosts would have a different view if eno was negotiated or not. In this situation the host that believes that eno was negotiated should fail at some point and abort the connection because the crypto negotiation or, in case of resumption, decryption would fail. But in theory it can happen that the host that believe eno is not used, would deliver garbage data to the application. I think that’s the point that needs separate noting. Or am I misunderstanding something?

> 
>> 8) section 7.2: The following is a normative requirement thats should be stated normatively (MUST) somewhere in the main part of the specification:
>> "To stay robust in the face of dropped segments, hosts must continue
>>  to include non-SYN form ENO options in segments until such point as
>>  they have received a non-SYN segment from the other side.“
> 
> I have deleted the word "must" from here.  Section 7 is not intended to
> be normative, but rather describe the rationale for having made this
> requirement elsewhere in the document.

Actually the word must is fine. I didn’t realize that this is already normatively stated in section 4.6. Maybe put a reference… or not.

> 
>> 9) I would recommend to move 7.1 into an own section (not as part of
>> section 7 on rational), or maybe rather as a subsection of section 8.
> 
> Is this because you thought section 7.1 was trying to establish
> normative requirements?  

No, it just that I think ‚future development‘ are not ‚design rationales‘. This comment is fully editorial though. The content is fine. I just think it be a better fit in section 8.

> A lot of those details were previously more
> closely intermingled with the specification, which the working group did
> not like.  Now that we've got those points separated out into section 7
> but still part of the document, I think it reads more cleanly.  Hence,
> my inclination would be to leave 7.1 as is (other than deleting "must"),
> but if you want to elaborate on why it should be moved, you can probably
> change my mind.
> 
>> 10) From reading the text in the IANA section, I believe you rather
>> what „RFC required“ as a policy (see
>> https://tools.ietf.org/html/rfc8126#section-4.7). Early allocation are
>> always possible and does not need to be noted separately (see
>> https://tools.ietf.org/html/rfc8126#section-3.4).
> 
> The specification required was intentional, because we wanted to avoid
> the same chicken-and-egg problem caused by the need for a TCP option.
> Since the point of TCP-ENO is to make it significantly easier to develop
> future TEPs than it would be to develop TCP encryption schemes from
> scratch, and since the TCP option for tcpcrypt and TCP-ENO caused a fair
> amount of friction in the TCPINC working group, I feel reasonably
> strongly that it would be best to give out TEP identifiers well in
> advance of RFC publication, provided there is a plausible draft
> specification.

This is what early allocation is for. We could have also asked for early allocation for tcpinc as soon as the spec was reasonably stable but given there was no-one who was actually about to deploy this in the Internet, it was probably not necessary. 

Also I assume your intention here is that you still want to have an RFC at the end. If you go for „Specification required“ any kind of spec would be accepted and it doesn’t have to be an RFC. Therefor „RFC Required“ would be better.

Also, as I said it is not necessary to note explicitly that early allocation is possible (because that’s always possible), but you can still make a point that people are encourage to ask for early allocation (at wg adoption time?).

> 
> My strong preference would thus be to leave things as is, but of course
> in the end I would defer to you if based on your experience you really
> think it needs to be RFC required.

Again, if I understand your intention correctly, the right policy still seems to be „RFC required“. Btw. RFC required does not mean IETF consensus required. This can also be an IRTF stream or ISE stream publication. (If you would like IETF consensus for the RFCs, the right policy would be „IETF Rewiew“; check RFC 8126 for more details).

> 
>> 11) Do you want to specially note Andrea’s contribution to this work
>> in the acknowledgements?
> 
> Hmm... we definitely want to leave Andrea as first author the same as if
> he hadn't died, reflecting the fact that he was the main person behind
> the work.  If we put anything in the acknowledgments, it could diminish
> his contributions by emphasizing the fact that other people had to
> "finish" the document, when really what we are doing at this point is
> just tiny little cleanups.

Yes, he definitely is and should stay the main author! However, you can even say exactly this (he did all the work and others where just wrapping up) in the acknowledgments! I’m mainly saying that it is not uncommon to saying something in the acknowledgements, so please feel free to add whatever you think is appropriate and would underline his mayor contribution he provided!

> 
>> 12) RFC5226 (now RFC8126), RFC6994 (after removing the exp option from
>> the body of the doc), and RFC7413 do not need to be normative
>> references. Please move to informative.
> 
> Fixed.
> 
>> 12) Finally, two tiny things from the nits check:
>> https://www.ietf.org/tools/idnits?url=https://www.ietf.org/archive/id/draft-ietf-tcpinc-tcpeno-08.txt
>> - There shouldn’t be reference in the abstract. You can just remove it; TLS is well known. You may want to add it in section 7.1 instead.
>> - RFC 5226 is obsolete
>> Both could be fixed by the editor but as you anyway have to make a new revision, so you can fix these as well.
> 
> Both fixed.
> 
> If you just confirm what to do about the TCP option kinds and obviously
> voice any objections to my proposed fixes above, we can get a new draft
> uploaded soon.

Great! Please also check Joe’s comments on SYN data. There might be another few things to note to improve clarity on the usage. 

Let me know if you have any more questions!

Thanks!
Mirja


> 
> Thanks,
> David