Thanks for the review! ]On Jul 31, 2024, at 04:30, Petr Špaček via Datatracker <noreply@xxxxxxxx> wrote: > Reviewer: Petr Špaček > Review result: On the Right Track > > I was assigned as the dnsdir reviewer for draft-ietf-dnsop-rfc7958bis. > > For more information about the DNS Directorate, please see > https://wiki.ietf.org/en/group/dnsdir > > Summary: On the Right Track > > I've read the document with fresh eyes and implementation experience based on > the previous version of the format: - Removed parts are okay from my > perspective. - The major problem I can see is under-specification related to > the XML format. > > I remember when I was implementing the previous version I just crossed my > fingers and hoped for the best, which worked because it the code was only ever > run on root zone XML. But if we are redoing the document I think we should fix > it OR declare the format to be root-only. The new PublicKey element also brings > new nasty corner cases which need clarification, see below. > > Details: > >> 2.2. XML Semantics >> The Zone element in the TrustAnchor element states to which DNS zone this >> container applies. The root zone is indicated by a single period (.) character >> without any quotation marks. >> > I understand it does not matter for the root zone, but if we want to make this > format generic I think it needs clarification for non-root Zone names. > Essentially it needs text which allows implementer to just take the string > produced by XML parser and pass it on into a normal DNS parser with confidence. > I suggest addition like: - Zone element contains DNS name in presentation > format [RFC reference] - Escaping applies the same way as in zone file format > [RFC reference] - Names MUST be absolute (?) > > An obvious alternative is to remove the element completely and declare the > format to be root-only. > > Or if backwards compatibility is required then we can declare that the value > MUST be equal to . > > BUT If backwards compatibility is a requirement it should be mentioned because > I can't see it mentioned anywhere in the current text. I agree that the current text is not clear on what the format of the name is. Short of ripping out all discussion of non-root use of this format, we can add "in presentation format as specified in RFC 1035, including the trailing dot". That implies the escaping rules, so there is no need to go to that detail. > >> Note that the validUntil attribute of the KeyDigest element is optional. If >> the relying party is using a trust anchor that has a KeyDigest element that >> does not have a validUntil attribute, it can change to a trust anchor with a >> KeyDigest element that does have a validUntil attribute, as long as that trust >> anchor's validUntil attribute is in the future and the DNSKEY elements of the >> KeyDigest are the same as the previous trust anchor. > > What are "DNSKEY elements" this paragraph refers to? I think I can guess, but > given optional presence of PublicKey field I think we should rephrase to make > it absolutely clear what fields MUST match. E.g. something like: "MUST match > values for KeyTag, Algorithm, DigestType, Digest" (ignoring id, both > timestamps, and PublicKey which is optional and there are no rules for its > presence). Good catch, will add words. > >> The KeyTag element in the KeyDigest element contains the key tag for the >> DNSKEY record represented in this KeyDigest. > > As courtesy for implementers I recommend a warning about possibility of > duplicate KeyTags. Something like "MUST NOT rely on unique KeyTag values". > (Again, this might not be really relevant for the Root zone but ... are we > still pretending the format is generic?) Why should such a warning be necessary? The record has more than the key tag: it also has the hash and/or the public key. I think such a warning would tend to confuse more than help. >> 2.4. Converting from XML to DNSKEY Records >> The published trust anchor does not provide values for DNSKEY protocol or >> flags. For the purpose of this mechanism, clients can assume valid trust >> anchors will be have the ZONE and SEP flag bits set to 1. > > I think it is extremely bad idea to ignore fields, especially Flags. There are > various proposals for new DNSKEY flag usage in the DD WG. > > Even if we say that DD WG will go down in flames before it produces anything I > think it's _extremely_ bad idea to omit pieces of information and assume that > client can reliably fill in missing pieces with constants. I would say that the > missing DNSKEY fields really really must be represented explicitly. (E.g. I > don't want to go down the rabbit hole of all REVOKE flag corner cases.) I don't understand how the quoted text suggests that users of the zone file ignore fields or flags. Can you suggest text to fix your concern? > On high level I also find confusing that the new element is optional - that > makes it unreliable for consumers because there are no rules for when it might > or might not be present. It is optional because some signing mechanisms don't automatically generate DNSKEY values. For example, the current trust anchor file only has the DS of KSK-2024 because IANA needs to make a software change to get the DNSKEY (which it will do in a few months). Other HSMs might be similar. A DS has always been sufficient; a DNSKEY would be an upgrade, but is not necessary. > Also there are no rules for what to do when reconstructed DS and DNSKEY don't > match - which can totally happen given the half-representation we have in the > current version. Fully agree; we will add words to say in such a case the specific key should not be trusted. > Is there implementation experience with the new format? What was the > implementer feedback? We have heard informally that some implementers have added the new features with no problems, but they obviously can't test it until there is a new trust anchor file from IANA, and that's waiting on the standard to be published. > TL;DR there are issues which can be addressed with smallish amendments. Thanks! Please see above for requests for additional wording in places where I don't understand the concern. The other bits will be added after the IETF Last Call is finished. --Paul Hoffman -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
