On 05/02/2012 05:44 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list