Subject: Re: [OPSAWG] AD review of draft-ietf-opsawg-tacacs-yang-07

From: "Wubo (lana)" <[email-masked]>
Date: Wed, 15 Jul 2020 07:07:07 +0000

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-masked]] 代表 Rob Wilton (rwilton)
发送时间: 2020年7月11日 0:53
收件人: [email-masked]
抄送: opsawg <[email-masked]>
主题: [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-masked]"

     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-masked]
https://www.ietf.org/mailman/listinfo/opsawg