On Aug 4, 2018, at 1:53 AM, Stewart Bryant <stewart.bryant@xxxxxxxxx> wrote: > > Reviewer: Stewart Bryant > Review result: On the Right Track > > 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 treat these comments just > like any other last call comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-ietf-doh-dns-over-https-12 > Reviewer: Stewart Bryant > Review Date: 2018-08-04 > IETF LC End Date: 2018-08-08 > IESG Telechat date: Not scheduled for a telechat > > Summary: > > ID-Nits picks up a downref, but the appropriate warning was included in the IETF LC email. > > Major issues: > > 4. Selection of DoH Server > > SB> I am surprised that there is no discussion of the need to specify > SB> the raw IP address of the sever as part of the URI, and that there > SB> is no discussion of the server address IP address family in the document. A URI can have either host names or IP addresses in the authority part. > SB> I would have thought that there should at least be a reference to > SB> "happy eyeballs" and and discussion of an optimization strategy of > SB> what to do if there was a response on Ipv4 and not IPv6 etc etc. Happy eyeballs is orthogonal to the document. How the client turns a host name in the URI into an IP address is completely up to the client using whatever strategies it wants to use. > > ========== > > The same DNS query for www.example.com, using the POST method would > be: > > :method = POST > :scheme = https > :authority = dnsserver.example.net > :path = /dns-query > accept = application/dns-message > content-type = application/dns-message > content-length = 33 > > <33 bytes represented by the following hex encoding> > 00 00 01 00 00 01 00 00 00 00 00 00 03 77 77 77 > 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00 00 01 00 > 01 > > SB> At this point in the text am at a loss to understand the 33 bytes. > SB> Having read to the end of the text, I assume that this is a binary encoded > SB> DNS query in "one the wire format" but this point needs to be made clear, > SB> as does the explicit encoding. Will do. > SB> It would be really useful if there was an > SB> example of both formats side by side. Could you explain more what you want here? The two formats will look the same: a bunch of hex. > > ========= > The DNS query is 94 bytes represented by the following hex encoding > SB> Again in what format? Wire format. We'll add a note of that. > > ========= > :method = GET > :scheme = https > :authority = dnsserver.example.net > :path = /dns-query? (no space or CR) > dns=AAABAAABAAAAAAAAAWE-NjJjaGFyYWN0ZXJsYWJl (no space or CR) > bC1tYWtlcy1iYXNlNjR1cmwtZGlzdGluY3QtZnJvbS1z (no space or CR) > dGFuZGFyZC1iYXNlNjQHZXhhbXBsZQNjb20AAAEAAQ > accept = application/dns-message > > SB> I am not a DNS or HTTP specialist, so please help me understand > SB> what the string starting "dns=" is. The path is the last part of the HTTP query. > > ========== > > 5.2.2. HTTP Response Example > > SB> Nits says: > > -- The document has examples using IPv4 documentation addresses according > to RFC6890, but does not use any IPv6 documentation addresses. Maybe > there should be IPv6 examples, too? > > SB> I am not sure if there are any IPv4 vs IPv6 issues but it would be helpful to > SB> clarify the point. We are changing the example to be for IPv6 (based on the AD review). > > ========= > > <64 bytes represented by the following hex encoding> > 00 00 81 80 00 01 00 01 00 00 00 00 03 77 77 77 > 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00 00 01 00 > 01 03 77 77 77 07 65 78 61 6d 70 6c 65 03 63 6f > 6d 00 00 01 00 01 00 00 00 80 00 04 C0 00 02 01 > > SB> At this point in the text I am still wondering what this encoding means, I assume that > SB> it is a hex representation of the payload received > SB> back from a normal DNS query. Correct. > > ========= > > 7. Definition of the application/dns-message media type > > The data payload for the application/dns-message media type is a > single message of the DNS on-the-wire format defined in Section 4.2.1 > of [RFC1035]. > > SB> That would have been useful to have known earlier > SB> However 4.2.1 just talks about UDP and not the message syntax. > SB> Also presumably we are talking about the payload of the UDP > SB> message excluding the UDP header. We will make the contents of the payloads clearer earlier in the document. The reason the reference is to Section 4.2.1 of RFC 1035 is that the DNS wire format changes if it is over TCP. > > SB> "on-the-wire" is a potentially confusing term, since it gets us > SB> into network byte order etc. > SB> I think the whole thing would be clearer is you said that your > SB> message was identical to the payload of the UDP payload of an > SB> RFC1035 as seen by a server Agree. > SB> (and then add some specification > SB> about the payload to hex method you are using) Not sure what you mean here. > SB> It would also be helpful to the reader to note whether > SB> any of the 25 updates to RFC1035 impact any of the specification > SB> described in this memo. None of them do. > > ======= > > The format was originally for DNS over UDP. Although > [RFC1035] says "Messages carried by UDP are restricted to 512 bytes", > that was later updated by [RFC6891]. This media type restricts the > maximum size of the DNS message to 65535 bytes. Note that the wire > format used in this media type is different than the wire format used > in [RFC7858] (which uses the format defined in Section 4.2.2 of > [RFC1035]). > > SB> 4.2.2 does not seem to describe a format, unless you are implicitly > SB> saying that 4.2.1 does not have a length 4.2.2 does That is exactly what we are saying because that is what RFC 1035 says. > SB> and you > SB> are encoding the length another way and hence use 4.2.1 format. The DNS wire format for UDP does not encode the length, and so DoH does not either. > SB> Either way neither of these sections of RFC1035 really specify the > SB> format, that is provided earlier in the RFC. We will clear this up. > > =========== > > Encoding considerations: This is a binary format. The contents are a > DNS message as defined in RFC 1035. The format used here is for DNS > over UDP, which is the format defined in the diagrams in RFC 1035. > > SB> With or without the UDP header? Without. RFC 1035 says nothing about the UDP header as part of the wire format. > > ========== > > Minor issues: > > Abstract > > This document describes how to make DNS queries over HTTPS. > > SB> This is the shortest abstract I have seen in a long while, if > SB> not forever. It would be helpful to the undecided reader if > SB> a little more context from the Introduction was included in the > SB> abstract. Fixed (based on the AD review). It now says: This document defines a protocol for sending DNS queries and getting DNS responses over HTTPS. Each DNS query-response pair is mapped into an HTTP exchange. > > ========= > > Nits/editorial comments: > > These caches may exist beteen the > SB> s/beteen/between/ Good catch; will fix. Thanks for the review! --Paul Hoffman
