Re: scram-sha-1-plus

[Simon Josefsson]

Jeremy Harris <address@hidden> writes:

>>   strcpy (global_application_state->tls_unique, "foo");
>>   /* the global callback now has the right tls-unique data for the
>>   session to start */
>>   res = gsasl_client_start (ctx, "SCRAM-SHA-256-PLUS", &client);
>
> That's incredibly fragile.  Almost begging to be broken by
> code-maintenance five years down the line.
>
>
>> For a threaded application, the answer (today) is to have one
>> gsasl_init() context per thread,
>
> That's... somewhat ugly.  It begs the question as to what the
> intent of this level of context.   Possibly documenting the
> intent would help, but then the question becomes what the
> added value of the session context is?
>
>> alternatively do the two operations
>> above under a mutex.
>
> Also ugly.

Agreed.

>> Looking at the code, the GS2 and GSSAPI server plugins have the same
>> "problem" today: they retrieve the GSASL_SERVICE and GSASL_HOSTNAME
>> properties during gsasl_server_start().  I suspect there aren't any
>> applications out there that want to provide different answers to these
>> callbacks depending on the session, so this was never a problem for
>> anyone.  They are application-global and static.
>
> You're probably right about those being application-static - though
> I could imagine a application wanting to offer multiple services...
> POP and IMAP for a start, if not also SMTP.

Yes, GS2/GSSAPI should be redesigned too.

>>> To keep the current early check, I think you would have to split the
>>> session startup into two calls, so the the application gets a session-
>>> context before the time it needs to provide the channel-binding data.
>> 
>> Yes, this would solve the generic problem, however we still have the
>> problem with gsasl_client_suggest_mechanism() -- that function should
>> recommend SCRAM-SHA-1-PLUS (or GS2-KRB5-PLUS) if the TLS channel binding
>> is available but not otherwise.  How to implement that?
>
> I think that facility should be session-dependent, because sessions
> differ.  Specifically, it should take a session-context handle not
> a toplevel context handle for the API.

Good point.  Ideally there should be a new guessing function that works
on the session-level.

> Exim doesn't use it - we assume the admin writing the exim config
> is prepared to take the hit of errors when he gets it wrong.  So
> the automatics offered by "suggestion" are not needed.

I think there are many applications that doesn't use it.  Usually the
selection of client mechanism comes from the user or through
application-specific logic.

> However, if you took the "split into two" choice:
>
> 1) API call: application gets fresh, bare, session-context
> 2) application couples that with the TLS channel in app-level data
>    struct.
> 3) Optionally, app sets channel-binding data
> 4) API call: with session-context, start requested METHOD
>    (triggers prop callback if needed for channel-binding data)
>
> - then if the app did indeed do (3), it could use a "suggestions"
> call for the session-context before doing (4).
>
> Since I don't need "suggestions" I've not thought too hard about it,
> but does that satisfy the need?

Yes.  We could introduce new gsasl_init_session() and a
gsasl_start_client/server().

However that is an API change and if the only real advantage is to fix
gsasl_client_suggest_mechanism() properly I would prefer to resolve this
in a more non-intrusive way to get 1.10.0 out.  Leaving the
gsasl_client_suggest_mechanism() problem aside, I think if we move the
uses of callbacks from start() into the first step() call, everything
works right?  Then modulo memory allocation problems
gsasl_client_start() will always get you a Gsasl_session handle.  You
can do gsasl_property_set() on it before doing the gsasl_step() calls.

If we really want to have a new variant of
gsasl_client_suggest_mechanism() that can take per-session specific
details into account, which it should, then we would need to split
gsasl_client/server_start() into
gsasl_init_session()+gsasl_start_client/server().

>>> Alternatively it should be possible to fail the flow later on, if it
>>> is -PLUS but the binding prop has not been provided at the time
>>> it is actually required.  After all, other props are required
>>> for the conversation (eg the authn) and the client application
>>> is not required to provide them so early...  On that view, the
>>> library should permit the application to try to do the wrong
>>> thing early on - only error-checking later.
>> 
>> Yes this is possible but it means SCRAM-SHA-256-PLUS will be attempted
>> all the time if the application trusts gsasl_client_suggest_mechanism(),
>> even if it won't even be able to produce the first token for the server
>> if the application doesn't provide the tls-unique data.
>> 
>> Which of the PLUS and non-PLUS mechanisms are advertised affects the
>> client/server messages, see RFC 5802:
>
> Yup.  Exim can do the advertising, because it has, at application level,
> knowlege of the availability of binding data.  There's no library
> involvement needed.
>
> I don't know if that applies in other applications though.

It is an unfortunate layering violation that this internal design choice
of the SCRAM mechanism leaks not only out into the general SASL library
layer but also into the SASL application layer.

>> Currently gsasl link availability of GSASL_CB_TLS_UNIQUE together with
>> advertisement of PLUS variants.
>
> I'm not sure where that library does any advertising, though?

Right the application does that but the library doesn't know what was
advertised.  It uses GSASL_CB_TLS_UNIQUE, and the application chosing
PLUS vs non-PLUS, as a way of guessing what the server advertised.

>> The more I think of this now, the problem is
>> 
>>    a) calling gsasl_client/server_start() should never trigger a
>>       callback, or at least not a callback for a session-specific
>>       property (because the application will be in a difficult situation
>>       to answer it correctly without per-session hooks).  Therefor, we
>>       should move the GSASL_CB_TLS_UNIQUE callback to
>>       gsasl_client/server_step().
>
> or the split session-start.
>
>>    b) gsasl_client_suggest_mechanism() was designed assuming the current
>>       broken design and has to be redesigned.  Maybe we can live with it
>>       returning SCRAM-*-PLUS which may not complete if the client does
>>       not provide GSASL_CB_TLS_UNIQUE?
>> 
>>    c) if we fix a) we should review SCRAM client/server code so that it
>>       does the right thing wrt PLUS.  The logic is a bit complicated.

I think this is still what we should do, together with documenting the
concern with gsasl_client_suggest_mechanism() since that will now
include SCRAM-SHA-*-PLUS even if GSASL_CB_TLS_UNIQUE may not be
available.

We can ALSO introduce the gsasl_init_session() +
gsasl_start_client/server() APIs but it is not necessary.

Let's see what I think tomorrow...

/Simon

signature.asc
Description: PGP signature