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

["Rob Wilton (rwilton)" <[e-mail]>]

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.

       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

       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?


    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?

       <CODE BEGINS> file "[e-mail]"

     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.  


       typedef tcsplus-server-type {
# I think that this should be "tacacsplus-server-type", shortening the name here is probably not helpful.

       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.

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

Regards,
Rob