Hi Mahesh,
Thanks for the review. A new version that integrates your comments is available
online:
https://author-tools.ietf.org/iddiff?url1=draft-ietf-opsawg-secure-tacacs-yang-09&url2=draft-ietf-opsawg-secure-tacacs-yang-10&difftype=--html.
As a general note, please note that we are not dealing with the management of
the server, but the client side. We are preserving the same scope as in
RFC9105. This is mentioned in the abstract:
This document defines a Terminal Access Controller Access-Control
System Plus (TACACS+) client YANG module that augments the System
^^^^^^^^^^^^
And introduction:
This document defines a YANG module for managing TACACS+ clients
^^^^^^^^^^^^^^^^^^^^^^^^
(Section 4), including TACACS+ over TLS 1.3 clients
[I-D.ietf-opsawg-tacacs-tls13]. This document obsoletes [RFC9105].
See inline for more context.
Cheers,
Med
> -----Message d'origine-----
> De : Mahesh Jethanandani <[email protected]>
> Envoyé : mercredi 30 avril 2025 04:17
> À : [email protected]
> Cc : opsawg <[email protected]>; [email protected]; Reshad Rahman
> <[email protected]>
> Objet : AD Evaluation of draft-ietf-opsawg-secure-tacacs-yang-09
>
>
> Authors,
>
> Thank you for working on this document. I also want to thank Tina
> Tsou for her OPSDIR and Reshad Rehman for his YANG Doctors review
> of the document. Both reviews have helped improve this document.
>
> Here are some of my comments on the document.
>
> -------------------------------------------------------------------
> COMMENT
> -------------------------------------------------------------------
>
> Section 3, paragraph 3
> > The 'statistics' container under the 'server' list is a
> collection of
> > read-only counters for sent and received messages from a
> configured
> > server.
>
> Are there no client counters that might be useful to document? As
> an example, how about 'cert-errors' for client authentication as an
> example?
[Med] We do already have the following:
+--ro cert-errors? yang:counter64
+--ro rpk-errors? yang:counter64
{tlsc:server-auth-raw-public-key}?
>
> Section 3, paragraph 7
> > * Earlier TLS versions MUST NOT be used.
>
> I know this comment applies equally or more to draft-ietf-opsawg-
> tacacs-tls13, but I thought that draft-ietf-uta-require-tls13
> states that while TLS 1.3 is a MUST, TLS 1.2 MAY be used. I can
> understand that 1.1 MUST NOT be used. Why a TLS version of 1.2
> cannot be used in this case?
>
[Med] We discussed this as part of the base spec and the outcome is to go
straight with TLS1.3. This is also a good design from an ops standpoint as we
don't need to deal with a variety of versions let alone that we leverage recent
enhanced features.
> Section 3, paragraph 15
> > 'hello-params': Controls TLS versions and cipher suites.
>
> This controls TLS versions and cipher suites for the hello
> exchange. Not for the version for the session. Right? Can that be
> made explicit?
[Med] Sure. Added "...to be used when establishing TLS sessions.".
>
> Section 4, paragraph 17
> > description
> > "Number of new connection requests sent to the
> server,
> > e.g., socket open.";
>
> If these are server stats, are these not connection requests
> *received* by the server? Same comment applies for the next few
> pieces of stats.
>
[Med] We are not covering the server part (per the generic note above). This
part is btw inherited from rfc9105.
> Section 4, paragraph 16
> > leaf messages-sent {
> > type yang:counter64;
> > description
> > "Number of messages sent to the server.";
> > }
>
> Again, if these are server stats, is this is the number of messages
> sent *by* the server?
[Med] Idem as above.
>
> Section 4, paragraph 15
> > leaf messages-received {
> > type yang:counter64;
> > description
> > "Number of messages received from the server.";
> > }
>
> Is this is the number of messages received *by* the server?
[Med] Idem as above.
>
> Section 4, paragraph 14
> > leaf errors-received {
> > type yang:counter64;
> > description
> > "Number of error messages received from the
> server.";
> > }
>
> Is this the number of error messages received *by* the server?
>
[Med] Idem as above.
> Section 4, paragraph 13
> > description
> > "Specifies a certificate that can be used for client
> > identity.";
>
> Not clear whether this is certificate used *by* the client for
> client authentication with the server.
>
[Med] The key term is "client identity". This is further contextualized in this
grouping will all client identity variants are called:
grouping client-identity {
description
"Identity credentials that a TLS client may present when
establishing a connection to a TLS server. When configured,
and requested by the TLS server when establishing a TLS
session, these credentials are passed in the Certificate
message.";
I think this is clear for this context.
> Section 4, paragraph 13
> > description
> > "Specifies raw private key (RPK) that can be used for
> > client identity.";
>
> Not clear whether this is private key used *by* the client for
> client authentication.
[Med] Same as previous comment.
>
> Section 4, paragraph 14
> > case certificate {
> > container certificate {
> > description
> > "Specifies the client identity using a
> certificate.";
> > uses certificate;
> > }
>
> If this is the only use of 'certificate' grouping, why not inline
> it here?
[Med] This is to provide a readily available grouping for future augmentations
that would need only need the certificate part.
>
> Section 4, paragraph 13
> > case raw-public-key {
> > if-feature "tlsc:client-ident-raw-public-key";
> > container raw-private-key {
> > description
> > "Specifies the client identity using RPK.";
> > uses raw-private-key;
> > }
> > }
>
> Same comment here regarding inlining.
[Med] Idem as above.
>
> Section 4, paragraph 12
> > case tls13-epsk {
> > if-feature "tlsc:client-ident-tls13-epsk";
> > container tls13-epsk {
> > description
> > "An EPSK is established or provisioned out-of-
> band.";
> > uses tls13-epsk;
> > }
>
> And here.
[Med] Idem as above.
>
> Section 4, paragraph 17
> > list server-credentials {
> > if-feature "credential-reference";
> > key "id";
> > description
> > "Identity credentials that a TLS client may present
> > when establishing a connection to a TLS server.";
>
> Either the naming or the description of this list seems wrong vis-
> a-vis 'client-credentials'. How come both are credentials that the
> client is presenting?
[Med] Updated as follows:
NEW:
"Identity credentials that a TLS client may use
to authenticate a TLS server.";
>
> Section 4, paragraph 16
> > leaf sni-enabled {
> > type boolean;
> > must '../domain-name' {
> > error-message
> > "A domain name must be provided to make use of
> Server
> > Name Indication (SNI).";
> > }
>
> What is the default of this boolean value?
[Med] We don' have any default recommended in the base spec.
>
> Section 4, paragraph 15
> > leaf port {
> > type inet:port-number;
> > description
> > "The port number of TACACS+ server port number.
> > Default port number for legacy TACACS+ is 49,
> > while it is TBD for TACACS+TLS.";
> > }
>
> It would be helpful to expand what "TBD" is. Preferably, along with
> a note to the RFC Editor to update the description statement with
> the port number that is assigned as part of approving this
> document.
>
[Med] We do already have such note:
Please apply the following replacements:
...
* TBD --> the assigned port number in Section 7 of
[I-D.ietf-opsawg-tacacs-tls13]
> Section 4, paragraph 15
> > case obfuscation {
> > leaf shared-secret {
> > type string {
> > length "1..max";
> > }
>
>
> Two comments. draft-ietf-opsawg-tacacs-tls13 makes it clear that
> 'shared-secret' used for obfuscation has been replaced by the
> authentication capabilities of TLS. Why then do we need this
> definition in the YANG model.
[Med] This is for backward compatibility with the installed TACACS+ base.
Even if we need it, do we want to
> model a 'shared-secret' as a string that is visible to anyone who
> has access to read the configuration? I see in Appendix A that the
> 'shared-key' is a visble string:
>
> "ietf-system-tacacs-plus:tacacs-plus": {
> "server": [
> {
> "name": "tac_plus1",
> "server-type": "authentication",
> "address": "192.0.2.2",
> "shared-secret": "QaEfThUkO198010075460923+h3TbE8n",
> "source-ip": "192.0.2.12",
> "timeout": 10
> }
> ]
> }
[Med] This is inherited from rfc9105 with the appropriate guards.
>
> I know NACM default is 'default-deny-all' but even then the key
> should be stored in a way or a format where it should not be
> visible as a string. Something like below?
> list asymmetric-key {
> key name;
> leaf name { type string; }
> leaf private-key {
> nacm:default-deny-all;
> type binary;
> config false; // Not modifiable directly
> }
> }
[Med] I don't think we need to touch this part and leave it as it was in 9105.
This is not recommended and the use of TLS is superior.
>
> DOWNREF [RFC9257] from this Proposed Standard to Informational
> RFC9257. (For IESG discussion. It seems this DOWNREF was not
> mentioned in the Last Call and also seems to not appear in the
> DOWNREF registry.)
>
> -------------------------------------------------------------------
> NIT
> -------------------------------------------------------------------
>
> All comments below are about very minor potential issues that you
> may choose to address in some way - or ignore - as you see fit.
> Some were flagged by automated tools (via
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg
> ithub.com%2Flarseggert%2Fietf-
> reviewtool&data=05%7C02%7Cmohamed.boucadair%40orange.com%7Cd18d55de
> fdf74a9ae83508dd878d0c53%7C90c7a20af34b40bfbc48b9253b6f5d20%7C0%7C0
> %7C638815762050328952%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd
> WUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3
> D%3D%7C0%7C%7C%7C&sdata=GiWJ9giCNpiHd0HdxzEHMF7sdjQfKGMVxu51vJvX3CU
> %3D&reserved=0), so there will likely be some false positives.
> There is no need to let me know what you did with these
> suggestions.
>
> Section 3, paragraph 2
> > The 'server' list, which is directly under the 'tacacs-plus'
> > container, holds a list of TACACS+ servers and uses 'server-
> type' to
> > distinguish between Authentication, Authorization, and
> Accounting
> > (AAA) services. The list of servers is for redundancy.
>
> s/The list of servers/A list of servers rather than a single server
> is suggested to provide redundancy/
>
[Med] The OLD is inherited from rfc9105.
> Section 4, paragraph 7
> > "This module provides configuration of TACACS+ clients.
>
> s/configuration/management/
[Med] Good catch.
>
> Section 4, paragraph 13
> > "Number of TACACS+ 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-closes'. If the Single Connection Mode
> was
> > enabled, a single TCP connection may contain
> multiple
> > TACACS+ sessions.";
>
> s/completed/established/
> s/was not enabled/is not enabled/
> s/was enabled/is enabled/
[Med] I prefer to leave the OLD as this is inherited from RFC9105. For example,
completed is more accurate here than established (see the reference to the
connection closes".
>
> Section 4, paragraph 5
> > also cite RFC SSSS - fixes a must statement under 'tacacs-plus'
> by adding a m
> > ^^^^^^^^^
> The word "statement" is a noun or an adjective. A verb or adverb is
> missing or misspelled here, or maybe a comma is missing.
>
[Med] The OLD is correct.
> Section 4, paragraph 28
> > n "Explicit configuration of a server credentials."; uses server-
> authenticati
> > ^^^^^^^^^^^^^^^^^^^^
> The plural noun "credentials" cannot be used with the article "a".
>
[Med] Changed to "of credentials of a server".
> Thanks
>
> Mahesh Jethanandani
> [email protected]
>
>
____________________________________________________________________________________________________________
Ce message et ses pieces jointes peuvent contenir des informations
confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce
message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages
electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou
falsifie. Merci.
This message and its attachments may contain confidential or privileged
information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete
this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been
modified, changed or falsified.
Thank you.
_______________________________________________
OPSAWG mailing list -- [email protected]
To unsubscribe send an email to [email protected]
