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

From: "Wubo (lana)" <[email-masked]>
Date: Fri, 12 Mar 2021 10:30:42 +0000

Hi Rob,

We have posted a -09 version that fixes the issues from you and Tom Petch. Please see below:
Diff:           https://www.ietf.org/rfcdiff?url2=draft-ietf-opsawg-tacacs-yang-09

Thanks,
Bo

-----邮件原件-----
发件人: Rob Wilton (rwilton) [mailto:[email-masked]] 
发送时间: 2021年3月11日 19:23
收件人: Wubo (lana) <[email-masked]>; opsawg <[email-masked]>; [email-masked]
主题: RE: AD review of draft-ietf-opsawg-tacacs-yang-07

Thanks Bo.

Perhaps make the choice description:

 "Encryption mechanism between TACACS+ client and server".

Let me know when you are done, and I'll start the IETF LC.

Regards,
Rob

> -----Original Message-----
> From: Wubo (lana) <[email-masked]>
> Sent: 11 March 2021 10:46
> To: Rob Wilton (rwilton) <[email-masked]>; opsawg 
> <[email-masked]>; [email-masked]
> Subject: Re: AD review of draft-ietf-opsawg-tacacs-yang-07
> 
> Hi Rob,
> 
> Thanks for your comments. I will submit rev-09 to resolve the issues 
> below.
> For the "shared-secret", will change the leaf to:
> 
> 		choice encryption {
>            mandatory true;
>            description
>              "A choice amongst available encryption types.";
>            case shared-secret {
> 		     leaf shared-secret {
> ...
>       }
> 
> Best regards,
> Bo
> 
> -----邮件原件-----
> 发件人: Rob Wilton (rwilton) [mailto:[email-masked]]
> 发送时间: 2021年3月10日 20:06
> 收件人: Wubo (lana) <[email-masked]>; opsawg <[email-masked]>; 
> [email-masked]
> 主题: RE: AD review of draft-ietf-opsawg-tacacs-yang-07
> 
> Hi Bo,
> 
> Sorry, this doc slipped off my radar.
> 
> I've provided comments inline, but I think that it is only the 
> "shared- secret" part that needs to be resolved.
> 
> 
> 
> > -----Original Message-----
> > From: Wubo (lana) <[email-masked]>
> > Sent: 19 September 2020 08:47
> > To: Rob Wilton (rwilton) <[email-masked]>; opsawg 
> > <[email-masked]>; [email-masked]
> > Subject: Re: AD review of draft-ietf-opsawg-tacacs-yang-07
> >
> > Hi Rob,
> >
> > Thanks for the reply. Please see inline.
> >
> > Best regards,
> > Bo
> >
> > -----邮件原件-----
> > 发件人: Rob Wilton (rwilton) [mailto:[email-masked]]
> > 发送时间: 2020年9月15日 18:53
> > 收件人: Wubo (lana) <[email-masked]>; opsawg <[email-masked]>; 
> > [email-masked]
> > 主题: RE: AD review of draft-ietf-opsawg-tacacs-yang-07
> >
> > Hi Bo,
> >
> > Thanks for addressing my previous comments.
> >
> > Please see inline ...
> >
> > > -----Original Message-----
> > > From: Wubo (lana) <[email-masked]>
> > > Sent: 29 August 2020 09:40
> > > To: Rob Wilton (rwilton) <[email-masked]>; opsawg 
> > > <[email-masked]>; [email-masked]
> > > 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?
> >
> > [Bo] Yes, this model is defined to cover the client functionality 
> > from
> > draft-ietf-opsawg-tacacs-11 or the latest version.
> > "shared-secret" is used to encrypt the TACACS+ packet body, and is 
> > the only body encryption method defined in the TACACS+ protocol.
> > During WG discussion, a similar issue has been discussed and it was 
> > suggested that in future an augmented model to be defined to reflect 
> > alternative methods, such as TLS encryption.
> [RW]
> 
> So, the problem that I see is that the shared-secret leaf is mandatory.
> I.e., even if an alternative method was specified then a shared-secret 
> would still always need to be specified.  I don't know whether that it 
> is a reasonable constraint to put on the model.
> 
> If there will always be a shared secret then your augmentation 
> approach should be fine.
> 
> But, if it is plausible or likely that a shared-secret would not 
> always be required then I would be better to have a mandatory choice 
> statement with shared-secret as one of the choices.
> 
> 
> 
> >
> > >
> > >
> > > 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.
> >
> > [Bo] Yes, the intention is the latter.
> > Perhaps you mean the YANG descriptions of 'tcsplus-server-type' are 
> > not clear enough. Please let us know if this addresses your concerns 
> > or if there is anything else.
> >
> >   typedef tacacs-plus-server-type {
> >     type bits {
> >       bit authentication {
> >         description
> >           "When set, the server is an authentication server."
> >          ->
> > 		  "When set, the device use the server for authentication
> service.";
> [RW]
> Yes, I think that is fine, but I propose perhaps "Indicates that the 
> TACACS server is providing authentication services".  The other two 
> comments should be adjusted similarly.
> 
> 
> 
> >
> >       }
> > ...
> >     }
> >     description
> >       "tacacs-plus-server-type can be set to
> >        authentication/authorization/accounting
> >        or any combination of the three types. When all three types are
> >        supported, all the three bits are set.";
> >      ->
> > 	 "When all the three bits are set, the device use all available 
> > services on the server.";
> [RW]
> 
> The last sentence probably isn't needed and could just be removed?
> 
> Regards,
> Rob
> 
> 
> >   }
> >
> > >
> > >
> > > 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.
> >
> > [Bo]Thanks for the suggestion.
> >
> >
> > >
> > >
> > > 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-masked]]
> > > 发送时间: 2020年8月20日 18:38
> > > 收件人: opsawg <[email-masked]>; draft-ietf-opsawg-tacacs- 
> > > [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: "Rob Wilton (rwilton)" <[email-masked]>
Date: Mon, 15 Mar 2021 09:12:14 +0000

Hi Bo,

Thanks for addressing my comments, and your work on this document.  I've now requested IETF LC on this document.

Regards,
Rob

> Sent: 12 March 2021 10:31
> To: Rob Wilton (rwilton) <[email-masked]>; opsawg <[email-masked]>;