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

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Thu, 01 Jul 2021 01:23:25 -0700

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.

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.

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.

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

Section 3

We list a mapping of YANG nodes and MIB objects, but will the respective
data types be trivially relatable?

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.

Section 7

I confess that I rather expected some note that NTPv3 (or at least RFC
1305) is classified as obsolete.

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?

  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.

  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)

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

  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.

    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.

  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?

  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.

  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.

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

        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.

    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?

        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]

      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.

      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.

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

          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

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.

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

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.

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.

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.

Section 2

I think the line for /ntp/interfaces got inadvertently removed from the
abbreviated tree diagram.

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?

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