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

["Douglas Gash (dcmgash)" <[e-mail]>]

As always, thanks for the comments!

Regards,

Doug

Inline...

On 17/05/2017 15:54, "Alan DeKok" <[e-mail]> wrote:

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

Agreed, though I¹m not sure how an unused field would not be ignored,
almost by definition,. The only circumstance I can think of where a field
has data but is unused may not be ignored is for logging, are you thinking
we should preclude such?


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


So I think the specification of secrets for obfuscated option is, as you
say, critical, and will provide some guidance such as non-empty. Also will
make sure we include that servers MUST support capability of secret per
client-server tuple (see next comment).

But I¹m not so sure what we should say about storage.

>
>>   ...  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".

Sure, that sounds like a good suggestion.

>
>  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"
Agreed.

>
>> * 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.

Agreed. 

>
>>    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?


Do you mean, if client replies to the request to
TAC_PLUS_AUTHEN_STATUS_GETUSER with an empty username. I would simply
repeat the request in case it was a user error. We can document this. The
protocol has a built-in limit of the number of interactions due to the
seq_no, but implementations can limit interactions.

We can add this detail.

>
>> * 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.
Agreed.

>
>> * 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.

Just to clarify my comment: it is not an aspect of the processing of the
T+ protocol that chooses the challenge. The protocol will get the
challenge and response as pre-packaged unit.

So I think we can demand that challenges be generated according to the
spec for CHAP. But reaching into that spec to add our own criteria would,
I think, be a mixing of concerns.

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


Sure, we will update the doc for MS-CHAP in whatever way we find best for
CHAP.


>
>>    ... 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."


Ok, I see what you mean. We can add the SHOULD not clause here.

Interesting though, as this is a significant use case for T+. Will be
insteresting to see what the main objects were for why it was removed from
RADIUS.

>
>> * 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"

Sounds sensible, will add.


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

Agreed.

>
>> * 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.

Will do.

>
>> * 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".

Sure. The original in the draft is weak. Will update.


>
>> * 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.

We can clarify, but the task concept as I see it was always pretty
insubstantial, so I don¹t want to elevate it into something it wasn¹t.
When it comes down to it, it is really just a way to link accounting
records for a specific session. Will move the test to clarify.


>
>>    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".

I think it is more a case of: it is redundant, but it was specified, so we
cannot choose to change it to make it more efficient.


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

Good point. As it is TCP, we can recommend:  If not on an active single
connect session then close the connection and create a new. Then in either
(single/non single) resend the request.

>
>> * 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.

Sure, will recommend minimum of 5 seconds, but I¹m not sure we can specify
an upper limit as it is not mandatory.

>
>> * 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.

Sure, will add.


>
>> * 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...

Well, can we always assume  ³users system²?. Often there can be something
between the user and the device (robots, scripts etc) (but I guess they
may all be part of an overall system). I guess it would be more general to
focus on the direction of the device port.

>
>  Pedanticism helps here.

Agreed!

> :(  We weren't pedantic in RADIUS, and multiple implementors screwed it
>up.
>
>  Alan DeKok.
>