Subject: Re: [OPSAWG] Yangdoctors last call review of draft-ietf-opsawg-tacacs-yang-03

From: "Wubo (lana)" <[email-masked]>
Date: Wed, 6 May 2020 06:40:32 +0000

Hi Lada,

Thanks for the review. Please see the response inline.

Regards,
Bo

-----邮件原件-----
发件人: Ladislav Lhotka via Datatracker [mailto:[email-masked]] 
发送时间: 2020年5月4日 21:17
收件人: [email-masked]
抄送: [email-masked]; [email-masked]; [email-masked]
主题: Yangdoctors last call review of draft-ietf-opsawg-tacacs-yang-03

Reviewer: Ladislav Lhotka
Review result: Ready with Nits

The YANG module specified in this I-D defines a relatively simple augmentation of the "ietf-system" module that enables configuration of TACACS+ authentication. The ietf-system-tacacsplus module is in a good shape, I found no substantial problems.

**** Comments

- In sec. 3, the text says: 'The ietf-system-tacacsplus module is intended to augment the "/sys:system" path defined in the ietf-system module with "tacacsplus" grouping.' It would be more precise to say '... with the contents of the "tacacsplus" grouping.'
[Bo] OK, I will change as suggested.

- Description of the leaf
/ietf-system-tacacsplus:tacacsplus/statistics/sessions is cryptic and unclear.
[Bo] OK, I will change as follows:
"Number of sessions completed with the server. If the Single Connection Mode was not enabled, the number of sessions is the same as the number of connection opens. 
If the Mode was enabled, a single TCP connection may contain multiple TACACS+ sessions."

- Typo in error-message of
/ietf-system:system/ietf-system-tacacsplus:tacacsplus: s/sysytem/system/
[Bo] OK, will correct.

- Is it correct that the server type may be either one of "authentication", "authorization" or "accounting", or all of them? Is it impossible for a server to be authentication & authorization but not accounting? Such a variant cannot be configured.
[Bo] OK, will correct when the final guidance on this issue is received.

- The "case" statements in ietf-system-tacacsplus:tacacsplus/source-type are unnecessary because each contains only one leaf of the same name; I suggest to remove them.
[Bo] I need to wait for the further guidance from WG. The "choice case" is added based on the email discussion of the WG, which provides some flexibility in specifying the IP address for server communication. Some vendors prefer IP addresses, and some vendors derive IP addresses through interfaces.

- Security Considerations should specifically address the "shared-secret" leaf.
[Bo] OK, will add this and also some other nodes as Tom Petch commented.

- The purpose of Appendix A is unclear, the information it provides is (or should be) in the previous text, the YANG module, and RFC 7317. Instead, it would be useful to provide an example of TACACS+ configuration, e.g. in JSON representation.
[Bo] OK, will change Appendix A into an example of TACACS+ configuration.

---

From: "Joe Clarke (jclarke)" <[email-masked]>
Date: Wed, 6 May 2020 14:41:01 +0000

> [Bo] OK, will correct when the final guidance on this issue is received.

Lada replied yesterday to say that the bit string is likely preferred similar to access-operations in ietf-netconf-acm.  I might personally discourage the use of ‘*’ for this given that there are only three types, but that’s just my individual thought.

Joe

---

From: Ladislav Lhotka <[email-masked]>
Date: Thu, 07 May 2020 08:38:29 +0200

> Lada replied yesterday to say that the bit string is likely preferred similar to access-operations in ietf-netconf-acm.  I might personally discourage the use of ‘*’ for this given that there are o…

+1

I think it is better to have all three types explicitly in the value. Perhaps this could also be the default?

Lada

-- 
Ladislav Lhotka 
Head, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67

---

From: Ladislav Lhotka <[email-masked]>
Date: Thu, 07 May 2020 08:55:50 +0200

> [Bo] I need to wait for the further guidance from WG. The "choice case" is added based on the email discussion of the WG, which provides some flexibility in specifying the IP address for server com…

I am not suggesting to remove this choice, this is of course not YANG Doctor's business. My comment is related exclusively to YANG syntax: if you have a "case" statement containing only one data node, then the "case" statement can be omitted, see sec. 7.9.2 in RFC 7950. Such a shorthand is IMO preferable (less clutter), unless you want to leave the possibility of augmenting the case node later - I don't think though it is an option here.

Thanks, Lada