Re: [Ntp] Benjamin Kaduk's Discuss on draft-ietf-ntp-yang-data-model-15: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

Hi Dhruv,

It seems that I myself must apologize for a late reply; it feels as if I
have been stumbling from one emergency to the next.  I'm happy to say,
though, that I've cleared my Discuss ballot.

I did have to fight a bit with indentation changes in the YANG module to
get a useful rfcdiff, but what I finally ended up with was quite helpful.

A few notes inline...

On Thu, Dec 09, 2021 at 10:28:00PM +0530, Dhruv Dhody wrote:
> Hi Ben,
> 
> Firstly let me apologize for this very late reply. Secondly thanks for your
> review. I was finally able to make all the changes.
> 
> On Thu, Jul 1, 2021 at 1:53 PM Benjamin Kaduk via Datatracker <
> [e-mail]> wrote:
> 
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-ntp-yang-data-model-15: Discuss
> >
> > When responding, please keep the subject line intact and reply to all
> > email addresses included in the To and CC lines. (Feel free to cut this
> > introductory paragraph, however.)
> >
> >
> > Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> > for more information about DISCUSS and COMMENT positions.
> >
> >
> > The document, along with other ballot positions, can be found here:
> > https://datatracker.ietf.org/doc/draft-ietf-ntp-yang-data-model/
> >
> >
> >
> > ----------------------------------------------------------------------
> > DISCUSS:
> > ----------------------------------------------------------------------
> >
> > Does the 'poll' leaf contain a value measured in seconds as currently
> > stated, or a log2 seconds value (what the "8-bit signed integer" of that
> > name in RFC 5905 holds)?  If the former, it should be a wider type than
> > uint8 in order to be able to represent the full set of values.
> >
> >
> Dhruv: Thanks for spotting this. I created a typedef for this -
> 
>   typedef log2seconds {
>     type int8;
>     description
>       "An 8-bit signed integer that represents signed log2
>        seconds.";
>   }
> 
> And using to all places where log 2 seconds is required.

Thanks for spotting those other places!

> 
> 
> > Let's also take another look at the use of nacm:default-deny-all for
> > sensitive authentication-related nodes.  My understanding is that
> > typically we only block of the actual secret key material in this way
> > and let the associated metadata (key names, algorithms, etc.) be
> > retrieved.  The current module may have default-deny-all in more places
> > than is needed, and we show an example of retrieving key information
> > that ought to have been denied by this ACL.
> >
> >
> Dhruv: Removed it from the grouping authentication-key and now
> nacm:default-deny-all is only used for the grouping key.
> 
> 
> 
> > It seems that the current module does not use RFC 8177 key-chain
> > functionality (despite listing RFC 8177 as a reference).  It seems that
> > best practices for cryptographic configuration would be to use the
> > key-chain functionality, though I may be misunderstanding things.
> >
> >
> >
> Dhruv: AFAIK none of the NTP implementations uses key-chain (they are more
> popular in the Routing). More importantly, key-chain require lifetimes
> which intern require clock synchronization beforehand and thus would create
> a circular dependency!

Ok.  I could perhaps argue that we should provide a way to achieve what the
best practice should be even if it's not achieved in practice, but I
probably could not argue that at a Discuss level.
(That said, RFC 8177 is basically unused as a reference, and should
probably be removed from the document.)

> 
> 
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > I support Francesca's Discuss.
> >
> > We describe a couple of reported values as an "offset"; it would be good
> > to be very clear about what the sign of this value indicates (i.e., if
> > the value is positive, which timestamp is nominally after the other).
> >
> >
> Dhruv: Added.
> 
> 
> 
> > Section 3
> >
> > We list a mapping of YANG nodes and MIB objects, but will the respective
> > data types be trivially relatable?
> >
> >
> Dhruv: The practice was quite common in early YANG models such as RFC 6991,
> 7223, etc to make sure some consistency between existing MIBs and the new
> YANG model is maintained. I see no harm here.
> 
> 
> 
> > Section 6
> >
> > This would be a great place to mention that nacm:default-deny-all is
> > used to prevent retrieval of the actual key information after it is set.
> >
> >
> Dhruv: Added.
> 
> 
> 
> > Section 7
> >
> > I confess that I rather expected some note that NTPv3 (or at least RFC
> > 1305) is classified as obsolete.
> >
> >
> Dhruv: Added.
> 
> 
> 
> > Section 8
> >
> >   identity uc-server {
> >     if-feature "unicast-configuration";
> >     base unicast-configuration-type;
> >     description
> >       "Use client association mode. This device
> >        will not provide synchronization to the
> >        configured NTP server.";
> >
> > A little jarring to have the node name be "server" but the description
> > say "client".  Perhaps some more explanation could be given?
> >
> >
> Dhruv: Updated.
> 
> 
> 
> >   identity clock-not-set {
> >     base ntp-sync-state;
> >     description
> >       "Indicates the clock is not updated.";
> >
> > The closest analogue to this that I could find in RFC 5905 is "#define
> > NSET            0       /* clock never set */", and there seems to be a
> > significant difference between "not" and "never", so I'm not sure if
> > this is correct.
> >
> >
> Dhruv: Agreed and updated.
> 
> 
> 
> >   identity hmac-sha-1-12 {
> >     base crypto-algorithm;
> >     description
> >       "The HMAC-SHA1-12 algorithm.";
> >
> > While HMAC-SHA1 is not known to be broken, the truncated authentication
> > tag is perhaps not up to best current practices and in some situations
> > there is some value in being able to say that SHA-1 is entirely unused.
> > Did we consider gating this algorithm behind "if-feature deprecated" as
> > well?
> >
> >   identity hmac-sha-1 {
> >     base crypto-algorithm;
> >     description
> >       "HMAC-SHA-1 authentication algorithm.";
> >
> > (only half of the above concerns would apply here, of course)
> >
> >
> Dhruv: Updated
> 
> 
> 
> >   grouping key {
> >     description
> >       "The key.";
> >     nacm:default-deny-all;
> >     choice key-string-style {
> >       description
> >         "Key string styles";
> >       case keystring {
> >         leaf keystring {
> >           type string;
> >           description
> >             "Key string in ASCII format.";
> >
> > There is perhaps some argument that ASCII key strings should be gated by
> > "deprecated" as well...
> >
> >
> Dhruv: Good idea, I don't see harm doing that as it pushes one to avoid
> using well-known and easy-to-remember keywords.
> 
> 
> 
> >   grouping authentication-key {
> >     description
> >       "To define an authentication key for a Network Time
> >        Protocol (NTP) time source.";
> >     nacm:default-deny-all;
> >
> > (Per the Discuss) why does the key metadata like key-id and algorithm
> > need to be default-deny-all?  Typically in YANG modules we only apply
> > this strict restriction to the actual secret key data but not the
> > metadata.
> >
> >
> Dhruv: Handled as part of Discuss
> 
> 
> 
> >     leaf istrusted {
> >       type boolean;
> >       description
> >         "Key-id is trusted or not";
> >     }
> >
> > Where are the semantics of "is trusted" specified?  The reference for
> > the overall grouping, §7.3 of RFC 5905, does not use the word "trust" or
> > any words with that stem.
> >
> >
> Dhruv: "trust" is mentioned in §7.4 when describing NKEY keyword. Updated
> the reference to also include §7.4
> 
> 
> 
> >   grouping authentication {
> >     description
> >       "Authentication.";
> >     choice authentication-type {
> >       description
> >         "Type of authentication.";
> >       case symmetric-key {
> >
> > We currently only have the one case listed.  Is the work in progress for
> > other authentication types that would give motivation for providing this
> > extension point?
> >
> >
> Dhruv: Yes, auto-key comes to mind but it is not in scope for this model.
> 
> 
> 
> >   identity aes-cmac {
> >     base crypto-algorithm;
> >     description
> >       "The AES-CMAC algorithm - required by
> >        RFC 8573 for MAC for the NTP";
> >     reference
> >       "RFC 4493: The AES-CMAC Algorithm";
> >
> > I suspect that RFC 8573 is a more useful reference for the semantics of
> > this YANG element than RFC 4493 is.
> >
> >
> Dhruv: Added 8573 as well.
> 
> 
> 
> >   grouping common-attributes {
> >     description
> >       "NTP common attributes for configuration.";
> >     leaf minpoll {
> >       type int8;
> >       default "6";
> >       description
> >         "The minimum poll interval used in this association.";
> >       reference
> >         "RFC 5905: Network Time Protocol Version 4: Protocol and
> >          Algorithms Specification, Section 7.2";
> >     }
> >     leaf maxpoll {
> >       type int8;
> >       default "10";
> >       description
> >         "The maximum poll interval used in this association.";
> >       reference
> >         "RFC 5905: Network Time Protocol Version 4: Protocol and
> >          Algorithms Specification, Section 7.2";
> >
> > The referenced section seems to define MINPOLL and MAXPOLL as 4 and 17,
> > respectively.  I assume that the different default values given here
> > were discussed in the WG.
> >
> >
> Dhruv: Yes it was discussed. See
> https://mailarchive.ietf.org/arch/msg/ntp/POuAh23uooGn4RU5Xy43GhEHrqM/
> 
> 
> 
> >     leaf port {
> >       if-feature "ntp-port";
> >       type inet:port-number {
> >         range "123 | 1025..max";
> >
> > It's not entirely clear that prohibiting the use of non-123 "system
> > ports" is needed at the YANG level.  (Seems to occur more than once.)
> >
> >
> Dhruv: The port configuration is important if 123 is blocked. Note that the
> leaf is optional, used only if the ntp-port feature is explicitly enabled.

To be clear, I'm happy that there is the option to configure the port.  My
question was more, why the YANG module forbids me from cofiguring port 124
or 999.  (I have no particular reason to do so, but I also don't know a
particular reason to forbid it if the operator so chooses.)

> 
> 
> >         leaf access-mode {
> >           type identityref {
> >             base access-mode;
> >           }
> >
> >           description
> >             "NTP access mode. The definition of each possible value:
> >              peer: Both time request and control query can be
> >              performed.
> >              server: Enables the server access and query.
> >              synchronization: Enables the server access only.
> >              query: Enables control query only.";
> >
> > It feels a little unusual to duplicate the description of the various
> > access-mode-derived YANG identities in the description here.
> >
> >
> Dhruv: Updated
> 
> 
> 
> >     container clock-state {
> >       config false;
> >
> > We had a comment "/* Configuration data nodes */" above the toplevel ntp
> > presence-container, which contains this clock-state container.  So
> > perhaps it is appropriate to have another comment here demarcating the
> > transition away from configuration nodes?
> >
> >
> Dhruv: Removed the comment.
> 
> 
> 
> >         leaf clock-state {
> >           [...]
> >           description
> >             "The state of system clock. The definition of each
> >              possible value is:
> >              synchronized: Indicates local clock is synchronized.
> >              unsynchronized: Indicates local clock is not
> >              synchronized.";
> >
> > [same comment about identities/descriptions]
> >
> >
> Dhruv: updated
> 
> 
> 
> >       leaf reach {
> >         type uint8;
> >         description
> >           "The reachability of the configured
> >            server or peer.";
> >         reference
> >           "RFC 5905: Network Time Protocol Version 4: Protocol and
> >            Algorithms Specification, Section 9.2 and 13";
> >       }
> >       leaf unreach {
> >         type uint8;
> >         description
> >           "The unreachability of the configured
> >            server or peer.";
> >         reference
> >           "RFC 5905: Network Time Protocol Version 4: Protocol and
> >            Algorithms Specification, Section 9.2 and 13";
> >
> > These two nodes hold qualitatively different values, yet we use
> > essentially the same language to describe them.  This seems needlessly
> > confusing.  ('reach' is an 8-bit shift register that tracks packet
> > generation and receipt, whereas 'unreach' is a count of how long the
> > 'reach' value has been zero.)  I think we should use more detailed
> > descriptions, and probably provide the units (seconds) for 'unreach' as
> > well.
> >
> >
> Dhruv: Agreed and updated.
> 
> 
> 
> >       leaf now {
> >         type uint32;
> >         units "seconds";
> >         description
> >           "The time since the last NTP packet was
> >            received or last synchronized.";
> >
> > The standalone word "now" doesn't appear in RFC 5905 in any location
> > that this might be referring to.  I'm pretty confused at how what is
> > described here relates to the protocol specification and why it is of
> > interest to expose in the data model.
> >
> >
> Dhruv: It is found to be a useful parameter for finding faults when things
> go wrong.
> 
> 
> 
> >       leaf dispersion {
> >         [...]
> >         reference
> >           "RFC 5905: Network Time Protocol Version 4: Protocol and
> >            Algorithms Specification, Section 10";
> >
> > For the "root-dispersion" leaf earlier, we listed as references Sections
> > 4 and 7.3 of RFC 5095; Section 10 seems a bit more helpful as it goes
> > into how the value is actually calculated and used.  I wonder if it
> > makes sense to reference section 10 as well as 4 and 7.3 for
> > "root-dispersion".
> >
> >
> **Dhruv: Updated**
> 
> 
> 
> >           description
> >             "Configuration of broadcast-client.";
> >           reference
> >             "RFC 5905: Network Time Protocol Version 4: Protocol and
> >              Algorithms Specification, Section 3.1";
> >         }
> >
> > Why are there no leafs within the broadcast-client container?  Is there
> > really no configuration at all (authentication?  port?) and no state to
> >
> >
> Dhruv: Yes, the only information needed is to mark that this interface will
> receive the NTP broadcast packets sent by any broadcast server and the
> actual association is made dynamically without any configuration

Thanks for confirming.

And thanks as well for all the other good stuff that I did not specifically
reply to -- well done!

-Ben

> 
> 
> > Section 9.3
> >
> > Thank you for providing an example using strong cryptographic
> > authentication!
> >
> > However (per the discuss), I'm not sure how the example of retrieving
> > the authentication-related configuration is consistent with the
> > nacm:default-deny-all ACL that is applied to "grouping key", and we
> > probably shouldn't be indicating that retrieving active keys is an
> > example of something that people should do.
> >
> >
> Dhruv: Removed the retrieval example
> 
> 
> 
> > Section 11
> >
> >       /ntp/authentication/authentication-keys - The entries in the list
> >       includes all the NTP authentication keys.  This information is
> >       sensitive and can be exploited and thus unauthorized access to
> >       this needs to be curtailed.
> >
> > Typically we would mention that due to the sensitivity the
> > nacm:default-deny-all policy is applied, to prevent their being read
> > out.  (Roman's suggestions are also good.)
> >
> >
> Dhruv: Added.
> 
> 
> 
> > In a similar vein to Roman's comments, it seems that changing the
> > /ntp/interfaces configuration could lead to performing time
> > synchronization over untrusted external interfaces and not performing
> > time synchronization over trusted internal interfaces, which can be
> > security-relevant.
> >
> >
> Dhruv: Added.
> 
> 
> 
> > Section 13.1
> >
> > It's not clear to me that RFC 5907 needs to be classified as normative,
> > since all we say about it is that a mapping is possible between subsets
> > of the MIB and the YANG.
> >
> > Likewise, RFCs 6241, 6242, and 8040 are just examples of how the YANG
> > module could be used/accessed, and are not required in order to
> > implement any part of the YANG module.
> >
> >
> Dhruv: Updated.
> 
> 
> 
> >
> > NITS
> >
> > I'm leaving out a number of things that I expect the RFC Editor staff to
> > find.
> >
> > Section 1
> >
> >    interface parameters.  It also provides information about running
> >    state of NTP implementations.
> >
> > Pedantically, I would say that the data model "provides access to
> > information" about the running state; the data model itself is an
> > abstract thing that is disconnected from what happens operationally.
> >
> >
> Dhruv: ACK
> 
> 
> 
> > Section 2
> >
> > I think the line for /ntp/interfaces got inadvertently removed from the
> > abbreviated tree diagram.
> >
> >
> Dhruv: Thanks for noticing this!
> 
> 
> 
> > Section 8
> >
> >   feature deprecated {
> >     description
> >       "Support deprecated MD5-based authentication (RFC 8573) or
> >        SHA-1 or any other deprecated authentication.
> >
> > "any other deprecated authentication mechanism"
> >
> >        It is enabled to support legacy compatibility when secure
> >        cryptographic algorithm is not availaible to use.";
> >
> > "secure cryptographic algorithms are not available to use".
> >
> >     description
> >       "This defines NTP access modes. These identifies
> >        how the ACL is applied with NTP.";
> >
> > s/identifies/identify/
> >
> >     list associations {
> >       [...]
> >       leaf address {
> >         type inet:ip-address;
> >         description
> >           "The address of this association. Represents the IP
> >            address of a unicast/multicast/broadcast address.";
> >
> > I wonder if we want to use the word "remote" or "peer" in here in some
> > form.
> >
> >       leaf isconfigured {
> >         type boolean;
> >         description
> >           "Indicates if this association is configured or
> >            dynamically learned.";
> >
> > I suggest adding "(true)" and "(false)" after "configured" and
> > "dynamically learned", respectively.
> >
> >       list interface {
> >         [...]
> >         container broadcast-server {
> >           if-feature "broadcast-server";
> >           presence "NTP broadcast-server is configured";
> >
> > Perhaps "on this interface" would make sense?
> >
> >           container authentication {
> >             if-feature "authentication";
> >             description
> >               "Authentication used for this association.";
> >             uses authentication;
> >
> > I think s/for this association/on this interface/ (since this is still
> > under "list interface").
> >
> >         container broadcast-client {
> >           if-feature "broadcast-client";
> >           presence "NTP broadcast-client is configured.";
> >
> > As for broadcast-server, perhaps "on this interface"?
> > report?
> >
> >
> Dhruv: All the above are updated as suggested.
> 
> 
> 
> >    This example describes how to get all association present in the
> >    system -
> >    [...]
> >    <data xmlns="urn:ietf:params:xml:ns:netconf:base:1.0">
> >      <ntp xmlns="urn:ietf:params:xml:ns:yang:ietf-ntp">
> >        <associations>
> >          <address>192.0.2.1</address>
> >            [...]
> >
> > This may well just be my ignorance, but I how does one detect the
> > boundary between associations inside the <associations> element?  Should
> > there be a containing (e.g.) <association> element around each one?  (I
> > see that there is only one in this example.)
> >
> >
> >
> Dhruv: Updated.
> 
> Thanks!
> Dhruv
> 
> Diff:
> https://www.ietf.org/rfcdiff?url1=draft-ietf-ntp-yang-data-model-15&url2=https://raw.githubusercontent.com/dhruvdhody/ietf/master/draft-ietf-ntp-yang-data-model-16.txt
> Working Copy:
> https://raw.githubusercontent.com/dhruvdhody/ietf/master/draft-ietf-ntp-yang-data-model-16.txt
> 
> Consolidated Review Status:
> https://notes.ietf.org/draft-ietf-ntp-yang-data-model