[Ntp] Benjamin Kaduk's Discuss on draft-ietf-ntp-yang-data-model-15: (with DISCUSS and COMMENT)
Benjamin Kaduk via Datatracker <noreply@ietf.org> Thu, 01 July 2021 08:23 UTC
Return-Path: <noreply@ietf.org>
X-Original-To: ntp@ietf.org
Delivered-To: ntp@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 360963A1C83; Thu, 1 Jul 2021 01:23:25 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
From: Benjamin Kaduk via Datatracker <noreply@ietf.org>
To: The IESG <iesg@ietf.org>
Cc: draft-ietf-ntp-yang-data-model@ietf.org, ntp-chairs@ietf.org, ntp@ietf.org, Dieter Sibold <dsibold.ietf@gmail.com>, dsibold.ietf@gmail.com
X-Test-IDTracker: no
X-IETF-IDTracker: 7.33.0
Auto-Submitted: auto-generated
Precedence: bulk
Reply-To: Benjamin Kaduk <kaduk@mit.edu>
Message-ID: <162512780466.24729.2677234182140688314@ietfa.amsl.com>
Date: Thu, 01 Jul 2021 01:23:25 -0700
Archived-At: <https://mailarchive.ietf.org/arch/msg/ntp/L7x92_Sbnxb_ZIIJnNHlT0xocDk>
Subject: [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
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, 01 Jul 2021 08:23:25 -0000
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.)
- [Ntp] Benjamin Kaduk's Discuss on draft-ietf-ntp-… Benjamin Kaduk via Datatracker
- Re: [Ntp] Benjamin Kaduk's Discuss on draft-ietf-… Dhruv Dhody
- Re: [Ntp] Benjamin Kaduk's Discuss on draft-ietf-… Benjamin Kaduk
- Re: [Ntp] Benjamin Kaduk's Discuss on draft-ietf-… Dhruv Dhody