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

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

Hi Alan,

Thanks again for responses, please see inline:

Just for accounting: I do owe you an initial response to one batch of your
comments that I missed first item, and we have the security section as a
whole. Below though, are the responses to the comments form Tuesday,
thanks!

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

>On May 20, 2017, at 8:24 AM, Douglas Gash (dcmgash) <[e-mail]>
>wrote:
>>> 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?
>
>  No.  Nut the draft needs to say what to do.
>
>  If the protocol has provisions for something to happen, then the draft
>needs to say what implementations should do when that thing happens.
>
>  In this case, it should state that unused fields are ignored, and
>treated as if they were not present.

Agreed, will clarify that, within the context of the protocol flow. These
may still be logged.


>
>>> 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).
>
>  And it should suggest a minimum secret length.
>
>> But I¹m not so sure what we should say about storage.
>
>  "secret keys need to be kept secret".

Sure, I’d hope this would be an axiom ;-) but not harm in emphasising.


> 
>
>>> 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.
>
>  Thanks.  My $0.02 is that the username SHOULD be in the first packet.
>If not, it MUST be in the second packet.  If not, the session is broken,
>and the server returns ERROR.
>
>  i.e. relying on the 256 packet limit for authentication is probably too
>lax.

Sure, makes sense.


>
>...  RE: CHAP challenges and length
>>> 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.
>
>   If the protocol transports data without looking at it, it's fine to
>treat that data as an opaque blob.  If the protocol looks at the data,
>the draft should have requirements on the data format.
>
>   For an example, see:
>
>https://tools.ietf.org/html/rfc2865#section-5.40
>
>  The CHAP-Challenge attribute in RADIUS is defined to have at least 5
>octets of challenge data.



We can follow a similar path with T+. I Will add. (covers below comment
too).



>
>> 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.
>
>  Nope.  The TACACS+ server which authenticates the user should be secure
>by design.  This means rejecting zero-length challenges, and challenges
>which "too short".
>
>  I would suggest that the challenges are a minimum of 8 octets in length.
>
>... RE: password changes.
>> 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.
>
>  It was insecure, and no one liked it.  The other reasons have been
>largely lost to time.
>
>>>>   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.
>
>  It's not about being "more efficient".  It's about specifying what this
>means.
>
>  I have no idea what it means to duplicate a START record.  Is the data
>in the second START the same?  Is it different?  What is done in either
>case?
>
>  Again, you need to document the protocol.  I'm not sure this point is
>getting across.  If you don't know what a portion of the protocol means,
>then either find out and document it, or document that "this exists, but
>we have no idea what it is, what it does, and we don't recommend that
>people use it".
>
>  Leaving portions of the protocol entirely undefined and
>implementation-defined is bad.
>
>  In RADIUS, I've authored or co-authored about 4 RFCs fixing things
>which were missed or undefined, and which cause interoperability issues.
>And I'm still finding things which are broken in the specs.  Two in the
>last month, in fact...
>
>  If TACACS is to be a standard, it should be clear what portions are
>well defined, and what portions are "I dunno...".  That gives guidance
>for future drafts as to what works, and what needs fixing.

I think then I committed a misunderstanding here. I thought you were
suggesting to drop the redundant accounting request, now I realist your
comment is to explain why it is used. Sure, can clarify this.



>
>>>> * 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.
>
>   It's always good to recommend best practices, even if the behaviour
>isn't mandatory.


I was thinking rather about the scenario that it may not recur at all...
However, we can say: for recurring accounting, this is the range (5
seconds to 1 hour), or updates are not sent at all.

>
>  Alan DeKok.
>