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

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

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