Re: [OPSAWG] Start of WGLC for TACACS+ document.

[Alan DeKok <[e-mail]>]

  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