Hi Tom, We have posted another update. Updated: https://datatracker.ietf.org/doc/draft-ietf-ntp-yang-data-model/ Diff: https://www.ietf.org/rfcdiff?url2=draft-ietf-ntp-yang-data-model-13 As suggested by Ben, added a YANG feature for MD5. Further details - > s.6 > /For this reason, this YANG module allow/ > For this reason, this YANG module allows/ > Updated > s.7 > This has lost its two character margin, needs insetting > Not sure where. I did run "pyang -f yang --keep-comments --yang-line-length 69" > typedef refid I like, much clearer, but perhaps could do with a > reference for MD5, RFC1321 > I added the reference around the new MD5 feature (as suggested by Ben) and not at refid. > /Discountinuities in the value/Discontinuities in the value/ > in three places > Updated. > when 'false() = boolean(/sys:system/sys:ntp)' { > I am sure that this is valid but is not something I am used to seeing. > An aspect of YANG that I have never understood is when you have to spell > things out and when they are implicit and I think that all you need here is > when not <presence container> > but would want to verify that with a YANG doctor > I have used when presence before but not sure what is the best way for "when not presence" and didn't see it before :) > The definition of each possible values:/ > The definition of each possible value:/ > Updated. > leaf poll { > type uint8; > This seems wrong to me. Looking at s.7.3 > Poll: 8-bit signed integer representing the maximum interval between > successive messages, in log2 seconds. Suggested default limits for > minimum and maximum poll intervals are 6 and 10, respectively. > ie signed integer and not uint > int8 is better, updated. > TTL good to have the reference - I was barking up the wrong tree > > I note > #define TTLMAX 8 /* max ttl manycast */ > I assume that this is a matter of choice - I see 255 in examples which > is at the other end of the spectrum > Updated example. > leaf beacon { > type uint8; > I cannot find a definition of this in the RFC (which I think a defect in > the RFC). Reverse engineering the code I see beacon compared to > unreach, which is defined, as > unreach: integer representing the number of seconds the server has > been unreachable > which suggests that beacon is a number of seconds, and not a log base 2 > value or a counter; is this right? (The RFC should say IMHO) > Yes, it is not log2. I have added more text in the description. > s.8 > /for illustration purporses./for illustration purposes./ > Updated. > > I have run out of time but have made a second pass. Some aspects still > confuse me, where I cannot be sure of the correlation between YANG and > RFC, notable time and mode; I want the words to be identical and they > are not! > > identity access-mode { > I do not understand. The RFC has protocol, association and packet modes > but not access mode. This is perhaps section 3, but I am uncertain, the > terminology is not quite the same, which is perhaps ok for an NTP > expert, which I am not, but not in a technical specification IMHO. > The access-mode is related to the ACL. It is not related to association and packet modes. The RFC does not go into detail but mentions its use in Section 9.2. > Thus you derive > identity query-access-mode { > but searching the RFC only yields a match in Security. I want the terms > to be identical! > This is not mentioned, but the granularity comes from the existing ACL configurations with NTP. > In contrast, association mode is in the RFC and so I can see > identity broadcast-client { > which lacks a value for mode - perhaps 6. But I would still like a > reference to section 3 for these. > I have made an update and aligned it as per the RFC > > identity ntp-sync-state { > is again troublesome. The RFC has > #define NSET 0 /* clock never set */ > #define FSET 1 /* frequency set from file */ > #define SPIK 2 /* spike detected */ > #define FREQ 3 /* frequency mode */ > #define SYNC 4 /* clock synchronized */ > whereas the I-D has > identity clock-not-set { > identity freq-set-by-cfg { > identity clock-set { > identity freq-not-determined { > identity clock-synchronized { > identity spike { > which is close but I want them to be identical or a note saying how they > are different. > Updated > > Then what is the difference between this and > identity clock-state { > which has > identity synchronized { > identity unsynchronized { > Why have both? > I have added the description to say one is a higher-level state and another a granular one. > Other thoughts > > I wonder about the role of > identity aes-cmac { > base key-chain:crypto-algorithm; > What does it add that a direct reference to AES-CMAC does not have? Why > import key-chain. I am not saying it has no value but am unclear of > that value. And > leaf algorithm { > could do with a reference for AES-CMAC. > We are no longer using key-chain to make sure we could use the YANG feature for MD5. > Some authors include the prefix on the module in the list of prefix; > seems logical. > > typedef refid > MD5 could do with a reference. > > leaf clock-stratum { > What is syspeer? I do not see it in the RFC:-) > Updated. > leaf clock-precision { > should be signed not unsigned I think > Updated. > leaf poll > is indeed poll in section 7.3 but I wonder why 13.1 uses hpoll? > Probably a comment on the RFC and not on the I-D. > Hal provided the details. > With ones from my previous post, below, that is it! > > Tom Petch > Thanks! Dhruv & Ankit On Mon, Feb 15, 2021 at 5:25 PM tom petch <daedulus@xxxxxxxxxxxxx> wrote: > > On 12/02/2021 12:01, tom petch wrote: > > On 11/02/2021 09:39, Dhruv Dhody wrote: > >> 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 > >> > > > > That was quick! I have done a first pass but have yet to chase down the > > references and one at least looks odd to me. I expect to complete the > > process on Monday am GMT. > > Ian > > I have run out of time but have made a second pass. Some aspects still > confuse me, where I cannot be sure of the correlation between YANG and > RFC, notable time and mode; I want the words to be identical and they > are not! > > identity access-mode { > I do not understand. The RFC has protocol, association and packet modes > but not access mode. This is perhaps section 3, but I am uncertain, the > terminology is not quite the same, which is perhaps ok for an NTP > expert, which I am not, but not in a technical specification IMHO. > > Thus you derive > identity query-access-mode { > but searching the RFC only yields a match in Security. I want the terms > to be identical! > > In contrast, association mode is in the RFC and so I can see > identity broadcast-client { > which lacks a value for mode - perhaps 6. But I would still like a > reference to section 3 for these. > > > identity ntp-sync-state { > is again troublesome. The RFC has > #define NSET 0 /* clock never set */ > #define FSET 1 /* frequency set from file */ > #define SPIK 2 /* spike detected */ > #define FREQ 3 /* frequency mode */ > #define SYNC 4 /* clock synchronized */ > whereas the I-D has > identity clock-not-set { > identity freq-set-by-cfg { > identity clock-set { > identity freq-not-determined { > identity clock-synchronized { > identity spike { > which is close but I want them to be identical or a note saying how they > are different. > > > Then what is the difference between this and > identity clock-state { > which has > identity synchronized { > identity unsynchronized { > Why have both? > > Other thoughts > > I wonder about the role of > identity aes-cmac { > base key-chain:crypto-algorithm; > What does it add that a direct reference to AES-CMAC does not have? Why > import key-chain. I am not saying it has no value but am unclear of > that value. And > leaf algorithm { > could do with a reference for AES-CMAC. > > Some authors include the prefix on the module in the list of prefix; > seems logical. > > typedef refid > MD5 could do with a reference. > > leaf clock-stratum { > What is syspeer? I do not see it in the RFC:-) > > leaf clock-precision { > should be signed not unsigned I think > > leaf poll > is indeed poll in section 7.3 but I wonder why 13.1 uses hpoll? > Probably a comment on the RFC and not on the I-D. > > With ones from my previous post, below, that is it! > > Tom Petch > > > Meanwhile, > > > > s.6 > > /For this reason, this YANG module allow/ > > For this reason, this YANG module allows/ > > > > s.7 > > This has lost its two character margin, needs insetting > > > > typedef refid I like, much clearer, but perhaps could do with a > > reference for MD5, RFC1321 > > > > /Discountinuities in the value/Discontinuities in the value/ > > in three places > > > > when 'false() = boolean(/sys:system/sys:ntp)' { > > I am sure that this is valid but is not something I am used to seeing. > > An aspect of YANG that I have never understood is when you have to spell > > things out and when they are implicit and I think that all you need here is > > when not <presence container> > > but would want to verify that with a YANG doctor > > > > The definition of each possible values:/ > > The definition of each possible value:/ > > > > leaf poll { > > type uint8; > > This seems wrong to me. Looking at s.7.3 > > Poll: 8-bit signed integer representing the maximum interval between > > successive messages, in log2 seconds. Suggested default limits for > > minimum and maximum poll intervals are 6 and 10, respectively. > > ie signed integer and not uint > > > > TTL good to have the reference - I was barking up the wrong tree > > > > I note > > #define TTLMAX 8 /* max ttl manycast */ > > I assume that this is a matter of choice - I see 255 in examples which > > is at the other end of the spectrum > > > > leaf beacon { > > type uint8; > > I cannot find a definition of this in the RFC (which I think a defect in > > the RFC). Reverse engineering the code I see beacon compared to > > unreach, which is defined, as > > unreach: integer representing the number of seconds the server has > > been unreachable > > which suggests that beacon is a number of seconds, and not a log base 2 > > value or a counter; is this right? (The RFC should say IMHO) > > > > s.8 > > /for illustration purporses./for illustration purposes./ > > > > > > Back on Monday, all being well > > > > I got a bounce message last time from Yi saying on holiday until 22nd > > February. > > > > Tom Petch. > > > > > >> 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: > >>> > >> -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
