Re: [OPSAWG] draft-ietf-opsawg-tacacs-06 Contributions, Status and Plans

["Douglas Gash (dcmgash)" <[e-mail]>]

Hi,

First batch of responses. This is in HTML encoding with intent to make responses easier to see due to their colour coding. If this does not work, please LMK, I will send a different format.

Many items are marked with just [Agree], if it seems there is a trivial way to adjust according to the comment.

The final batch of initial responses will be sent in a few days. All may prefer to wait for final batch.

The security section will be dealt with on a separate thread.

Regards, Doug.

------

3.3.  Single Connect Mode


   The packet header (see below) contains a flag to allow sessions to be
   multiplexed on a connection.

* it would be good to name the flag here:
[Agreed. New Version:
"Single Connection Mode is intended to improve performance by allowing
   a client to multiplex multiple session on a single TCP connection.

   The packet header contains the TAC_PLUS_SINGLE_CONNECT_FLAG used by
   the client and server to negotiate the use of Single Connect Mode."]

   If the server sets this flag in the first reply packet in response to
   the first packet from a client, this indicates its willingness to
   support single-connection over the current connection.  The server
   may set this flag even if the client does not set it, but the client
   is under no obligation to honor it.

* what happens if the client does not honour the flag?
[It will close the connection after the session completes. Update:
   "The server may set this flag even if the client does not
   set it, but the client may ignore the flag and close the connection
   after the session completes."]

   The flag is only relevant for the first two packets on a connection,
   to allow the client and server to establish single connection mode.
   The flag MUST be ignored after these two packets since the single-
   connect status of a connection, once established, must not be
   changed.  The connection must instead be closed and a new connection
   opened, if required.

  Why would it be required to change the mode?  It would be good to
explain the use-cases, and why (or not) they make sense.
[We mandate it must not happen. To be honest, I’m not aware of any use case for wanting to change]

   TAC_PLUS_UNENCRYPTED_FLAG := 0x01

   If this flag is set, then body encryption is not used.  If this flag
   is cleared, the packet is encrypted.  Unencrypted packets are
   intended for testing, and are not recommended for normal use.

* it would be good to have a pointer to the security considerations, with
a discussion of the implications.
[Agreed]

*  Also, it's odd that a completely insecure mode is just "not recommended
for normal use".  I would suggest more panic-inducing phrasing.  e.g.
."Unencrypted packets offer a complete compromise of all security related
to the protocol and MUST NOT be used outside of a testing environment"
[Partial Agree: Will change to MUST NOT, but do not want to give impression that MD5 is secure.]

   TAC_PLUS_SINGLE_CONNECT_FLAG := 0x04

   This flag is ussequernt, ed to allow a client and server to agree
whether
   multiple sessions may be multiplexed onto a single connection.

* I find the "agree" phrasing odd.  I've mentioned before that the tone of
the document is often philosophical.  Perhaps saying instead "this flag
signals whether the client and server are capable of performing session
multiplexing"
[Agree. Changed to "This flag is used to allow a client and server to negotiate Single
   Connection Mode."]

* i.e. the flag is a signal, not a capability for the systems to agree.
[Agree]

   session_id

   The Id for this TACACS+ session.  The session id is to be selected
   randomly.  This field does not change for the duration of the TACACS+
   session.  (If this value is not a cryptographically strong random
   number, it will compromise the protocol's security, see  RFC 1750

* it would be good to update the paragraph to be more prescriptive.  i.e.
[Agree, changed text to: "The Id for this TACACS+ session.  This field does not change for the
   duration of the TACACS+ session.  This number MUST be generated by a
   cryptographically strong random number generation method.  Failure to
   do so will compromise security of the session.  For more details
   refer to RFC 1750 [RFC1750]"]

   The Id for this TACACS+ session.  The session id MUST be taken
   fro a cryptographically strong random number generation.  If not,
   the protocol's security will be compromised.  See  RFC 1750
   for further information

...

   length

   The total length of the packet body (not including the header).  This
   value is in network byte order.  Packets are never padded beyond this
   length.

* Why is the text on "padding" there?  The TACACS+ packets are sent over
TCP, not UDP.  So any "padding" would be outside of the scope of the
current packet, and be seen by the other end as a subsequent, but invalid,
TACACS+ packet.
[Agree: Removed part about padding]

3.5 ...

      - Unused fixed length fields SHOULD have values of zero.

* what happens when they don't?  I suggest adding text saying that the
fields are ignored.
[Agree, changed text to: "To signal that any variable length data fields are unused, their
      length value is set to zero." As to what happens when fields are unused, this will depend upon the fields]

     - There will be no padding in any of the fields or at the end of a
      packet.

* I wonder again what "padding" means here.
[Agree, removed]

3.6.  Encryption


   The body of packets may be encrypted.   ...

* Which is not a standard encryption method.  Security people will
complain here.  I suggest (as before) using the term "obfuscated".  With a
note that for historical reasons, the flags / fields / etc. refer to
TACACS+ "encryption".
[Agreed, changed generally to obfuscation, apart from where field names are directly referenced]

...  This document does not discuss the management and storage of
   those keys.  ...

* there should be some discussion in the Security Considerations section
about this.  Otherwise, this subject will be brought up in the security
review.
[Will be happy to stand corrected, but that strikes me as both outside the documentation of the protocol, and likely to lock in practices that will become out-of-date.]

  ...  It is an implementation detail of the server and client,
   as to whether they will maintain only one key, or a different key for
   each client or server with which they communicate.  For security
   reasons, the latter options MUST be available, but it is a site
   dependent decision as to whether the use of separate keys is
   appropriate.

* those sentences seem to disagree with each other.  It's an
"implementation detail", but it MUST be supported.  I suggest requiring
the capability of per-server and per-client keys.
[Agree, the requirements must be supported by implementations, however we leave it for the implementor as to whether this is used]

  ...   The session id is used in network byte order.

* nit: if the session ID is a 32-bit opaque token, it's not in any byte
order.  Maybe just "as taken from the packet header" ?
[Agree]

   When a server detects that the secret(s) it has configured for the
   device mismatch, it MUST return ERROR.  The handling of the TCP
   connection by the server is implementation independent.

* I repeat my objection here.  There is just no reason for the TCP
connection to remain open after the connection has been determined to be
fraudulent.  This subject also came up in the RADIUS over TCP drafts.  The
consensus there was the same: if it's not a real / known / good client,
there is every reason to close the connection, and zero reasons to keep it
open.
[see next]

   After a packet body is decrypted, the lengths of the component values
   in the packet are summed.  If the sum is not identical to the
   cleartext datalength value from the header, the packet MUST be
   discarded, and an error signalled.  The underlying TCP connection MAY
   also be closed, if it is not being used for other sessions in single-
   connect mode.

* This last sentence is confusing.  Are there different secrets for
different packets in one TCP stream?  If so, nothing in the document says
so.  If not, then there is every reason to believe that the secret is
mismatched. And, the *next* packet will decrypt incorrectly.  Which means
(again) you might as well just close the TCP connection.
[though we would not want to restrict to a single secret on server for a client, we can restrict the clint that it should us just a single secret on a client at a time for a connection to a server. This would mean that we could not have single connect, multi session interaction from a client with multiple secrets so then we could close the connection]

   If an error must be declared but the type of the incoming packet
   cannot be determined, a packet with the identical cleartext header
   but with a sequence number incremented by one and the length set to
   zero MUST be returned to indicate an error.

* this mix of clear-text and encrypted signalling opens up the protocol to
an attacker.  Since this "NAK'" is not protected, an attacker can forge
the NAK at any time, and force the connection to be closed.
[Agree, this is probably one of the ideas that is supported by the old spec that we can drop/ The more that we can clean it out, the better. Will add to the list of deprecated features.]

* this attack (along with all others) should be noted in the Security
Considerations section.


...
   Packet fields are as follows:

   action

   This describes the authentication action to be performed.  Legal
   values are:

* what happens if a client/server receives values outside of the legal
range?  I suggest sending ERROR, and closing the TCP connection.
[Agreed, though we have a section "Treatment of Enumerated Protocol Values", so that we don't need to repeat it for each item]

* similar comments apply for other fields with "legal values".  While it's
good to define what happens when things go as planned, it's also good to
define what happens when things *don't* go according to plan.
[Agreed, the section "Treatment of Enumerated Protocol Values" should mop this all up.]

* review of 4.2 and subsequent sections will follow in a later message.

  Summary: the spec is still fairly descriptive / philosophical instead of
prescriptive.  Many optional parts of the protocol are described, "A and B
are allowed", but there is often no description of what to *do* when
either A or B is present.  I've suggested descriptive text, or places
where descriptive text can be added.
[Agreed, in general, I believe we can document the positive cases, and to stipulate in one place what occurs if values are not present in case of server, or client]

The larger worry here is what happens if the descriptive text conflicts
with existing implementations?  This is a topic which should be discussed
in the "Security Considerations" section.

  i.e. the spec documents the existing protocol, with recommendations for
security and inter-operability.  As the protocol has historically been
poorly specified, implementations are likely to follow the broad
requirements of this specification.  They are also likely to not follow
many of the newly-clarified behaviours / requirements.  The hope is that
this specification can guide implementors towards a common understanding
of the protocol.

  Also, vague requirements and open options in a protocol spec are ripe
for implementation bugs and security issues.  That worry drives many of my
comments.


-----

4.2 ...

server_msg, server_msg_len

   c A message to be displayed to the user.  This field is optional.  If
   it exists, it is intended to be presented to the user.  US-ASCII
   charset MUST be used.  The server_msg_len MUST indicate the length of
   the server_msg field, in bytes.

* The last sentence is atypical of RFC 2119 language.  Then language is
typically about system behaviour or inter-field comparisons.  I'm not sure
what it means that a field MUST indicate a length.

* i.e. the "server_msg_len" field is *defined* to encode the length of the
"server_msg" field.  No "MUST" is necessary.
[Agree, will remove MUST].

* similar comments apply to many of the other "foo_len" fields, elsewhere
in the document
[Will apply universally]

rem_addr, rem_addr_len

   An US-ASCII string this is a "best effort" description of the remote
   location from which the user has connected to the client.

* perhaps instead of "best effort", just re-use language from the "port"
description.  The contents of this field are the clients description of
the remote location...
[Agree: best effort removed. Proposed New Text: "   An US-ASCII string indicating the remote location from which the user
   has connected to the client.  It is intended to hold a network
   address if the user is connected via a network, a caller ID is the
   user is connected via ISDN or a POTS, or any other remote location
   information that is available.  This field is optional (since the
   information may not be available).  The rem_addr_len indicates the
   length of the user field, in bytes."]

4.4 ...


   When the REPLY status equals TAC_PLUS_AUTHEN_STATUS_GETDATA,
   TAC_PLUS_AUTHEN_STATUS_GETUSER or TAC_PLUS_AUTHEN_STATUS_GETPASS,
   then authentication continues and the SHOULD provide server_msg

* editorial: the WHAT should provide...
[Agreed. Answer is of course: "server"]

   ... quest for more information to flexibly support future
   requirements.  If the TAC_PLUS_REPLY_FLAG_NOECHO flag is set in the
   REPLY, then the user response must not be echoed as it is entered.

* the implicit assumption here is that there is a person who is entering
text on a terminal.  It would be good to have a bit more explanation of
this expected use-case.
[Agreed. T+ is used in many and varied use case, "echoed" is too restrictive, and should be replaced by something like "disclosed in a user interface"]

  NOTE: Only those requests which have changed from their minor_version
   0 implementation (i.e.  CHAP, MS-CHAP and PAP authentications) will
   use the new minor_version number of 1.  All other requests (i.e.  all
   authorisation and accounting and ASCII authentication) MUST continue
   to use the same minor_version number of 0.

* that's an odd phrasing of the requirement.  Perhaps something more
prescriptive would be useful.  e.g.:

  NOTE: Authentication requests which use ASCII login or chpass MUST use
minor version 0.  Authentication requests using PAP, CHAP, or MS-CHAP MUST
  use minor version 1.  All authorization and accounting packets MUST use
minor version 0.
[Agreed: will revamp this section].

4.4.2...

   This section describes common authentication flows.  If the options
   are implemented, they MUST follow the description.  If the server
   does not implement an option, it will respond with
   TAC_PLUS_AUTHEN_STATUS_ERROR.

* It's not typically required to say that implementations MUST follow the
spec.
[Agreed]

* also, perspective terminology is better than descriptive. Not "will do",
but "MUST do".
[Agreed]

*  I suggest re-phrasing:

  This section describes common authentication flows.  If the server
   does not implement an option, it MUST respond with
   TAC_PLUS_AUTHEN_STATUS_ERROR.

   This is a standard ASCII authentication.  The START packet may
   contain the username, but need not do so.

* what happens when the username is / is-not included?  i.e. what do
implementations *do*?
[Agreed to expand. the server can of course, finish the session at any time, as detailed in the "session termination" section. But let's assume that we want the server to carry on the process, then it will retrieve the username as described in the following proposed text: "The START packet MAY
   contain the username.  If the user does not include the username then
   the server MAY obtain it from the client with a CONTINUE
   TAC_PLUS_AUTHEN_STATUS_GETUSER.  When the server has the username, it
   will obtain the password using a continue with
   TAC_PLUS_AUTHEN_STATUS_GETPASS.  ASCII login uses the user_msg field
   for both the username and password.  The data fields in both the
   START and CONTINUE packets are not used for ASCII logins, any content
   MUST be ignored.  The session is composed of a single START followed
   by zero or more pairs of REPLYs and CONTINUEs, followed by a final
   REPLY indicating PASS, FAIL or ERROR."]

* if this is a valid protocol flow, then it should be described.  e.g.
"Any username in the START packet is ignored", or "any username in the
START packet MUST match the username in subsequent packets"
[Normally, the username is in just one packet]

   ... The data fields in both
   the START and CONTINUE packets are not used for ASCII logins.

* what happens if there is data in the fields?  Is it ignored?
[Agreed From perspective of protocol, yes]

* RFC 2119 language is recommended here.  "any data (if present) MUST be
ignored", or "the data field MUST NOT be present"

* I have no idea how to make these requirements compatible with existing
implementations.  Sorry...
[I'm not sure though, that it is relevant to the protocol specification.]

  CHAP:

   The length of the challenge value can be determined from the length
   of the data field minus the length of the id (always 1 octet) and the
   length of the response field (always 16 octets).

* it would be good to recommend a minimum length for challenge.  1 octet
is clearly insufficient, but is (silently) allowed by the spec.  I suggest
at least 8 octets/
[See next response]

* also, the challenge MUST be derived from a cryptographically strong
pseudo-random number generator, and MUST change on every CHAP request.
Otherwise, the spec allows (also silently) a fixes challenge.
[This is interesting because, of course, the challenge is not generated for T+ but for a different interaction. the result of that other interaction is passed along to T+. These recommendations are good, but can we actually police this data?]

   To perform the authentication, the server will run MD5 over the id,
   the user's secret and the challenge, as defined in the PPP
   Authentication RFC 1334 and then compare that value
   with the response.

* again "server WILL run".  As opposed to "calculates..."
[Agreed]

MSCHAPv1 / MSCHAPv2:

   The length of the challenge value can be determined from the length
   of the data field minus the length of the id (always 1 octet) and the
   length of the response field (always 49 octets).

*  RFC 2433 says that the challenge is 8 octets.  RFC 2759 says that the
challenge is 16 octets.  It would be good to re-iterate that here.

* i.e. smaller challenges are insecure, and should be forbidden
[again, as T+ interaction is not involved with the generation of the challenges, is this something we can actually meaningfully enforce?]

   ... To perform the authentication, the server will use a the algorithm
   specified  RFC2759

* editorial: "specified IN RFC ..."
[agreed]

... ASCII change password request

* I suspect that the security area review will recommend that this be
removed.  Even RADIUS had "change password" removed at the behest of the
security people.
[Noted. I take it this not a request yet to remove though?]

4.4.3.  Aborting an Authentication Session


   The client may prematurely terminate a session by setting the
   TAC_PLUS_CONTINUE_FLAG_ABORT flag in the CONTINUE message.  If this
   flag is set, the data portion of the message may contain an ASCII
   message explaining the reason for the abort.

* again, what happens if the field is / is-not present?
[Agree, We will have a lack of information regarding the reason for the abort. this seems an obvious impact, but we can add]

* what does the server do with this message?  Should it be logged
somewhere?
[I would say, this is out of scope of the protocol, and a matter for implementation]

... The session is
   terminated and no REPLY message is sent.

* what happens to the TCP connection?  Does it remain open?  It may be
good to say "The session is terminated, and the session Id is no longer
valid.  However, other sessions on the same TCP connection may continue..."
[Agreed]

   The host is specified as either a fully qualified domain name, or an
   ASCII numeric IPV4 address specified as octets separated by dots
   ('.'), or IPV6 address test representation defined in  RFC 4291.

* editorial ".. text .. "representation, not "test"
[Agreed]

   If a key is supplied, the client MAY use the key in order to
   authenticate to that host.  The client may use a preconfigured key
   for the host if it has one.

* so the server is pushing keys to clients?  The security implications of
this should be discussed
[SIDE Note: I think this part of the original draft spec is really worth culling. I certainly specify it should not be implemented in our server. I had intended to propose that is done in the next version when we add TLS, however, am considering proposing to do this sooner unless anyone objects.]

* what happens if the client gets pushed a key, and then also has a
pre-configured key?  Which one takes precedence?  Why?
[... this is the reason why i want to cull this section

Lets drill into these only if we decide this section is worth keeping.]

  Continuing with 4.4.3

     While the order of hosts in this packet indicates a preference, but
     the client is not obliged to use that ordering.

* the use of "obliged" is unusual here.  Perhaps "the order used by the
client is implementation specific, and does not have to follow the order
sent by the server"

   If the status equals TAC_PLUS_AUTHEN_STATUS_RESTART, then the
   authentication sequence is restarted with a new START packet from the
   client. ...

* nit: it may be good to say that the next session does not have to use
the current TCP connection

* "restart" may also be ambiguous.  Does the session ID change for the
next session?  It may be good to say instead "If the status equals
TAC_PLUS_AUTHEN_STATUS_RESTART, then the client re-authenticates with a
new session, beginning with a new START packet"

...   This REPLY packet indicates that the current authen_type
   value (as specified in the START packet) is not acceptable for this
   session, but that others may be.

* Which others "may" be acceptable?  And why "acceptable for this
session"?   is the client free to re-try the same authentication type in a
different session?  i.e. what does the client *do*?

* perhaps say "the REPLY indicates a negative acknowledgement that the
current authen_type is not accepted by the server.  The client SHOULD
start a new session with a different authen_type"

   If a client chooses not to accept the TAC_PLUS_AUTHEN_STATUS_RESTART
   packet, then it is TREATED as if the status was
   TAC_PLUS_AUTHEN_STATUS_FAIL.

* why uppercase "TREATED" ?

* why "chooses not to accept" ?  again, prescriptive is better than
philosophical

* Perhaps "If the client does not start a new session after receiving a
TAC_PLUS_AUTHEN_STATUS_RESTART, it MUST behave as if it received a
TAC_PLUS_AUTHEN_STATUS_FAIL."
[.... end of the section that should probably be culled]

5.  Authorization
...

   The authorization REQUEST message contains a fixed set of fields that
   indicate how the user was authenticated or processed

* nit: remove "or processed"
[Agreed]

* from a security point of view, it's unusual for a client to indicate to
a server how a user was authenticated.  This allows the client to claim
authentication when none existed.
[Areed, and server can accord appropriate confidence in this information with that proviso]

...
     This field matches the port field in the authentication section
    (Section 4) above

* what is meant by "matches"?  Perhaps "The definition of this field is
the same as in Section 4".  Or does the field in the authorization session
have to contain the same data as the similar field from the authentication
session?
[Agreed, the first meaning]

* similar comments apply for other uses of "matches"

   It is not legal for an attribute name to contain either of the
   separators.  It is legal for attribute values to contain the
   separators.

* what are the legal characters for an attribute name?  The previous
paragraph says US-ASCII.  So is "($#@!" a legal attribute name?
[Agreed. Yes, that is a valid name, because it does not include = or *. the text is explicit though, do you think it needs further clarification?]

* a reference to Section 7 would be useful here.
[Agreed]

   Optional arguments are ones that may be disregarded by either client
   or server.  Mandatory arguments require that the receiving side
   understands the attribute and will act on it.  If the client receives
   a mandatory argument that it cannot oblige or does not understand, it
   MUST consider the authorization to have failed.

* "oblige" is an unusual word to use here.   Perhaps "If the client
receives a mandatory argument it cannot implement, it MUST consider the
authorization to have failed"
[Agreed, we will replace "obliged" and clean up]

5.2 The Authorization RESPONSE Packet Body
...
   server_msg, server_msg_len

   This is an US-ASCII string that may be presented to the user.  The
   decision to present this message is client specific.

* nit: perhaps "This field is a message from the server to the client.
The client MAY display it to the user"

* saying "decision is client specific" is a long way of saying "the client
MAY do it", but doesn't always have to
[it was shorter :) but less consistent. Agreed]

...

   The attribute-value pair that describes the authorization to be
   performed.  (see below),

* That's not clear to me.  Perhaps "if present, the list of additional
authorizations permitted the user"
[will clarify]

    If the status equals TAC_PLUS_AUTHOR_STATUS_FAIL, then the
   appropriate action is to deny the user action.

* again, a bit more philosophical than prescriptive.  Perhaps "If the
status equals TAC_PLUS_AUTHOR_STATUS_FAIL, then the requested action MUST
be denied"
[Agreed]

   If the status equals TAC_PLUS_AUTHOR_STATUS_PASS_ADD, then the
   arguments specified in the request are authorized and the arguments
   in the response are to be used IN ADDITION to those arguments.

* uppercase "IN ADDITION" is unusual.  Perhaps:

   If the status equals TAC_PLUS_AUTHOR_STATUS_PASS_ADD, then the
   arguments specified in the request are authorized and the client MUST
   also enforce the authorization attribute-value pairs (if any) in the
response.

* similar comments apply here:

   If the status equals TAC_PLUS_AUTHOR_STATUS_PASS_REPL then the
   arguments in the request are to be completely replaced by the
   arguments in the response.

* perhaps

   If the status equals TAC_PLUS_AUTHOR_STATUS_PASS_REPL then the
   client MUST use the authorization attribute-value pairs (if any) in the
response,
   instead of any authorization attribute-value pairs from the request.

...

    If the intended action is to approve the authorization with no
   modifications, then the status is set to
   TAC_PLUS_AUTHOR_STATUS_PASS_ADD and the arg_cnt is set to 0.

* should be prescriptive.  Perhaps:

   Servers can signal to client that the authorization is approved with no
    modifications by setting the status to
   TAC_PLUS_AUTHOR_STATUS_PASS_ADD and the arg_cnt is set to 0.

...

   A status of TAC_PLUS_AUTHOR_STATUS_ERROR indicates an error occurred
   on the server.

* what does the client do then?  Perhaps "the client MAY show an error to
the user (how???), and MUST treat the response as
TAC_PLUS_AUTHOR_STATUS_FAIL"

* Also, the last sentence of the subsequent paragraph also talks about
ERROR, and seems misplaced.

6.1.  The Account REQUEST Packet Body

   TACACS+ accounting is very similar to authorization.  The packet
   format is also similar.

* nit: that seems redundant.  All of the packets use a common format.
[The header is more consistent than the body rather than the body, is that what you mean?]

  ...   There is a fixed portion and an extensible
   portion.  The extensible portion uses all the same attribute-value
   pairs that authorization uses, and adds several more.

* except that the authorization section didn't list any attribute-value
pairs.  So it's hard to say that those (non-existent) pairs are re-used
here
[All the attributes are in section 7. 1 for author, 7.2 for accounting]

   All other fields are defined in the authorization and authentication
   sections above and have the same semantics.

* Can an accounting packet indicate an authen_type and authen_service?  If
so, what does that mean?
[yes, indicating circumstances (if known) on the device]

6.2.  The Accounting REPLY Packet Body

   The response to an accounting message is used to indicate that the
   accounting function on the server has completed.

* what does that mean?  Do servers always implement "functions"?  As
background, RFC 2866 (RADIUS accounting) says:

...
     Upon receipt of an Accounting-Request, the server MUST transmit an
      Accounting-Response reply if it successfully records the
      accounting packet, and MUST NOT transmit any reply if it fails to
      record the accounting packet.
...

* perhaps similar text could be used here.
[agreed. Text is: "  The purpose of accounting is to record the action that has occurred
   on the client.  The server MUST reply with success only when the
   accounting request has been recorded.  If the server did not record
   the accounting request then it MUST reply with ERROR."]

   ... The server will
   reply with success only when the record has been committed to the
   required level of security,

* what is a "required level of security"?  Is that indicated in the
packet?  Elsewhere?


* my recommendation is just to delete that...

   ... relieving the burden on the client from
   ensuring any better form of accounting is required.

* I find that statement awkward.  Perhaps re-using / re-phrasing text from
RFC 2866 would be good here.
[Removed this text, relying on the text in the previous response]

6.2.  The Accounting REPLY Packet Body

* there's more duplication definition of fields here.  It may be useful to
just say "server_msg has the same meaning and behaviour as defined in
Section 4.2"

  ...    The server MUST terminate the session after sending a REPLY.

* this is the only reference to a session being terminated in the
document.  What does it mean?  Perhaps that the next REQUEST packet has to
have a different session ID?  If so, it would be good to say that in the
REQUEST section.
[Propose add a whole section, earlier in the doc, on session completion: "3.4 Session Completion

   The REPLY packets defined for the packets types in the sections below
   (Authentication, Authorization and Accounting) contain a status
   field.  The complete set of options for this field depend upon the

   packet type, but all three REPLY packet types define values
   representing PASS, ERROR and FAIL, which indicate the last packet of
   a regular session (one which is not aborted).

   The server responds with a PASS or a FAIL to indicate that the
   processing of the request completed and the client can apply the
   result (PASS or FAIL) to control the execution of the action which
   prompted the request to be sent to the server.

   The server responds with an ERROR to indicate that the processing of
   the request did not complete.  The client can not apply the result
   and it MUST behave as if the server could not be connected to.  For
   example, the client try alternative methods, if they are available,
   such as sending the request to a backup server, or using local
   configuration to determine whether the action which prompted the
   request should be executed.

   Refer to the section (Section 4.4.3) on Aborting Authentication
   Sessions for details on handling additional status options .

   When the session is complete, then the TCP connection should be
   handled as follows, according to whether Single Connect Mode was
   negotiated:

   If Single Connection Mode was not negotiated, then the connection
   should be closed

   If Single Connection Mode was enabled, then the connection SHOULD be
   left open (see section (Section 3.3) ), but may still be closed after
   a timeout period to preserve deployment resources

   If Single Connection Mode was enabled, but an ERROR occurred due to
   connection issues (such as an incorrect secret, see section
   (Section 3.7) ), then any further new sessions MUST NOT be accepted
   on the connection.  If there are any sessions that have already been
   established then they MAY be completed.  Once all active sessions are
   completed then the connection MUST be closed."]

   The TAC_PLUS_ACCT_FLAG_START flag indicates that this is a start
   accounting message.  Start messages will only be sent once when a
   task is started.  The TAC_PLUS_ACCT_FLAG_STOP indicates that this is
   a stop record and that the task has terminated.  The
   TAC_PLUS_ACCT_FLAG_WATCHDOG flag means that this is an update record.
   Update records are sent at the client's discretion when the task is
   still running.

* what's a "task" ?  This is the first reference in the document to a
"task".  Perhaps more explanation here as to the expected use-case
[Added text: "TACACS+ accounting is intended to record various types of events on clients, for example: login sessions, command entry, and others as required by the client implementation.  These events are collectively referred to in `The Draft' [TheDraft] as "tasks"."]

   The START and STOP flags are mutually exclusive.  When the WATCHDOG
   flag is set along with the START flag, it indicates that the update
   record is a duplicate of the original START record.

* why would an update duplicate the original START record?  This seems
redundant.
[Well, here we are constrained by a clear definition form original draft.]

  ... If the START
   flag is not set, then this indicates a minimal record indicating only
   that task is still running.

* nit: perhaps delete "a minimal record indicating only".
[Agreed]

* Some more questions...

* does the client retry an accounting packet if it doesn't get a response
from the server?
[client will get PASS or FAIL. do you mean, if the client gets a force disconnect for the TCP connection?]

* how often do the updates get sent?  Saying "implementation defined" is
OK, but some recommendations would be good.  i.e. not more than 100x per
second, and not less than 1 day apart (as extreme examples)
[so lets stick with implementation defined, the range of T+ devices we see is so diverse and use cases are such that I would not like to constrain.]

* I have similar questions about the TCP connection.  What happens if a
client can't connect to a server?  How often should it retry?  Should it
treat "unable to connect" as "authentication / authorization fail"?
Should the user be forbidden *all* access if the client cannot connect to
the server?   Should the user be given minimal access in order to be able
to diagnose server connection problems?
[I would say that the normal approach is to treat a failure to connect as an ERROR condition, so that any support failover or equivalent to method-list can be followed. Frequently a device will have a local option at the end of a method list, and this local method would contain the options for worst case access, but I would say that this behaviour is out-of-scope of the protocol description.]

7.  Attribute-Value Pairs

   It is recommended that hosts be specified as a numeric address so as
   to avoid any ambiguities.

* Is this IPv6 safe?  I suspect not... but it should be stated
[Agreed. Proposed new Text: “It is recommended that hosts be specified as a IP address so as to avoid any ambiguities.  ASCII numeric IPV4 address are specified as octets separated by dots ('.'), IPV6 address text representation in RFC 4291.”]

   Absolute times are specified in seconds since the epoch, 12:00am Jan
   1 1970.  The timezone MUST be UTC unless a timezone attribute is
   specified.

* nit: perhaps "a decimal number representing seconds since..." except
what about time zones?
[Agreed. Proposed new Text: “Absolute times are specified in seconds since the epoch, 12:00am Jan 1 1970.  The timezone MUST be UTC unless a timezone attribute is specified.”]

* how is a time zone specified?  what format is the time?  Examples would
help here.
[Agreed. Will ]

* nit: the section defines numbers, booleans, hosts, and time.  But no
"text" type.  RADIUS is just in the process of fixing this (20+ years
later...), which is why I noticed
[There is some description of text encoding as a whole (see below), but can add something specific for attributes “3.6.  Text Encoding All text fields in TACACS+ MUST be US-ASCII, excepting special consideration given to user field and data fields used for passwords.]

* adding a data type to each attribute definition would be useful
[Agreed]

* also, it would be good to say which attributes are mandatory, and which
are optional
[Agreed. The list of mandatory attributes is very small, and there are some dependencies]

   Attributes may be submitted with no value, in which case they consist
   of the name and the mandatory or optional separator.

* this sounds like the separator is optional.  Perhaps "... consist of the
name and separator only"
[Agreed, will clean to remove this dependency]

7.1.  Authorization Attributes

   ...
   a protocol that is a subset of a service.  An example would be any
   PPP NCP.

* nit: expand acronyms on first use.  What's a "PPP NCP" ?
[Agreed, will expand]

   cmd

   a shell (exec) command.  This indicates the command name for a shell
   command that is to be run.  This attribute MUST be specified if
   service equals "shell".  If no value is present then the shell itself
   is being referred to.

* perhaps a bit more explanation of the use-case here. What is a "shell"
in reference to an administrative management protocol?
[Agreed, will add. In fact, now that T+ is not used for Network access and only for Device Management, this could do with rather more description. Proposed text:
“  a shell (exec) command.  This indicates the command name of the
   command that is to be run.  The "cmd" attribute MUST be specified if
   service equals "shell".

   Authorization of shell commands is a common use-case for the TACACS+
   protocol.  Command Authorization generally takes one of two forms:
   session-based and command-based.

   For session-based shell authorization, the "cmd" argument will have
   an empty value.  The client determines which commands are allowed in
   a session according to the arguments present in the authorization.

   In command-based authorization, the client requests that the server
   determine whether a command is allowed by making an authorization
   request for each command.  The "cmd" argument will have the command
   name as its value.

   cmd-arg

   an argument to a shell (exec) command.  This indicates an argument
   for the shell command that is to be run.  Multiple cmd-arg attributes
   may be specified, and they are order dependent.”

   routing

   A boolean. ...

* note that "routing" is defined to be a boolean, but the other attributes
don't have data types defined.  I *think* "cmd" is a textual attribute,
but it's not defined that way
[Agreed, along with the similar comment above]

   timeout

   an absolute timer for the connection (in minutes).  A value of zero
   indicates no timeout.

* probably "numeric" data type?
[Definately]

   autocmd

   an auto-command to run.  Used only when service=shell and cmd=NULL

* it's confusing to have an explicit NULL in "cmd".  Perhaps quotes would
clarify it here.  e.g.

  n auto-command to run.  Used only when the packet contains
attribute-values of "service=shell" and "cmd="
[Agreed. The text in comment for command should help. Proposed new Text:
“   an auto-command to run.  Applicable only to session-based shell
   authorization.”]

   noescape

   Boolean.  ...

* nit: consistent terminology.  "Boolean" ? or "A boolean ..." ??
[Agreed. Or even: consistent indefinite article employment.]

   callback-dialstring

   Indicates that callback is to be done.  Value is NULL, or a
   dialstring.  A NULL value indicates that the service MAY choose to
   get the dialstring through other means.

* nit: explicit NULL is confusing.  Perhaps "value is a dial string, or
when omitted, indicates that the server may choose to get the value via
other means"
[Agreed. Proposed Text: “Indicates that callback is to be done.  Value is a dialstring, or empty.  Empty value indicates that the service MAY choose to get the dialstring through other means.”]

* nit: what's a "dialstring" ?

* what's a callback?  How does that work?
[These two are rather legacy, and really related to an old form of Network Access, so propose we can exclude]

7.2.  Accounting Attributes

...

   The following new attributes are defined for TACACS+ accounting only.
   When these attribute-value pairs are included in the argument list,
   they will precede any attribute-value pairs that are defined in the
   authorization section (Section 5) above.

* "will" precede?  Perhaps "MUST precede"
[Agreed]

   task_id

   Start and stop records for the same event MUST have matching task_id
   attribute values.  The client must not reuse a specific task_id in a
   start record until it has sent a stop record for that task_id.

* how big is the task_id?  Since values can be omitted, is it OK to have a
zero-length task_id?  or 1 octet?

* are there recommendations for the content of task_id? e.g. incrementing
numbers, strings, etc.

* is task_id mandatory to occur in accounting packets?
[task_id is best practice, and I think we can promote it to mandatory, however this would be a change to spec. It is a text value (that can clearly encode a number), but it is widely enough used that I think restricting its validity would be difficult.]

   start_time

   The time the action started ().

* nit: what are the brackets for?
[Will update as for stop_time]

   stop_time

   The time the action stopped (in seconds since the epoch.)

* which attributes are allowed in which packets?  I presume it's not
recommended to have a START which contains "stop_time"
[other than task_id, there are no accepted standards for this mapping]

   elapsed_time

   The elapsed time in seconds for the action.  Useful when the device
   does not keep real time.

* I'd recommend removing that last sentence.  it also indicates that the
device may not have accurate time, notwithstanding the definition of
"absolute times" at the start of Section 7.
[Agreed]

   bytes_in

   The number of input bytes transferred by this action

* input to where?  This was poorly specified in RADIUS, and some
implementations got it wrong...
[Proposed text: The number of input bytes transferred by this action to the port]

* this number might be large.  Should implementations allow 16 bit
numbers? 32-bit?  64-bit?  What are the expected / recommended limits?

* i.e. implementations MUST be able to parse numbers up to 2^32, and
SHOULD be able to parse numbers up to 2^64


   status

   The numeric status value associated with the action.  This is a
   signed four (4) byte word in network byte order. 0 is defined as
   success.  Negative numbers indicate errors.  Positive numbers
   indicate non-error failures.  The exact status values may be defined
   by the client.

* This is the first (and only) time that a field is defined as non-ASCII.
That's very off.
[I agree, however, we can change this in the follow up doc]

* nit: values "may" be defined by the client?  Perhaps just saying "values
are implementation specific" would be better
[Will update]

   ...  Privilege Levels are ordered values from 0 to 15
   with each level representing a privilege level that is a superset of
   the next lower value.

* nit: perhaps this could be "each level is defined to be a superset of
the next lower value", instead of "representing".
[or even more simply: each level is a superset of the next lower value]

   If a client uses a different privilege level scheme, then it must map
   the privilege level to scheme above.

* editorial :"... to A scheme..."

* and how is mapping done?  It might be better to just omit that sentence.

   Privilege Levels are applied in two ways in the TACACS+ protocol:

* nit: perhaps "used" instead of "applied"

      - As an argument in authorization EXEC phase (when service=shell
      and cmd=NULL), where it is primarily used to set the initial
      privilege level for the EXEC session.

* how does that work?  And what does it mean to set an initial privilege
level?

      - In the packet headers for Authentication, Authorization and
      Accounting.  The privilege level in the header is primarily
      significant in the Authentication phase for enable authentication
      where a different privilege level is required.

* again, how does that work?  Additional text would be useful.  e.g.

In typical use-cases, an administrator can perform a series of commands at
a low privilege level.  When additional privileges are required, the
administrator can authenticate at the desired level, perform a series of
commands, and the drop privilege level to the lower one.  This methodology
minimizes possible errors by using high privilege levels only when
necessary, and using low privilege levels for most commands.

* expanding on the use-case and work flow would be of great benefit.
[Agreed, please see text below]

   The use of Privilege levels to determine session-based access to
   commands and resources is not mandatory for clients, but it is in
   common use so SHOULD be supported by servers.

* and clients, presumably?  Perhaps instead, say:

It is RECOMMENDED that clients and servers use Privilege levels to signal
and control session-based access.  It is RECOMMENDED that clients permit
users to change their Privilege levels, so as to ensure that commands are
executed with the minimum Privilege level required.
[Proposed new section: “  The TACACS+ Protocol supports flexible authorization schemes through
   the extensible attributes.

   One scheme is built in to the protocol and has been extensively used
   for Session-based shell authorization: Privilege Levels.  Privilege
   Levels are ordered values from 0 to 15 with each level is a
   superset of the next lower value.  Configuration and implementation
   of the client will map actions (such as the permission to execute of
   specific commands) to different privilege levels.  Pre-defined values are:

      TAC_PLUS_PRIV_LVL_MAX := 0x0f

      TAC_PLUS_PRIV_LVL_ROOT := 0x0f

      TAC_PLUS_PRIV_LVL_USER := 0x01

      TAC_PLUS_PRIV_LVL_MIN := 0x00

   A Privilege level can be assigned to a shell (EXEC) session when it
   starts starts (for example, TAC_PLUS_PRIV_LVL_USER).  The client will
   permit the actions associated with this level to be executed.  This
   privilege level is returned by the server in a session-based shell
   authorization (when "service" equals "shell" and "cmd" is empty).
   When a user required to perform actions that are mapped to a higher
   privilege level, then an ENABLE type reuthentication can be initiated
   by the client, in a way similar to su in unix.  The client will
   insert the required privilege level into the authentication header
   for enable authentication request.

   The use of Privilege levels to determine session-based access to
   commands and resources is not mandatory for clients.  Although the
   privilege level scheme is widely supported, its lack of flexibility
   in requiring a single monotonic hierarchy of permissions means that
   other session-based command authorization schemes have evolved, and
   so it is no longer mandatory for clients to use it.  However, it is
   still common enough that it SHOULD be supported by servers.”]