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

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