Subject: [OPSAWG] TACACS+ informational document.

From: Warren Kumari <[email-masked]>
Date: Wed, 20 Apr 2016 19:49:02 +0000

Hi there all,

The authors of draft-ietf-opsawg-tacacs have updated the document and would
like some review.

As a reminder, this document is supposed to describe how TACACS currently
works.
We will then publish a new document which improves the security of TACACS.

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.

W

---

From: Alan DeKok <[email-masked]>
Date: Thu, 21 Apr 2016 10:18:16 -0400

On Apr 20, 2016, at 3:49 PM, Warren Kumari <[email-masked]> 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.

---

From: Alan DeKok <[email-masked]>
Date: Thu, 21 Apr 2016 22:06:25 -0400

More comments...

4.4.  Description of Authentication Process

   Authentications are classified by the action, authen_type and service
   fields in the START packet of the authentication Session.  The user,
   priv_lvl, service, port and rem_addr in the START packet are all
   provided to help identify the conditions on the client

  What is meant by "conditions on the client"?  It would be best to describe what those fields mean instead.

  ... The information necessary to transact the authentication is passed ...

  What is "information necessary to transact the authentication"?  This sounds unclear to me.

  ...  A set of standard authentication classifications is defined in this
   document.  

  What is meant by "authentication classifications" ?  It's not clear from the context or the rest of the document.  Perhaps it means "authentication protocols" ?  i.e. one exchange is done for PAP, another for MS-CHAP, another for OTP, etc.

  ... 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 server_msg may be used by the
   client to prompt the user for more information.

  "may be used"?  What happens when the client doesn't use server_msg to prompt the user?

   ... All three cause the same action to be performed, but in the case of
   TAC_PLUS_AUTHEN_STATUS_GETUSER, the client can know that the
   information that the user responds with is a username, and for
   TAC_PLUS_AUTHEN_STATUS_GETPASS, that the user response represents a
   password.

  I'm wary of statements such as "the client can know..."   Is that a philosophical statement?

  It's generally best to describe actions instead of knowledge.  e..g. "for prompts using TAC_PLUS_AUTHEN_STATUS_GETUSER, the users response is interpreted by the client as a username, for use in subsequent exchanges".

  ... TAC_PLUS_AUTHEN_STATUS_GETPASS, that the user response represents a
   password.

  Similar comments apply here.

   ... TAC_PLUS_AUTHEN_STATUS_GETDATA is the generic request for
   more information.

  And what happens then with the data?  When is this message used?

4.4.1.  Version Behaviour

   ... in the table below:

          LOGIN    CHPASS   SENDAUTH
   ASCII      v0         v0       NA
   PAP        v1        NA       v1
   CHAP       v1        NA       v1
   MS-CHAP    v1        NA       v1

  What does "NA" mean?  I suspect I know, but it's good to be explicit instead of assuming that the reader knows the authors intent.

  ... The removal of SENDPASS
   was prompted by security concerns, and is no longer considered part
   of the TACACS+ protocol.

  It may be worth considering the removal of all documentation of SENDPASS.  If it's not just deprecated by removed entirely, then it should probably be just removed entirely.

  4.4.2.  Common Authentication Flows

   This section describes the authentication flows that should be
   supported.

  What is meant by "should be supported"?  Is this an RFC 2119 statement?  Or are these examples of authentication flows which are in common use, and that all clients && servers are expected to support?

   ... A PAP authentication only consists of a username and password 
   RFC 1334 [RFC1334] .

  I think referencing a later RFC is preferable.

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

  This would seem to impose limits on the "date len" field.  It would be good to state these explicitly.

  Also, to indicate that the challenges MUST have a minimal length, which should probably be no less than 8, though 16 is suggested.  The alternative is to allow zero-length challenges, which is not good.

  And how are the challenge, ID, and response fields packed into the data field?  i.e. in what order are they packed?  ASCII art would be *very* helpful here. 

  Similar comments apply for MS-CHAPv1 and MS-CHAPv2 authentication.

 4.4.3

  ... When the status equals TAC_PLUS_AUTHEN_STATUS_FOLLOW the packet
   indicates that the TACACS+ server requests that authentication should
   be performed with an alternate server.  The data field MUST contain
   ASCII text describing one or more servers.  A server description
   appears like this:

   [@<protocol>@]<host>>[@<key>]

  Examples would help here.  The above text isn't ABNF, or any other machine-readable definition of a text string.

  Perhaps the text could be updated to say that "@" is a delimiter. for text strings:

0 @ = host
1 @ = host@key
2 @ = @protocol@host
3 @ = @protocol@host@key

 ...  If a key is supplied, the client MAY use the key in order to
   authenticate to that host.  If more than one host is specified, they
   MUST be separated by an ASCII Carriage Return (0x0D).

  Some questions here.  What happens if the client doesn't want to use that key?  What happens if the client is pre-provisioned with a different key than what is supplied here?  If there are multiple hosts, does the client use the supplied key for all of them?  What if the client has a disparate set of keys for the list of hosts?

 ...  This packet is
   only currently intended for PPP authentication when multiple
   authentication mechanisms are available and can be negotiated between
   the client and the remote peer.  This also requires future PPP
   authentication extensions which have not yet been passed through the
   IETF.

  Are users performing device administration over PPP?  If that isn't the 99% case, then I suggest re-wording the paragraph.

  Also, updating it to clarify what it means for "future PPP extensions".  Perhaps just removing that sentence would be best.

5.  Authorization

   ...  The authorization REQUEST message contains a fixed set of fields that
   describe the authenticity of the user or process,

  What is "authenticity" of a user?

 ...  priv_lvl

   This field matches the priv_lvl field in authentication request a

  What does this statement mean?  Does the field (length and value) have to be identical to the field in the authentication request?  If not, why not, and what happens?  If so, it would be better to just say that it MUST be identical.  Similar comments apply for the other fields.  "match" is a word which should be used in the context of "A must match B", instead of "is used for matching".  The first is concrete and implementable.  The second is more philosophical than protocol oriented.

  ...  Attribute-value strings are not NULL terminated, rather their length
   value indicates their end.  The maximum length of an attribute-value
   string is 255 characters.

  Are zero-length strings allowed?  I presume not, but it would be good to say so.

7.  Attribute-Value Pairs

  ... A value of NULL means an attribute with a zero length string for its
   value i.e. cmd=NULL is actually transmitted as the string of 4
   characters "cmd=".

  That seems contradictory.. NULL is encoded by not encoding it..  Perhaps the text could instead say:

   Attributes which have no value are transmitted as the attribute name, followed by the mandatory or optional flag.  The value is elided.  For example, the attribute "cmd" which has no value is transmitted as a string of 4 characters "cmd="

  ... acl

   US-ASCII number representing a connection access list.  Used only
   when service=shell and cmd=NULL

  I would suggest eliding the NULL here, too.  The temptation for naive implementors would be to encode an actual string "NULL".

  There should be a separate "Security Considerations" section as Section 9.  It should explain all of the issues with the protocol, such as the use of "encryption", etc.

  Alan DeKok.

---

From: Stefan Winter <[email-masked]>
Date: Fri, 22 Apr 2016 14:13:41 +0200

Hi,

the draft states:

 user

The old expired draft from 1997 states:

user

   The username. It is optional in this packet.

Somehow, an explicit encoding requirement made it into the new draft. It's
good to be explicit when designing a protocol; but when it comes
to documenting deployed reality it's important to be sure that
this really *is* the deployed reality.

Are you sure / how sure are you that deployed reality agrees with this change of wording?

Greetings,

Stefan Winter

Am 20.04.2016 um 21:49 schrieb Warren Kumari:
> Hi there all,
>
> The authors of draft-ietf-opsawg-tacacs have updated the document and
> would like some review. 
>
> As a reminder, this document is supposed to describe how TACACS
> currently works.
> 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.
>
> W

---

From: Alan DeKok <[email-masked]>
Date: Fri, 22 Apr 2016 08:38:43 -0400

A short summary:

- many fields are named but not defined

- structures with multiple fields are described, but field order is not defined

- terms are used inconsistently

- the document is silent on critical points

  - how do user logins map to TCP connections?  1-1?  1-N?  N-M?

  - can the same session_id be used in multiple TCP connections?

- the general tone seems philosophical: systems "know" things, not prescriptive: systems "do" things.

- edge cases are not discussed

 - what happens with zero-length fields?

- common use-cases aren't described (e.g. inter-site use of the protocol)

- the security considerations section is minimal

  - how do the edge cases affect security?

  - is the TCP connection closed when the key is found to be wrong?  If not, why not?

  - what are best practice recommendations for deployment?

  - what impact does inter-site deployment have on security?

  As an implementor, I would have to guess at large portions of the protocol, or I would have to read the source to existing implementations.  The draft as is stands today can get me ~90% of the way to implementing the protocol, but critical portions are not present.

  Alan DeKok.

---

From: Joel Jaeggli <[email-masked]>
Date: Fri, 22 Apr 2016 08:41:30 -0700

Thanks, 

This is useful feedback.

> On Apr 22, 2016, at 05:38, Alan DeKok <[email-masked]> wrote:
> 
>  A short summary:
> 
> _______________________________________________
> OPSAWG mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/opsawg
>