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

[Snir Sheriber]

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