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

Dhruv Dhody <dhruv.ietf@gmail.com> Thu, 09 December 2021 16:58 UTC

Return-Path: <dhruv.ietf@gmail.com>
X-Original-To: ntp@ietfa.amsl.com
Delivered-To: ntp@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 092193A1007; Thu, 9 Dec 2021 08:58:44 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.097
X-Spam-Level:
X-Spam-Status: No, score=-2.097 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Qc_kZqDnexkP; Thu, 9 Dec 2021 08:58:39 -0800 (PST)
Received: from mail-io1-xd2a.google.com (mail-io1-xd2a.google.com [IPv6:2607:f8b0:4864:20::d2a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id C2FD23A0FD4; Thu, 9 Dec 2021 08:58:38 -0800 (PST)
Received: by mail-io1-xd2a.google.com with SMTP id c3so7366053iob.6; Thu, 09 Dec 2021 08:58:38 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=KZmF30wKr9FNE1OQf/oqM6q5u/Fh85rMMSs1KoOk5h0=; b=NFzKYo1XAt+OcVkuN9/RaLhqhE/K8Aw9s8c3MOoKfr6ZcJOVx4/c1FR6QrwI+E+Otc ONAABVlxZ2yRK0u3juEcHb9K0GacbCHlBGH0FE++Quw21SlvOGIH53YJnvt9T/3WBdtc t5W+/n2xxIPqbrTWpRlhz650VFuzLHldtp40sV9ecuR3R+3/CZGSH8/W3oR5cplalY7a oWNXKHr1ILXik5mz0+5zw/3zG0CaOQXqKkimAwfZqdt5818wtHLSwYYBbC1rIAx+0EKT MtMQG1qUQWvRbUoaO4Lpd+ZbV/0UoytMwnu5Ba3HxkGa0J7aieUYwKhGBKi9AH8SiDBS Xv3Q==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KZmF30wKr9FNE1OQf/oqM6q5u/Fh85rMMSs1KoOk5h0=; b=fxcIQSoMLXhLZVf1Ku4mHtqDRuBEpfhiH2vLFtIeY2lHjIdYsfA0IQFwcb00fAU8UV tn1q8WKyTbYRB9F27qUSDvT+UomaYq3ft6vJJalwktsYMhwhcX43MRg3BS5WNQi57HW/ SN/Xvw9OC+m4fIGvJNh6+QsqDOL37+FzFoHVcY7LP3h998DVqmmxCpPkhvbWXnuGLfUg Pxozw7RAeeYutH6IUMrpeNRb2JUjOSQPOTc3+ElyEE5x1pmid1pyy06ytpTTu5huhBlO GU2yoS5Q8Ep/QwcX9G3x2DOke18FlcA9OpsjJQAgxnCcGpALoeqmMBweNg5hbXCt+0Ea pn9g==
X-Gm-Message-State: AOAM530FfuVP2u6TVekev6Ltz+YkMuS0QYat2FLIWMC3bcatk5We7Ay+ RJ2zQEtG2iFfZcISgA7R3xm5drPs4R3T52bpF0+HQpAFIxU=
X-Google-Smtp-Source: ABdhPJwYISHUUbjU46NCAoMc7hGq8vJQBOVx3uJIapO3Zzq95vL8NWQ/66YrCEsE1jAo4fJ8/cVDx3otk3QwY6Qf8dA=
X-Received: by 2002:a05:6638:3049:: with SMTP id u9mr10821995jak.132.1639069117017; Thu, 09 Dec 2021 08:58:37 -0800 (PST)
MIME-Version: 1.0
References: <162512780466.24729.2677234182140688314@ietfa.amsl.com>
In-Reply-To: <162512780466.24729.2677234182140688314@ietfa.amsl.com>
From: Dhruv Dhody <dhruv.ietf@gmail.com>
Date: Thu, 9 Dec 2021 22:28:00 +0530
Message-ID: <CAB75xn4NUpHjNowequRcOSz56b4pvHGPjnmsMppRdM27+-2Ytg@mail.gmail.com>
To: Benjamin Kaduk <kaduk@mit.edu>
Cc: The IESG <iesg@ietf.org>, draft-ietf-ntp-yang-data-model@ietf.org, ntp-chairs@ietf.org, NTP WG <ntp@ietf.org>, Dieter Sibold <dsibold.ietf@gmail.com>
Content-Type: multipart/alternative; boundary="0000000000007b74b405d2b98423"
Archived-At: <https://mailarchive.ietf.org/arch/msg/ntp/nCvuFuk40qN-_C6mrXBby5trrXU>
Subject: Re: [Ntp] Benjamin Kaduk's Discuss on draft-ietf-ntp-yang-data-model-15: (with DISCUSS and COMMENT)
X-BeenThere: ntp@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Network Time Protocol <ntp.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/ntp>, <mailto:ntp-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/ntp/>
List-Post: <mailto:ntp@ietf.org>
List-Help: <mailto:ntp-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/ntp>, <mailto:ntp-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 09 Dec 2021 16:58:45 -0000

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 <
noreply@ietf.org> 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.



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



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



>         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



> 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