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