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

From: tom petch <[email-masked]>
Date: Wed, 2 Sep 2020 16:13:27 +0000

Looking at -08

Title suggest /yang/YANG/

leaf vrf-instance 
might benefit from a reference to RFC8529
(ie I did not know where to look:-)

security
/cause the device vulnerable to attacks/make the device vulnerable to attacks/

IANA
Namespace /yang: ietf/yang:ietf/

Tom Petch
________________________________________
From: OPSAWG <[email-masked]> on behalf of Rob Wilton (rwilton) <rwilton=[email-masked]>
Sent: 20 August 2020 11:38

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

_______________________________________________
OPSAWG mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/opsawg

---

From: "Wubo (lana)" <[email-masked]>
Date: Thu, 3 Sep 2020 11:27:12 +0000

-----邮件原件-----
发件人: tom petch [mailto:[email-masked]] 
发送时间: 2020年9月3日 19:17
收件人: Wubo (lana) <[email-masked]>; Rob Wilton (rwilton) <rwilton=[email-masked]>; opsawg <[email-masked]>; [email-masked]
主题: Re: AD review of draft-ietf-opsawg-tacacs-yang-07

From: Wubo (lana) <[email-masked]>
Sent: 03 September 2020 12:11

Hi Tom,

Many thanks for the review, will fix in the rev-09.

<tp>

Or leave it and treat the comments as part of IETF Last Call (which I hope will happen soon) - I am easy either way.

Tom Petch
[Bo] Thanks for the suggestion, If Rob has additional AD comments, I'll post the rev-09.

Thanks,
Bo

-----邮件原件-----
发件人: tom petch [mailto:[email-masked]]
发送时间: 2020年9月3日 0:13
收件人: Rob Wilton (rwilton) <rwilton=[email-masked]>; opsawg <[email-masked]>; [email-masked]
主题: Re: AD review of draft-ietf-opsawg-tacacs-yang-07

Looking at -08

leaf vrf-instance
might benefit from a reference to RFC8529 (ie I did not know where to look:-)

security

IANA

Tom Petch

Regards,
Rob

> To: opsawg <[email-masked]>;
> [email-masked]
> This is my AD review of draft-ietf-opsawg-tacacs-yang-07.  Sorry that 
> it has been a little while in coming.
>
> 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.
>
>
> 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?
>
> 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":
> 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:
>    System Plus (TACACS+) client allows ...-> The Terminal Access 
> Controller Access Control System Plus (TACACS+) client data model 
> allows ...
>
> Introduction:
>

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Tue, 15 Sep 2020 10:53:16 +0000

Hi Bo,

Thanks for addressing my previous comments.

Please see inline ...

> Sent: 29 August 2020 09:40
> To: Rob Wilton (rwilton) <[email-masked]>; opsawg <[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.
> 
> [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, 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]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.

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

---

From: "Wubo (lana)" <[email-masked]>
Date: Sat, 19 Sep 2020 07:46:59 +0000

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,

Please see inline ...

> To: Rob Wilton (rwilton) <[email-masked]>; opsawg 
> <[email-masked]>; [email-masked]
> [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] 

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

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

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

[Bo]Thanks for the suggestion.

> [Bo] Thanks. Agree that the must statement does not prohibit 
> accounting or authorization TACACS+ server configuration.
[RW] 

Okay.

Regards,
Rob

> > 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":
> >
> > 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.
> >
> >
> > 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.
> >
> > 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?
> >
> > 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":
> > 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 
> > 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:
> > 16. In the example, possibly delete the "single-connection" leaf 
> > since that is a default value.
> >
> > Regards,
> > Rob