On Apr 20, 2016, at 3:49 PM, Warren Kumari <[email protected]> wrote:
> I'd really appreciate it if people could review the current document (Link
> for easy clicking:
> https://datatracker.ietf.org/doc/draft-ietf-opsawg-tacacs/) and provide
> feedback to the list by Friday April 29th.
Overall, it's a lot more focussed than the original draft. This is good.
More detailed comments:
Section 2
... Authentication generally takes place when the user first logs in to a
machine or requests a service of it.
What is meant by "requests a service of it" ? The subsequent paragraph
explains in a bit more detail, but the phrasing here sounds awkward.
Also "log into a machine" is a bit vague. Perhaps also use "privilege" as
per the subsequent text:
... Authentication generally takes place when the user first connects to the
machine for device administration, and may also happen later when
additional privileges are requested.
Authentication is not mandatory; it is a site-configured option.
I would suggest adding "... not mandatory in the protocol". From a security
point of view, unauthenticated device administration is bad.
... The server receives TACACS+ protocol requests, and replies according
to its business model
What's a "business model" in relation to a protocol? Perhaps it could
instead say
... The server receives TACACS+ protocol requests, and replies according
to its configured policy
Section 3.1.1
... It is NOT recommended to deploy TACACS+ without Body encryption,
other than for test environments.
Perhaps "NOT RECOMMENDED"
3.2. Session
The concept of a session is used throughout this document. A TACACS+
session is a single authentication sequence, a single authorization
exchange, or a single accounting exchange.
I find that a little confusing. When an admin logs into a device, the common
terminology is to call one particular connection as a session. i.e. user logs
in at 10am, runs 3 commands, and logs out at 10:10am.
Using "session" to refer to request/reply exchanges is a bit confusing.
3.3. Single Connect Mode
The packet header (see below) contains a flag to allow sessions to be
multiplexed on a connection.
If a client sets this flag, this indicates that it supports
multiplexing TACACS+ sessions over a single TCP connection.
Can exchanges for multiple *users* be sent over a single TCP connection? The
document doesn't seem to say anything about this.
From Section 3.4:
... The sequence number must never wrap i.e. if the sequence number 2^8-1
is ever reached, that session must terminate and be restarted with a
sequence number of 1.
I have a few questions about this. The use of "session" here is confusing.
It was earlier defined to be a request/reply exchange. But here the
implication is that it's something else. Perhaps what's meant is a
"connection"?
There is also the implication that the authentication / authorization process
starts again. It would be good to clarify this text to use a term other than
"session". And to explain what happens when a session/connection is restarted.
If the intention is to open a new TCP connection, we presume that the user
must re-authenticate. Do they use the same credentials as last time? How are
these cached on the client?
... session_id
The Id for this TACACS+ session. The session id should be randomly
chosen. 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
Again, what is meant by "session" here? If we use the definition from
Section 3.2, a session is a request/reply exchange. So can the "session_id" be
different for every exchange? I presume not, otherwise the authentication step
couldn't be associated with subsequent authorization an accounting steps.
I suggest changing Section 3.2 to use a term other than "session". Perhaps
"exchange" would be better. The term "session" should be reserved for a series
of authentication / authorization / accounting exchanges, which are all tied to
the same user login. I see this change as critical to getting the document
published. As it stands today, I have no real idea what a "session" is, as
it's used in multiple contradictory ways.
... 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.
I'm not sure what is meant by "packets are never padded". This is TCP, not
UDP. UDP packets can be larger than the application data packets they
transport. TCP cannot, as it is a stream protocol.
Perhaps what is meant here is that the data following "length" octets MUST be
another valid TACACS+ header.
And the corollary (from a security perspective) is that any time the data is
*not* a valid TACACS+ header, the TCP connection MUST be dropped. The
alternative is to read through streams of random garbage, hoping that perhaps
something will decode as a correct TACACS+ packet.
Section 3.5;
- There should be no padding in any of the fields or at the end of
a packet.
This is another way of saying that the "length" field isn't lying to you.
I'm not opposed to this statement, I'm just wondering why it's necessary.
3.6. Encryption
3.6.1. Body Encryption
In general it's bad form to have 2 section headers immediately following one
another.
I would suggest adding a sentence after the 3.6 header which re-iterates the
earlier comments saying "yes, this isn't really encryption, but it was designed
20 years ago, and this is the terminology used in the protocol."
... NOTE: Implementations should take care not to skip decryption simply
because an incoming packet indicates that it is not encrypted. If
the unencrypted flag is not set, and the packet is not encrypted, it
must be dropped.
That isn't very clear to me. Double negatives tend to be hard understand.
The use of encryption should be an administrative decision, and enforced on
the wire. e.g. If the configuration has a shared secret and/or says that
encryption is required, then packets received without encryption MUST be
dropped, and the TCP connection MUST be closed.
Similarly, if the configuration has no shared secret or says that encryption
is not used, then packets received with encryption MUST be dropped, and the TCP
connection MUST be closed.
... After a packet body is decrypted, the lengths of the component values
in the packet should be summed and checked against the cleartext
datalength value from the header. Any packets which fail this check
should be discarded and an error signalled.
And also the TCP connection should be dropped. If the packet is malformed,
and the shared secret doesn't match, there is no point in keeping the TCP
connection open. The only thing that can be exchanged is garbage data, which
isn't useful to anyone.
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.
And then the TCP connection should be dropped.
Section 4.1
It would be good to explain what the various "len" fields mean. It's implied
from context, but as a protocol specification, explicit description is
preferred to implicit understanding.
Similar comments apply for the rest of the packet bodies. The 'len" fields
are given in the ASCII diagram, but not further explained or referred to.
... user
The username. It is encoded in [UTF-8]. It is optional in this
packet, depending upon the class of authentication.
What does it mean to be "optional"? Perhaps the "user_len" field is equal to
zero, which means that the "user" field does not exist.
Also, since "user_len" is 8 bits, it would be good to note that the maximum
"user" name which can be exchanged is 255 octets.
Similar text should be added to the other fields.
That's it for now. I'll post more later.
Alan DeKok.
_______________________________________________
OPSAWG mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/opsawg
