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

2017-02-19 Thread Snir Sheriber 
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"));
+
+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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2017-02-20 Thread Christophe de Dinechin 

> On 19 Feb 2017, at 15:47, Snir Sheriber  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_BADAUTHAuthentication failure.
SASL_NOAUTHZAuthorization failure.
SASL_EXPIREDThe passphrase expired and must be reset.
SASL_DISABLED   Account disabled.
SASL_NOUSER User not found.
SASL_BADVERSVersion 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_TOOWEAKThe mechanism is too weak for this user.
SASL_ENCRYPTEncryption 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 ”.


> +
> +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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2017-02-21 Thread Snir Sheriber 

Hi,


On 02/20/2017 07:00 PM, Christophe de Dinechin wrote:

On 19 Feb 2017, at 15:47, Snir Sheriber  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_BADAUTHAuthentication failure.
SASL_NOAUTHZAuthorization failure.
SASL_EXPIREDThe passphrase expired and must be reset.
SASL_DISABLED   Account disabled.
SASL_NOUSER User not found.
SASL_BADVERSVersion 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_TOOWEAKThe mechanism is too weak for this user.
SASL_ENCRYPTEncryption 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 ”.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2017-02-21 Thread Christophe Fergeau 
On Sun, Feb 19, 2017 at 04:47:17PM +0200, Snir Sheriber 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"));

I don't understand what you are aiming for with this series. I thought
the goal was to get better error messages on autentication failures, but
this change merges 3 distinct error messages
_("Authentication failed: password and username are required"));
_("Authentication failed: username is required"));
_("Authentication failed: password is required"));

to just

_("SASL authentication failed"));

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2017-02-22 Thread Snir Sheriber 

Hi,


On 02/21/2017 06:37 PM, Christophe Fergeau wrote:

On Sun, Feb 19, 2017 at 04:47:17PM +0200, Snir Sheriber 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"));

Is there a mechanism that allows only username ?

-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"));

I don't understand what you are aiming for with this series. I thought
the goal was to get better error messages on autentication failures, but
this change merges 3 distinct error messages
_("Authentication failed: password and username are required"));
_("Authentication failed: username is required"));
_("Authentication failed: password is required"));

to just

_("SASL authentication failed"));

Christophe
It basically similar issue as in rhbz#1365736 but for the sasl 
authentication case..


The auth_needs_username & auth_needs_password are used for signaling the 
application if the selected
mechanism requires them, according to that the username\password fields 
will be enabled\disabled, imo it's

enough for letting the user know "password and username are required"
The authentication type string is just for letting the user know what 
authentication is in use.


This is not significant , but i guess it somehow more clear (or maybe 
less confusing) to the user that way..


Snir.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2017-02-22 Thread Christophe Fergeau 
On Wed, Feb 22, 2017 at 11:50:21AM +0200, Snir Sheriber wrote:
> Hi,
> 
> 
> On 02/21/2017 06:37 PM, Christophe Fergeau wrote:
> > On Sun, Feb 19, 2017 at 04:47:17PM +0200, Snir Sheriber 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"));
> Is there a mechanism that allows only username ?

I guess in SSO setups, it makes sense to first ask for just a username,
then check for a valid kerberos ticket for that username (or whatever
you use for SSO), and if there is no such ticket, then ask for an
additional authentication token.

> > > -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"));
> > I don't understand what you are aiming for with this series. I thought
> > the goal was to get better error messages on autentication failures, but
> > this change merges 3 distinct error messages
> > _("Authentication failed: password and username are required"));
> > _("Authentication failed: username is required"));
> > _("Authentication failed: password is required"));
> > 
> > to just
> > 
> > _("SASL authentication failed"));
> > 
> > Christophe
> It basically similar issue as in rhbz#1365736 but for the sasl
> authentication case..
> 
> The auth_needs_username & auth_needs_password are used for signaling the
> application if the selected
> mechanism requires them, according to that the username\password fields will
> be enabled\disabled,

Ah, right, so yes, library users can now about the need for username
and/or password through the SPICE_CLIENT_ERROR_AUTH_NEEDS_xxx error code
which is set in the GError returned through a channel-event signal. Then
application *should* disable unneeded fields. (I would not say "will" as
an application could do differently there ;)


> imo it's
> enough for letting the user know "password and username are required"
> The authentication type string is just for letting the user know what
> authentication is in use.
> 
> This is not significant , but i guess it somehow more clear (or maybe less
> confusing) to the user that way..

Now I agree, but the commit log really should explain *why* you are
doing the change, and *why* this is not making the error reporting worse
for the user.
In this case, we could even add a few words in the documentation about
the expected behaviour of client applications to the
SPICE_CLIENT_ERROR_AUTH_NEEDS_xxx errors (assuming there is a good place
to document that).

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2017-02-22 Thread Christophe de Dinechin 

> On 21 Feb 2017, at 16:28, Snir Sheriber  wrote:
> 
> Hi,
> 
> 
> On 02/20/2017 07:00 PM, Christophe de Dinechin wrote:
>>> On 19 Feb 2017, at 15:47, Snir Sheriber  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_DISABLEDAccount disabled.
>> SASL_NOUSER  User not found.
>> SASL_BADVERS Version mismatch with plug-in.
>> SASL_NOVERIFYThe user exists, but there is no verifier for the user.
>> SASL_WEAKPASSThe 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 ”.
>> 
> 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);
>>>

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

2017-02-22 Thread Christophe Fergeau 
Hey,

On Wed, Feb 22, 2017 at 11:50:21AM +0200, Snir Sheriber wrote:
> The authentication type string is just for letting the user know what
> authentication is in use.

Sorry, forgot to reply to this bit, I don't think saying "SASL" in this
string is useful, this is going to be very cryptic to the users (I'm not
even sure I knew what SASL was before working on SPICE), and
imo is just relevant to sysadmins and spice-gtk developers, so making
sure debug logs indicate which kind of auth mechanism is used should be
enough.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2017-02-22 Thread Daniel P. Berrange 
On Wed, Feb 22, 2017 at 11:47:10AM +0100, Christophe Fergeau wrote:
> On Wed, Feb 22, 2017 at 11:50:21AM +0200, Snir Sheriber wrote:
> > Hi,
> > 
> > 
> > On 02/21/2017 06:37 PM, Christophe Fergeau wrote:
> > > On Sun, Feb 19, 2017 at 04:47:17PM +0200, Snir Sheriber 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"));
> > Is there a mechanism that allows only username ?
> 
> I guess in SSO setups, it makes sense to first ask for just a username,
> then check for a valid kerberos ticket for that username (or whatever
> you use for SSO), and if there is no such ticket, then ask for an
> additional authentication token.

If you want to correctly use SASL then you should not make any
assumptions about which credentials you'll be asked for. Even if a
mechanism wants the username *and* password, it is permitted to
ask for them in separate steps of the handshake. So you might need
to popup a dialog to ask for username, and then later ask for
password in a new dialog popup. It is upto the mechanism plugin
to decide which to ask for at which point, so the app can not
predict that.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2017-02-22 Thread Snir Sheriber 



On 02/22/2017 12:58 PM, Christophe Fergeau wrote:

Hey,

On Wed, Feb 22, 2017 at 11:50:21AM +0200, Snir Sheriber wrote:

The authentication type string is just for letting the user know what
authentication is in use.

Sorry, forgot to reply to this bit, I don't think saying "SASL" in this
string is useful, this is going to be very cryptic to the users (I'm not
even sure I knew what SASL was before working on SPICE), and
imo is just relevant to sysadmins and spice-gtk developers, so making
sure debug logs indicate which kind of auth mechanism is used should be
enough.
Yes, you are right, i guess it's enough (and the log also shows the 
selected mechanism by sasl lib)

Christophe

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel