Re: [OPSAWG] draft-ietf-opsawg-tacacs-06 Contributions, Status and Plans

[Alan DeKok <[e-mail]>]

On May 16, 2017, at 4:06 PM, Douglas Gash (dcmgash) <[e-mail]> wrote:
> Many items are marked with just [Agree], if it seems there is a trivial way to adjust according to the comment.

  Thanks.  I'll comment on things I have comments on, and remove everything else.

> 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.
> [Agree, changed text to: "To signal that any variable length data fields are unused, their
>       length value is set to zero." As to what happens when fields are unused, this will depend upon the fields]

  I meant what happens when an unused field has a non-zero length?

  If the field is unused, the spec should say the field is ignored, and treated as if it did not exist.

> ...  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.
> [Will be happy to stand corrected, but that strikes me as both outside the documentation of the protocol, and likely to lock in practices that will become out-of-date.]

  The draft should also document recommended practices for how to use the protocol, such as how keys should be managed / generated / stored.

   i.e. "As the security of the obfuscation method is unknown, keys SHOULD be produced via a CSPRNG, keys SHOULD be different for every client/server pair", etc.

  The original RADIUS RFC used a secret key, but didn't forbid a key of zero length.  I ran into at least one vendor who didn't allow for the entry of secret keys, and always set it to a zero-length string.  The next rev of the RFC forbade that...

>   ...  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.
> [Agree, the requirements must be supported by implementations, however we leave it for the implementor as to whether this is used]

  The wording is awkward.  Perhaps "client implementations MUST support per-server keys, and server implementations MUST support per-client keys".

  The sentence on "site dependent decision" can probably be removed.  Or maybe replaced with a suggestion that "using the same key for multiple hosts is NOT RECOMMENDED"

> * 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.
> [though we would not want to restrict to a single secret on server for a client, we can restrict the clint that it should us just a single secret on a client at a time for a connection to a server. This would mean that we could not have single connect, multi session interaction from a client with multiple secrets so then we could close the connection]

  I'm not sure what that means...

  It should say that the same key is used for the lifetime of a TCP connection.  Though the keys can be changed in between TCP connections.

  And, if there's a key mismatch, all bets are off, and the TCP connection is closed.

>    This is a standard ASCII authentication.  The START packet may
>    contain the username, but need not do so.
>  
> * what happens when the username is / is-not included?  i.e. what do
> implementations *do*?
> [Agreed to expand. the server can of course, finish the session at any time, as detailed in the "session termination" section. But let's assume that we want the server to carry on the process, then it will retrieve the username as described in the following proposed text: "The START packet MAY
>    contain the username.  If the user does not include the username then
>    the server MAY obtain it from the client with a CONTINUE
>    TAC_PLUS_AUTHEN_STATUS_GETUSER.  When the server has the username, it
>    will obtain the password using a continue with
>    TAC_PLUS_AUTHEN_STATUS_GETPASS.  ASCII login uses the user_msg field
>    for both the username and password.  The data fields in both the
>    START and CONTINUE packets are not used for ASCII logins, any content
>    MUST be ignored.  The session is composed of a single START followed
>    by zero or more pairs of REPLYs and CONTINUEs, followed by a final
>    REPLY indicating PASS, FAIL or ERROR."]

  OK. And if there's still no username?

> * if this is a valid protocol flow, then it should be described.  e.g.
> "Any username in the START packet is ignored", or "any username in the
> START packet MUST match the username in subsequent packets"
> [Normally, the username is in just one packet]
>  
>    ... The data fields in both
>    the START and CONTINUE packets are not used for ASCII logins.
>  
> * what happens if there is data in the fields?  Is it ignored?
> [Agreed From perspective of protocol, yes]

  The draft needs to say so.

> * also, the challenge MUST be derived from a cryptographically strong
> pseudo-random number generator, and MUST change on every CHAP request.
> Otherwise, the spec allows (also silently) a fixes challenge.
> [This is interesting because, of course, the challenge is not generated for T+ but for a different interaction. the result of that other interaction is passed along to T+. These recommendations are good, but can we actually police this data?]

  That reply make implementation-specific assumptions.  There are TACACS+ implementations which don't get the challenge from a third-party.

  The draft needs to state security requirements on CHAP and the challenges.  If an implementation chooses to ignore them, then the implementation is insecure and wrong.

  But the protocol needs to be specified securely in the draft.

> MSCHAPv1 / MSCHAPv2:
>  
>    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 49 octets).
>  
> *  RFC 2433 says that the challenge is 8 octets.  RFC 2759 says that the
> challenge is 16 octets.  It would be good to re-iterate that here.
>  
> * i.e. smaller challenges are insecure, and should be forbidden
> [again, as T+ interaction is not involved with the generation of the challenges, is this something we can actually meaningfully enforce?]

  The same comments as for CHAP apply here.  And no, we can't "enforce" it, but we can demand it.  Which is what the protocol specification is about.

>    ... To perform the authentication, the server will use a the algorithm
>    specified  RFC2759
>  
> * editorial: "specified IN RFC ..."
> [agreed]
>  
> ... ASCII change password request
>  
> * I suspect that the security area review will recommend that this be
> removed.  Even RADIUS had "change password" removed at the behest of the
> security people.
> [Noted. I take it this not a request yet to remove though?]

  I predict that the Security Area review will raise questions about it.

  If the functionality isn't really used in current practice, it would be best to have the draft note that the functionality exists.  But that all specification of the functionality is removed from the document, and implementations SHOULD NOT support it.

  e.g. "flag FOO was historical, but has been removed for security reasons.  That is all."

> * what does the server do with this message?  Should it be logged
> somewhere?
> [I would say, this is out of scope of the protocol, and a matter for implementation]

  It's a useful suggestion to implementors.

  "This message is typically logged, which helps administrators determine reasons for failure"
>  
>    If a key is supplied, the client MAY use the key in order to
>    authenticate to that host.  The client may use a preconfigured key
>    for the host if it has one.
>  
> * so the server is pushing keys to clients?  The security implications of
> this should be discussed
> [SIDE Note: I think this part of the original draft spec is really worth culling. I certainly specify it should not be implemented in our server. I had intended to propose that is done in the next version when we add TLS, however, am considering proposing to do this sooner unless anyone objects.]

  I would suggest removing it, but noting it exists, as with the password change above.

> * 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.
> [Areed, and server can accord appropriate confidence in this information with that proviso]

  That should be noted here, and in the Security Considerations section.

> ...
>      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?
> [Agreed, the first meaning]

  The draft should be updated to clarify this.

> * what are the legal characters for an attribute name?  The previous
> paragraph says US-ASCII.  So is "($#@!" a legal attribute name?
> [Agreed. Yes, that is a valid name, because it does not include = or *. the text is explicit though, do you think it needs further clarification?]

  It wouldn't hurt to re-iterate it.
 
> 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.
> [The header is more consistent than the body rather than the body, is that what you mean?]

  I mean it would be clearer to say "both leverage a common header, with type-specific packets using a standard layout".

> * Can an accounting packet indicate an authen_type and authen_service?  If
> so, what does that mean?
> [yes, indicating circumstances (if known) on the device]

  That should be clarified.
 
> * 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
> [Added text: "TACACS+ accounting is intended to record various types of events on clients, for example: login sessions, command entry, and others as required by the client implementation.  These events are collectively referred to in `The Draft' [TheDraft] as "tasks"."]

  That's still a little unclear to me, but OK.

>    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.
> [Well, here we are constrained by a clear definition form original draft.]

  What does it mean, tho?  This document should say.  i.e. portions of the protocol should not be left as "we have no idea what this means".

  If implementors don't use / support this functionality, then the description of it can be removed from the draft.  If it is used, implementors can speak up, and help describe what this functionality does.
 
> * Some more questions...
>  
> * does the client retry an accounting packet if it doesn't get a response
> from the server?
> [client will get PASS or FAIL. do you mean, if the client gets a force disconnect for the TCP connection?]

  I mean, what happens if the client sends a packet, the TCP connection stays open, and there's no response packet?

  Does the client retry?  If so, when?  If not, what happens?

  I guess similar comments should apply for accounting and authorization...

  As we're not trying to extend the protocol, it may be good enough to note that responses may not be received, and it's up to implementations to decide what to do.

> * 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)
> [so lets stick with implementation defined, the range of T+ devices we see is so diverse and use cases are such that I would not like to constrain.]

  It would be good to have recommendations.  e.g. "updates more than once per second are NOT RECOMMEND, updates more than an hour apart are NOT RECOMMENDED".

  The goal is to guide implementors to choosing values which won't cause problems for everyone else.

> * 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?
> [I would say that the normal approach is to treat a failure to connect as an ERROR condition, so that any support failover or equivalent to method-list can be followed. Frequently a device will have a local option at the end of a method list, and this local method would contain the options for worst case access, but I would say that this behaviour is out-of-scope of the protocol description.]

  The draft still needs to suggest what happens when a client can't talk to a server.  Saying "treat it like an ERROR" condition" is OK.  Also "implementations may fail-over to implementation-defined backup methods".

  It's bad to have the draft be silent on the topic.

> * nit: what's a "dialstring" ?
>  
> * what's a callback?  How does that work?
> [These two are rather legacy, and really related to an old form of Network Access, so propose we can exclude]

  Sounds good.

> * 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?
> [task_id is best practice, and I think we can promote it to mandatory, however this would be a change to spec. It is a text value (that can clearly encode a number), but it is widely enough used that I think restricting its validity would be difficult.]

  It's fine to say that task_id SHOULD be in all packets.
>  
> * which attributes are allowed in which packets?  I presume it's not
> recommended to have a START which contains "stop_time"
> [other than task_id, there are no accepted standards for this mapping]

  The draft should say so, then.
 
>    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...
> [Proposed text: The number of input bytes transferred by this action to the port]

  from the users system to the port...

  Pedanticism helps here. :(  We weren't pedantic in RADIUS, and multiple implementors screwed it up.

  Alan DeKok.