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

tom petch <daedulus@xxxxxxxxxxxxx> · Fri, 12 Feb 2021 12:01:22 +0000

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 <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 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: ntpwg@xxxxxxxxxxxxx
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" <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 Standard

Email 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 Petch

Email 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