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

[Alon Levy]

On Fri, Feb 25, 2011 at 12:46:34AM +0100, Marc-André Lureau wrote:
> 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.

Forget it, not sure it helps.

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

Ok, I think it's fine the way it is.

> >>      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?)

I'm a pythonista too, and I actually think it looks better with the squiglies.
It's just blocks, c style - and being consistent everywhere, python style.

> >> +    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?
Before we had always user doing the dereference, you changed it to a more oop
style, I think that's good, so actually I take back my request (didn't like the
fact we now have two function calls, but I guess the compiler optimizes this so
I shouldn't worry so much).

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