Hi Bo, Thanks for addressing my previous comments.
Please see inline ... > -----Original Message----- > From: Wubo (lana) <[email protected]> > Sent: 29 August 2020 09:40 > To: Rob Wilton (rwilton) <[email protected]>; opsawg <[email protected]>; > [email protected] > Subject: Re: AD review of draft-ietf-opsawg-tacacs-yang-07 > > 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? [RW] I'm not sure I entirely follow. By "be updated when the new TACACS+ protocol defined", do you mean: https://tools.ietf.org/html/draft-ietf-opsawg-tacacs-11, or something else? If it is this draft, then this is a normative reference and will be published shortly. I had presumed that this model covered the client functionality from draft-ietf-opsawg-tacacs-11? > > > 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? > [Bo]Yes, tcsplus-server-type indicates what type the server is. But I > don't quite understand this comment. [RW] The distinction that I was trying to make is: Server 'S', might have the capabilities of an authentication and accounting server, but Network Device 'D', that is making use of Server 'S', might only be making use of its authentication capabilities. In this scenario, do the configuration bits on device 'D' list Server 'S' as supporting authentication and accounting (since that is what the server supports) or do they only list Server 'S' capabilities as "authentication" (since that is what is being used by 'D')? If the intended behaviour is the latter, then I think that we should tweak the descriptions to be clear. > > > 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? [RW] I agree that being consistent with ietf-system is a good choice. I do wonder more generally if this will mean that different vendors will impose different limits using deviations, which would effectively make interop harder. But that doesn't need to be solved here/now. > > > 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. [RW] Okay. Regards, Rob > > Thanks, > Bo > > -----邮件原件----- > 发件人: Rob Wilton (rwilton) [mailto:[email protected]] > 发送时间: 2020年8月20日 18:38 > 收件人: opsawg <[email protected]>; draft-ietf-opsawg-tacacs- > [email protected] > 主题: 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 protected]> > > Sent: 20 August 2020 11:23 > > To: opsawg <[email protected]>; > > [email protected] > > 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 _______________________________________________ OPSAWG mailing list [email protected] https://www.ietf.org/mailman/listinfo/opsawg
