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?
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?
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?
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.
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?
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?
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?
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.
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.
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?
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.
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.
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?
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?
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.
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.
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
}
]
}
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
}
}
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://github.com/larseggert/ietf-reviewtool), 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/
Section 4, paragraph 7
> "This module provides configuration of TACACS+ clients.
s/configuration/management/
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/
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.
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".
Thanks
Mahesh Jethanandani
mjethanand...@gmail.com
_______________________________________________
OPSAWG mailing list -- opsawg@ietf.org
To unsubscribe send an email to opsawg-le...@ietf.org
