[Spice-devel] [PATCH 13/13] server: add SASL support

[Marc-André Lureau]

On Thu, Feb 24, 2011 at 10:19 PM, Alon Levy <alevy at redhat.com> wrote:
> On Tue, Feb 22, 2011 at 05:09:07PM +0100, Marc-André Lureau wrote:
>> 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)
>>
>
> Do you think this could be made a separate file? reds_sasl.c? Just a thought.

We could, I don't really mind. But the API should be availble either
or not SASL is compiled in.

>>  void reds_channel_init_auth_caps(Channel *channel)
>>  {
>> -    reds_channel_set_common_caps(channel, SPICE_COMMON_CAP_AUTH_SPICE, TRUE);
>> +    if (sasl_enabled)
>> +        reds_channel_set_common_caps(channel, SPICE_COMMON_CAP_AUTH_SASL, TRUE);
>> +    else
>> +        reds_channel_set_common_caps(channel, SPICE_COMMON_CAP_AUTH_SPICE, TRUE);
>
> Just to be sure I understand: we either support SASL authentication, or SPICE, but
> never both? unless we talk to a client that doesn't understand the AUTH_SELECTION,
> and then we fall back to SPICE?

No, if SASL is enabled, we should only accept SASL (it's enforce
later, not only in the caps).

We could do that only at the client/qemu level, and not enforce it
here. Your thoughts?

>>      reds_channel_set_common_caps(channel, SPICE_COMMON_CAP_PROTOCOL_AUTH_SELECTION, TRUE);
>>  }
>>
>> @@ -1487,7 +1498,7 @@ static void reds_handle_main_link(RedLinkInfo *link)
>>      link_mess = link->link_mess;
>>      reds_disconnect();
>>
>> -    if (!link_mess->connection_id) {
>> +    if (link_mess->connection_id == 0) {
>>          reds_send_link_result(link, SPICE_LINK_ERR_OK);
>>          while((connection_id = rand()) == 0);
>>          reds->agent_state.num_tokens = 0;
>> @@ -1671,6 +1682,100 @@ static inline void async_read_clear_handlers(AsyncRead *obj)
>>      obj->stream->watch = NULL;
>>  }
>>
>> +#if HAVE_SASL
>> +static int sync_write_u8(RedsStream *s, uint8_t n)
>> +{
>> +    return sync_write(s, &n, sizeof(uint8_t));
>> +}
>> +
>> +static int sync_write_u32(RedsStream *s, uint32_t n)
>> +{
>> +    return sync_write(s, &n, sizeof(uint32_t));
>> +}
>> +
>> +ssize_t reds_stream_sasl_write(RedsStream *s, const void *buf, size_t nbyte)
>> +{
>> +    ssize_t ret;
>> +
>> +    if (!s->sasl.encoded) {
>> +        int err;
>> +        err = sasl_encode(s->sasl.conn, (char *)buf, nbyte,
>> +                          (const char **)&s->sasl.encoded,
>> +                          &s->sasl.encodedLength);
>> +        if (err != SASL_OK) {
>> +            red_printf("sasl_encode error: %d", err);
>> +            return -1;
>> +        }
>> +
>> +        if (s->sasl.encodedLength == 0)
>> +            return 0;
> Missing {}

Argh.. is that in the coding style? Tbh, I don't like the extra {},
they just make the code more heavy for one line imho.. (perhaps this
is some python influence on me?)
>> +    if (sasl->len < 1 || sasl->len > 100) {
>> +        red_printf("Got client mechname len %d", sasl->len);
> "Got bad client mechname len %d" FTFY.

ok

>>  ssize_t reds_stream_write(RedsStream *s, const void *buf, size_t nbyte)
>>  {
>> -    return s->write(s, buf, nbyte);
>> +    ssize_t ret;
>> +
>> +#if HAVE_SASL
>> +    if (s->sasl.conn && s->sasl.runSSF)
>> +        ret = reds_stream_sasl_write(s, buf, nbyte);
> {}
>> +    else
>> +#endif
>> +        ret = s->write(s, buf, nbyte);
> {}
>> +
>
> Wouldn't this be nicer using just a callback? (like olden times) Well, I don't
> care to write that patch, so I don't mind if this stays.

you mean set s->write to reds_stream_sasl_write() at appropriate time?
That sounds possible. Is that what you asked?

>> +#if HAVE_SASL
>> +typedef struct RedsSASLContext {
>
> So you use Context here after removing it from RedsStream??
> How about just RedsSASL?

Yeah, sometime I make irony! ;) ok, it was not intended, I probably
did it because of RedsStreamContext, then I changed RedsStream but
forgot RedsSASL...

Agreed, it needs to be fixed.

thanks

-- 
Marc-André Lureau