[Libvir] PATCH: 1/10: SASL authentication support

[Daniel P. Berrange]

On Thu, Nov 29, 2007 at 01:08:52PM -0500, Daniel Veillard wrote:
> On Thu, Nov 29, 2007 at 05:16:34PM +0000, Daniel P. Berrange wrote:
> > This patch hooks up the basic authentication RPC calls, and the specific
> > SASL implementation. The SASL impl can be enabled/disable via the configurre
> > script with --without-sasl / --with-sasl - it'll auto-enable it if it finds
> > the headers & libs OK.
> > 
> > The sample /etc/sasl2/libvirt.conf file enables the DIGEST-MD5 mechanism
> > by default, since it is by far the easiest to setup for admins. No need for
> > a Kerberos server, or certificates - it just uses username/password which
> > can be set with 'saslpasswd2 -a libvirt [username]' and a list of all active
> > users viewed with 'sasldblistusers2 -a libvirt'
> 
>   Hum, shouldn't the spec file then 
>    Requires: cyrus-sasl-md5 
> to make sure this works ?

Yes & no.  Technically we only require the base library. Anything else is
an admin choice. I'm inclined to add a dep on cyrus-sasl-md5 though because
it will be our default auth mechanism, so it'll safe plenty of BZ reports if
we just always require it.

> [...]
> > diff -r 1c3780349e89 libvirt.spec.in
> > --- a/libvirt.spec.in	Wed Nov 28 12:02:28 2007 -0500
> > +++ b/libvirt.spec.in	Thu Nov 29 09:24:10 2007 -0500
> > @@ -16,6 +16,8 @@ Requires: dnsmasq
> >  Requires: dnsmasq
> >  Requires: bridge-utils
> >  Requires: iptables
> > +Requires: cyrus-sasl
> > +Requires: cyrus-sasl-gssapi
> 
> Requires: cyrus-sasl-md5 ?

Yes, and I'm going to remove the gssapi one, since that's not eenabled by
default in my latest patches & only a minority of people will use it.

> > @@ -730,15 +735,28 @@ static struct qemud_server *qemudInitial
> >  
> >      virStateInitialize();
> >  
> > +#if HAVE_SASL
> > +    if ((err = sasl_server_init(NULL, "libvirt")) != SASL_OK) {
> > +        qemudLog(QEMUD_ERR, "Failed to initialize SASL authentication %s",
> > +                 sasl_errstring(err, NULL, NULL));
> > +        goto cleanup;
> > +    }
> > +#endif
> > +
> 
>   So if libvirt was configured/compiled with SASL but for some reason we fail
> to initialize SASL we can't start the daemon at all ? Isn't that a bit
> excessive ?

Failure of the sasl_server_init  function is a pretty serious problem - something
is majorly messed up in your OS install.  We shouldn't pretend we can safely
continue.

In any case, the 4th patch in this serieschanges this call so that sasl_server_init
is only run, if the SASL auth is actually  configured on one of the sockets. So 
if someone did have a problem with this call failing, they could simply edit the
config file & turn off SASL auth (or fix the root problem).

> 
> [...]
> >          if (listen_tls) {
> >              if (remoteInitializeGnuTLS () < 0)
> >                  goto cleanup;
> >  
> > -            if (remoteListenTCP (server, tls_port, 1) < 0)
> > +            if (remoteListenTCP (server, tls_port, 1, REMOTE_AUTH_NONE) < 0)
> >                  goto cleanup;
> >          }
> >      }
> 
>   Looks to me that as long as TLS can still work we should not fail on 
> sasl_server_init error, right ?

The 4th patch will make it possible to run SASL on all the sockets - UNIX,
TCP and TLS.

> > @@ -325,6 +356,12 @@ doRemoteOpen (virConnectPtr conn, struct
> >      } else
> >          port = NULL;           /* Port not used for unix, ext. */
> >  
> > +
> > +    priv->hostname = strdup (uri->server ? uri->server : "localhost");
> > +    if (!priv->hostname) {
> > +        error (NULL, VIR_ERR_NO_MEMORY, "allocating priv->hostname");
> > +        goto failed;
> > +    }
> >      if (uri->user) {
> >          username = strdup (uri->user);
> >          if (!username) goto out_of_memory;
> 
>    Actually there we should looks for a password and store it, that's very
> common and convenient, e.g. use
>    xen://foo:bar@server/
> 
> as the connection URI, libxml2 will just return the user as 'foo:bar'
> which could subsequently be split here to store the password (bar).

The virConnectCredentialPtr struct which is populated for the auth
callback function contains a 'defresult' field where the default value
of the credential should go. I intended to populate this value with the
username part of the URI for VIR_CRED_AUTHNAME credentials, but forgot.
Will add that in....

Using passwords in URIs is seriously frowned upon. URIs get into log files,
in the command line ARGV, into gconf, into bug reports. We absolutely do 
not want passwords visible in any of those places.

RFC 2396  explicitly recommends against using passwords in URIs

  "Some URL schemes use the format "user:password" in the userinfo
   field. This practice is NOT RECOMMENDED, because the passing of
   authentication information in clear text (such as URI) has proven to
   be a security risk in almost every case where it has been used."

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|