Hi Tom, Thanks for your review. I have posted an updated version of the YANG. https://datatracker.ietf.org/doc/draft-ietf-ntp-yang-data-model/ https://www.ietf.org/rfcdiff?url1=draft-ietf-ntp-yang-data-model-10&url2=draft-ietf-ntp-yang-data-model-12 Please find my consolidated response below - Email 1: ======== On Mon, Feb 8, 2021 at 5:52 PM tom petch <daedulus@xxxxxxxxxxxxx> wrote: > > I have two main problems and a lot of lesser ones with this I-D; given > the number, about 50, I am not optimistic that a single cycle of > revision will see them addressed. > > I see a potential loophole in the security which I will post separately > since the audience is likely to be different. > > References are missing or not specific enough so when I try to compare > values in the I-D with those of the protocol, either I cannot find them > or they seem to be different. Giving values to enumerations is unusual > in YANG, since NETCONF does not transmit them, and their presence > suggests that they are protocol values, in which case I want to see what > the protocol says. A reference to a 110 page I-D, with two updating > I-D, is inadequate IMO - section numbers are needed in every case. > I have added section numbers in most of the cases. I have changed enums to identity. > Introduction > should mention support, or lack thereof, for NMDA > Its present in Section 1.1 already, I have added keyword NMDA as well now. > 1.4. Prefixes in Data Node Names > | ianach | iana-crypt-hash | [RFC7317] | > the reference is wrong; this is an IANA- maintained module so the > reference must be to the IANA website > No longer using ianach in this update. > 1.5. Refrences in the Model > /Refrences/References/ > /refrenced in /referenced in / > Updated > 2. NTP data model > > I do not see the value of a condensed model followed immediately by a > full model. Perhaps the full model should be an Appendix although at > less than three pages, this is quite small and would be ok on its own > IMHO. > Updated > 4. Relationship with RFC 7317 > /supports per-interface configurations / > support per-interface configuration/ > Updated > 5. Access Rules > /refer access-mode) and attach different acl-rule/ > see access-mode) and attach a different acl-rule/ > Updated > 6. Key Management > /32-bits unsigned /32-bit unsigned/ > > /this YANG modules/this YANG module/ > Updated > NTP association (for example unicast-server), > /specefied/specified/ > Updated > 7. NTP YANG Module > > import iana-crypt-hash { > reference "RFC 7317: A YANG Data Model for System > Management"; > wrong reference - this module is IANA-maintained so the reference must > be to the IANA website > Not using iana-crypt-hash anymore! > contact > WG List: <mailto: ntpwg@xxxxxxxxxxxxx > this is not the address I see on the datatracker > > the I-D has five editors but there are only two here > There are 2 co-authors marked as editors. RFC 8407 says the document author or editor contact information needs to be present. > typedef access-mode { > I cannot find this in RFC5905 > Section 9.2 and Appendix A.5.4. I have added section number in reference. > typedef association-mode { > this I can find but it ranges from 0 to 7 whereas the I-D has 0 to 4 - is > this intended? > Changed to identity. > typedef ntp-sync-state { > this I cannot find; a search for 'spike' yields a value of 2 in the > RFC, 5 here - is this intended? > Instead of enum and values, changed to identity. > effect in XXX seconds."; > for what value of XXX? > Removed. > leaf packet-sent { > leaf packet-received { > leaf packet-dropped { > discontinuities in the value of sysUpTime."; > those who have been involved with network management for ten years or > less will likely not recognise this object. You could add a reference > but I suggest you replace it with a YANG-based approach; see for example > how draft-ietf-ospf-yang handles discontinuities > Updated as suggested. > leaf access-mode { > /defination/definition/ > Updated > leaf clock-refid { > ... reference clock of the peer to > which clock is synchronized."; > > I do not understand this. Presumably this corresponds to > type string { > length "4"; > from the three type union but what object is this? > I created a typedef and cleaned it up. This is as described in Section 7.3 of RFC5905 under Reference ID. > leaf clock-offset { > examples could do with units to make it clear that it is '1.232mS' and > not '1.232s' > Updated > leaf address { > type inet:host; > this includes the domain name, which I see no mention of in the RFC > Changed to inet:ip-address > list associations { > /and isconfigured is required/and isconfigured are required/ > > leaf address { > type inet:host; > as above, the description seems to ignore the option of the domain name > > leaf refid { > same union as for leaf clock-refid, but a completely different > description, neither of which I understand. > Updated (all of the above). > '20.1.1.1' > this address would appear to be assigned to Microsoft, not an > affiliation I see among the authors. Is the company ok with this? > Updated to 203.0.113.1 > leaf reach { > type uint8; > is this the 8-bit p.reach shift register? reference needed (again:-) > > leaf unreach { > ditto > Added. > leaf poll { > type uint8; > units "seconds"; > description > "The polling interval for current association"; > is there a useful default? 2s appears in the RFC in places > No default in this case. > leaf offset { > as above, the example values would be clearer with units > Updated > leaf transmit-time { > type yang:date-and-time; > description > "This is the local time, in timestamp format, > at which the NTP packet departed the peer(T3). > If the peer becomes unreachable the value is set to zero."; > I think, but am not sure, that a yang:date-and-time can never be set to > zero, the syntax does not allow it; the usual approach with YANG is a > union with another type which can indicate a special condition - int, > boolean, etc > > leaf input-time { > type yang:date-and-time; > ditto > Thanks, updated as per your suggestion. > leaf ttl { > type uint8; > description > "Specifies the time to live (TTL) > TTL does not exist in IPv6 > This TTL is not related to IP. See section 3.1 for manycast and updated with reference. > uses common-attributes { > description > /attribute like/attributes such as/ > Updated > leaf ttl { > type uint8; > description > "Specifies the maximum time to live (TTL) for > TTL does not exist in IPv6 > As above. > uses common-attributes { > /attributes like/attributes such as/ > Updated > leaf beacon { > what are the units and is there a default? Is there a maximum of 15? As > ever, a reference could tell me. > Added reference > 8. Usage Example > lots of examples but none for IPv6 or JSON > Added one IPv6. I dont think we need add one for JSON as well. > 8.1 > <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp"> > sys: is a defined prefix and must not be re-used > Oops, updated here and all other instance! > <refid>20.1.1.1</refid> > as above, is Microsoft ok with this? > Updated to 203.0.113.1 > 8.2 > <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp"> > sys: is a defined prefix and must not be re-used > > 8.3 > <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp"> > sys: is a defined prefix and must not be re-used > > 8.4 > <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp"> > sys: is a defined prefix and must not be re-used > > 8.5 > "224.1.1.1" > would appear to be a reserved address. Other RFC used 224.0.1.1 > Updated > <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp"> > and again, twice > > <address>224.1.1.1</address> > as above > > 8.6 > "224.1.1.1" > as above > > <sys:ntp xmlns:sys="urn:ietf:params:xml:ns:yang:ietf-ntp"> > as above, twice > > 8.7 > <ntp xmlns="urn:ietf:params:xml:ns:yang:ietf-ntp"> > as above > > 8.8 > <ntp xmlns="urn:ietf:params:xml:ns:yang:ietf-ntp"> > as above > > <refid>20.1.1.1</refid> > as above > > 8.9 > <ntp xmlns="urn:ietf:params:xml:ns:yang:ietf-ntp"> > as above > Updated in all cases. > 12.2 > [RFC7317] Bierman, A. and M. Bjorklund, "A YANG Data Model > wrong reference in the wrong place > this is an IANA-maintained module and so the reference must be to the > IANA website; and since the module is imported, the reference must be > Normative. > No longer using iana-crypt-hash. > Tom Petch > > > ----- Original Message ----- > From: "The IESG" <iesg-secretary@xxxxxxxx> > To: "IETF-Announce" <ietf-announce@xxxxxxxx> > Cc: <ek.ietf@xxxxxxxxx>; <ntp-chairs@xxxxxxxx>; <ntp@xxxxxxxx>; > <dsibold.ietf@xxxxxxxxx>; <draft-ietf-ntp-yang-data-model@xxxxxxxx> > Sent: Friday, January 29, 2021 10:39 PM > Subject: Last Call: <draft-ietf-ntp-yang-data-model-10.txt> (A YANG Data > Model for NTP) to Proposed Standard > Email 2: ======== On Mon, Feb 8, 2021 at 6:07 PM tom petch <daedulus@xxxxxxxxxxxxx> wrote: > > This is my second response to this Last Call, about a possible security > issue. > > RFC8573 seems clear that MD5 must not be used to effect security for NTP > but this I-D imports iana-crypt-hash which allows MD5 without any > restriction, so is MD5 allowed or not? > > There are features defined which allow the hash in iana-crypt-hash to be > restricted but this I-D does not use them. > > Probably iana-crypt-hash should be updated - I will raise that on the > NETMOD WG list. > No longer using iana-crypt-hash, instead made this change to allow all sort of key formats - OLD: | +--rw key? ianach:crypt-hash NEW: | +--rw key | | +--rw (key-string-style)? | | +--:(keystring) | | | +--rw keystring? string | | +--:(hexadecimal) {hex-key-string}? | | +--rw hexadecimal-string? yang:hex-string END The comment related to use of MD5 still applies. I have not yet added a check for the algorithm. I have updated the description and security consideration to highligh that MD5 is depricated as per RFC 8573. Lets see what the IESG suggests... > The I-D also uses MD5 in a way that would appear not to be security > related, to hash an IPv6 address. > This is as per RFC 5905 - "If using the IPv6 address family, it is the first four octets of the MD5 hash of the IPv6 address." > In passing, this I-D has three references to RFC7317. This is wrong - > the module is IANA-maintained and so the references should be to the > IANA website. > No longer using iana-crypt-hash. > The secdir reviewer might be interested in my thoughts. > > Tom Petch Email 3: ======== On Tue, Feb 9, 2021 at 4:40 PM tom petch <daedulus@xxxxxxxxxxxxx> wrote: > > A separate thought to my previous two. > > Section 4 is very keen that this I-D and the system module which > configures ntp SHOULD NOT coexist but this is not enforced by the YANG. > > It could be. Import the system module and make the presence container > in this I-D dependent on the absence of the presence container in > /system/ntp. > Updated as per your suggestion! ==== Thanks again for your detailed review. It has improved the YANG model significantly! Thanks! Dhruv & Ankit On Sat, Jan 30, 2021 at 4:09 AM The IESG <iesg-secretary@xxxxxxxx> wrote: > > > The IESG has received a request from the Network Time Protocol WG (ntp) to > consider the following document: - 'A YANG Data Model for NTP' > <draft-ietf-ntp-yang-data-model-10.txt> as Proposed Standard > > The IESG plans to make a decision in the next few weeks, and solicits final > comments on this action. Please send substantive comments to the > last-call@xxxxxxxx mailing lists by 2021-02-12. Exceptionally, comments may > be sent to iesg@xxxxxxxx instead. In either case, please retain the beginning > of the Subject line to allow automated sorting. > > Abstract > > > This document defines a YANG data model for Network Time Protocol > (NTP) implementations. The data model includes configuration data > and state data. > > > > > > The file can be obtained via > https://datatracker.ietf.org/doc/draft-ietf-ntp-yang-data-model/ > > > > No IPR declarations have been submitted directly on this I-D. > > > > > -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
