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

From: "Wubo (lana)" <[email-masked]>
Date: Fri, 21 Aug 2020 07:38:52 +0000

Hi Rob,

Many thanks for your another thorough AD review, and I will consider these comments along with previous ones.

Regards,
Bo

-----邮件原件-----
发件人: Rob Wilton (rwilton) [mailto:[email-masked]] 
发送时间: 2020年8月20日 18:38
收件人: opsawg <[email-masked]>; [email-masked]
主题: RE: AD review of draft-ietf-opsawg-tacacs-yang-07

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,
> 
> 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: "Wubo (lana)" <[email-masked]>
Date: Sat, 29 Aug 2020 08:39:41 +0000

Hi Rob,

v-08 is posted, to address most of the your comments in the two AD reviews. 
https://www.ietf.org/rfcdiff?url2=draft-ietf-opsawg-tacacs-yang-08

There are still some comments to confirm with you.

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?
[Bo] This issue has been discussed in WG before, and it was recommended that the module be updated when the new TACACS+ protocol defined. What's your opinion?

[Bo]Yes, tcsplus-server-type indicates what type the server is. But I don't quite understand this comment.

6. Should there be a limit on the length of a server name?
[Bo]The TACACS+ protocol does not have any restrictions, and I also think this model could follow current ietf-system model, since this module is the augmentation of the system model and there is no restriction on the RADIUS server name in the ietf-system model. 
How do you think?

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.
[Bo] Thanks. Agree that the must statement does not prohibit accounting or authorization TACACS+ server configuration.
I updated the must statement with authentication server type validation.

Thanks,
Bo

-----邮件原件-----
发件人: Rob Wilton (rwilton) [mailto:[email-masked]] 
发送时间: 2020年8月20日 18:38
收件人: opsawg <[email-masked]>; [email-masked]
主题: RE: AD review of draft-ietf-opsawg-tacacs-yang-07

Regards,
Rob