Hi Med, Please see inline with [mj].
> On Apr 30, 2025, at 12:10 AM, [email protected] wrote: > > 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 > > <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. [mj] I understand that this is a client module, but it is confusing to have statements like “server-related statistics” or “number of aborted connections to the server”. More on it later in this thread. > > Cheers, > Med > >> -----Message d'origine----- >> De : Mahesh Jethanandani <[email protected] >> <mailto:[email protected]>> >> Envoyé : mercredi 30 avril 2025 04:17 >> À : [email protected] >> <mailto:[email protected]> >> Cc : opsawg <[email protected] <mailto:[email protected]>>; [email protected] >> <mailto:[email protected]>; Reshad Rahman >> <[email protected] <mailto:[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. [mj] The ship might have sailed for changing this now, but calling these “server-related statistics” is confusing if not wrong. They are client statistics. BTW, if this a cumulative count, as it appears from the description, is there a way to know how many connection requests are outstanding at any point in time? > > >> 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. [mj] A boolean is either ’true’ or ‘false’, and the bit is going to reflect one of the values. Given that the must statement is checking for the existence of ‘domain-name’, why not set a default value? > >> >> 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] [mj] Ahh. I missed that. > >> 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. [mj] Can you point me to where the document it says that obfusicaton is NOT RECOMMENDED and that TLS MUST be used? Thanks. > >> >> 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 >> <https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg> >> ithub.com <http://ithub.com/>%2Flarseggert%2Fietf- >> reviewtool&data=05%7C02%7Cmohamed.boucadair%40orange.com >> <http://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] <mailto:[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. Mahesh Jethanandani [email protected]
_______________________________________________ OPSAWG mailing list -- [email protected] To unsubscribe send an email to [email protected]
