Hi Mahesh, Please see inline.
Cheers, Med De : Mahesh Jethanandani <[email protected]> Envoyé : vendredi 2 mai 2025 01:01 À : BOUCADAIR Mohamed INNOV/NET <[email protected]> Cc : [email protected]; opsawg <[email protected]>; [email protected]; Reshad Rahman <[email protected]> Objet : Re: AD Evaluation of draft-ietf-opsawg-secure-tacacs-yang-09 Hi Med, Please see inline with [mj]. On Apr 30, 2025, at 12:10 AM, [email protected]<mailto:[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. 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? [Med] Relying on TCP for tracking active connections would be sufficient here (likely, one connection if SCM is in used. Please refer to https://datatracker.ietf.org/doc/html/rfc8907#name-connection. 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? [Med] It is true that SNI support is mandatory and that a name is mandatory when SNI is in use (hence the check in YANG), but making use of SNI is really deployment-specific. 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? [Med] RFC8907 (which was used as base for RFC) already says: With respect to the observations about the security issues described above, a network administrator MUST NOT rely on the obfuscation of the TACACS+ protocol. RFC9105 says (and inherited in this draft): Note that this security mechanism is best described as 'obfuscation' and not 'encryption' as it does not provide any meaningful integrity, privacy, or replay protection."; I added an explicit mention to make this clear: https://github.com/boucadair/secure-tacacs-yang/commit/9e05571a8bb5804226475523904fe654ed4fc1e4. Thanks. ____________________________________________________________________________________________________________ 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]
