Subject: [DNSOP] Eric Rescorla's No Objection on draft-ietf-dnsop-session-signal-12: (with COMMENT)

From: Eric Rescorla <[email-masked]>
Date: Tue, 31 Jul 2018 15:28:28 -0700

Eric Rescorla has entered the following ballot position for
draft-ietf-dnsop-session-signal-12: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-dnsop-session-signal/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Rich version of this review at:
https://mozphab-ietf.devsvcdev.mozaws.net/D4358

IMPORTANT
S 5.3.
>      field set to zero, and MUST NOT elicit a response.
>   
>      Every DSO request message (QR=0) with a nonzero MESSAGE ID field is
>      an acknowledged DSO request, and MUST elicit a corresponding response
>      (QR=1), which MUST have the same MESSAGE ID in the DNS message header
>      as in the corresponding request.

How do I handle duplicate message IDs on the responder? Did I miss
where you said this? Is this just an error?

S 9.3.
>   
>      Requests to register additional new DSO Type Codes in the
>      "Unassigned" range 0040-F7FF are to be recorded by IANA after Expert
>      Review [RFC8126].  At the time of publication of this document, the
>      Designated Expert for the newly created DSO Type Code registry is
>      [*TBD*].

What is the standard for the expert to follow

COMMENTS
S 1.
>      is appended to the end of the DNS message header.  When displayed
>      using packet analyzer tools that have not been updated to recognize
>      the DSO format, this will result in the DSO data being displayed as
>      unknown additional data after the end of the DNS message.  It is
>      likely that future updates to these tools will add the ability to
>      recognize, decode, and display the DSO data.

I'm sure you will get to this soon, but what are the backward
compatibility implications for the two endpoints.

S 3.
>      The unqualified term "session" in the context of this document means
>      the exchange of DNS messages over a connection where:
>   
>      o  The connection between client and server is persistent and
>         relatively long-lived (i.e., minutes or hours, rather than
>         seconds).

This is a surprising taxonomy. I would assume that some of the options
you are proposing would be relevant with a 30s connection (very long
by HTTP standards!)

S 3.
>      Where this specification says, "close gracefully," that means sending
>      a TLS close_notify (if TLS is in use) followed by a TCP FIN, or the
>      equivalents for other protocols.  Where this specification requires a
>      connection to be closed gracefully, the requirement to initiate that
>      graceful close is placed on the client, to place the burden of TCP's
>      TIME-WAIT state on the client rather than the server.

Does this mean that the server will ask the client to close?

S 3.
>      connection to be closed gracefully, the requirement to initiate that
>      graceful close is placed on the client, to place the burden of TCP's
>      TIME-WAIT state on the client rather than the server.
>   
>      Where this specification says, "forcibly abort," that means sending a
>      TCP RST, or the equivalent for other protocols.  In the BSD Sockets

Because you bother to mention TLS above, what about non-close_notify
TLS alerts?

S 3.
>      the server's listening socket.
>   
>      The terms "initiator" and "responder" correspond respectively to the
>      initial sender and subsequent receiver of a DSO request message or
>      unacknowledged message, regardless of which was the "client" and
>      "server" in the usual DNS sense.

Might be helpful to say earlier that this is a request/response
protocol

S 3.
>   
>      DNS Stateful Operations uses three kinds of message: "DSO request
>      messages", "DSO response messages", and "DSO unacknowledged
>      messages".  A DSO request message elicits a DSO response message.
>      DSO unacknowledged messages are unidirectional messages and do not
>      generate any response.

This would be useful further up.

S 5.1.
>   
>      DNS over plain UDP [RFC0768] is not appropriate since it fails on the
>      requirement for in-order message delivery, and, in the presence of
>      NAT gateways and firewalls with short UDP timeouts, it fails to
>      provide a persistent bi-directional communication channel unless an
>      excessive amount of keepalive traffic is used.

Note that this is going to make things not work super-well with DNS-
over-QUIC unless you use one stream only.

S 5.1.
>   
>      If the RCODE is set to any value other than NOERROR (0) or DSOTYPENI
>      ([TBA2] tentatively 11), then the client MUST assume that the server
>      does not implement DSO at all.  In this case the client is permitted
>      to continue sending DNS messages on that connection, but the client
>      SHOULD NOT issue further DSO messages on that connection.

Why is this a SHOULD and not a MUST?

S 5.1.3.
>      to any problems that could be result from the inadvertent replay that
>      can occur with zero round-trip operation.
>   
>   5.1.3.  Middlebox Considerations
>   
>      Where an application-layer middlebox (e.g., a DNS proxy, forwarder,

I'm having trouble with this section. Is it a set of requirements on
middleboxes or statements of fact? If the latter, it seems like there
are a bunch of ways for middleboxes to mess things up,

S 5.4.
>         generate a response, the application-layer software informs the
>         TCP implementation that it should go ahead and send the TCP ACK
>         and window update immediately, without waiting for the Delayed ACK
>         timer.  Unfortunately it is not known at this time which (if any)
>         of the widely-available networking APIs currently include this
>         capability.

Are you going to make a recommendation here?

S 6.6.1.2.
>      client a Retry Delay message, or by forcibly aborting the underlying
>      transport connection) the client SHOULD try to reconnect, to that
>      service instance, or to another suitable service instance, if more
>      than one is available.  If reconnecting to the same service instance,
>      the client MUST respect the indicated delay, if available, before
>      attempting to reconnect.

Do you want to recommend some sort of randomness around this value to
avoid avalanche?

S 8.2.
>      The table below indicates, for each of the three TLVs defined in this
>      document, whether they are valid in each of ten different contexts.
>   
>      The first five contexts are requests or unacknowledged messages from
>      client to server, and the corresponding responses from server back to
>      client:

Nit. This text is a tiny bit hard to read, because you don't list S-P,
etc.

S 10.
>      messages are subject to the same constraints as any other DNS-over-
>      TLS messages and MUST NOT be sent in the clear before the TLS session
>      is established.
>   
>      The data field of the "Encryption Padding" TLV could be used as a
>      covert channel.

Why not require this to be 0, then?

---

From: Ted Lemon <[email-masked]>
Date: Tue, 31 Jul 2018 22:28:10 -0400

> where you said this? Is this just an error?

In order for the responder to detect this condition, it has to track
message IDs.   The existing implementation that I have doesn't track
message IDs—it just responds with the message ID that it got.   There's no
hash table of known incoming message IDs, and maintaining such a table
would add complexity.   I think that this is okay, because it's the
requester that loses in this case—it's effectively attacking itself.
 However, if the responder does track message IDs, then it could be an
issue.   So I've added the following text:

 If a client or server receives a response (QR=1) where the MESSAGE ID is
zero, or is
 any other value that does not match the MESSAGE ID of any of its
outstanding operations,
 this is a fatal error and the recipient MUST forcibly abort the connection
immediately.

+If a responder receives a request (QR=0) where the MESSAGE ID is not zero,
and
+the responder tracks query MESSAGE IDs, and the MESSAGE ID
+matches the MESSAGE ID of a query it received for which a response has not
yet been sent,
+it MUST forcibly abort the connection immediately.   This behavior is
required to prevent
+a hypothetical attack that takes advantage of undefined behavior in this
case.   However,
+if the server does not track MESSAGE IDs in this way, no such risk exists,
so tracking
+MESSAGE IDs just to implement this sanity check is not required.

 Does this address your concern?

> What is the standard for the expert to follow

I added this (the text has changed somewhat as a result of a previous
comment):

 Requests to register additional new DSO Type Codes
 in the "Unassigned" range 0040-F7FF
 are to be recorded by IANA after Expert Review {{!RFC8126}}.
+The expert review should validate that the requested type code
+is specified in a way that conforms to this specification, and
+that the intended use for the code would not be addressed with
+an experimental/local assignment.

 DSO Type Codes in the "experimental/local" range F800-FBFF
 may be used as Experimental Use or Private Use values {{!RFC8126}}

OK?

> compatibility implications for the two endpoints.

Do you mean in terms of requesters and responders that don't implement DSO
but receive a DSO message?   I think we've specified that adequately—the
responder is expected to return "Not Implemented" because of the status
code.   As I mentioned in a different response, this is the behavior of at
least BIND 9 and Google's 8.8.8.8 server.   1.1.1.1 just didn't reply to
the message.   So I realized, and maybe this is what you intended me to
realize, that the text doesn't address several possible outcomes of trying
to establish a DSO session.   I believe the following changes should do it:

 sending DNS messages on that connection, but the client SHOULD NOT
 issue further DSO messages on that connection.

+Two other possibilities exist: the server might drop the connection, or
+the server might send no response to the DSO message.   In the first
+case, the client SHOULD mark the server as not supporting DSO, and not
+attempt a DSO connection for some period of time (at least an hour)
+after the failed attempt.   The client MAY reconnect but not use
+DSO, if appropriate.
+
+In the second case, the client SHOULD set a reasonable timeout, after
+which time the server will be assumed not to support DSO.   At this
+point the client MUST drop the connection to the server, since the
+server's behavior is out of spec, and hence its state is undefined.
+The client MAY reconnect, but not use DSO, if appropriate.
+
 When the server receives a DSO request message
 from a client, and transmits a successful NOERROR response to that
 request, the server considers the DSO Session established.
@@ -477,7 +490,11 @@ clients and servers should behave as described in this
specification with
 regard to inactivity timeouts and session termination, not as previously
 prescribed in the earlier specification for DNS over TCP {{!RFC7766}}.

-Note that for clients that implement only the DSO-TYPEs defined in
+Because the Keepalive TLV can't fail (that is, can't return an RCODE
+other than NOERROR), it is an ideal candidate for use in establishing
+a DSO session.   Any other option that can only succeed MAY also be
+used to establish a DSO session.
+For clients that implement only the DSO-TYPEs defined in
 this base specification, sending a Keepalive TLV is the only
 DSO request message they have available to initiate a DSO Session.
 Even for clients that do implement other future DSO-TYPEs, for simplicity

> you are proposing would be relevant with a 30s connection (very long

Hm.   I think this text is motivated by the first and second identified use
cases for this document: DNS Push and DNSSD Discovery Relay.   Neither
would be expected to have 30-second connections.   However, in principle I
think that you are correct, and actually I think specifying the time is
unnecessary, so I'd propose the following change:

 DNS messages over a connection where:

 - The connection between client and server is persistent and relatively
-  long-lived (i.e., minutes or hours, rather than seconds).
+  long-lived.
 - Either end of the connection may initiate messages to the other.

 In this document the term "session" is used exclusively as described above.

S 3.
> Does this mean that the server will ask the client to close?

Yes, this is specified in 6.6.

S 3.
> Because you bother to mention TLS above, what about non-close_notify

That's the only alert that would be used here.   TLS alerts used in
connection establishment and in ongoing connections are deferred to the
DNS-over-TLS spec, which in turn defers to BCP 195.

S 3.
> Might be helpful to say earlier that this is a request/response

It's not.   Not all requests require responses: Unacknowledged requests
explicitly don't.

> This would be useful further up.

Do you mean in the introduction?

> over-QUIC unless you use one stream only.

We don't expect this to work with DNS-over-QUIC without some restriction
like this, hence the text in paragraph 3 of section 5.1.

S 5.1.
> Why is this a SHOULD and not a MUST?

Agreed, fixed.

> are a bunch of ways for middleboxes to mess things up,

This was covered in the gen-art review, and the text was updated to address
the comment that was made there about this.   We believe that we have
thought this through and that the text as written is correct.

S 5.4.
> Are you going to make a recommendation here?

Out of scope for DNSOP to update TCP stack APIs.

> Do you want to recommend some sort of randomness around this value to

The server is specifying the retry delay, so if the client adds randomness,
that could result in more collisions rather than fewer.

> Nit. This text is a tiny bit hard to read, because you don't list S-P,

Hm, I believe that this should be using the requester/responder vernacular,
not the client/server vernacular.   I've asked the team to confirm.

S 10.
> Why not require this to be 0, then?

The text describing the option currently says this:

As specified for the EDNS(0) Padding Option {{!RFC7830}}
the PADDING bytes SHOULD be set to 0x00.  Other values MAY be used,
for example, in cases where there is a concern that the padded
message could be subject to compression before encryption.
PADDING bytes of any value MUST be accepted in the messages received.

Do you believe that this is an invalid concern?

---

From: Eric Rescorla <[email-masked]>
Date: Tue, 31 Jul 2018 19:55:50 -0700

On Tue, Jul 31, 2018 at 7:28 PM, Ted Lemon <[email-masked]> wrote:

>  this is a fatal error and the recipient MUST forcibly abort the
> connection immediately.
>
> +If a responder receives a request (QR=0) where the MESSAGE ID is not
> zero, and
> +matches the MESSAGE ID of a query it received for which a response has
> not yet been sent,
> +if the server does not track MESSAGE IDs in this way, no such risk
> exists, so tracking

Yeah, this seems fine. Didn't mean to make you do a lot of work here, just
noticed as I was reading.

>  may be used as Experimental Use or Private Use values {{!RFC8126}}

LGTM.

> @@ -477,7 +490,11 @@ clients and servers should behave as described in
> this specification with

You're doing more than I asked for here (thanks!). I just found myself
wondering at this point in the spec how this worked and so was looking for
an overview. But this text is very helpful, so I think it's good to add.

> Hm.   I think this text is motivated by the first and second identified
> use cases for this document: DNS Push and DNSSD Discovery Relay.   Neither

LGTM.

> Yes, this is specified in 6.6.

Yep. I just make these comments as I go and it wasn't clear here. Sorry if
that was confusing.

> DNS-over-TLS spec, which in turn defers to BCP 195.

OK.

> It's not.   Not all requests require responses: Unacknowledged requests

Fair enough. I guess where I was going was that you talk about request here
but it's not clear till later that there might be responses. Not a big deal.

> Do you mean in the introduction?

Yes.

> like this, hence the text in paragraph 3 of section 5.1.

Sure. I just wanted to point out that that was a consequence of this design

S 5.1.
> This was covered in the gen-art review, and the text was updated to
> address the comment that was made there about this.   We believe that we
> have thought this through and that the text as written is correct.
>

Sorry, has this been changed in a new version?

> Out of scope for DNSOP to update TCP stack APIs.

Sorry, it wasn't clear from context what I was referring to here. You
discuss a number
of strategies and then say they are all bad. Do you had a recommendation
for what
people ought to do?

> The server is specifying the retry delay, so if the client adds
> randomness, that could result in more collisions rather than fewer.
>

Sure, if the server actually randomizes it itself. Perhaps that's worth
recommending?

> Hm, I believe that this should be using the requester/responder
> vernacular, not the client/server vernacular.   I've asked the team to
> confirm.
>
> S 10.

Mostly I just missed this. With that said, generally we are discouraging
compression in cryptographic protocols.
OTOH, I'm not that worried about the covert-channel either, so it's kind of
a tossup
-Ekr

---

From: Ted Lemon <[email-masked]>
Date: Tue, 31 Jul 2018 23:06:36 -0400

> vernacular, not the client/server vernacular.   I've asked the team to

No, in fact I was incorrect about this.   I've added text to explain the
S-* messages as well.

---

From: Ted Lemon <[email-masked]>
Date: Tue, 31 Jul 2018 23:21:42 -0400

> noticed as I was reading.

No problem, I think your comments on this and some other points that you
probably thought of as asides resulted in significant improvements. :)

> an overview. But this text is very helpful, so I think it's good to add.

I agree.   I think not having this text here would likely result in some
head scratching or unexpected behavior, so it's better to make it explicit.

 S 3.

>> Do you mean in the introduction?

I think this is a tossup, because more text makes the document longer, and
I don't think this adds much value for someone who's actually going to
implement the document.

> Sure. I just wanted to point out that that was a consequence of this design

OK!

The text you commented on is the new version, with some additional text to
address a point raised in the gen-art review.   We gamed this out pretty
thoroughly—I don't think there's an actual problem here, although I
understand why it looks that way.   The worst case scenario here is that a
middlebox breaks the session signaling, which just looks like "the server
doesn't support session signaling," which is fine.

> of strategies and then say they are all bad. Do you had a recommendation

We don't.   This is text that was heavily touched by Mirja's DISCUSS, so I
suspect that by the time Stuart is done addressing her DISCUSS, this will
have been fixed.   However, I will try to remember to check just to be
sure. :)

> Sure, if the server actually randomizes it itself. Perhaps that's worth

Yeah, I was dragging my feet on this, but inclined the same way.

diff --git a/draft-ietf-dnsop-session-signal.md
b/draft-ietf-dnsop-session-signal.md
index a9037e0..85bc28d 100644
--- a/draft-ietf-dnsop-session-signal.md
+++ b/draft-ietf-dnsop-session-signal.md
@@ -1463,6 +1463,12 @@ are described in {{delay}}.
 After sending a Retry Delay message,
 the server MUST NOT send any further messages on that DSO Session.

+The server MAY randomize retry delays in situations where many retry
delays are sent
+in quick succession, so as to avoid all the clients attempting to
reconnect at once.
+In general, implementations should avoid using the Retry Delay message in
a way that
+would result in many clients reconnecting at the same time, if every
client attempts
+to reconnect at the exact time specified.
+
 Upon receipt of a Retry Delay message from the server,
 the client MUST make note of the reconnect delay for this server,
 and then immediately close the connection gracefully.
@@ -1520,7 +1526,9 @@ or by forcibly aborting the underlying transport
connection)
 the client SHOULD try to reconnect,
 to that service instance, or to another suitable service instance, if more
than one is available.
 If reconnecting to the same service instance, the client MUST respect the
indicated delay,
-if available, before attempting to reconnect.
+if available, before attempting to reconnect.   Clients should not attempt
to randomize the delay;
+the server will randomly jitter the retry delay values it sends to client
if this behavior is
+desired.

 If the service instance will only be out of service for a short
maintenance period,
 it should use a value a little longer that the expected maintenance window.

> OTOH, I'm not that worried about the covert-channel either, so it's kind
> of a tossup
>

OK.   I'm going to call it done then, at least until we hear back from
Stuart and Mirja.

---

From: Eric Rescorla <[email-masked]>
Date: Tue, 31 Jul 2018 20:38:54 -0700

> doesn't support session signaling," which is fine.

I think I'm just confused about the meaning of this text. Is it an analysis
or a recommendation?

OK.

S 6.6.1.2.
>  to that service instance, or to another suitable service instance, if
> more than one is available.
> +if available, before attempting to reconnect.   Clients should not
> attempt to randomize the delay;

LGTM.

> OK.   I'm going to call it done then, at least until we hear back from

WFM.