[Spice-devel] [PATCH v2 spice-gtk 1/2] authentication: Handle failed SASL authentication separately

[Christophe de Dinechin]

> On 21 Feb 2017, at 16:28, Snir Sheriber <ssheribe at redhat.com> wrote:
> 
> Hi,
> 
> 
> On 02/20/2017 07:00 PM, Christophe de Dinechin wrote:
>>> On 19 Feb 2017, at 15:47, Snir Sheriber <ssheribe at redhat.com> wrote:
>>> 
>>> Remove handling with failures in the SASL authentication
>>> process to separate function
>>> ---
>>> src/spice-channel.c | 44 +++++++++++++++++++++++++++-----------------
>>> 1 file changed, 27 insertions(+), 17 deletions(-)
>>> 
>>> diff --git a/src/spice-channel.c b/src/spice-channel.c
>>> index af67931..cbf1291 100644
>>> --- a/src/spice-channel.c
>>> +++ b/src/spice-channel.c
>>> @@ -1113,28 +1113,38 @@ static int spice_channel_read(SpiceChannel *channel, void *data, size_t length)
>>>     return length;
>>> }
>>> 
>>> +#if HAVE_SASL
>>> /* coroutine context */
>>> -static void spice_channel_failed_authentication(SpiceChannel *channel,
>>> -                                                gboolean invalidPassword)
>>> +static void spice_channel_failed_sasl_authentication(SpiceChannel *channel)
>>> {
>>>     SpiceChannelPrivate *c = channel->priv;
>>> +    gint err_code; /* Affects the authentication window activated fileds */
>>> 
>>>     if (c->auth_needs_username && c->auth_needs_password)
>>> -        g_set_error_literal(&c->error,
>>> -                            SPICE_CLIENT_ERROR,
>>> -                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
>>> -                            _("Authentication failed: password and username are required"));
>>> +        err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME;
>>>     else if (c->auth_needs_username)
>>> -        g_set_error_literal(&c->error,
>>> -                            SPICE_CLIENT_ERROR,
>>> -                            SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME,
>>> -                            _("Authentication failed: username is required"));
>>> -    else if (c->auth_needs_password)
>>> -        g_set_error_literal(&c->error,
>>> -                            SPICE_CLIENT_ERROR,
>>> -                            SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
>>> -                            _("Authentication failed: password is required"));
>>> -    else if (invalidPassword)
>>> +        err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME;
>>> +    else
>>> +        err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD;
>>> +
>>> +    g_set_error_literal(&c->error,
>>> +                        SPICE_CLIENT_ERROR,
>>> +                        err_code,
>>> +                        _("SASL authentication failed"));
>> Per the recent discussion (Feb 14 with Christophe F), can’t we map common SASL errors to Spice messages? To me, it’s different if the problem is that I used a wrong password or if the server is down. The message as is seems quite terse.
>> 
>> Errors that seem be reportable (although not all of them seem relevant to Spice):
>> 
>> SASL_BADAUTH	Authentication failure.
>> SASL_NOAUTHZ	Authorization failure.
>> SASL_EXPIRED	The passphrase expired and must be reset.
>> SASL_DISABLED	Account disabled.
>> SASL_NOUSER	User not found.
>> SASL_BADVERS	Version mismatch with plug-in.
>> SASL_NOVERIFY	The user exists, but there is no verifier for the user.
>> SASL_WEAKPASS	The passphrase is too weak for security policy.
>> SASL_NOUSERPASS	User supplied passwords are not permitted.
>> 
>> 
>> Some that may need to be “translated” in Spicese if they ever get back to us:
>> 
>> SASL_TOOWEAK	The mechanism is too weak for this user.
>> SASL_ENCRYPT	Encryption is needed to use this mechanism.
>> SASL_TRANS		One time use of a plaintext password will enable requested mechanism for user.
>> 
>> Others should probably collected into a “default” in a switch statement, something like “Unexpected SASL error code <blah>”.
>> 
> As link result/error? I guess it would be the best , but first it requires to inform the client in some other way that it can stop waiting for the sasl server start\step result (currently it just freeing the link)
> btw according to sasl docs the full error string should be sent

Yes, but not translated. We can always add the SASL errors to the po files.

Christophe

>>> +
>>> +    c->event = SPICE_CHANNEL_ERROR_AUTH;
>>> +
>>> +    c->has_error = TRUE; /* force disconnect */
>>> +}
>>> +#endif
>>> +
>>> +/* coroutine context */
>>> +static void spice_channel_failed_authentication(SpiceChannel *channel,
>>> +                                                gboolean invalidPassword)
>>> +{
>>> +    SpiceChannelPrivate *c = channel->priv;
>>> +
>>> +    if (invalidPassword)
>>>         g_set_error_literal(&c->error,
>>>                             SPICE_CLIENT_ERROR,
>>>                             SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD,
>>> @@ -1808,7 +1818,7 @@ error:
>>>     if (saslconn)
>>>         sasl_dispose(&saslconn);
>>> 
>>> -    spice_channel_failed_authentication(channel, FALSE);
>>> +    spice_channel_failed_sasl_authentication(channel);
>>>     ret = FALSE;
>>> 
>>> cleanup:
>>> -- 
>>> 2.9.3
>>> 
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170222/3c649622/attachment-0001.html>