Re: [dns-privacy] AD review of draft-ietf-dprive-dnsoquic

[Sara Dickinson <[e-mail]>]

Hi Eric, 

Many thanks for the review. I’ve created PR https://github.com/huitema/dnsoquic/pull/134 with proposed updates from your comments.

> On 10 Dec 2021, at 06:42, Eric Vyncke (evyncke) <evyncke=[e-mail]> wrote:
> 
> Dear authors,
> 
> In the revised I-D, please also consider Martin Duke (the transport AD)'s WGLC comments, see the thread at
> https://mailarchive.ietf.org/arch/msg/dns-privacy/F35q1jpUEPPqaq5gYVM8aYFoQUQ/
> 
> Especially around section 10.2 and the demux of DTLS & QUIC if both protocols run on the same port. Martin has suggested some text for this part, let's use it and remove any potential friction in the publication process.

PR for this approved and merged (thanks Martin!)

> 
> Regards
> 
> -éric
> 
> 
> 
> On 09/12/2021, 14:40, "Eric Vyncke (evyncke)" <[e-mail]> wrote:
> 
>   As usual, when a document is submitted for publication, as the responsible AD, I do my own review of the draft before going to the IETF Last Call.
> 
>   First, thank you Brian for a detailed doc shepherd write-up. Your points about the 'down-ref' are valid and for now I would leave them like they are.

Yes - thank you Brian. 

FYI: 
- PR https://github.com/huitema/dnsoquic/pull/135 includes a change to make RFC8094 informative and 
- PR https://github.com/huitema/dnsoquic/pull/132 provides updated text based on Alex’s review also making RFC8467 informative.

> 
>   Thanks, as well to the WG and the authors Christian, Sara, and Alisson for their work. The document is well written and easy to read: congratulations !
> 
>   Please find below my review comments:
> 
>   Abstract: should it state where DoQ could be used (i.e., between which DNS functions notably by citing XFR) ?

Yes - good point. I’ve added text.

> 
>   Section 1: s/ this document is intended to specify/ this document specifies/ ?
> 
>   Section 2: please use the real template from BCP14 (section 2 of RFC 8174)

Ack to both. 

> 
>   Section 5: should there be a sentence "this section is normative" to be consistent with a similar sentence in section 4 ?

Would it be better to clarify the text in section 4?

“Whilst all other sections in this document are normative, this section is informative in nature."

> 
>   Section 5.1.2: isn't the 3rd § obvious ? Hence could it be removed ?

Yes - I suppose it is an implementation detail, removed.

> Unsure whether there is value in the last §, especially as it appears to contradict the previous ones.

This was added in response to having read *multiple* papers/presentations on DoT vs DoH where a statement like ‘DoT must run on port 853 whereas DoH runs on port 443’ is made regarding deployment obstacles for DoT. For a wider audience it does seem useful to point out that 443 is a possibility for DoQ deployments.

> 
>   Section 5.3: should DOQ_REQUEST_CANCELLED also be available when the server wants to close a transaction (e.g., when there are multiple responses/XFR) ?

Well, section 5.3.2 really deals with the server side having issues and using a stream reset + DOQ_INTERNAL_ERROR to signal this. I’m not sure when a server would choose to send DOQ_REQUEST_CANCELLED as the error here as opposed to DOQ_INTERNAL_ERROR since the only reason to not send all the responses would be an error of some kind (even for XFR)… or is there another use case I’m missing?

> 
>   Section 6.3: contains two SHOULD, the general expectation is that the document describes when it is acceptable not to follow the SHOULD. Or perhaps should RECOMMENDED be used ?

The kind of reasons I always assume are behind these kind of SHOULDs are: is an API for this exposed, does it increase code complexity too much or does it add too much performance overhead, etc. I’m not sure we have a specific case for section 6.3 but Christian may be able to point to one.

> 
>   Section 6.4: same comment as above and also in other places.

6.4 is proposed to change to a MUST for the use of padding in PR https://github.com/huitema/dnsoquic/pull/132. The main reason I can see for not using a QUIC padding API if it existed would be code complexity e.g. the implementation may choose to re-use padding logic already implemented in the DNS layer for DoT/DoH. I can add text about this if you think it is useful?

> 
>   Section 6.7: should this document formally update RFC 1995, 5936, 7766 ? I think so as it is similar to RFC 9103, which updates those 3 RFCs.

Good question but I don’t think so - all of those documents are specific to DNS over TCP. This document doesn’t define any new behaviour for XFR that changes how XFR over TCP behaves (which RFC9103 did) nor does it update anything in RFC7766.

> 
>   Section 7.1: s/ To our knowledge/To the authors' knowledge/ ?

Agreed.

>   Also in " it performs well compared" does it mean "better" or "similar" ?

Adguard haven’t published exact data yet but did say in a presentation  “it seems that…. it does provide advantage over DoH in cellular data networks, as expected” and their user feedback "ranges from very positive to neutral”.  I’m reluctant to declare it ‘better’ without more raw data….

> 
>   Section 9.3: " due to the prevalence of NAT" is not the only cause as IPv6 nodes often change their IPv6 addresses. Please extend the text here as the DoQ may not have an API to check whether the IPv6 address has changed.

Text added. 

> 
>   Finally, while I am not a native English speaker (but I relied on the Microsoft Word spell-checker on the attached document), I think that there are some nits in the text:
>   - Missing a lot of '-' in some constructions: "general purpose transport", "server initiated transactions", "long term state ", ...
>   - "however" should be followed by a comma
>   - No "n" in "an" in " an unidirectional QUIC stream"
>   - "acknowledgeemnts"
>   - "Provisonal"

All fixed, I hope (but please check as I had to use a different checker tool).

Best regards

Sara.