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]>]

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:
>>>>>
>>>>
> .
>