Bug 34795

Summary: server: add SASL support
Product: Spice Reporter: Marc-Andre Lureau <marcandre.lureau>
Component: serverAssignee: Marc-Andre Lureau <marcandre.lureau>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: build: make it silent
server/reds: remove unused readv
server: s/RedsStreamContext/RedsStream
server: add reds_stream_{read,write,free,remove_watch}()
server: use reds_stream_remove_watch() helper
server: use reds_{link,stream}_free()
server: remove cb_free, not needed anymore
server: use the new reds_stream_{read,write}
server/reds: remove the void* ctx field
server: rename s/peer/stream
server/reds: make writev fallback more generic
common: add SpiceBuffer - based on qemu-vnc Buffer
build: add --with-sasl
server: pull out reds_handle_link(), for future reuse
server: simplify and constify sync_write()
server: add reds_channel_dispose()
server: add auth mechanism selection
server: add SASL support

Description Marc-Andre Lureau 2011-02-27 07:55:17 UTC
This patch set implements SASL channel authentication as described in the
protocol patch entitled "protocol: add common channel caps for AUTH
mechanism selection".

SASL is enabled via spice_server_set_sasl(), that a following QEMU patch
implements.

For testing, current spice-gtk git implements the client side.

Changed since v1:
- removed the RFC SSL, since it's not doing anything useful
- splitted the patch moving to stream_{read,write}() functions
- fixed the if {} else {} style
- renamed s/RedsSASLContext/RedsSASL
Comment 1 Marc-Andre Lureau 2011-02-27 07:55:22 UTC
Created attachment 43871 [details] [review]
build: make it silent

This patch make it easier to spot warnings in compilation.  It should
work with older versions of automake that don't support silent rules.

If you want verbose build, make V=1.

Signed-off-by: Uri Lublin <uril@redhat.com>
Comment 2 Marc-Andre Lureau 2011-02-27 07:55:29 UTC
Created attachment 43872 [details] [review]
server/reds: remove unused readv

Let's not bother with it since nobody uses it, and it's not implemented for SSL anyway
Comment 3 Marc-Andre Lureau 2011-02-27 07:55:31 UTC
Created attachment 43873 [details] [review]
server: s/RedsStreamContext/RedsStream
Comment 4 Marc-Andre Lureau 2011-02-27 07:55:37 UTC
Created attachment 43874 [details] [review]
server: add reds_stream_{read,write,free,remove_watch}()
Comment 5 Marc-Andre Lureau 2011-02-27 07:55:41 UTC
Created attachment 43875 [details] [review]
server: use reds_stream_remove_watch() helper
Comment 6 Marc-Andre Lureau 2011-02-27 07:55:44 UTC
Created attachment 43876 [details] [review]
server: use reds_{link,stream}_free()

Be carefull removing the watch before, like __release_link
Comment 7 Marc-Andre Lureau 2011-02-27 07:55:47 UTC
Created attachment 43877 [details] [review]
server: remove cb_free, not needed anymore
Comment 8 Marc-Andre Lureau 2011-02-27 07:55:49 UTC
Created attachment 43878 [details] [review]
server: use the new reds_stream_{read,write}
Comment 9 Marc-Andre Lureau 2011-02-27 07:55:52 UTC
Created attachment 43879 [details] [review]
server/reds: remove the void* ctx field
Comment 10 Marc-Andre Lureau 2011-02-27 07:55:56 UTC
Created attachment 43880 [details] [review]
server: rename s/peer/stream

This is stylish change again. We are talking about a RedStream object,
so let's just name the variable "stream" everywhere, to avoid
confusion with a non existent RedPeer object.
Comment 11 Marc-Andre Lureau 2011-02-27 07:55:59 UTC
Created attachment 43881 [details] [review]
server/reds: make writev fallback more generic

We are going to reuse it for SASL/SSF encode write().
Comment 12 Marc-Andre Lureau 2011-02-27 07:56:02 UTC
Created attachment 43882 [details] [review]
common: add SpiceBuffer - based on qemu-vnc Buffer
Comment 13 Marc-Andre Lureau 2011-02-27 07:56:05 UTC
Created attachment 43883 [details] [review]
build: add --with-sasl

Using cyrus SASL library (same as gtk-vnc/qemu).
Comment 14 Marc-Andre Lureau 2011-02-27 07:56:09 UTC
Created attachment 43884 [details] [review]
server: pull out reds_handle_link(), for future reuse

+ a couple of indent, style change
Comment 15 Marc-Andre Lureau 2011-02-27 07:56:15 UTC
Created attachment 43885 [details] [review]
server: simplify and constify sync_write()

+ symplify, improving style of code using it.
Comment 16 Marc-Andre Lureau 2011-02-27 07:56:20 UTC
Created attachment 43886 [details] [review]
server: add reds_channel_dispose()

Try to have a common base dispose() method for channels. For now, it
just free the caps.

Make use of it in snd_worker, and in sync_write() - sync_write() is
going to have default caps later on.
Comment 17 Marc-Andre Lureau 2011-02-27 07:56:26 UTC
Created attachment 43887 [details] [review]
server: add auth mechanism selection
Comment 18 Marc-Andre Lureau 2011-02-27 07:56:30 UTC
Created attachment 43888 [details] [review]
server: add SASL support

We introduce 2 public functions to integrate with the library user.

spice_server_set_sasl() - turn on SASL
spice_server_set_sasl_appname() - specify the name of the app (It is
used for where to find the default configuration file)

The patch for QEMU is on its way.
Comment 19 Alon Levy 2011-02-27 12:07:37 UTC
Comment on attachment 43885 [details] [review]
server: simplify and constify sync_write()

>From 1049e3f5983b8fb502fd2228b10f2bf3b6fb00fc Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@redhat.com>
>Date: Fri, 11 Feb 2011 04:00:27 +0100
>Subject: [PATCH] server: simplify and constify sync_write()
>
>+ symplify, improving style of code using it.
>
>https://bugs.freedesktop.org/show_bug.cgi?id=34795
>---
> server/reds.c |   31 +++++++++++++++----------------
> 1 files changed, 15 insertions(+), 16 deletions(-)
>
>diff --git a/server/reds.c b/server/reds.c
>index eb1ea44..1e7e318 100644
>--- a/server/reds.c
>+++ b/server/reds.c
>@@ -1321,9 +1321,9 @@ void reds_on_main_receive_migrate_data(MainMigrateData *data, uint8_t *end)
>     while (write_to_vdi_port() || read_from_vdi_port());
> }
> 
>-static int sync_write(RedsStream *stream, void *in_buf, size_t n)
>+static int sync_write(RedsStream *stream, const void *in_buf, size_t n)
> {
>-    uint8_t *buf = (uint8_t *)in_buf;
>+    const uint8_t *buf = (uint8_t *)in_buf;
>     while (n) {
>         int now = reds_stream_write(stream, buf, n);
>         if (now <= 0) {
>@@ -1342,6 +1342,7 @@ static int reds_send_link_ack(RedLinkInfo *link)
> {
>     SpiceLinkHeader header;
>     SpiceLinkReply ack;
>+    Channel caps = { 0, };
>     Channel *channel;
>     BUF_MEM *bmBuf;
>     BIO *bio;
>@@ -1354,14 +1355,14 @@ static int reds_send_link_ack(RedLinkInfo *link)
> 
>     ack.error = SPICE_LINK_ERR_OK;
> 
>-    if ((channel = reds_find_channel(link->link_mess->channel_type, 0))) {
>-        ack.num_common_caps = channel->num_common_caps;
>-        ack.num_channel_caps = channel->num_caps;
>-        header.size += (ack.num_common_caps + ack.num_channel_caps) * sizeof(uint32_t);
>-    } else {
>-        ack.num_common_caps = 0;
>-        ack.num_channel_caps = 0;
>+    channel = reds_find_channel(link->link_mess->channel_type, 0);
>+    if (!channel) {
>+        channel = &caps;
>     }
>+
>+    ack.num_common_caps = channel->num_common_caps;
>+    ack.num_channel_caps = channel->num_caps;
>+    header.size += (ack.num_common_caps + ack.num_channel_caps) * sizeof(uint32_t);
>     ack.caps_offset = sizeof(SpiceLinkReply);
> 
>     if (!(link->tiTicketing.rsa = RSA_new())) {
>@@ -1382,13 +1383,11 @@ static int reds_send_link_ack(RedLinkInfo *link)
>     BIO_get_mem_ptr(bio, &bmBuf);
>     memcpy(ack.pub_key, bmBuf->data, sizeof(ack.pub_key));
> 
>-    ret = sync_write(link->stream, &header, sizeof(header)) && sync_write(link->stream, &ack,
>-                                                                        sizeof(ack));
>-    if (channel) {
>-        ret = ret && sync_write(link->stream, channel->common_caps,
>-                                channel->num_common_caps * sizeof(uint32_t)) &&
>-              sync_write(link->stream, channel->caps, channel->num_caps * sizeof(uint32_t));
>-    }
>+    ret = sync_write(link->stream, &header, sizeof(header));
>+    ret &= sync_write(link->stream, &ack, sizeof(ack));
>+    ret &= sync_write(link->stream, channel->common_caps, channel->num_common_caps * sizeof(uint32_t));
>+    ret &= sync_write(link->stream, channel->caps, channel->num_caps * sizeof(uint32_t));
>+

So what about this - in the last round you said you weren't sure if it is good enough? you
said it does sync_write's even if the channel is not found - it looks like it still does.

>     BIO_free(bio);
>     return ret;
> }
>-- 
>1.7.4
Comment 20 Marc-Andre Lureau 2011-02-27 23:36:01 UTC
(In reply to comment #19)

> >-    if (channel) {
> >-        ret = ret && sync_write(link->stream, channel->common_caps,
> >-                                channel->num_common_caps * sizeof(uint32_t)) &&
> >-              sync_write(link->stream, channel->caps, channel->num_caps * sizeof(uint32_t));
> >-    }
> >+    ret = sync_write(link->stream, &header, sizeof(header));
> >+    ret &= sync_write(link->stream, &ack, sizeof(ack));
> >+    ret &= sync_write(link->stream, channel->common_caps, channel->num_common_caps * sizeof(uint32_t));
> >+    ret &= sync_write(link->stream, channel->caps, channel->num_caps * sizeof(uint32_t));
> >+
> 
> So what about this - in the last round you said you weren't sure if it is good
> enough? you
> said it does sync_write's even if the channel is not found - it looks like it
> still does.
> 

Yeah, I wasn't sure about it, but I realized that the previous version of the code had channel->num_{,common}_caps == 0. So that's why there was a if(channel) condition.

Now, it goes through sync_write anyway, either channel->num_{,common}_caps == 0 (without the AUTH_SELECTION cap, or with it, then channel->num_common_caps == 1)

So it looks fine to me. Agree?
Comment 21 Alon Levy 2011-02-28 06:56:37 UTC
ACK to all. (don't forget the SPICE_UNUSED and the fix to the sync_write - please post the updated patches here for future reference).
Comment 22 Marc-Andre Lureau 2011-02-28 07:46:12 UTC
committed to master (ghhh, I forgot to reorder: build add --with-sasl)

commit 8f9cbd19dbf3612abeafae60bdc4df2a97552b91
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Fri Feb 11 04:04:28 2011 +0100

    server: add SASL support
    
    We introduce 2 public functions to integrate with the library user.
    
    spice_server_set_sasl() - turn on SASL
    spice_server_set_sasl_appname() - specify the name of the app (It is
    used for where to find the default configuration file)
    
    The patch for QEMU is on its way.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit f4dddc50f06f90d7ff97d4ed60c6197c85814c5e
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Tue Feb 22 13:54:09 2011 +0100

    server: add auth mechanism selection
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 4f983b2c9c642ac96abf214cefc1c51fad90791e
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Tue Feb 22 03:52:38 2011 +0100

    server: add reds_channel_dispose()
    
    Try to have a common base dispose() method for channels. For now, it
    just free the caps.
    
    Make use of it in snd_worker, and in sync_write() - sync_write() is
    going to have default caps later on.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 09c01c9516045cfad1a95108f0386e50fcd9dfa0
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Fri Feb 11 04:00:27 2011 +0100

    server: simplify and constify sync_write()
    
    + symplify, improving style of code using it.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 37dbb8aec917f02075094d991b471b56f5f0a4a6
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Fri Feb 11 03:57:59 2011 +0100

    server: pull out reds_handle_link(), for future reuse
    
    + a couple of indent, style change
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 414e1de7207c1c807832836d56f5e301e5cb156f
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Fri Feb 11 03:50:51 2011 +0100

    build: add --with-sasl
    
    Using cyrus SASL library (same as gtk-vnc/qemu).
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 2bbb4fca0c97043b183fb65126b7198bc0750a78
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Wed Feb 9 15:11:22 2011 +0100

    common: add SpiceBuffer - based on qemu-vnc Buffer
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 2ced8996c86605bad91c7d2f73ea5460252b4517
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Thu Feb 10 04:26:00 2011 +0100

    server/reds: make writev fallback more generic
    
    We are going to reuse it for SASL/SSF encode write().
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 76dc27f08a15bd2cb0e0f536972e361e6d7acc25
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Wed Feb 9 21:44:45 2011 +0100

    server: rename s/peer/stream
    
    This is stylish change again. We are talking about a RedStream object,
    so let's just name the variable "stream" everywhere, to avoid
    confusion with a non existent RedPeer object.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795


commit 1a4923c2107c89314714718ca1431d232cc23edf
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Thu Feb 10 03:00:57 2011 +0100

    server/reds: remove the void* ctx field
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit fc5d7f76257bf51c693dfb888c1a3dd61d8d2318
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Sun Feb 27 13:14:00 2011 +0100

    server: use the new reds_stream_{read,write}
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit f32503258fa1f0a88de410005609770bd23cac97
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Sun Feb 27 13:40:52 2011 +0100

    server: remove cb_free, not needed anymore
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 07d6dd61080efdfe97fbce7a9a12abb7f315bdc3
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Sun Feb 27 13:33:28 2011 +0100

    server: use reds_{link,stream}_free()
    
    Be carefull removing the watch before, like __release_link
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit fca602124ded4b3333344a8737a08751635bacdb
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Sun Feb 27 13:31:31 2011 +0100

    server: use reds_stream_remove_watch() helper
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit b79e7320de6b2cd07000b9713ec77f3439eaf80d
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Sun Feb 27 09:46:14 2011 +0100

    server: add reds_stream_{read,write,free,remove_watch}()
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit d47912241f4bc263cf3c19c07ed3907681826277
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Sun Feb 27 09:38:04 2011 +0100

    server: s/RedsStreamContext/RedsStream
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 29be54f6d3549c44b2b771ca3c21952d2d1b7026
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Wed Feb 9 18:01:18 2011 +0100

    server/reds: remove unused readv
    
    Let's not bother with it since nobody uses it, and it's not implemented for SSL anyway
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

commit 0e64e2d02c7353ccad8951dfb9b5ec8b03cd59ee
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Tue Nov 9 15:16:41 2010 +0100

    build: make it silent
    
    This patch make it easier to spot warnings in compilation.  It should
    work with older versions of automake that don't support silent rules.
    
    If you want verbose build, make V=1.
    
    Signed-off-by: Uri Lublin <uril@redhat.com>
    
    https://bugs.freedesktop.org/show_bug.cgi?id=34795

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.