-13 looks betterPerhaps a comment for the AD. You have added a reference for MD5 which I think necessary but as Informative which I find debatable given its use to generate a 32-bit identifier from a IPv6 address, ie its use is going to go up and up, not fade away. I am aware that the RFC is Informational not Standards Track so a Normative Reference would be a downref which needs calling out in a Last Call which means a new Last Call. (Given the number of changes made since -10, perhaps that is not such a left field idea:-) The reference in RFC5905 is Normative. Like I say, one for the AD.
Hash algorilthms - a minefield. You have lots of choice but that increases the probability of incompatability between client and server. One is good, obsolescent and now recommended is good, but a choice of eight without even including SHA3? I note too that the Netconf WG regard SHA1 as obsolete.
Access mode I think deserves more text, a sub-section in s.5, not just a mention. Authors vary as to how much text there is in a YANG I-D but I think that the better ones have more rather than less, bridging the gap from protocol specification to YANG module, and, as you have seen, I went looking in the RFC for an explanation, which was a waste of time:-) And I really do not understand the text.
'can be performed on the local NTP service'I find ambiguous. Does it mean that the entity in which this YANG module is will send such requests? Or does it mean that the entity will respond to such requests? And how does that vary with server, client, peer? I cannot tell from the I-D.
identity broadcast-client could do with mode 6 to bring it in line with other identity
s.8.5You slip in a version 3 which I guarantee will attract the attention of an AD!. You mentioned earlier that you intended the module to work with V3 which I did not pick up from reading the I-D. If that is your intention, then I think that this needs calling out, perhaps in the Introduction as well as the text preceding s.8.5
Tom Petch On 17/02/2021 12:32, tom petch wrote:
On 17/02/2021 11:34, Dhruv Dhody wrote: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-13Indeed and I have downloaded it. I will look at Friday or perhaps Thursday. I am conscious that the Last Call period has expired and that sometimes ADs press the 'Go' button while I am still thinking:-( Ben did provide a link to text he had crafted about non-cryptographic use of MD5 which I rather liked - up to you whether or not you want it include it. Tom PetchAs 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/Updateds.7 This has lost its two character margin, needs insettingNot 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, RFC1321I 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 placesUpdated.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 doctorI 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 uintint8 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 spectrumUpdated 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 RFCidentity 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.UpdatedThen 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 thinkUpdated.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 PetchThanks! 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-12That 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 PetchMeanwhile, 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 NMDAIts 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 websiteNo longer using ianach in this update.1.5. Refrences in the Model /Refrences/References/ /refrenced in /referenced in /Updated2. 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.Updated4. Relationship with RFC 7317 /supports per-interface configurations / support per-interface configuration/Updated5. Access Rules /refer access-mode) and attach different acl-rule/ see access-mode) and attach a different acl-rule/Updated6. Key Management /32-bits unsigned /32-bit unsigned/ /this YANG modules/this YANG module/UpdatedNTP association (for example unicast-server), /specefied/specified/Updated7. 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 websiteNot 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 hereThere 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 RFC5905Section 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 discontinuitiesUpdated as suggested.leaf access-mode { /defination/definition/Updatedleaf 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'Updatedleaf address { type inet:host; this includes the domain name, which I see no mention of in the RFCChanged to inet:ip-addresslist 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.1leaf reach { type uint8; is this the 8-bit p.reach shift register? reference needed (again:-) leaf unreach { dittoAdded.leaf poll { type uint8; units "seconds"; description "The polling interval for current association"; is there a useful default? 2s appears in the RFC in placesNo default in this case.leaf offset { as above, the example values would be clearer with unitsUpdatedleaf 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; dittoThanks, updated as per your suggestion.leaf ttl { type uint8; description "Specifies the time to live (TTL) TTL does not exist in IPv6This 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/Updatedleaf ttl { type uint8; description "Specifies the maximum time to live (TTL) for TTL does not exist in IPv6As above.uses common-attributes { /attributes like/attributes such as/Updatedleaf beacon { what are the units and is there a default? Is there a maximum of 15? As ever, a reference could tell me.Added reference8. Usage Example lots of examples but none for IPv6 or JSONAdded 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-usedOops, updated here and all other instance!<refid>20.1.1.1</refid> as above, is Microsoft ok with this?Updated to 203.0.113.18.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.1Updated<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 aboveUpdated 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 StandardEmail 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 PetchEmail 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
