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

["Wubo (lana)" <[e-mail]>]

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.

> 
> 
> 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.";		  
		  
      }
...
    }
    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.";  
  }

> 
> 
> 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