[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Solaris least privilege support



On Thu, Jan 15, 2009 at 09:19:39AM -0800, john levon sun com wrote:

> +#ifdef __sun
> +static void
> +qemudSetupPrivs (struct qemud_server *server)
> +{
> +    chown ("/var/run/libvirt", SYSTEM_UID, SYSTEM_UID);
> +    chown (server->logDir, SYSTEM_UID, SYSTEM_UID);
> +
> +    if (__init_daemon_priv (PU_RESETGROUPS | PU_CLEARLIMITSET,
> +        SYSTEM_UID, SYSTEM_UID, PRIV_XVM_CONTROL, NULL)) {
> +        fprintf (stderr, "additional privileges are required\n");
> +        exit (1);
> +    }
> +
> +    if (priv_set (PRIV_OFF, PRIV_ALLSETS, PRIV_FILE_LINK_ANY, PRIV_PROC_INFO,
> +        PRIV_PROC_SESSION, PRIV_PROC_EXEC, PRIV_PROC_FORK, NULL)) {
> +        fprintf (stderr, "failed to set reduced privileges\n");
> +        exit (1);
> +    }
> +}
> +#else
> +#define qemudSetupPrivs(a)
> +#endif

I think this function should be made to return int 0/-1 for
failure, and let the caller propagate errors & do any cleanup
it may desire, since that's the way most other failures in
the daemon initialization are dealt with.

>  
>  /* Print command-line usage. */
>  static void
> @@ -2417,6 +2493,20 @@ int main(int argc, char **argv) {
>      sig_action.sa_handler = SIG_IGN;
>      sigaction(SIGPIPE, &sig_action, NULL);
>  
> +    /* Change the group ownership of /var/run/libvirt to unix_sock_gid */
> +    if (geteuid () == 0) {
> +        const char *rundir = LOCAL_STATE_DIR "/run/libvirt";
> +
> +        if (mkdir (rundir, 0755)) {
> +            if (errno != EEXIST) {
> +                VIR_ERROR0 (_("unable to create rundir"));
> +                return (-1);
> +            }
> +        }
> +    }
> +
> +    qemudSetupPrivs(server);

This would need a   "if(qemudSetupPrivs(server) < 0) return -1;"


> diff --git a/src/remote_internal.c b/src/remote_internal.c
> --- a/src/remote_internal.c
> +++ b/src/remote_internal.c
> @@ -90,7 +90,7 @@
>  #define MAGIC 999               /* private_data->magic if OK */
>  #define DEAD 998                /* private_data->magic if dead/closed */
>  
> -static int inside_daemon = 0;
> +int inside_daemon = 0;

Eek no, the Xen code shouldn't reference this variable directly - it
should track its own. We need to avoid compile dependancies inbetween
any of the drivers, because all drivers need to be able to be turned
on/off independantly, and may be dlopen'd at runtime, so you can't
guarentee you'll have access to another driver's data.

> diff --git a/src/xen_unified.c b/src/xen_unified.c
> --- a/src/xen_unified.c
> +++ b/src/xen_unified.c
> @@ -231,6 +231,16 @@ xenUnifiedOpen (virConnectPtr conn, virC
>      xenUnifiedPrivatePtr priv;
>      virDomainEventCallbackListPtr cbList;
>  
> +#ifdef __sun
> +    extern int inside_daemon;

This flags needs to be tracked by the Xen driver explicitly &
independantly of the daemon code.


Aside from those two issues, I think this patch looks pretty good now.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]