Hi Rob, Thanks a lot for your detailed review and valuable comments.
We will post -08 revision to address all the comments: 1. Change "tacacsplus" to "tacacs-plus" for the entire draft. Joe made the comments before but we missed them. 2. Revise all the editorial comments you have suggested. Please also see the replies in-line below for some comments to be confirmed. Thanks, Bo -----邮件原件----- 发件人: OPSAWG [mailto:[email protected]] 代表 Rob Wilton (rwilton) 发送时间: 2020年7月11日 0:53 收件人: [email protected] 抄送: opsawg <[email protected]> 主题: [OPSAWG] AD review of draft-ietf-opsawg-tacacs-yang-07 Apologies for the delay, but please find my AD review of the TACACS+ YANG module draft. I would like to thank the authors for their work on this document, and the WG for providing reviews and input in this document. I believe that the document is in good shape but propose some minor changes to some of the wording in places. One particular question that I would like to pull to the top is the naming of the module and identifiers: These generally use "tacacsplus", but I think that "tacacs-plus" might be better and more readable. Full comments are inline in the document below (marked as #) The YANG model can be used with network management protocols such as NETCONF[RFC6241] to install, manipulate, and delete the configuration of network devices. Abstract This document defines a YANG module that augment the System Management data model defined in the RFC 7317 with TACACS+ client model. The data model of Terminal Access Controller Access Control System Plus (TACACS+) client allows the configuration of TACACS+ servers for centralized Authentication, Authorization and Accounting. # Perhaps tweak the first paragraph of the abstract slightly to: This document defines a TACACS+ client YANG module, that augments the System Management data model, defined in RFC 7317, to allow devices to make use of TACACS+ servers for centralized Authentication, Authorization and Accounting. This document defines a YANG module that augment the System Management data model defined in the [RFC7317] with TACACS+ client model. # augment -> augments # with TACACS+ client -> to support the configuration and management of TACACS+ clients. TACACS+ provides Device Administration for routers, network access servers and other networked computing devices via one or more centralized servers which is defined in the TACACS+ Protocol. [I-D.ietf-opsawg-tacacs] # TACACS+ provides -> "TACACS+ [I-D.ietf-opsawg-tacacs] provides" [and remove the reference at the end of the paragraph]. # networked computing devices -> networked devices # centralized servers which ... -> delete from which ... to the end of the sentence. The System Management Model [RFC7317] defines two YANG features to support local or RADIUS authentication: #two YANG features -> separate functionality #or -> and o User Authentication Model: Defines a list of usernames and passwords and control the order in which local or RADIUS authentication is used. # I suggest modifying this to -> o User Authentication Model: Defines a list of usernames with associated passwords and a configuration leaf to decide the order in which local or RADIUS authentication is used. o RADIUS Client Model: Defines a list of RADIUS servers that a device uses. # device uses. -> devices uses to manage users. Since TACACS+ is also used for device management and the feature is not contained in the System Management model, this document defines a YANG data model that allows users to configure TACACS+ client functions on a device for centralized Authentication, Authorization and Accounting provided by TACACS+ servers. # I suggest rewording this paragraph to something like: The System Management Model is augmented with the TACACS+ YANG module defined in this document to allow the use of TACACS+ servers as an alternative to RADIUS servers or local user configuration. Zheng, et al. Expires December 22, 2020 [Page 2] Internet-Draft TACACS+ YANG model June 2020 The YANG model can be used with network management protocols such as NETCONF[RFC6241] to install, manipulate, and delete the configuration of network devices. # I would suggest deleting "to install ..." to the end. The ietf-system-tacacsplus module is intended to augment the "/sys:system" path defined in the ietf-system module with the contents of the"tacacsplus" grouping. Therefore, a device can use local, Remote Authentication Dial In User Service (RADIUS), or Terminal Access Controller Access Control System Plus (TACACS+) to validate users who attempt to access the router by several mechanisms, e.g. a command line interface or a web-based user interface. #intended to augment -> augments #I think that you should just use RADIUS and TACACS+ here rather then spelling our the full names. The "server" list is directly under the "tacacsplus" container, which holds a list of TACACS+ servers and uses server-type to distinguish between the three protocols. The list of servers is for redundancy. # I was confused by "the three protocols" (thought you meant RADIUS, TACACS+ and local), hence suggest explicitly listing the AAA elements here. [Bo] OK, Will correct to "the three protocols-> Authentication, Authorization and Accounting (AAA) " Most of the parameters in the "server" list are taken directly from the TACACS+ protocol [I-D.ietf-opsawg-tacacs], and some are derived from the various implementations by network equipment manufacturers. For example, when there are multiple interfaces connected to the TACACS+ client or server, the source address of outgoing TACACS+ packets could be specified, or the source address could be specified through the interface setting, or derived from the out-bound interface from the local FIB. For the TACACS+ server located in a Virtual Private Network(VPN), a VRF instance needs to be specified. # Unclear what is meant by "or the source address could be specified through the interface setting"? # out-bound -> outbound [Bo] We meant that source address could be the address of a particular interface. How about the following text? "source address could be specified through a particular interface" The "statistics" container under the "server list" is to record session statistics and usage information during user access which include the amount of data a user has sent and/or received during a session. # Does it measure the amount of data sent or recieved, or the number of messages? # Also the statistics don't appear to be per user at all, but instead per server? [Bo] Thanks for pointing this out. This should be the messages per server. How about the following text? "The "statistics" container under the "server list" is a collection of read-only counters for connections and sent and received messages between the TACACS+ client and the configured server." 4. TACACS+ Client Module This YANG module imports typedefs from [RFC6991]. # Do you want to list the RFCs of the other modules that are referenced in the YANG module? [Bo] Yes. I will add the following text and also normal references. "This module also uses the interface typedef from [RFC8342], a VRF instance leafref from [RFC 8529], and "default-deny-all" extension statement from [RFC8341]." <CODE BEGINS> file "[email protected]" module ietf-system-tacacsplus { # This might be worth discussing, but I'm not sure whether "tacacs-plus" wouldn't be better than "tacacsplus" in all the identifiers below. [Bo]Agree, will clean up. typedef tcsplus-server-type { # I think that this should be "tacacsplus-server-type", shortening the name here is probably not helpful. [Bo]Agree. feature tacacsplus { description "Indicates that the device can be configured as a TACACS+ client."; reference "RFC XXXX : The TACACS+ Protocol "; } # This feature isn't required and can be deleted. Support for TACACS+ is implicit by whether or not this YANG module is supported. [Bo] I don't quite understand this comment. Could you explain this a little more? list server { key "name"; ordered-by user; description "List of TACACS+ servers used by the device."; leaf name { type string; description "An arbitrary name for the TACACS+ server."; # Any restrictions? Are spaces allowed in the TACACS+ server name? Does the TACACS+ protocol limit this at all? [Bo] The TACACS+ protocol does not have any restrictions about the server name. And during WG review, it was recommended that the model could be defined by referring to the Radius client model of System Management data model. Therefore, no restriction is added. } Regards, Rob _______________________________________________ OPSAWG mailing list [email protected] https://www.ietf.org/mailman/listinfo/opsawg _______________________________________________ OPSAWG mailing list [email protected] https://www.ietf.org/mailman/listinfo/opsawg
