5:06 p.m.
On 05/02/2012 05:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
A bit sparse on the commit message, maybe even mentioning that
src/access/apis.txt contains the details would help.
You are adding a new directory; do any of the cfg.mk syntax check rules
need to be updated to state which other drivers can include the headers
from this directory?
+++ b/src/access/apis.txt
@@ -0,0 +1,577 @@
Copyright header? Also, can you add a short description about what the
rest of this file is describing?
+
+Non-driver based APIs
+
+
+virConnCopyLastError:
+virResetError:
+virResetLastError:
+virSaveLastError:
+virSetErrorFunc:
+virConnGetLastError:
+virConnResetLastError:
+virConnSetErrorFunc:
+virCopyLastError:
+virDefaultErrorFunc:
+virFreeError:
+virGetLastError:
You know, we really ought to divide libvirt.h into multiple headers, one
per object type, to make this a bit easier to track.
+
+virConnectBaselineCPU:
+
+ - No state access
+
+virConnectCompareCPU:
+
+ - Access host CPU
If I understand this file correctly, you were basically enumerating
which APIs need which access controls. Does that mean that adding a new
API to libvirt.h will now also require updating this file? Is there any
way to write a syntax-check rule that ensures that all public functions
also have an entry in this file, to make sure we don't forget?
+++ b/src/access/viraccessdriver.h
+++ b/src/access/viraccessdrivernop.c
@@ -0,0 +1,44 @@
+/*
+ * viraccessdrivernop.c: no-op access control driver
+ *
+ * Copyright (C) 2011 Red Hat, Inc.
2012 now (wow, you've been working on this for some time now...)
+++ b/src/access/viraccessdrivernop.h
@@ -0,0 +1,28 @@
+/*
+ * viraccessdrivernop.h: no-op access control driver
+ *
+ * Copyright (C) 2011 Red Hat, Inc.
2012
+++ b/src/access/viraccessdriverstack.c
+static bool
+virAccessDriverStackCheckConnect(virAccessManagerPtr manager,
+ virAccessPermConnect av)
+{
+ virAccessDriverStackPrivatePtr priv = virAccessManagerGetPrivateData(manager);
+ bool ret = true;
+ size_t i;
+
+ for (i = 0 ; i < priv->managersLen ; i++) {
+ /* We do not short-circuit on first denial - always check all drivers */
+ if (!virAccessManagerCheckConnect(priv->managers[i], av))
+ ret = false;
So anyone can deny, but all are given a shot at it in case the access
manager has side effects such as auditing their own decision (but note
that an audited success may be countermanded by a different manager's
denial). Seems reasonable.
+++ b/src/access/viraccessmanager.c
+
+virIdentityPtr virAccessManagerGetSystemIdentity(void)
+{
+ char *username = NULL;
+ char *groupname = NULL;
+ char *seccontext = NULL;
+ virIdentityPtr ret = NULL;
+ gid_t gid = getgid();
+ uid_t uid = getuid();
+#if HAVE_SELINUX
+ security_context_t con;
+#endif
+
+ if (!(username = virGetUserName(uid)))
+ goto cleanup;
+ if (!(groupname = virGetGroupName(gid)))
+ goto cleanup;
+
+#if HAVE_SELINUX
+ if (getcon(&con) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to lookup SELinux process context"));
+ goto cleanup;
Does this fail even if SELinux is not enabled? And if so, what is the
impact of failing here?
+ }
+ seccontext = strdup(con);
+ freecon(con);
+ if (!seccontext) {
+ virReportOOMError();
+ goto cleanup;
+ }
+#endif
+
+ if (!(ret = virIdentityNew()))
+ goto cleanup;
+
+ if (username &&
+ virIdentitySetAttr(ret, VIR_IDENTITY_ATTR_UNIX_USER_NAME, username) < 0)
+ goto error;
+ if (groupname &&
+ virIdentitySetAttr(ret, VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, groupname) < 0)
+ goto error;
+ if (seccontext &&
+ virIdentitySetAttr(ret, VIR_IDENTITY_ATTR_SECURITY_CONTEXT, seccontext) < 0)
+ goto error;
Especially since here, you seem to be catering to the case of a NULL
seccontext when SELinux was not compiled in.
+int virAccessManagerSetEffectiveIdentity(virIdentityPtr identity)
+{
+ virIdentityPtr old;
+
+ if (!virAccessManagerInit())
+ return -1;
+
+ old = virThreadLocalGet(&effectiveIdentity);
+ if (old)
+ virIdentityFree(old);
This loses the old identity,
+
+ if (virThreadLocalSet(&effectiveIdentity, identity) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to set thread local identity"));
+ return -1;
even if we fail to set the new one. I think you need to swap these two
cases (and keep old in force on failure to set a new identity).
+int virAccessManagerSetRealIdentity(virIdentityPtr identity)
+{
+ virIdentityPtr old;
+
+ if (!virAccessManagerInit())
+ return -1;
+
+ old = virThreadLocalGet(&realIdentity);
+ if (old)
+ virIdentityFree(old);
+
+ if (virThreadLocalSet(&realIdentity, identity) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to set thread local identity"));
+ return -1;
+ }
Same comment.
+void *virAccessManagerGetPrivateData(virAccessManagerPtr mgr)
+{
+ /* This accesses the memory just beyond mgr, which was allocated
+ * via VIR_ALLOC_VAR earlier. */
+ return mgr + 1;
Phew - we learned our lesson with the security manager.
+
+typedef enum {
+ VIR_ACCESS_PERM_CONNECT_GETATTR,
+ VIR_ACCESS_PERM_CONNECT_SEARCH_DOMAINS,
+
+ VIR_ACCESS_PERM_CONNECT_LAST,
+} virAccessPermConnect;
+
+typedef enum {
+ VIR_ACCESS_PERM_DOMAIN_GETATTR, /* Name/ID/UUID access */
+ VIR_ACCESS_PERM_DOMAIN_READ, /* Config access */
+ VIR_ACCESS_PERM_DOMAIN_WRITE, /* Config change */
+ VIR_ACCESS_PERM_DOMAIN_READ_SECURE,
+
+ VIR_ACCESS_PERM_DOMAIN_START,
+ VIR_ACCESS_PERM_DOMAIN_STOP,
+
+ VIR_ACCESS_PERM_DOMAIN_SAVE,
+ VIR_ACCESS_PERM_DOMAIN_DELETE,
+
+ /* Merge these 3 into 1 ? */
+ VIR_ACCESS_PERM_DOMAIN_SHUTDOWN,
+ VIR_ACCESS_PERM_DOMAIN_REBOOT,
+ VIR_ACCESS_PERM_DOMAIN_RESET,
Maybe not. While reboot can reuse an existing qemu process,
shutdown/start will create a new qemu process, and there may be
ramifications to that difference (such as whether a vnc session to qemu
has to reconnect). Reboot and Reset might be mergeable, but I think
I've made a case for keeping shutdown separate.
+++ b/src/libvirt_private.syms
@@ -1181,6 +1181,27 @@ virAuthConfigNew;
virAuthConfigNewData;
+# viraccessmanager.h
Sorting: viraccessmanager.h comes before virauth.h, and...
+virAccessManagerInit;
+virAccessManagerGetSystemIdentity;
+virAccessManagerGetRealIdentity;
+virAccessManagerGetEffectiveIdentity;
+virAccessManagerSetRealIdentity;
+virAccessManagerSetEffectiveIdentity;
+virAccessManagerNew;
+virAccessManagerNewStack;
+virAccessManagerFree;
+virAccessManagerCheckConnect;
+virAccessManagerCheckDomain;
within the file, sort the function names.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org