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

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

Hi Bo,

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

Regards,
Rob


> -----Original Message-----
> From: Wubo (lana) <[e-mail]>
> Sent: 12 March 2021 10:31
> To: Rob Wilton (rwilton) <[e-mail]>; opsawg <[e-mail]>;
> [e-mail]
> Subject: Re: AD review of draft-ietf-opsawg-tacacs-yang-07
> 
> 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:[e-mail]]
> 发送时间: 2021年3月11日 19:23
> 收件人: Wubo (lana) <[e-mail]>; opsawg <[e-mail]>;
> [e-mail]
> 主题: 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) <[e-mail]>
> > Sent: 11 March 2021 10:46
> > To: Rob Wilton (rwilton) <[e-mail]>; opsawg
> > <[e-mail]>; [e-mail]
> > 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:[e-mail]]
> > 发送时间: 2021年3月10日 20:06
> > 收件人: Wubo (lana) <[e-mail]>; opsawg <[e-mail]>;
> > [e-mail]
> > 主题: 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) <[e-mail]>
> > > Sent: 19 September 2020 08:47
> > > To: Rob Wilton (rwilton) <[e-mail]>; opsawg
> > > <[e-mail]>; [e-mail]
> > > 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:[e-mail]]
> > > 发送时间: 2020年9月15日 18:53
> > > 收件人: Wubo (lana) <[e-mail]>; opsawg <[e-mail]>;
> > > [e-mail]
> > > 主题: 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) <[e-mail]>
> > > > Sent: 29 August 2020 09:40
> > > > To: Rob Wilton (rwilton) <[e-mail]>; opsawg
> > > > <[e-mail]>; [e-mail]
> > > > 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:[e-mail]]
> > > > 发送时间: 2020年8月20日 18:38
> > > > 收件人: opsawg <[e-mail]>; draft-ietf-opsawg-tacacs-
> > > > [e-mail]
> > > > 主题: 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) <[e-mail]>
> > > > > Sent: 20 August 2020 11:23
> > > > > To: opsawg <[e-mail]>;
> > > > > [e-mail]
> > > > > 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