Re: [DNSOP] Updated KSK Sentinel document

[Warren Kumari <[e-mail]>]

On Wed, Feb 14, 2018 at 5:05 PM, 神明達哉 <[e-mail]> wrote:
> At Mon, 12 Feb 2018 15:28:50 -0500,
> Warren Kumari <[e-mail]> wrote:
>
>> Anyway, we've finally posted an updated version -
>> https://datatracker.ietf.org/doc/draft-ietf-dnsop-kskroll-sentinel/
>
> I've read draft-ietf-dnsop-kskroll-sentinel-01 (this is my first
> careful read of this draft) and found it generally well-written.

Thank you!

>
> I have some comments on 01.  These are basically editorial.

Even more thank you!

>
> - General
>   I think it's worth pointing out that SERVFAIL can be returned for
>   various other reasons and the detection mechanism relying on it
>   isn't reliable.  This is probably okay given the purpose of this
>   detection, but I think it's better to note it explicitly.
>

Good point. I had some trickiness writing this - how is "The
techniques describes in this document rely on (DNSSEC validating)
resolvers responding with SERVFAIL (RCODE 2) to valid answers. Note
that a slew of other issues can also cause SERVFAIL responses, so
false positive or negative results may sometimes occur." ?

> - Section 1.1
>    Address Record: Throughout this document we use the term Address
>    Record (AR) to mean an A or AAAA record.  [...]
>
>   I actually didn't find this term other than here and Section 9
>   (change log).
>

Ah, thanks, removed.

> - Section 1.1 and throughout the draft
>
>    AAAA records and the IPv6 documentation prefix (2001:DB8::/32) as
>
>   I'd suggest using lower-case letters to show IPv6 addresses and
>   prefixes, following the recommendation of RFC5952.  It's not a big
>   deal but it would be better if we can be more consistent on the
>   choice of letter case in RFCs.

Good point. Fixed.


>
> - Section 2
>
>    Charlie's resolvers are validating, but they have not been upgraded
>    [...]
>    resolve it).  He is able to fetch both of the other resources - from
>    this he knows (see the logic below) that he is using legacy, non-
>    validating resolvers.  [...]
>
>   Do you mean "he is using legacy validating resolvers"?  If it's not
>   a typo, the behind logic isn't obvious to me and it would help if
>   you explain it in more detail (instead of just referring to
>   'below').

Wow, fixed! Yup, Charlie is using a legacy, *validating* resolver.
Thanks for catching this!.


>
> - Section 3
>
>    [...] Note that
>    the test is "Is there a key with this KeyID in the resolver's current
>    trust store for the DNS root", not "Is there any key with this KeyID
>    in the trust store", nor "Was a key with this KeyID used to validate
>    this query?".
>
>   Unless I miss something, the draft doesn't clarify its use is
>   limited to root KSK until the next paragraph of this, so I suspect
>   this statement will confuse a fresh reader who reads this doc from
>   top to bottom without a particular knowledge of background
>   discussion.  I'd suggest noting it somewhere before this part,
>   perhaps in the introduction (and maybe also in abstract).

Thanks, done (it was actually mentioned much earlier, but is was easy to miss).

>
> - Section 3
>
>    [...]  Note
>    that the <tag-index> is specified in the DNS label using hexadecimal
>    notation.
>
>   I suggest clarifying whether '<tag-index>' contains leading 0s
>   somewhere in the document (this may not be the best place to do so,
>   but this is the first reference to 'tag-index' that made the
>   question occur to me).

Hmmm... Good point. I personally actually preferred having these as
"decimal" keys.
RFC1034, Sec 5.3: The DS RR Presentation Format sayeth:
   " The Key Tag field MUST be represented as an unsigned decimal
integer.", things like dig +multiline DNSKEY . shows it as decimal,
etc.
My initial demos (www.ksk-test.net) also all used decimal. Petr
recently pointed out to me that I'd messed up, and so I converted my
demo to use hex, as the draft states.
What does the WG prefer? Is the new KSK called "20326" or it is "4f66"?

Note that Knot Resolver 2.1  already implements (thank you!) this, and as hex.
But, yes, either way, we need to note if it is padded.
[TODO(WK): Raise the hex vs Dec issue]

>
> - Section 3
>
>    If the resolver has not placed a
>    Root Zone Key Signing Key with tag index value matching the value
>    specified in the query into the local resolver's store of trusted
>    keys, then the resolver should return a response indicating that the
>    response contains authenticated data according to section 5.8 of
>    [RFC6840].
>
>   Should we perhaps define "store of trusted keys" more precisely?
>   For example, if a key is in the "AddPend" status (per RFC5011) for
>   the resolver, should it be considered in the "store of trusted
>   keys"?

Nope. Well, yes, we need to better define it, but no, a key in AddPend
is not considered.
We explicitly mean "keys which are active and can be used for
validating entries in the root zone."

We have: "In particular, this response mechanism can be used to
determine whether a certain Root Zone KSK is ready to be used as a
trusted key within the context of a key roll by this resolver."
and I added:
"An active key is one which could currently be used for validation (ie
not in AddPend or Revoked state (RFC5011)).

Sounds reasonable?

>
> - Section 3
>
>    This mechanism is to be applied only by resolvers that are performing
>    DNSSEC validation, and applies only to RRset responses to an A or
>    AAAA query (Query Type value 1 or 28) where the resolver has
>    authenticated the response RRset according to the DNSSEC validation
>    process and where the query name contains either of the labels
>    described in this section as its left-most label.
>
>   Is the RRset in 'response RRset' intentionally added (or in other
>   words can't it just be 'response')?  Perhaps is the intent to
>   exclude negative responses?  In any case I think the intent should
>   be clearer here.  And if a mere 'response' suffices it's probably
>   less confusing to just say so.

Good point. I removed responses:
"This mechanism is to be applied only by resolvers that are performing
DNSSEC validation, and applies only to responses to an A or AAAA
query..."
There are some other places where RRSet is still needed, so I left
some of them in.


>
>   BTW, you might want to say 'a query for an Address Record' or 'an
>   Address Record query' instead of 'an A or AAAA query' (I guess
>   that's the intent of the introduction of this term.  See also my
>   first comment on Section 1.1 above).

Actually, there are small enough number of occurrences that explicitly
listing them (and not creating a new term) seemed better, so (see
ablove) I removed the "Address Record" term and left in the "A or
AAAA" stuff - please let me know if you agree.

>
> - Section 4
>
>    o  Vleg: A DNSSEC-Validating resolver that does not implement this
>       mechanism will respond with an A or AAAA RRSET response for
>       "kskroll-sentinel-is-ta", an A record response for "kskroll-
>       sentinel-not-ta" and SERVFAIL for the invalid name.
>
>   + s/RRSET/RRset/?
>   + 'an A record response' should be 'an A or AAAA record response' or
>     it's revised using the "Address Record" term.  Same comment
>     applies to other part of this section including the table.

Thank you, good catch. I s/RRSET/RRset/ and added "or AAAA".


Thank you very much for the comments and feedback - I've just pushed a
new version to GitHub (
https://github.com/APNIC-Labs/draft-kskroll-sentinel ) and will post a
new draft on a few minutes.

W


>
> --
> JINMEI, Tatuya



-- 
I don't think the execution is relevant when it was obviously a bad
idea in the first place.
This is like putting rabid weasels in your pants, and later expressing
regret at having chosen those particular rabid weasels and that pair
of pants.
   ---maf