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

[Dhruv Dhody <[e-mail]>]

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

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:
>
>
> The IESG has received a request from the Network Time Protocol WG (ntp) to
> consider the following document: - 'A YANG Data Model for NTP'
>   <draft-ietf-ntp-yang-data-model-10.txt> as Proposed Standard
>
> The IESG plans to make a decision in the next few weeks, and solicits final
> comments on this action. Please send substantive comments to the
> [e-mail] mailing lists by 2021-02-12. Exceptionally, comments may
> be sent to [e-mail] instead. In either case, please retain the beginning
> of the Subject line to allow automated sorting.
>
> Abstract
>
>
>    This document defines a YANG data model for Network Time Protocol
>    (NTP) implementations.  The data model includes configuration data
>    and state data.
>
>
>
>
>
> The file can be obtained via
> https://datatracker.ietf.org/doc/draft-ietf-ntp-yang-data-model/
>
>
>
> No IPR declarations have been submitted directly on this I-D.
>
>
>
>
>