On Mon, Dec 23, 2024 at 1:58 PM Patrick Mevzek via Datatracker <noreply@xxxxxxxx> wrote:
Reviewer: Patrick Mevzek
Review result: Ready with Nits
Thanks for the review ...
However, I do find in §3 this to be a little weak:
" While it could support NSEC3 too, there is no benefit in introducing the
additional complexity associated with it." Because Motivation in §1 clearly
explains that this new scheme allows fewer number of NSEC records... and
mentions 3 of them are needed in NSEC3 case, so the benefit is (should be) even
better here for NSEC3 than NSEC. So I would suggest either giving more details
here on what would be additional complexity for NSEC3, or just removing the
whole line and stating unambiguously that the document applies only to zones
using NSEC.
Please see the separate thread I started on this point, since there are now
already implementations that use NSEC3 and we may need to make some
updates to the draft to acknowledge that and/or describe the small set of
changes needed to support NSEC3.
I do have some remarks on §3.4:
It defines a new extended code as
"Invalid Query Type" but it seems to be defined to be used in case of a
resolver asking for "NXNAME" resource record type. Wouldn't it better to be
clearer that it applies to the resource record type, as otherwise I fear "query
type" could be understood at what RFC 1035 calls OpCode aka Query vs Inverse
Query vs Notify vs Update, etc.? Specifically since RFC6895 has: " Data TYPEs
are the means of storing data. QTYPES can only be used in
queries. Meta-TYPEs designate transient data"
And NXNAME is clearly defined as being of Meta type, not QType.
The intention is that the Invalid Query Type EDE has broader applicability
than just this draft.
"Note that this EDE code is generally applicable to any RR type that should not appear in DNS queries."
So, we could imagine it being used with other meta types too (and that's what I hope will happen).
Also, RFC8914 already defines code 20 as:
"The requested operation or query is not supported."
Isn't that good enough, or is really a new code needed (30)?
For a generic code (since document says "Note that this EDE code is generally
applicable to any RR type that should not appear in DNS queries.") maybe
existing 20 could be enough, or otherwise 30 should be repurposed specifically
about and only NXNAME queries.
The EDE space is large and it is better to have more surgical codes
that precisely specify the issue. The "requested operation or query" is
broader than it should be. Was the problem with the query type or
some other "operation"?
Not sure myself, so only in case you find it useful:
- about caching, RFC2308 has "Negative responses without SOA records SHOULD NOT
be cached", which makes sense when not having the SOA Negative TTL field to
use, but here with NSEC(NXNAME), there is a TTL on this record, should it be
used (or not) as caching hint?
We don't call it out specifically, but the NODATA responses always have a SOA
record in the Authority section too. And we expect the NSEC record to have same
TTL as the SOA min (per RFC 4034, Section 4).
Maybe add a sentence for that, if relevant. -
because of §4.1., shouldn't also RFC6891 be marked as updated by this document,
specifically since it defines Z (which is mutated here) as "Set to zero by
senders and ignored by receivers, **unless modified
in a subsequent specification.**
"
(emphasis mine)
I don't think an update to 6891 is needed. The phrase you quoted, "unless modified
in a subsequent specification" takes care of that.
Also RFC6891 states the following about the fixed part vs the variable one:
"The fixed part holds some DNS metadata,
and also a small collection of basic extension elements that we
expect to be so popular that it would be a waste of wire space to
encode them as {attribute, value} pairs."
So this new CO flag being in the fixed part, does it pass the "extension
elements we expect to be so popular" criteria? If not, and to resolve the above
(no change in RFC6891), instead of a new CO flag, create a new variable part,
by requesting a new EDNS0 Option Code? (with a length of 0, its mere presence
would mean the same as CO=1)
This is 1-bit of information, so a good match for an EDNS header flag, rather
than wasting more space in an option. Secondarily, from experience we know
that EDNS header flags are more likely to traverse broken middleboxes and
deficient DNS actors than options, so that's another reason we chose that.
I cannot answer the question of whether we expect it to pass a popularity test.
I suspect many in the DNSOP crowd would have already objected to this by now
if it were a concern.
Related, and even if obvious, maybe specify that the flag should be set (value
= 1) to enable the feature, and how it should be in replies and what the
receiver should do with it (discard I guess, but better to specify - or should
0/1 on answer give an hint if server supports the feature, even if not relevant
in that specific answer because it was not an NXDOMAIN/NXNAME case?).
It is the latter (I understand or support the rcode substitution feature). Let me
see if we can make that clearer in the draft.
Nitpicks:
- there is both "type bitmap" and "Type Bitmaps" and "type bitmaps", so both
variations in casing and singular/plural form. Even if not really confusing,
possibly better to align to just one name? RFC4034 has "Type Bit Maps".
Good point. Let's match the phrasing in existing specs by saying "Type Bit Maps"
throughout.
- §3.1
says creating a response with no records in Answer section, and then says to
create a NSEC record. While normally obvious, just specify that it goes in the
Authority section? (specifically since Authority section is mentioned in §3.3)
- in §6.2 for the new paragraph to add, there is a dangling open ( with no ) to
close it.
Ok, yes, we can make that clearer.
Shumon.
-- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
