Subject: [DNSOP] Genart telechat review of draft-ietf-dnsop-session-signal-11

From: Joel Halpern <[email-masked]>
Date: Thu, 05 Jul 2018 14:48:44 -0700

Reviewer: Joel Halpern
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 wait for direction from your
document shepherd or AD before posting a new version of the draft.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-dnsop-session-signal-11
Reviewer: Joel Halpern
Review Date: 2018-07-05
IETF LC End Date: 2018-06-25
IESG Telechat date: 2018-08-02

Summary: This document is ready for publication as a Proposed Standard RFC

Some of my earlier comments have been addressed.  It appears that an effort was
made to address more, but I was apparently unclear.  I have copied the comments
that seem to still apply, with elaboration.  If I am still unclear, please
contact me.

Major issues: N/A

Minor issues:
    Section 5.1.3 places some requirements on application level middleboxes,
    and includes a very clear explanation of why it places these requirements.
    While it may be "obvious" to one who lives and breathes DNS, I think it
    would help to explain why the usual operation of an existing middlebox will
    (typically? always?  inherently?) meet this requirement.  To rephrase, the
    text says things like "the middlebox MUST NOT blindly forward DSO messages
    in either direction." Apparently, somehow, the existing world middleboxes
    will do comply with this.  How?

    The third and fourth paragraphs of section 5.2.2 do not talk about optional
    additional TLVs.  It would be helpful if the document stated that in
    addition to those additional TLVs required by the primary TLV, other TLVs
    may be included based on their individual definition, independent of the
    definition of the primary TLV.  (Both the Encryption padding and the delay
    retry TLVs may be included in suitable messages without being called out in
    the definition of the primary TLVs.)
    An effort appears to have been made to address this, which suggests I was
    unclear.  The text says:
        A DSO response message may contain no TLVs, or it may be specified to
        contain one or more TLVs appropriate to the information being
        communicated.
    The definition of the specific response messages does not discuss the
    encryption padding or delay response TLVs.  They are clearly intended to
    be allowed.  So can we tune the text to make that clear.  I think the
    intention is that the specification of the response message indicates which
    TVLs are required, and that others are allowed.  So say that.

Nits/editorial comments:
    Section 5.4 talks about by default the TCP data ack and the DSO reply
    message being combined.  Doesn't this depend upon the responsiveness of the
    DSO engine?  Is there an implicit assumption about such timeliness (sub 200
    ms)?
    I suspect from the lack of comment on this that I am missing something
    obvious?

---

From: Ted Lemon <[email-masked]>
Date: Thu, 5 Jul 2018 18:13:16 -0400

Joel, it's immaterial whether the DSO engine responds in time or not.   If
it responds in time, the ack and the response will be combined; if it does
not, then Nagle's algorithm will ensure that the ack goes out, and the
response will go out in a later packet.   Either outcome is fine.   There
is no need to caution the implementor that they should ensure a quick
response—if they don't care to get the response out in 200ms, they
obviously don't care about performance, and that's perfectly fine.   It is
absolutely *not* a requirement that they do so.   The point of this text in
the document is to inform implementors that *do* care about performance
about the interaction between Nagle and DA.

We looked at your comment about middleboxes and couldn't figure out what
problem you are trying to solve here.   If a middlebox is not DSO-aware,
it's going to prevent DSO from working (which would be correct), or else
forward it unchanged (which would also be correct).   The text is an
admonishment for implementations that are DSO-aware.   If an implementation
is not DSO-aware, then adding text to instruct the implementor, who
presumably will not read it, doesn't make sense.

Your proposed change to 5.2.2 seems fine to me—I don't remember what
happened with that.

---

From: "Joel M. Halpern" <[email-masked]>
Date: Thu, 5 Jul 2018 18:19:46 -0400

In line.  The general point is that the document should be clear to 
readers who understand the space but do not live it at the detail of 
those who authored it.

Joel

On 7/5/18 6:13 PM, Ted Lemon wrote:
> Joel, it's immaterial whether the DSO engine responds in time or not.   
> If it responds in time, the ack and the response will be combined; if it 
> does not, then Nagle's algorithm will ensure that the ack goes out, and 
> the response will go out in a later packet.   Either outcome is fine.   
> There is no need to caution the implementor that they should ensure a 
> quick response—if they don't care to get the response out in 200ms, they 
> obviously don't care about performance, and that's perfectly fine.   It 
> is absolutely /not/ a requirement that they do so.   The point of this 
> text in the document is to inform implementors that /do/ care about 
> performance about the interaction between Nagle and DA.

The text says that combining will happen.  I am well aware that if the 
processing is in time, that is normal processing.  But that is not what 
the text says.  It would take about a sentence to fix the text to match 
what you describe above.

> admonishment for implementations that are DSO-aware.   If an 
> implementation is not DSO-aware, then adding text to instruct the 
> implementor, who presumably will not read it, doesn't make sense.

The text says that middleboxes MUST NOT blindly forward DSO messages. 
Your text above says that actually it doesn't matter, and no, existing 
middleboxes will not comply with this MUST NOT.  A MUST NOT in a 
document is generally a statement that things will break if they behave 
wrong.  Thus, what the document says, combined with your behavioral 
description, is that existing middleboxes will break DSO.  I doubt that 
(which is why I consider this minor rather than major.)  Please fix the 
text to either not require changes to existing middleboxes or to explain 
to readers why and how existing middlebox will comply with the MUST NOT.

> On Thu, Jul 5, 2018 at 5:48 PM, Joel Halpern <[email-masked] 
> <mailto:[email-masked]>> wrote:
> 
>     Reviewer: Joel Halpern
>     <https://trac.ietf.org/trac/gen/wiki/GenArtfaq
>     <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>>.
>

---

From: Ted Lemon <[email-masked]>
Date: Thu, 5 Jul 2018 18:28:00 -0400

The text also says that it's fine to blindly forward DSO messages if the
middlebox isn't modifying the stream, e.g. in a NAT.   It really is quite
clear on that point.   The case where it's bad to blindly forward DSO
messages is when there is no stream that's the same stream on both sides of
the middlebox.   In this case, it really is a MUST NOT.   What reader's
need are you trying to address here?   Who is going to be reading the
document and is going to be confused about this?

As far as I can tell, the text on Nagle is correct, and you haven't pointed
to an error.  You appear to have said that you want it to place a new
requirement on the sender to respond within the 200ms DA window.   This is
unnecessary.   What am I missing?

On Thu, Jul 5, 2018 at 6:19 PM, Joel M. Halpern <[email-masked]> wrote:

> readers who understand the space but do not live it at the detail of those
> who authored it.
>
> Joel
>
>> does not, then Nagle's algorithm will ensure that the ack goes out, and the
>> absolutely /not/ a requirement that they do so.   The point of this text in
>> the document is to inform implementors that /do/ care about performance
> processing is in time, that is normal processing.  But that is not what the
> text says.  It would take about a sentence to fix the text to match what
> you describe above.
>
>
> The text says that middleboxes MUST NOT blindly forward DSO messages. Your
> text above says that actually it doesn't matter, and no, existing
> middleboxes will not comply with this MUST NOT.  A MUST NOT in a document
> is generally a statement that things will break if they behave wrong.
> Thus, what the document says, combined with your behavioral description, is
> that existing middleboxes will break DSO.  I doubt that (which is why I
> consider this minor rather than major.)  Please fix the text to either not
> require changes to existing middleboxes or to explain to readers why and
> how existing middlebox will comply with the MUST NOT.
>
>
>

---

From: "Joel M. Halpern" <[email-masked]>
Date: Thu, 5 Jul 2018 18:41:15 -0400

I will try to elaborate on the problems below.
Joel

On 7/5/18 6:28 PM, Ted Lemon wrote:
> middlebox isn't modifying the stream, e.g. in a NAT.   It really is 
> quite clear on that point.   The case where it's bad to blindly forward 
> DSO messages is when there is no stream that's the same stream on both 
> sides of the middlebox.   In this case, it really is a MUST NOT.   What 
> reader's need are you trying to address here?   Who is going to be 
> reading the document and is going to be confused about this?

The text in 5.3.1 says that middleboxes MUST NOT do blind forwarding. 
Regarding the further text, given the wroding it is actually very hard 
to understand whether that is supposed to be an exception to the MUST 
NOT, or a description of some other case.
Even if the further text covers a lot of cases, the MUST NOT is 
presumably describing a situation that existing middleboxes do find 
themselves in.  I do think it is fair to ask for an explanation of why 
existing middleboxes that do fall into the relevant case will do the 
right thing.  Are there no such existing middleboxes (seems unlikely)? 
Do such existing middleboxes already for some reason do things that will 
prevent the blind forwarding (that would be great, just explain it)?  Or 
is it actually okay that existing middleboxes violate the MUST NOT (in 
which case, why is it a MUST NOT)?

> 
> As far as I can tell, the text on Nagle is correct, and you haven't 
> pointed to an error.  You appear to have said that you want it to place 
> a new requirement on the sender to respond within the 200ms DA window.   
> This is unnecessary.   What am I missing?

What the text in 5.4 actually says is that magically the data will be 
piggybacked.   It treats it as if there are only two cases, DSO requests 
that generate a response that automatically combine the two, and DSO 
requests that do not generate responses.  The reality is that there are 
three cases.
1) DSO requests that get responses and are responded fast enough to get 
nagle behavior and the resulting performance benefit,
2) DSO requests that get responses and for whatever reason are responded 
to more slowly
3) DSO requests that do not generate responses.

The fix, it seems to me is to make two small changes:
A) In the first sentence of 5.4, change "for DSO requests that generate 
a response" to "for DSO requests that generate a response and are 
responded to in a sufficiently timely fashion"
B) At the start of the second paragraph, change "For DSO requests that 
do not generate a response" to "For DSO requests that do not generate a 
response or those that generate a response but are not responded to in a 
sufficiently timely fashion"

This has the side benefit of making clear to implementors that ensuring 
responsiveness of their implementation will improve the network utilization.

---

From: Ted Lemon <[email-masked]>
Date: Mon, 9 Jul 2018 21:07:58 -0400

I got a bit wound up about not making the three changes that Joel called out in the updated session signal comment, and insisted on not making the changes because they didn't seem that important.   I had a bit more private conversation with Joel about it, and after some reflection decided that it might be worth suggesting some changes after all based on Joel's comments.

What I would propose to fix these three issues are the following three changes.

In 5.1.3:

OLD:
   Where an application-layer middlebox (e.g., a DNS proxy, forwarder,
   or session multiplexer) is in the path, the middlebox MUST NOT
   blindly forward DSO messages in either direction, and MUST treat the
   inbound and outbound connections as separate sessions.  This does not
   preclude the use of DSO messages in the presence of an IP-layer
   middlebox, such as a NAT that rewrites IP-layer and/or transport-
   layer headers but otherwise preserves the effect of a single session
   between the client and the server.
NEW:
   Where an application-layer middlebox (e.g., a DNS proxy, forwarder,
   or session multiplexer) is in the path, care must be taken to avoid
   inappropriately passing session signaling through the middlebox.

   In cases where a DSO session is terminated on one side of a middlebox,
   and then some session is opened on the other side of the middlebox in
   order to satisfy requests sent over the first DSO session, any such session
   MUST be treated as a separate session.

   This does not
   preclude the use of DSO messages in the presence of an IP-layer
   middlebox, such as a NAT that rewrites IP-layer and/or transport-
   layer headers but otherwise preserves the effect of a single session
   between the client and the server.  And of course it does not apply
   to middleboxes that do not implement DNS Stateless Operations.

   These restrictions do not apply to middleboxes that do not implement
   DSO: since they have no way to understand a DSO message, a pass-through
   middlebox like the one described in the previous paragraph will pass
   DSO messages unchanged or drop them (or possibly drop the connection).
   A middlebox that is not doing a strict pass-through will have no way
   to know on which connection to forward a DSO message, and therefore
   will not be able to behave incorrectly.

In 5.2.2:
OLD:
   A DSO response message may contain no TLVs, or it may be specified to
   contain one or more TLVs appropriate to the information being
   communicated.  This includes "Primary TLVs" and "Additional TLVs"
   defined in this document as well as in future TLV definitions.

NEW:
   A DSO response message may contain no TLVs, or it may be specified to
   contain one or more TLVs appropriate to the information being
   communicated.  This includes "Primary TLVs" and "Additional TLVs"
   defined in this document as well as in future TLV definitions.
   It may be permissible for an additional TLV to appear in a response
   to a primary TLV even though the specification of that primary TLV
   does not specify it explicitly.   See section 8.2 for more information.

In 5.4:
OLD:
   With most TCP implementations, for DSO requests that generate a
   response, the TCP data acknowledgement (generated because data has
   been received by TCP), the TCP window update (generated because TCP
   has delivered that data to the receiving software), and the DSO
   response (generated by the receiving application-layer software
   itself) are all combined into a single IP packet.  Combining these
   three elements into a single IP packet can give a significant
   improvement in network efficiency.

NEW:
   With most TCP implementations, for DSO requests that generate a
   response, the TCP data acknowledgement (generated because data has
   been received by TCP), the TCP window update (generated because TCP
   has delivered that data to the receiving software), and the DSO
   response (generated by the receiving application-layer software
   itself) are all combined into a single IP packet.  Combining these
   three elements into a single IP packet can give a significant
   improvement in network efficiency, assuming that the DSO response
   is sent before the TCP Delayed Acknowledgement timer goes off.

---

From: "Joel M. Halpern" <[email-masked]>
Date: Mon, 9 Jul 2018 21:27:33 -0400

Thank you Ted.  Those address my concerns.
Yours,
Joel

On 7/9/18 9:07 PM, Ted Lemon wrote:
> I got a bit wound up about not making the three changes that Joel called 
> out in the updated session signal comment, and insisted on not making 
> the changes because they didn't seem that important.   I had a bit more 
> private conversation with Joel about it, and after some reflection 
> decided that it might be worth suggesting some changes after all based 
> on Joel's comments.
>