[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 06/12] Define basic internal API for access control



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]