Re: [Ntp] [Last-Call] Last Call: <draft-ietf-ntp-yang-data-model-10.txt> (A YANG Data Model for NTP) to Proposed Standard

[tom petch <[e-mail]>]

-13 looks better

Perhaps 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.5
You 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-13
>
> Indeed 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 Petch
>
>
>>
>> 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 <[e-mail]> 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 <[e-mail]>
>>>>> 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: [e-mail]
>>>>>> 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" <[e-mail]>
>>>>>> To: "IETF-Announce" <[e-mail]>
>>>>>> Cc: <[e-mail]>; <[e-mail]>; <[e-mail]>;
>>>>>> <[e-mail]>; <[e-mail]>
>>>>>> 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 <[e-mail]>
>>>>> 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 <[e-mail]>
>>>>> 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 <[e-mail]>
>>>>> wrote:
>>>>>>
>>>>>
>> .
>>