On 05/02/2012 05:44 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org