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