On Wed, Mar 06, 2013 at 03:05:55PM +0100, Michal Privoznik wrote:
> For now, only two APIs are implemented:
> virFileSetACL for setting requested permissions for a specific user,
> virFileRemoveACL to remove those permissions.
>
> Both these traverse the whole path from root directory and set or
> unset S_IXUSR flag on all directories met so user can really
> access the file.
> ---
> configure.ac | 1 +
> libvirt.spec.in | 3 +
> src/libvirt_private.syms | 2 +
> src/util/virfile.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virfile.h | 7 ++
> 5 files changed, 215 insertions(+)
>
> diff --git a/configure.ac b/configure.ac
> index cbb20c4..0df8a72 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -276,6 +276,7 @@ AM_CONDITIONAL([HAVE_LIBTASN1], [test "x$ac_cv_header_libtasn1_h" = "xyes"])
>
> AC_CHECK_LIB([intl],[gettext],[])
> AC_CHECK_LIB([attr],[getxattr])
> +AC_CHECK_LIB([acl], [acl_init])
Don't do this - add a m4/virt-acl.m4 file, following the
example of virt-attr.m4 and then just call it from the
same place as other library checks.
You'll need to add libs + cflags variables to the Makefile.am too
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index aa4579e..5d4254d 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -42,6 +42,11 @@
> # include <sys/types.h>
> #endif
>
> +#ifdef HAVE_LIBACL
> +# include <sys/acl.h>
> +# include <sys/types.h>
> +#endif
> +
> #include "vircommand.h"
> #include "configmake.h"
> #include "viralloc.h"
> @@ -750,3 +755,200 @@ virFileRemoveAttr(const char *file ATTRIBUTE_UNUSED,
> return -1;
> }
> #endif /* HAVE_LIBATTR */
> +
> +#ifdef HAVE_LIBACL /* HAVE_LIBACL */
Pointless comment at end of line
> +static acl_entry_t
> +findACLEntry(acl_t acl, acl_tag_t type, id_t id)
Please use virFileACL as prefix for all methods, even static ones
> +{
> + acl_entry_t ent;
> + acl_tag_t e_type;
> + id_t *e_id_p;
> +
> + /* acl_get_entry returns 1 if there's an entry in @acl */
> + if (acl_get_entry(acl, ACL_FIRST_ENTRY, &ent) != 1)
> + return NULL;
> +
> + while (true) {
> + acl_get_tag_type(ent, &e_type);
> + if (e_type == type) {
> + if (id == ACL_UNDEFINED_ID)
> + return ent;
> +
> + if (!(e_id_p = acl_get_qualifier(ent)))
> + return NULL;
> + if (*e_id_p == id) {
> + acl_free(e_id_p);
> + return ent;
> + }
> + acl_free(e_id_p);
> + }
> + if (acl_get_entry(acl, ACL_NEXT_ENTRY, &ent) != 1)
> + return NULL;
> + }
Instead of while (true); use
do {
...
} while(acl_get_entry(acl, ACL_NEXT_ENTRY) != 0);
> +}
> +
> +static void
> +setACLPerms(acl_entry_t ent, mode_t perms)
> +{
> + acl_permset_t set;
> +
> + acl_get_permset(ent, &set);
> + if (perms & S_IRUSR)
> + acl_add_perm(set, ACL_READ);
> + else
> + acl_delete_perm(set, ACL_READ);
> + if (perms & S_IWUSR)
> + acl_add_perm(set, ACL_WRITE);
> + else
> + acl_delete_perm(set, ACL_WRITE);
> + if (perms & S_IXUSR)
> + acl_add_perm(set, ACL_EXECUTE);
> + else
> + acl_delete_perm(set, ACL_EXECUTE);
> +}
> +
> +static int
> +cloneACLEntry(acl_t fromAcl, acl_tag_t fromType, id_t user,
> + acl_t *toAcl, acl_tag_t toType)
> +{
> + acl_entry_t fromEntry, toEntry;
> + if (!(fromEntry = findACLEntry(fromAcl, fromType, user)))
> + return 1;
> +
> + if (acl_create_entry(toAcl, &toEntry) != 0) {
> + virReportSystemError(errno, "%s", _("Unable to clone ACL entry"));
> + return -1;
> + }
> +
> + acl_copy_entry(toEntry, fromEntry);
> + acl_set_tag_type(toEntry, toType);
> + return 0;
> +}
> +
> +static int
> +virFileSetOrRemoveACLHelper(const char *path,
> + uid_t user,
> + mode_t perms,
> + bool set)
> +{
> + int ret = -1;
> + acl_t acl;
> + acl_entry_t ent;
> +
> + if (!(acl = acl_get_file(path, ACL_TYPE_ACCESS))) {
> + virReportSystemError(errno, _("Unable to get ACL on %s"), path);
> + return ret;
> + }
> +
> + ent = findACLEntry(acl, ACL_USER, user);
> + if (!ent && set) {
> + if (acl_create_entry(&acl, &ent) < 0) {
> + virReportSystemError(errno, "%s", _("Unable to create ACL entity"));
> + goto cleanup;
> + }
> + acl_set_tag_type(ent, ACL_USER);
> + acl_set_qualifier(ent, &user);
> +
> + setACLPerms(ent, perms);
> +
> + /* Recompute mask */
> + if (!findACLEntry(acl, ACL_MASK, ACL_UNDEFINED_ID) &&
> + cloneACLEntry(acl, ACL_USER, user, &acl, ACL_MASK) < 0)
> + goto cleanup;
> + } else if (ent && !set) {
> + if (acl_delete_entry(acl, ent) < 0) {
> + virReportSystemError(errno, "%s", _("Unable to delete ACL entity"));
> + goto cleanup;
> + }
> +
> + if ((ent = findACLEntry(acl, ACL_MASK, user)) &&
> + acl_delete_entry(acl, ent) < 0) {
> + virReportSystemError(errno, "%s", _("Unable to delete ACL entity"));
> + goto cleanup;
> + }
> + }
> +
> + if (acl_set_file(path, ACL_TYPE_ACCESS, acl) < 0) {
> + virReportSystemError(errno, _("Unable to set ACL on %s"), path);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +cleanup:
> + acl_free(acl);
> + return ret;
> +}
> +
> +static int
> +virFileSetOrRemoveACL(const char *file,
> + uid_t user,
> + mode_t perms,
> + bool set)
> +{
> + int ret = -1;
> + char *fileDup = NULL;
> + char *p;
> +
> + if (!(fileDup = strdup(file))) {
> + virReportOOMError();
> + return ret;
> + }
> +
> + if (virFileSetOrRemoveACLHelper(file, user, perms, set) < 0)
> + goto cleanup;
> +
> + /* For parent directories we want executable flag */
> + perms |= S_IXUSR;
> +
> + while ((p = strrchr(fileDup, '/')) != fileDup) {
> + if (!p) {
> + virReportSystemError(EINVAL, _("Invalid relative path '%s'"), file);
> + goto cleanup;
> + }
> +
> + *p = '\0';
> +
> + if (virFileSetOrRemoveACLHelper(fileDup, user, perms, set) < 0)
> + goto cleanup;
> + }
Hmm, this is recursively setting ACLs all the way up the directory
tree. How is this going to work when some files share some parent
directories. eg
virFileSetACL("/var/lib/libvirt/images/foo.img");
Sets ACLs on /var, /var/lib, /var/lib/libvirt, /var/lib/libvirt/images
virFileSetACL("/var/lib/libvirt/images/bar.img");
This does the same
virFileRemoveACL("/var/lib/libvirt/images/foo.img");
Removes ACLs on /var, /var/lib, /var/lib/libvirt, /var/lib/libvirt/images
....but bar.img still wanted them set.
IMHO this recursive setting of ACLs doesn't belong in these APIs
because they can't safely unset things recursively.
Also such changing of directories is not related to conversion
from chmod -> acls. This is a completely new feature, so should
be done in a separate patch.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list