Re: [kitten] Review of draft-ietf-kitten-channel-bound-flag-01

[Benjamin Kaduk <[e-mail]>]

Thanks for the review, Greg.

Nico, will you be able to prepare a document update?

(one note inline)

On Sat, Jun 24, 2017 at 10:38:53AM -0400, Greg Hudson wrote:
> Here are my semantic concerns:
> 
> Section 1:
> 
> * Paragraph 2 discusses taking advantage of implementation behavior to
>   deploy channel bindings on initiators first, then acceptors.  A
>   naive reader might conclude that this is sufficient, because an
>   acceptor can be configured to not provide channel bindings if it
>   does not wish to require all initiators to provide them as well.  It
>   would be helpful to explain that the value obtained by providing
>   channel bindings on the acceptor without enforcing them, which is
>   that participating initiators gain protection against MITM attacks.
> 
> * Paragraph 4 should either be omitted or made more specific.  It's
>   interesting to me that SSPI implements these semantics (all of them
>   or just the existence of a channel-bound ret flag?), but that should
>   be stated directly or just skipped.
> 
> Section 1.1:
> 
> * "[RFC2743] seems to indicate that mechanisms must ignore channel
>   bindings when one party provided none."  What part of RFC 2743?  How
>   does this statement square with "This facility is meant to be
>   all-or-nothing" in the introduction?
> 
> * The remainder of this paragraph seems redundant with the top material
>   in section 1.
> 
> Sections 1.2-1.4: these sections are kind of informal and may not all
> be of interest to implementors.  I think it's worth saying that
> gss_create_sec_context() is expected to lead to simpler stepper
> functions in the future, but the rest can probably go.
> 
> Section 2.2:
> 
> * The term "understood" is a little unclear, and aside from the new
>   channel-bound flag, it's unclear how a mechanism should process the
>   absence of an understood flag.  Out of band, Nico has said that he
>   doesn't expect the mechanism to pare down its set of ret_flags based
>   on ret_flags_understood (e.g. it should not omit the mutual-auth
>   flag if the application doesn't include it in ret_flags_understood).
>   So the mechanism should only consult ret_flags_understood when it
>   really needs to know, such as with the new channel-bound flag.  The
>   draft should make that clear.
> 
> * The new gss_set_context_flags() call allows us to specify req_flags
>   to gss_accept_sec_context(), which is an intentional addition.
>   However, we have no immediate use for it, and the draft doesn't make
>   it clear whether an existing mechanism should do anything with
>   existing req_flags on the acceptor.
> 
> Section 2.2.1:
> 
> * Naming the parameter "ret_flags" rather than "ret_flags_understood"
>   in the C bindings appears to create some confusion as to what should
>   be done with the flags.
> 
> Section 2.4 and 2.5:
> 
> * For background, the overall purpose of this draft is to make channel
>   bindings fit the GSS-API model where the caller asks for things and
>   then checks whether it actually got them.  On the initiator it is
>   awkward to achieve this goal, since the krb5 mech does not have
>   channel binding confirmation and other existing mechanisms may not
>   have it either.  Thus the draft specifies new mechanism attributes
>   to express whether a mechanism has channel binding confirmation, and
>   a request flag to filter SPNEGO mechanisms according to whether they
>   can confirm channel bindings.  Nico has a plan to extend the krb5
>   mech's mutual-auth response to confirm channel bindings, likely
>   using GSS_C_CB_CONFIRM_FLAG (2048) in the 8003 checksum flags value
>   as a negotiation bit.
> 
> * GSS_C_MA_CBINDING_MAY_CONFIRM does not really express where the krb5
>   mech will wind up after the mutual auth response is extended, as
>   channel bindings will only be confirmed if the initiator requests
>   mutual auth, and possibly only if the mechanism choose an RFC 4121
>   enctype.
> 
> * The specification of req_cb_confirmation_flag is wishy-washy as to
>   whether it allows SPNEGO to choose a mechanism that might only
>   possibly be able to confirm channel bindings.  I'm not sure that
>   this request flag will ever wind up being useful to callers.
> 
> * "the pseudo-mechanism MUST NOT negotiate any mechanisms that lack
>    the GSS_C_MA_CBINDING_CONFIRM or GSS_C_MA_CBINDING_MAY_CONFIRM
>    mechanism attributes" reads to me as "mechanisms must have both
>    attributes to qualify", but from context I think it is supposed to
>    mean "mechanisms must have one of the two attributes to qualify".

I agree that's the apparent intent.

-Ben

> Section 2.6:
> 
> * Here we specify the behavior of gss_delete_sec_context on an empty
>   context.  We should also specify the behavior an the application
>   tries to use an empty context with gss_inquire_sec_context(),
>   gss_wrap(), etc..  Per discussion with Nico, doing so should result
>   in GSS_S_NO_CONTEXT, just as if the application supplied
>   GSS_C_NO_CONTEXT to those functions.
> 
> Section 3:
> 
> * The third requirement says that init_sec_context SHOULD be lax about
>   the acceptor not providing channel bindings, but notes that "It is
>   possible that not all security mechanism protocols can implement
>   this requirement easily."  In contrast, the fourth requirement says
>   that init_sec_context MUST be lax if the caller understands
>   ret_channel_bound_flag.  The fourth requirement doesn't seem to
>   square with the proviso on the third.  If a mechanism can conform to
>   the fourth requirement, then it seems like it can also conform to
>   the third requirement.
> 
> And here are a few copy-editing notes:
> 
> Abstract:
> 
> * "This document addresses both, the..." should not have the comma.
> 
> Section 2.2:
> 
> * undestands -> understands
> 
> * "individual elements -one-per-flag- instead" ->
>   "individual elements--one per flag--instead"
> 
> Section 3:
> 
> * There are multiple instances of "Whenever [something] shall have
>   [done something]", which doesn't look right to me.  Omitting the
>   word "shall", and sometimes changing "have" to "has", makes them
>   look better.
> 
> * "Whenever both, the initiator..." should not have the comma.
> 
> * "This is a restatement... restated for the reader's convenience"
>   should either omit "a restatement of" or the final clause.