[Spice-devel] [PATCH spice-server v3 12/12] red-stream: Encapsulate all authentication state in RedSASLAuth

[Christophe Fergeau]

On Fri, Dec 22, 2017 at 10:07:13AM +0000, Frediano Ziglio wrote:
> Instead of having half state in RedSASL and half in RedSASLAuth
> move everything in RedSASLAuth. This also reduces memory usage
> when we are using SASL but we finish the authentication step.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-stream.c | 138 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 72 insertions(+), 66 deletions(-)
> 
> diff --git a/server/red-stream.c b/server/red-stream.c
> index 0e372743a..898253c2e 100644
> --- a/server/red-stream.c
> +++ b/server/red-stream.c
> @@ -67,13 +67,6 @@ typedef struct RedSASL {
>      unsigned int encodedOffset;
>  
>      SpiceBuffer inbuffer;
> -
> -    char *mechlist;
> -    char *mechname;
> -
> -    /* temporary data during authentication */
> -    unsigned int len;
> -    char *data;
>  } RedSASL;
>  #endif
>  
> @@ -351,13 +344,8 @@ void red_stream_free(RedStream *s)
>  #if HAVE_SASL
>      if (s->priv->sasl.conn) {
>          s->priv->sasl.runSSF = s->priv->sasl.wantSSF = 0;
> -        s->priv->sasl.len = 0;
>          s->priv->sasl.encodedLength = s->priv->sasl.encodedOffset = 0;
>          s->priv->sasl.encoded = NULL;
> -        g_free(s->priv->sasl.mechlist);
> -        g_free(s->priv->sasl.mechname);
> -        s->priv->sasl.mechlist = NULL;
> -        g_free(s->priv->sasl.data);
>          sasl_dispose(&s->priv->sasl.conn);
>          s->priv->sasl.conn = NULL;
>      }
> @@ -735,17 +723,37 @@ static int auth_sasl_check_ssf(RedSASL *sasl, int *runSSF)
>  
>  typedef struct RedSASLAuth {
>      RedStream *stream;
> +    // list of mech allowed, allocated and freed by SASL

mech->mechanisms?

> +    char *mechlist;
> +    // mech received
> +    char *mechname;
> +    uint32_t len;
> +    char *data;


> +    // callback to call if success
>      RedSaslResult result_cb;
>      void *result_opaque;
> +    // saved Async callback, we need to call if failed as
> +    // we need to chain it in order to use a different opaque data
>      AsyncReadError saved_error;

These 2 comments could have been added in the commit introducing
RedSASLAuth.

>  } RedSASLAuth;
>  
> +static void red_sasl_async_deinit(RedSASLAuth *opaque)
> +{
> +    g_free(opaque->data);
> +    opaque->data = NULL;
> +    g_free(opaque->mechname);
> +    opaque->mechname = NULL;
> +    g_free(opaque->mechlist);
> +    opaque->mechlist = NULL;
> +    opaque->stream->priv->async_read.error = opaque->saved_error;

Not using red_stream_set_async_error_handler() here? (but this also
applies to "Handle SASL initialisation mainly in red-stream.c"

> +}
> +
>  // handle SASL termination, either success or error
>  // NOTE: After this function is called usually there should be a
>  // return or the function should exit
>  static void red_sasl_async_result(RedSASLAuth *auth, RedSaslError err)
>  {
> -    auth->stream->priv->async_read.error = auth->saved_error;
> +    red_sasl_async_deinit(auth);
>      auth->result_cb(auth->result_opaque, err);
>      g_free(auth);
>  }

I would keep the red_stream_set_async_error_handler() call here, and
instead have a red_sasl_auth_free() helper:

static void red_sasl_async_result(RedSASLAuth *auth, RedSaslError err)
{
    auth->stream->priv->async_read.error = auth->saved_error;
    auth->result_cb(auth->result_opaque, err);
    red_sasl_auth_free(auth);
}



> @@ -753,7 +761,7 @@ static void red_sasl_async_result(RedSASLAuth *auth, RedSaslError err)
>  static void red_sasl_error(void *opaque, int err)
>  {
>      RedSASLAuth *auth = opaque;
> -    auth->stream->priv->async_read.error = auth->saved_error;
> +    red_sasl_async_deinit(auth);

Same here

>      if (auth->saved_error) {
>          auth->saved_error(auth->result_opaque, err);
>      }
>
> [...]
>
>  static void red_sasl_handle_auth_mechlen(void *opaque)
>  {
> -    RedStream *stream = ((RedSASLAuth *)opaque)->stream;
> -    RedSASL *sasl = &stream->priv->sasl;
> +    RedSASLAuth *auth = opaque;
>  
> -    sasl->len = GUINT32_FROM_LE(sasl->len);
> -    if (sasl->len < 1 || sasl->len > SASL_MAX_MECHNAME_LEN) {
> -        spice_warning("Got bad client mechname len %d", sasl->len);
> -        return red_sasl_async_result(opaque, RED_SASL_ERROR_INVALID_DATA);
> +    uint32_t len = GUINT32_FROM_LE(auth->len);

You need to update auth->len here:
auth->len = GUINT32_FROM_LE(auth->len);
uint32_t len = auth->len;

I think I would do without the 'len' variable, auth->len is not that
long ;)

> +    if (len < 1 || len > SASL_MAX_MECHNAME_LEN) {
> +        spice_warning("Got bad client mechname len %d", auth->len);
> +        return red_sasl_async_result(auth, RED_SASL_ERROR_INVALID_DATA);
>      }
>  
> -    sasl->mechname = g_malloc(sasl->len + 1);
> +    auth->mechname = g_malloc(len + 1);
>  
>      spice_debug("Wait for client mechname");
> -    red_stream_async_read(stream, (uint8_t *)sasl->mechname, sasl->len,
> -                          red_sasl_handle_auth_mechname, opaque);
> +    red_stream_async_read(auth->stream, (uint8_t *)auth->mechname, auth->len,
> +                          red_sasl_handle_auth_mechname, auth);
>  }
>  
> -bool red_sasl_start_auth(RedStream *stream, RedSaslResult result_cb, void *opaque)
> +bool red_sasl_start_auth(RedStream *stream, RedSaslResult result_cb, void *result_opaque)

This could go in "Handle SASL initialisation mainly in red-stream.c"

>  {
>      const char *mechlist = NULL;
>      sasl_security_properties_t secprops;
> @@ -1047,11 +1054,9 @@ bool red_sasl_start_auth(RedStream *stream, RedSaslResult result_cb, void *opaqu
>  
>      spice_debug("Available mechanisms for client: '%s'", mechlist);
>  
> -    sasl->mechlist = g_strdup(mechlist);
> -
>      mechlistlen = strlen(mechlist);
>      if (!red_stream_write_u32(stream, mechlistlen)
> -        || !red_stream_write_all(stream, sasl->mechlist, mechlistlen)) {
> +        || !red_stream_write_all(stream, mechlist, mechlistlen)) {
>          spice_warning("SASL mechanisms write error");
>          goto error;
>      }
> @@ -1059,12 +1064,13 @@ bool red_sasl_start_auth(RedStream *stream, RedSaslResult result_cb, void *opaqu
>      auth = g_new0(RedSASLAuth, 1);
>      auth->stream = stream;
>      auth->result_cb = result_cb;
> -    auth->result_opaque = opaque;
> +    auth->result_opaque = result_opaque;
>      auth->saved_error = stream->priv->async_read.error;
> -    stream->priv->async_read.error = red_sasl_error;
> +    red_stream_set_async_error_handler(stream, red_sasl_error);

Same for this change

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180105/ad0c3cae/attachment-0001.sig>