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

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Thu, 20 Aug 2020 10:22:31 +0000

Hi,

This is my AD review of draft-ietf-opsawg-tacacs-yang-07.  Sorry that it has been a little while in coming.

Thank you for this document, I believe that it is in good shape.  I've given my slightly more significant comments first, followed by some editorial comments.

COMMENTS:

"Section 3":

   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.

1. Looking at the module, the statistics only seem to cover the number of messages rather than the amount of data.  Possibly delete the part of the sentence from "which include" ... to the end of the sentence.

"Regarding the YANG module":

2. I suggest changing "tacacsplus" to "tacacs-plus" (e.g., in the module title and top level nodes).

3. "shared-secret", should that be put under a choice statement?  Is it likely that alternative methods of authenticating the server are likely in future?

4. I'm not sure that the "tacacsplus" feature is required.  Supporting the ietf-system-tacacsplus module should be sufficient to indicate that the device supports TACACS+ client configuration.

5. Does the tcsplus-server-type indicate what the server is, or how the server is used?  E.g., could a server have the authentication bit set, but then not be used for user authentication?  Or should that be prevented with a must statement?
 
6. Should there be a limit on the length of a server name?

7. I dont' know whether this matters, but the must statement doesn't seem to be quite complete, in that it would still allow TACACS+ to be listed as an authentication mechanims, but only include an accounting server in the TACACS+ server list.

"Security section":
   /system/tacacsplus/server:  This list contains the objects used to
      control the TACACS+ servers used by the device.  Unauthorized
      access to this list could cause a user management failure on the
      device.

8. I don't know TACACS+, but I would presume that the risk of accessing this list is much greater than just user management failure.  If it was possible to modify this configuration to point to a compromised TACACS+ server then would it not be possible to obtain complete control over the device?  If, so I think then I think that it would be helpful to make this risk clear.  [As a nit, we should probably also use 'data nodes' rather than 'objects']

"References":

9. Please can you ensure that your normative references to all RFCs that define YANG modules that are imported by the YANG modules defined in this document.  From a quick scan, it looked like some might be missing.

EDITORIAL COMMENTS:

Abstract:
1. that augment -> that augments
2. in the RFC 7317 with TACACS+ client model. -> in RFC 7317 with a TACACS+ client data model.

3. The data model of Terminal Access Controller Access Control
   System Plus (TACACS+) client allows ...->
The Terminal Access Controller Access Control System Plus (TACACS+) client data model allows ...

Introduction:

4. This document defines a YANG module that augment the System
   Management data model defined in the [RFC7317] with TACACS+ client
   model.
   
   ->
   
   This document defines a YANG module that augments the System
   Management data model defined in [RFC7317] with a TACACS+ client
   data model.
   
5. TACACS+ provides Device Administration ->
   TACACS+ provides device administration
   
6. 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+ [I-D.ietf-opsawg-tacacs] provides Device Administration for
   routers, network access servers and other networked computing devices
   via one or more centralized servers.

7. o  User Authentication Model: Defines a list of usernames and
      passwords and control the order in which local or RADIUS
      authentication is used.
      
   ->
   
   o  User Authentication Model: Defines a list of local usernames and
      passwords.  It also controls the order in which local or RADIUS
      authentication is used.
      
8.  System Management model -> System Management Model?

9.  The YANG model can be used -> The YANG module can be used 
    The YANG data model in this document" => The YANG module in this document

"3.  Design of the Data Model"

10. Recommend changing heading to "Design of the TACAS+ Data Model"

11. client on the device -> client on a device

12. user's name and password -> user's username and password?

13. user and accounting -> user, and accounting

14. is intended to augment -> augments

15. A couple of places "e.g." should be replaced with "e.g.,"

Appendix A:

16. In the example, possibly delete the "single-connection" leaf since that is a default value.

Regards,
Rob

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Thu, 20 Aug 2020 10:38:02 +0000

Ok, my bad.  It seems that I had already done an AD review of this document :-)

Bo, there may be some additional comments that you would like to consider below in your -08 update.

Regards,
Rob

> -----Original Message-----
> From: Rob Wilton (rwilton) <[email-masked]>
> Sent: 20 August 2020 11:23
> To: opsawg <[email-masked]>; [email-masked]
> Subject: AD review of draft-ietf-opsawg-tacacs-yang-07
> 
> Hi,
> 
> Thank you for this document, I believe that it is in good shape.  I've
> given my slightly more significant comments first, followed by some
> editorial comments.
> 
> 
> COMMENTS:
> 
> "Section 3":
> 
> 1. Looking at the module, the statistics only seem to cover the number of
> messages rather than the amount of data.  Possibly delete the part of the
> sentence from "which include" ... to the end of the sentence.
> 
> 
> 3. "shared-secret", should that be put under a choice statement?  Is it
> likely that alternative methods of authenticating the server are likely in
> future?
> 
> 4. I'm not sure that the "tacacsplus" feature is required.  Supporting the
> ietf-system-tacacsplus module should be sufficient to indicate that the
> device supports TACACS+ client configuration.
> 
> 5. Does the tcsplus-server-type indicate what the server is, or how the
> server is used?  E.g., could a server have the authentication bit set, but
> then not be used for user authentication?  Or should that be prevented
> with a must statement?
> 
> 7. I dont' know whether this matters, but the must statement doesn't seem
> to be quite complete, in that it would still allow TACACS+ to be listed as
> an authentication mechanims, but only include an accounting server in the
> TACACS+ server list.
> 
> 
> "Security section":
> 8. I don't know TACACS+, but I would presume that the risk of accessing
> this list is much greater than just user management failure.  If it was
> possible to modify this configuration to point to a compromised TACACS+
> server then would it not be possible to obtain complete control over the
> device?  If, so I think then I think that it would be helpful to make this
> risk clear.  [As a nit, we should probably also use 'data nodes' rather
> than 'objects']
> 
> 
> "References":
> 
> 9. Please can you ensure that your normative references to all RFCs that
> define YANG modules that are imported by the YANG modules defined in this
> document.  From a quick scan, it looked like some might be missing.
> 
> 
> 
> EDITORIAL COMMENTS:
> 
> 
> Abstract: