The Terminal Access Controller Access-Control System Plus (TACACS+) Protocol
draft-ietf-opsawg-tacacs-18

Thanks for addressing my DISCUSS and COMMENT points; and finding a middle ground in this draft by documenting the as-is, but highlight the issues.

Thanks for the work everyone has put into this document. I have some comments and some nits. I also support most of the other comments issued by the IESG members.

And, I appreciate the time taken to document a protocol that I used in the mid 90's ;-) it took sometime to document it... I also appreciate the use of 'obfuscation' in section 4.7.

== COMMENTS ==
- section 1, I am unsure whether the 'today' in 'It is primarily used today...' still stands in 2019.
- a little late to ask, but, is there any reason why draft-dahm-opsawg-tacacs does not refer to 'The Draft' in the datatracker?
- section 4.8, the flag values are 0x01 and 0x04, but what about the other bits? Should they be considered as 0 and ignored on reception?
- section 5.1, should TAC_PLUS_AUTHEN_SVC_ARAP := 0x04 also be deprecated ?
- section 5.4.2.1 about 'ASCII login' while I understand that years ago it was ASCII only hence the name of the value but the text is unclear whether UTF-8 could be used (assuming that the network devices support this character set)
- section 8.2, the route attribute is defined only for IPv4 while the T+ can send IPv6 addresses to the client. Is it sensible?


== NITS ==
- abstract TACACS is an acronym which should be expanded in the abstract
- section 3 could be updated esp around "character mode front end and then allow the user to telnet"
- section 4.1 add that the port 49 has been allocated by the IANA ?
- section 4.3 talks about flags but the packet format is also presented in section . Not a logical flow
- section 8.1 s/IPV6 address text representation defined/IPv6 address text representation is defined/ (lower case V as well)
- section 8.2, please clarify the inacl value, is it an ACL name or an ASCII representation of the list of ACL entries?

Thanks for addressing my DISCUSS points.

Thank you for addressing most of my DISCUSS/comments. Only one little thing remains:

KRB5 and KRB4 need normative references.

My DISCUSS moved to Alexey's ballot. My original comment is below:

I agree with Deborah's comment, and would further suggest that the lack of modern security mechanisms in this protocol needs to be called out in the introduction, with a reference to Section 10.

Please respond to the Gen-ART review, which makes several suggestions for needed clarifications.

In Section 2, please use the RFC 8174 boilerplate.

[Procedure Nit -- This is a comments for the Shepherd/Chairs/ADs]

It would be useful to tag this document (or draft-dahm-opsawg-tacacs) as Replacing "the draft".  

It seems clear to me from the explanation in the Introduction that tagging the documents that way is the appropriate thing to do.  I didn't do it myself in case the Shepherd/Chairs/AD know of a reason not to do it.

Thanks for posting version -15.  It resolves my DISCUSS issues with respect to character encoding, though it adds a couple of oddities -- not at the level of DISCUSS, but worthy of comment, which I ask that you consider:

1. Three times in the new Section 3.7 you refer to "byte arrays":

   original draft intended that these strings would be treated as byte
   arrays where each byte would represent a US-ASCII character.

   Where specifically mentioned, data fields contain arrays of arbitrary
   bytes as required for protocol processing.

   All other text fields in TACACS+ MUST be treated as printable byte
   arrays of US-ASCII

I'm sort of OK with the use of "arrays of arbitrary bytes" in the second case, as you're not talking about character strings, though we usually reserve "array" to refer to a particular data structure with a particular mechanism of access.  But I won't quibble about that.  In the other two cases, it seems an odd distraction, and I wish you would use "strings" instead of "arrays".

2. In many places in the document you have added a space after ")" or "]".  For example, "For IPv6 address text representation defined please see RFC 5952 [RFC5952] ." and "For more details please refer to security section (Section 10) ."  Why did you do that?  The RFC Editor will only have to take them back out, so I recommend that you take them out now, instead.

Again, thanks for the update and for addressing my DISCUSS.

While the status is an Informational document, not PS, I still would
prefer if earlier in the document it provided the reader with deployment
concerns e.g. the introduction.

The current introduction states a "wide range" of clients and servers
are already deployed. Not until section 10 is the reader informed
"Multiple implementations of the protocol described in the original
TACACS+ Draft `The Draft' [TheDraft] have been deployed.  As the
protocol was never standardized, current implementations may be
incompatible in non-obvious ways". And section 10.5 on Best
Practices which has the new restriction that it not be deployed
with other traffic. This information is needed much
earlier in the document to give context for the reader.

A couple of comments/question:

1) I would like to see it more explicitly mentioned in the title, abstract, and introduction that this is only documenting the protocol as deployed today. Usually we use titles like “Company X’s TACACS+ protocol”; this is probably not applicable here but maybe something like “Documentation of the TACACS+ Protocol” would work as well…?

2) If Single Connection Mode is used and the connection is idle for a while, it can be possible that some middlebox or the server lost state and the connection is actually not available anymore. I guess it could be good to explicit mention this case and say something like if a RST or timeout is encountered for a connection in Single Connection Mode, the client should try to open a new connection and resend the request immediately.

3) I would recommend to define the term “session” in section 3 (as it seems to a central term that is important to understand correctly).

4) Sec 10.5.1: “TACACS+ servers MUST NOT leak sensitive data.”
Not sure if that is an actionable requirement, as I would assume leaking is often done by accident. 
Maybe “TACACS+ servers MUST store sensitive data securely.”… or something…? Not sure  how much better that is…
Actually the next sentence could probably be normative:
S/TACACS+ servers should not expose shared secrets in logs./TACACS+ servers MUST NOT expose shared secrets in logs./

5) One question regarding the unencrypted/non-obfuscated mode: Why was this mode (and TAC_PLUS_UNENCRYPTED_FLAG=1) not deprecated completely? You mention briefly somewhere that this is/was used for debugging but then later say that modern tools should also support debugging of obfuscated traffic.

6) Sec 10.5.5: “TACACS+ servers SHOULD deprecate the redirection mechanism.”
I believe you want to use a MUST here because you anyway only specify this for servers that follow or update to this spec; it will of course not change existing implementations…

* Section 8.1.

"IPV6 address text representation defined in RFC 4291 [RFC4291]"

I would much prefer using RFC5952 here as it tightens rules a bit over RFC4291 and cuts down the flexibility to make text comparisons easier. 

"Stardate is canonically inconsistent and so SHOULD NOT be used."

as in "Acting Captain's log, Stardate 2258.42. We have had no word from Captain Pike..."? I agree that it is canonically inconsistent but this will be very confusing for non-Trekkies. Is this really needed here?