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
_______________________________________________
OPSAWG mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/opsawg
