On Wed, Jan 14, 2009 at 07:32:28PM -0800, john.levon@xxxxxxx 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list