On Wed, Jan 14, 2009 at 07:32:28PM -0800, john.levon@sun.com wrote:

@@ -638,10 +657,32 @@ static int qemudInitPaths(struct qemud_s static int qemudInitPaths(struct qemud_server *server, char *sockname, char *roSockname, - int maxlen) { + int maxlen) +{ uid_t uid = geteuid(); - +#ifdef __sun + char *base = NULL; + + if (virAsprintf (&base, "%s/run/libvirt", LOCAL_STATE_DIR) == -1) { + VIR_ERROR0(_("Out of memory")); + return -1; + } + if (mkdir (base, 0755)) { + if (errno != EEXIST) { + VIR_ERROR0(_("unable to create rundir")); + free (base); + exit(-1); + } + } + + free (base); +#endif

This chunk is potentially usefull on Linux too, so can just remove the #ifdef here. Also make sure to use VIR_FREE rather than free() Finally, the exit(-1) should be a return -1;

+#ifdef __sun + if (uid == 60) { +#else if (!uid) { +#endif

Rather than have this magic value in the code, lets put a #define at the top of the file #ifdef __sun #define SYSTEM_ACCOUNT 60 #else #define SYSTEM_ACCOUNT 0 #endif And then just change all use of !uid to 'uid == SYSTEM_ACCOUNT'

+#ifdef __sun + { + ucred_t *ucred = NULL; + const priv_set_t *privs; + + if (getpeerucred (fd, &ucred) == -1 || + (privs = ucred_getprivset (ucred, PRIV_EFFECTIVE)) == NULL) { + if (ucred != NULL) + ucred_free (ucred); + close (fd); + return -1; + } + + if (!priv_ismember (privs, PRIV_VIRT_MANAGE)) { + ucred_free (ucred); + close (fd); + return -1; + } + + ucred_free (ucred);

Can move the ucred_free up before priv_ismember() call and thus avoid the need for the call in the cleanup path.

+ } +#endif /* __sun */ + /* Disable Nagle. Unix sockets will ignore this. */ setsockopt (fd, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, sizeof no_slow_start); @@ -2140,6 +2204,10 @@ remoteReadConfigFile (struct qemud_serve if (auth_unix_rw == REMOTE_AUTH_POLKIT) unix_sock_rw_mask = 0777; #endif +#ifdef __sun + unix_sock_rw_mask = 0666; +#endif

Can we move this up to the declaration point - this function should only be reacting to config file settings. FOr the global default we can just declare it #ifdef __sun static int unix_sock_rw_mask = 0666; /* Allow world */ static int unix_sock_ro_mask = 0777; /* Allow world */ #else static int unix_sock_rw_mask = 0700; /* Allow user only */ static int unix_sock_ro_mask = 0777; /* Allow world */ #endif

+#ifdef __sun +static void +qemudSetupPrivs (struct qemud_server *server) +{ + chown ("/var/run/libvirt", 60, 60); + chown ("/var/run/libvirt/libvirt-sock", 60, 60); + chmod ("/var/run/libvirt/libvirt-sock", 0666); + chown (server->logDir, 60, 60);

Again prefer to see the #define SYTEM_ACCOUNT instead of the magic numbers. Is the chmod of the socket really required for solaris ? We already set the umask before creating the socket, so it ought to get desired permissions from moment it appears. We avoided an explicit chmod because that leaves a gap between socket creation, and correct ownership and permissions being configured. Also, if this libvirtd is running as UID 60, so the chown really needed ? We also setgid before creating the socket so that it gets desired group ownership at time of creation, rather than having to change it post-create.

+ + if (__init_daemon_priv (PU_RESETGROUPS | PU_CLEARLIMITSET, + 60, 60, 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

/* Print command-line usage. */ static void @@ -2417,6 +2510,8 @@ int main(int argc, char **argv) { sig_action.sa_handler = SIG_IGN; sigaction(SIGPIPE, &sig_action, NULL);

+ qemudSetupPrivs(server); + if (!(server = qemudInitialize(sigpipe[0]))) {

This would appear to make it try to change the socket ownership and permissions, before we've actually created the socket, which is much later in the main() method where we call NetworkInit

diff --git a/qemud/remote.c b/qemud/remote.c --- a/qemud/remote.c +++ b/qemud/remote.c @@ -424,6 +424,15 @@ remoteDispatchOpen (struct qemud_server flags = args->flags; if (client->readonly) flags |= VIR_CONNECT_RO;

+#ifdef __sun + /* + * On Solaris, all clients are forced to go via virtd. As a result, + * virtd must indicate it really does want to connect to the + * hypervisor. + */ + name = "xen:///"; +#endif

This should not be neccessary if the client end + drivers are correctly written. The RPC calls handlers should not be trying to re-interpret args, they should just pass everything straight through to the libvirt APIs.

diff --git a/src/libvirt.c b/src/libvirt.c --- a/src/libvirt.c +++ b/src/libvirt.c @@ -46,6 +46,7 @@ #include "test.h" #endif #ifdef WITH_XEN +#include "xen_internal.h" #include "xen_unified.h" #endif #ifdef WITH_REMOTE @@ -825,6 +826,17 @@ do_open (const char *name, } }

+#ifdef __sun + /* + * If we're not libvirtd, force us to go via the daemon, unless we + * want the test hypervisor. + */ + if (name == NULL || !STRCASEEQLEN (name, "test://", 7)) { + if (geteuid() == 0 || !xenHavePrivilege()) + name = "remote+unix:///"; + } +#endif

This should also not be neccessary. If you want Xen to always go via the demon, the only change that should be required, is to make xenUnifiedOpen() return VIR_DRV_OPEN_DECLINED. The main virConnectOpen() logic will then fallthrough to the remote driver which will always accept any URI not handled by an earlier driver. So basically just need to edit xenUnifiedOpen() and make it rejects all open requests when not in the daemon context - I guess a privilege check would suffic for that logic ? If not, then we can register a trivial virStateDrv impl that just sets a flag when run as part of the daemon - see the 'remoteStartup' method in remote_internal.c for example of how to set a flag when running inside the daemon.

diff --git a/src/remote_internal.c b/src/remote_internal.c --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -903,18 +903,21 @@ remoteOpen (virConnectPtr conn, }

/* - * If URI is NULL, then do a UNIX connection - * possibly auto-spawning unprivileged server - * and probe remote server for URI + * If URI is NULL, then do a UNIX connection possibly auto-spawning + * unprivileged server and probe remote server for URI. On Solaris, + * this isn't supported, but we may be privileged enough to connect + * to the UNIX socket anyway. */ if (!conn->uri) { DEBUG0("Auto-probe remote URI"); rflags |= VIR_DRV_OPEN_REMOTE_UNIX; +#ifndef __sun if (getuid() > 0) { DEBUG0("Auto-spawn user daemon instance"); rflags |= VIR_DRV_OPEN_REMOTE_USER; rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART; } +#endif

Ok, not a big deal - it'll still work if explicitly requested with a qemu:///session URI - if we make QEMU driver work on Solaris later.

@@ -5086,8 +5089,7 @@ really_read_buf (virConnectPtr conn, str return -1; } if (err == 0) { - error (in_open ? NULL : conn, - VIR_ERR_RPC, _("socket closed unexpectedly")); + DEBUG("conn %p: socket closed unexpectedly", conn); return -1; } } else { @@ -5101,8 +5103,7 @@ really_read_buf (virConnectPtr conn, str return -1; } if (err == 0) { - error (in_open ? NULL : conn, - VIR_ERR_RPC, _("socket closed unexpectedly")); + DEBUG("conn %p: socket closed unexpectedly", conn); return -1; } }

These two I/O methods here have been completely re-written in my thread patches. Why is removing the error messages required ?

diff --git a/src/virsh.c b/src/virsh.c --- a/src/virsh.c +++ b/src/virsh.c @@ -28,6 +28,7 @@ #include <limits.h> #include <assert.h> #include <errno.h> +#include <signal.h> #include <sys/stat.h> #include <sys/wait.h> #include <inttypes.h> @@ -46,6 +47,7 @@ #include "util.h"

static char *progname; +static int sigpipe;

#ifndef TRUE #define TRUE 1 @@ -6984,12 +6986,22 @@ vshParseArgv(vshControl *ctl, int argc, return TRUE; }

+static void sigpipe_handler(int sig ATTRIBUTE_UNUSED) +{ + sigpipe = 1; + /* + * Force readline() to exit. + */ + close(STDIN_FILENO); +} + int main(int argc, char **argv) { vshControl _ctl, *ctl = &_ctl; char *defaultConn; int ret = TRUE; + struct sigaction sig_action;

if (!setlocale(LC_ALL, "")) { perror("setlocale"); @@ -7021,6 +7033,12 @@ main(int argc, char **argv) vshDeinit(ctl); exit(EXIT_FAILURE); } + + sig_action.sa_handler = sigpipe_handler; + sig_action.sa_flags = 0; + sigemptyset(&sig_action.sa_mask); + + sigaction(SIGPIPE, &sig_action, NULL);

if (!vshInit(ctl)) { vshDeinit(ctl); @@ -7061,6 +7079,13 @@ main(int argc, char **argv) fputc('\n', stdout); /* line break after alone prompt */ }

+ /* + * If the connection over a socket failed abruptly, it's probably + * due to not having the right privileges. + */ + if (sigpipe) + vshError(ctl, TRUE, _("failed to connect (insufficient privileges?)")); +

It will also be seen if the daemon drops the connection due to an OOM condition, or the max-clients limit being exceeded, so perhaps a little more detailed message.

diff --git a/src/xen_internal.c b/src/xen_internal.c --- a/src/xen_internal.c +++ b/src/xen_internal.c @@ -26,6 +26,17 @@ #include <errno.h> #include <sys/utsname.h>

+#ifdef __sun +#include <sys/systeminfo.h> + +#include <priv.h> + +#ifndef PRIV_XVM_CONTROL +#define PRIV_XVM_CONTROL ((const char *)"xvm_control") +#endif + +#endif /* __sun */ + /* required for dom0_getdomaininfo_t */ #include <xen/dom0_ops.h> #include <xen/version.h> @@ -35,10 +46,6 @@ #ifdef HAVE_XEN_SYS_PRIVCMD_H #include <xen/sys/privcmd.h> #endif -#endif - -#ifdef __sun -#include <sys/systeminfo.h> #endif

/* required for shutdown flags */ @@ -3387,3 +3394,17 @@ xenHypervisorGetVcpuMax(virDomainPtr dom return maxcpu; }

+/** + * xenHavePrivilege() + * + * Return true if the current process should be able to connect to Xen. + */ +int +xenHavePrivilege() +{ +#ifdef __sun + return priv_ineffect(PRIV_XVM_CONTROL); +#else + return getuid () == 0; +#endif +}

ACK, we've reviewed this bit several times before.

diff --git a/src/xen_internal.h b/src/xen_internal.h --- a/src/xen_internal.h +++ b/src/xen_internal.h @@ -104,4 +104,6 @@ int xenHypervisorNodeGetCellsFreeMem int startCell, int maxCells);

+int xenHavePrivilege(void); + #endif /* __VIR_XEN_INTERNAL_H__ */ diff --git a/src/xen_unified.c b/src/xen_unified.c --- a/src/xen_unified.c +++ b/src/xen_unified.c @@ -283,8 +283,8 @@ xenUnifiedOpen (virConnectPtr conn, virC priv->proxy = -1;

- /* Hypervisor is only run as root & required to succeed */ - if (getuid() == 0) { + /* Hypervisor is only run with privilege & required to succeed */ + if (xenHavePrivilege()) { DEBUG0("Trying hypervisor sub-driver"); if (drivers[XEN_UNIFIED_HYPERVISOR_OFFSET]->open(conn, auth, flags) == VIR_DRV_OPEN_SUCCESS) { @@ -293,7 +293,7 @@ xenUnifiedOpen (virConnectPtr conn, virC } }

- /* XenD is required to suceed if root. + /* XenD is required to succeed if privileged. * If it fails as non-root, then the proxy driver may take over */ DEBUG0("Trying XenD sub-driver"); @@ -318,12 +318,12 @@ xenUnifiedOpen (virConnectPtr conn, virC DEBUG0("Activated XS sub-driver"); priv->opened[XEN_UNIFIED_XS_OFFSET] = 1; } else { - if (getuid() == 0) - goto fail; /* XS is mandatory as root */ + if (xenHavePrivilege()) + goto fail; /* XS is mandatory when privileged */ } } else { - if (getuid() == 0) { - goto fail; /* XenD is mandatory as root */ + if (xenHavePrivilege()) { + goto fail; /* XenD is mandatory when privileged */ } else { #if WITH_PROXY DEBUG0("Trying proxy sub-driver");

ACK to this bit too.

diff --git a/src/xend_internal.c b/src/xend_internal.c --- a/src/xend_internal.c +++ b/src/xend_internal.c @@ -42,7 +42,7 @@ #include "buf.h" #include "uuid.h" #include "xen_unified.h" -#include "xen_internal.h" /* for DOM0_INTERFACE_VERSION */ +#include "xen_internal.h" #include "xs_internal.h" /* To extract VNC port & Serial console TTY */ #include "memory.h"

@@ -151,9 +151,10 @@ do_connect(virConnectPtr xend) s = -1;

/* - * Connecting to XenD as root is mandatory, so log this error + * Connecting to XenD when privileged is mandatory, so log this + * error */ - if (getuid() == 0) { + if (xenHavePrivilege()) { virXendError(xend, VIR_ERR_INTERNAL_ERROR, "%s", _("failed to connect to xend")); }

ACK

diff --git a/src/xs_internal.c b/src/xs_internal.c --- a/src/xs_internal.c +++ b/src/xs_internal.c @@ -35,7 +35,7 @@ #include "uuid.h" #include "xen_unified.h" #include "xs_internal.h" -#include "xen_internal.h" /* for xenHypervisorCheckID */ +#include "xen_internal.h"

#ifdef __linux__ #define XEN_HYPERVISOR_SOCKET "/proc/xen/privcmd" @@ -299,11 +299,11 @@ xenStoreOpen(virConnectPtr conn,

if (priv->xshandle == NULL) { /* - * not being able to connect via the socket as a normal user - * is rather normal, this should fallback to the proxy (or + * not being able to connect via the socket as an unprivileged + * user is rather normal, this should fallback to the proxy (or * remote) mechanism. */ - if (getuid() == 0) { + if (xenHavePrivilege()) { virXenStoreError(NULL, VIR_ERR_NO_XEN, "%s", _("failed to connect to Xen Store")); }

ACK 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 :|