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

[Dhruv Dhody <[e-mail]>]

Hi Ben,

Thanks for clearing the DISCUSS. More inline...

On Wed, Mar 16, 2022 at 12:11 PM Benjamin Kaduk <[e-mail]> wrote:

> 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.)
>
>
Dhruv 2: Agreed. Will take care of it.



> >
> >
> > > ----------------------------------------------------------------------
> > > 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.)
>
>
Dhruv 2: Apologies for misunderstanding your otherwise clear comment. The
reason for this feature is the firewall blocking of well-known system
ports, so it makes sense to me to pick a new port that is not from the
well-known range. I checked an implementation that I am aware of and they
do block the selection of other system ports.



> >
> >
> > >         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
>
>
Dhruv 2: You also said

- I don't think FIPS 180-4 is a good reference for the truncated HMAC-SHA1-12.
Perhaps an older version would have covered it, though (I didn't check)?

I guess I will remove the reference statement entirely, I see RFC 8177 also
providing no reference for this identity.

I will post a new version once the draft submission blockade is lifted.

Thanks!
Dhruv




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