Last Call Review of draft-ietf-add-ddr-08
review-ietf-add-ddr-08-secdir-lc-schwartz-2022-07-07-00

I think there are some interesting open questions about the structure of this
document, and some details about how recommendations are described, but the
technical components are sound.

Section 1:

> When DNS clients wish to use encrypted DNS protocols such as DNS-over-TLS
(DoT) [RFC7858], DNS-over-QUIC (DoQ) [RFC9250], or DNS-over-HTTPS (DoH)
[RFC8484], they require additional information beyond the IP address of the DNS
server, such as the resolver's hostname, non-standard ports, or URI templates.

This is arguably true, but none of these things are relevant in the simplest
cases for DoT and DoQ using IP addresses (Section 4).  In those cases, the
purpose of this specification is simply to indicate whether the resolver
supports DoT or DoQ.  That probably bears mentioning here, ahead of the less
common metadata like nonstandard ports.

> This document defines two mechanisms

I think this is probably a mistake.  The fact that there are two very different
mechanisms defined here, both known as DDR, has already generated a fair bit of
confusion.  The second mechanism (Section 5) also seems largely to be repeating
the recommendations in draft-ietf-svcb-dns.  Given that these two drafts are
entering review together, it might make sense to reconsider how material is
split between them.

Ideally, I think we would adjust the document so that "DDR" means "query
_dns.resolver.arpa", as that is what everyone seems to think it means.

Section 3:

Most of this text seems redundant with draft-ietf-svcb-dns.  I understand the
desire to avoid making readers read both documents, but some of the text seems
especially duplicative, e.g.

> If the client encounters a mandatory parameter in an SVCB record it does not
understand, it MUST NOT use that record to discover a Designated Resolver.

This is repeating the normative definition of "mandatory" from
draft-ietf-dnsop-svcb-https.  If it must be repeated, the repetition could be
flagged (e.g. 'As required by Section 8 of ...').

Section 4:

> When a DNS client is configured with an Unencrypted Resolver IP address, it
SHOULD query the resolver for SVCB records for the name "resolver.arpa" before
making other queries. Specifically, the client issues a query for
_dns.resolver.arpa ...

This seems contradictory at a glance.  It might be more correct to say '... for
SVCB records of a service with a scheme of "dns" and an Authority of
"resolver.arpa" ...'.  Also, SVCB-DNS defines the term "Binding Authority"
specifically to enable unambiguous specification of DDR, but DDR doesn't use
this term.

> Because this query is for an SUDN, which no entity can claim ownership over,
the ServiceMode SVCB response MUST NOT use the "." value for the TargetName.

This rationale is mysterious to me.  We've just told the resolver that they can
place SVCB records on "_dns.resolver.arpa", so it's not at all clear why they
can't place AAAA records there as well, and this is all that TargetName means
in SVCB.  It also doesn't quite address the question of whether TargetName can
be "resolver.arpa", which would be more natural in SVCB.

Personally, I don't see the need for this restriction, which I think is a
holdover from previous drafts that imposed different semantics on TargetName. 
Placing AAAA records on "resolver.arpa" seems like a fine way for the resolver
to tell the client what it thinks its own public ingress IPs are.

If the authors think this should be prohibited, perhaps they can clarify the
rationale (e.g. 'To enable reliable identification of the Designated Resolver
for debugging and future extensions, TargetName SHOULD NOT indicate a SUDN.').

Relatedly, if "resolver.arpa" is not the expected TargetName, this section
should probably have a sentence noting that the "speculative A and/or AAAA
queries" recommended in Section 5 of draft-ietf-svcb-https are not applicable
here.

> If the recursive resolver that receives this query has no Designated
Resolvers, it SHOULD return NODATA for queries to the "resolver.arpa" SUDN.

I think this should say something like 'for queries to the "resolver.arpa"
zone'.  Note that NXDOMAIN means there is nothing underneath (RFC 8020), but
NODATA does not.

Section 4.1:

> A client MUST NOT re-use a designation ...

This recommendation is obscure.  Perhaps an example of a plausible violation
would help.

Section 4.1.1:

> having no delay in establishing an encrypted DNS connection

This seems a bit optimistic.  For DoQ with resumption "no delay" might be
possible, but for DoT a delay of at least 1 RTT is required.  Perhaps 'no delay
before establishing' would be more precise.

Section 4.2:

Perhaps cite RFC5280 for subjectAltName?

> Additionally, the client SHOULD suppress any further queries for Designated
Resolvers using this Unencrypted Resolver for the length of time indicated by
the SVCB record's Time to Live (TTL).

Why? I agree that the client shouldn't retry in a fast loop, but consider a
passive attacker who can occasionally become active via some difficult
mechanism (e.g. a Kaminsky-type attack).  If this attacker can inject a single
DDR record with a very long TTL, it can disable DDR passively monitor the DNS
queries for a long time.

It would be better for the client to retry SVCB resolution more frequently than
the (potentially attacker-controlled) TTL, to force an active attacker to be
more persistent.

> If resolving the name of a Designated Resolver from an SVCB record

I think this should say 'TargetName'.  (This is not "the name of a Designated
Resolver" under the standard interpretation of TargetName.)

> yields an IP address that was not presented in the Additional Answers section
or ipv4hint or ipv6hint fields of the original SVCB query, the connection made
to that IP address MUST pass the same TLS certificate checks before being
allowed to replace a previously known and validated IP address for the same
Designated Resolver name.

I find this confusing.  There are potentially four classes of IP addresses
involved here: A. The IP of the unencrypted resolver, provided out of band
(e.g. via DHCP) B. ipv4hint/ipv6hint C. Additional Section records for
TargetName D. Answer Section records for TargetName.

This text makes it sound like A-C or B+C have something in common that D does
not, but that is not true.  In fact, only A is treated specially, and B-D are
completely equivalent for DDR.  Maybe this text is trying to warn against some
implementation mistake?  If so, better to make that explicit.

Section 4.3:

> the SVCB record for "resolver.arpa"

I find this confusing, since the SVCB record is at "_dns.resolver.arpa", not
"resolver.arpa". (Really, it's the SVCB record for "dns://resolver.arpa", but
"dns:" URIs are controversial at best.)

Section 5:

As discussed earlier, I think it might be better to shrink or eliminate this
section.

Section 6.1

> A DNS forwarder SHOULD NOT forward queries for "resolver.arpa" upstream.

'"resolver.arpa" and subdomains'?  Are we sure this advice applies to all
future uses of the "resolver.arpa" zone?

> ... or that the operator expects clients of the unencrypted resolver to use
the SVCB information opportunistically.

This seems to contradict Section 4.3 (Opportunistic Discovery), which prohibits
the use of the SVCB information in this configuration due to differing IPs of
the local and upstream resolvers.  Perhaps this should match Section 4.1's
language about "implementation-specific policy" and "future mechanism", e.g.
"... expects clients to validate the connection via some future mechanism".

Section 7:

Typo: "provides provides"

> once a client discovers a compatible Designated Resolver, it SHOULD NOT use
unencrypted DNS until the SVCB record expires, unless verification of the
resolver fails.

I think the caveat needs to be removed.  Otherwise, an attacker on the
transport path can cause verification to fail, in order to trigger unencrypted
fallback.

> DoH resolvers that allow discovery using DNS SVCB answers over unencrypted
DNS MUST NOT provide differentiated behavior based on the HTTP path alone ...

I think the language on this in draft-ietf-add-svcb-dns is more precise and
complete.  For example, this description does not cover alternate port numbers,
and the use of "separate resolver IP addresses" is unclear (it means "separate
_unencrypted_ resolver IP addresses").  The language here should probably
match, or be removed and rely on the other draft.