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

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

Dear Alan,

Below is a simple textual concatenation and minor (hopefully lossless)
edit of comments you kindly sent for the October revision (version 5),

We will work through this list, and reply with an item-by item response,
(In place of previous mode of updating the doc to make v6) and then
hopefully move towards a consensus for the content for version 7.

Thanks,

Regards,

Doug.

...
 Some comments.  Mostly little nits and clarifications.

  The major points for me are related to security.  The "Security
Considerations" section is almost empty, and will certainly not pass a
review from the security area.  They will in all likelihood block
publication until substantive comments have been added.

------

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.

   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?

   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.

   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.

*  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"

   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"

* i.e. the flag is a signal, not a capability for the systems to 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.

   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.

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.

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

* I wonder again what "padding" means here.

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".

...  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.

  ...  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.

  ...   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" ?

   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.

   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.

   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.

* 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.

* 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.

* 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.

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.

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

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...

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...

   ... 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.

  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.

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.

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

*  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*?

* 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"

   ... 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?

* 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...

  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/

* 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.

   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..."

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

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

* editorial: "specified IN RFC ..."

... 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.

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?

* what does the server do with this message?  Should it be logged
somewhere?

... 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..."

   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"

   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

* what happens if the client gets pushed a key, and then also has a
pre-configured key?  Which one takes precedence?  Why?


  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."

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"

* 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.

...
     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?

* 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?

* a reference to Section 7 would be useful here.

   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"

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

...

   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"

    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"

   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.

  ...   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 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?

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.

   ... 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.

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.

   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

   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.

  ... 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".

* Some more questions...

* does the client retry an accounting packet if it doesn't get a response
from the server?

* 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)

* 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?

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

   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?

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

* 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

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

* also, it would be good to say which attributes are mandatory, and which
are optional

   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"

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" ?

   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?

   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

   timeout

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

* probably "numeric" data type?

   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="

   noescape

   Boolean.  ...

* nit: consistent terminology.  "Boolean" ? or "A boolean ..." ??

   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"

* nit: what's a "dialstring" ?

* what's a callback?  How does that work?

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"

   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?

   start_time

   The time the action started ().

* nit: what are the brackets for?

   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"

   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.

   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...

* 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.

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

   ...  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".

   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.

   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.

9.  TACACS+ Security Considerations

* there is a lot which can be said here.  My proposal here is a 
substantial amount of text.  I'll include it in another message

  My $0.02 on the contents of the Security Considerations section.   I'm 
sure I've missed things.

  Please also comment if the suggestions here are wrong or unworkable.

----
9. Security Considerations

The protocol described in this document has been in widespread use
since specified in "The Draft" (1998).  However it does not meet
modern security standards, and faces vulnerabilities with privacy and
authenticity.

This section describes issues with the TACACS+ specification, then the
protocol, deployments, followed by recommendations for implementations.

Because of the security issues with TACACS+, the authors intend to
follow up this document with an enhanced specification of the
protocol employing modern security mechanisms.


9.1 Security of The Draft Specification

TACACS+ was originally specified in "The Draft" (1998).  However, this
draft is incomplete, and leaves key points unspecified.  As a result,
software authors have had to make implementation choices about what
should, or should not, be done in certain situations.  These
implementation choices are somewhat constrained by ad hoc
interoperability tests.  That is, all TACACS+ clients and servers
interoperate, so there is a rough consensus on how the protocol works.

While the authors have made every effort to maintain compatibility,
there is no guarantee, however, that this specification matches the
behavior of current or historical implementations.  We believe that
any incompatibilites are due to modern security requirements.  We
therefore suggest that implementors should read this document, and
verify that their implementations follow the security practices
recommended here.


9.2 Security of The Protocol

The major security issue with the TACACS+ protocol is the ad hoc
"encryption" defined in Section 3.6.  This "encryption" is more
properly called "obfuscation", and has had no security analysis.  That
is, the security or insecurity of the protocol is entirely unknown.

Attacks on cryptographic hashes are well known and are getting better
with time, as discussed in [RFC4270].  The security of the TACACS+
protocol is dependent on MD5 [RFC1321], which has security issues as
discussed in [RFC6151].  It is not known if the issues described in
[RFC6151] apply to TACACS+.  <<AD: lifted from RFC 6929 Section 11 >>

The choice of encrypting the body but not the packet header is
unusual.  The lack of security on the packet header means that an
attacker can modify the header without detection.

For example, a "session_id" can be replaced by an alternate one, which
could allow an unprivileged administrator to "steal" the authorization
from a session for a privileged administrator.  An attacker could also
update the "flags" field to indicate that one or the other end of a
connection requires TAC_PLUS_UNENCRYPTED_FLAG, which could remove what
little security is available.

What little security that exists is dependent on implementations
limiting access to known clients, and on the strenght of the secret
key.  Attacks who can guess the key or break the obfuscastion method
can gain unrestricted and undetected access to all TACACS+ traffic.
This access permits the attacker to execute any command on any system,
leaving the attacker in complete control of the network.  The negative
side effects of such a successful attack cannot be overstated.

9.2.1 Security of Authentication Sessions

There are a number of security issues with authentication sessions.
As the sessions enable the exchange of clear-text passwords, an
attacker capable of observing the unencrypted traffic or breakign the
encrypted traffic, can gain complete network access.  Similarly,
MSCHAPv1 and MS-CHAPv2 offer little additional security, while ARAP is
insecure and MUST NOT be used.  As of the publication of this
document, there has been no similar attacks on the CHAP protocol.

Section 4.4.3 permits the redirection of a session to another server
via the TAC_PLUS_AUTHEN_STATUS_FOLLOW mechanism.  As part of this
process, the secret key for a new server can be sent to the client.
This public exchange of secret keys means that once one session is
broken, it may be possible to leverage that attack to attacking
connections to other servers.  Section 9.4, below, has recommendations
for securing this portion of the protocol.

9.2.2 Security of Authorization Sessions

The primary issue with authorization sessions is that unauthenticated
authorization is allowed.  Almost as bad, authorization is permitted
where authentication was done by a third party, such as with
TAC_PLUS_AUTHEN_METH_KRB5.  This third party authentication is based
entirely on the client claiming that the user has been authenticated,
and the server trusting that request.  This practice is entirely
insecure.

There is also no way to cryptographically tie an authorization session
to a particular authentication session.  That is, when the client
indicates "authen_method" in the packet header, the authentication and
authorization sessions are tied together implicitly by the contents of
the other fields, such as "use", "port", "rem_addr", etc.

<<AD: it would be good for Section 5.1 to contain recommendations on
how the server can use these fields to tie together authentication /
authorization sessions.  Perhaps recommend that all AAA include a
"task_id"? >>

The specification allows for the exchange of attribute-value pairs.
While a few such attributes are defined here, the protocol is
extensible, and vendors can define their own attributes.  There is no
registry for such attributes, and in the absence of a published
specification, no way for a client or server to know the meaning of a
new attribute.

As a result, vendors SHOULD NOT define new attribute-value pairs,
unless such pairs are used only to communicate between client and
server implementations both written by the same vendor.

The "cmd" and "cmd-arg" attributes define shell commands and
arguments, but does not specify what these are.  The specific
commands, therefore, are undefined.


9.2.3 Security of Accounting Sessions

The security considerations for accounting sessions are largely the
same as for authorization sessions.  This section describes additional
issues specific to accounting sessions.

There is no way in TACACS+ to signal that accounting is required.
There is no way for a server to signal a client how often accounting
is required.  The accounting packets are received solely at the
clients discretion.  Adding such functionality would assist with
auditing of user actions.

The "task_id" field is defined only for accounting packets, and not
for authentication or authorization packets.  As such, it is difficult
to correlate accounting data with a previous authentication or
authorization request.

<<AD: re-use the recommendations suggested for Section 5.1, above? >>


9.4 Security of Deployments

Due to the above concerns with the protocol, it is critical that it be
deployed in a secure manner.

Systems using TACACS+ MUST be configured so that TACACS+ traffic is
secured.  Any user or service who is not interacting with the TACACS+
protocol MUST NOT be able to observe TACACS+ traffic, even when
TACACS+ "encryption" is used.

When used inside of enterprises, TACACS+ traffic MUST be sent over
management networks.  When used on the wider internet, TACACS+ traffic
MUST be secured via a modern security protocol such as TLS or IPSec.
As this specification describes the historic protocol, recommendations
on TLS and IPSec parameters are outside of the scope of this document.


9.4 Recommendations for Implementations

The following recommendations are for clients and servers which
implement the TACACS+ protocol.  These recommendations describe end
user interaction or user interface design.  While not part of the
protocol itself, good designs can prevent insecure configurations.

Clients and servers MUST default to rejecting all connections which
have the TAC_PLUS_UNENCRYPTED_FLAG set.  Clients and servers MAY
permit such connections when a debugging or test flag is set.  Clients
and servers MUST NOT run with this flag set in normal environments.

Similarly, the default for both clients and servers MUST be to require
a secret key when configuring a new server or client.  The secret key
MUST NOT be empty, and SHOULD be at least eight (8) characters in
length.  Clients and servers SHOULD permit the user of keys unique to
each server or client which they are connecting to.  They SHOULD warn
the administrator if a secret key is re-used.  Documentation for
implementations SHOULD recommend the use of strong secret keys.

Servers should maintain list of known clients, and reject connections
which originate from other systems.  Servers SHOULD NOT permit the
configuration of wildcard clients.  e.g. clients which map to a
network range.

Where a client or server detects that the secret key is incorrect, it
SHOULD terminate the underlying TCP connection, and log a message to
the system administrator.  There is no good reason to keep a
connection open when the systems are unable to communicate in a
meaningful fashion.

Implementions MUST default to using TAC_PLUS_AUTHEN_TYPE_CHAP for
authentication.  We recognize, however, that this authentication
method is incompatible with many password storage systems.  As a
result, other authentication methods are still in the protocol, and
can be used so long as administrators recognize the security issues
with their use.

Clients MUST default to treating TAC_PLUS_AUTHEN_STATUS_FOLLOW as if
TAC_PLUS_AUTHEN_STATUS_FAIL had been received instead.  Clients
SHOULD allow administrators to explicitly enable the
TAC_PLUS_AUTHEN_STATUS_FOLLOW functionality.  When the functionality
is enabled, clients SHOULD permit administrators to specify ranges of
addresses, and/or sets of hosts, along with corresponding secret keys,
for which redirection will be permitted.  Clients SHOULD ignore
redirects to hosts which are outside of the pre-configured range or
list.  Clients SHOULD ignore any key provided via
TAC_PLUS_AUTHEN_STATUS_FOLLOW, and SHOULD instead use a preconfigured
key for that host.

For authorization, servers SHOULD default to requiring
TAC_PLUS_AUTHEN_METH_TACACSPLUS.  Servers SHOULD track authentication
sessions via methods "to be described" in Section 5.1.

Where a server receives an unknown attribute-value pair, it SHOULD
reply with TAC_PLUS_AUTHOR_STATUS_FAIL.  Where a server receives an
unrecognized values in authorization attributes such as "cmd",
"cmd-arg", "acl", etc., the server SHOULD reply with
TAC_PLUS_AUTHOR_STATUS_FAIL.  When a client receives an unknown
authorization attribute, it SHOULD behave as if it had received
TAC_PLUS_AUTHOR_STATUS_FAIL.

>