Merge ~jslarraz/ubuntu-qa-tools:fix-acl into ubuntu-qa-tools:master

Proposed by Jorge Sancho Larraz
Status: Merged
Merged at revision: 0b3f2edf7a51829289f74237854e95ce7adcfa56
Proposed branch: ~jslarraz/ubuntu-qa-tools:fix-acl
Merge into: ubuntu-qa-tools:master
Diff against target: 15 lines (+3/-1)
1 file modified
vm-tools/uvt (+3/-1)
Reviewer Review Type Date Requested Status
Marc Deslauriers Approve
Review via email: mp+464752@code.launchpad.net

Commit message

Fix acl permission check

Description of the change

When using acl, the `mask` attribute sets the maximum permissions a non-owner user may have. Thus, this field also needs to be check to ensure the `libvirt-qemu` has effective search permissions on the required directories

To post a comment you must log in.
Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

LGTM, ack.

review: Approve
Revision history for this message
Steve Beattie (sbeattie) wrote :

On Mon, Apr 22, 2024 at 12:11:21PM -0000, Jorge Sancho Larraz wrote:
> When using acl, the `mask` attribute sets the maximum permissions a non-owner user may have. Thus, this field also needs to be check to ensure the `libvirt-qemu` has effective search permissions on the required directories
> --
> You are subscribed to branch ubuntu-qa-tools:master.

> diff --git a/vm-tools/uvt b/vm-tools/uvt
> index 2f6eca3..36a3c31 100755
> --- a/vm-tools/uvt
> +++ b/vm-tools/uvt
> @@ -3626,7 +3626,9 @@ def load_uvt_config():
> path = config[d]
> while path != "/":
> rc, out = runcmd(["getfacl", "-e", path])
> - if (not os.stat(path).st_mode & 0o001) and (re.search("user:libvirt-qemu:..x", out) is None):
> + if (not os.stat(path).st_mode & 0o001) and \
> + ((re.search("user:libvirt-qemu:..x", out) is None) or
> + (re.search("mask::..x", out) is None)):

Oh, I hadn't noticed we'd started using ACLs here as a fallback, yay!

This has already been merged, so treat this as a possible suggestion
for a future improvement, but rather than trying to parse the
output of the getfacl command (and making sure that things like the
LANG environment variable are set), would it make sense to use the
python3-pylibacl library to query acl status directly instead? It's
available in all the currently supported Ubuntu releases.

That said, it's not exactly the easiest interface to use, being a python
wrapper around libacl:

>>> import posix1e
>>> import pwd
>>> acls = posix1e.ACL(file="testimage-focal-amd64.qcow2")
>>> for acl in acls:
... print(acl)
... if acl.tag_type == posix1e.ACL_USER:
... print(f"--> username = {pwd.getpwuid(acl.qualifier).pw_name}")
... if acl.tag_type in (posix1e.ACL_MASK, posix1e.ACL_USER):
... print(f"--> read = {acl.permset.read}")
... print(f"--> write = {acl.permset.write}")
... print(f"--> execute = {acl.permset.execute}")
...
ACL entry for the owner
ACL entry for user with uid 64055
--> username = libvirt-qemu
--> read = True
--> write = False
--> execute = True
ACL entry for the group
ACL entry for the mask
--> read = True
--> write = False
--> execute = True
ACL entry for the others

--
Steve Beattie
<email address hidden>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/vm-tools/uvt b/vm-tools/uvt
2index 2f6eca3..36a3c31 100755
3--- a/vm-tools/uvt
4+++ b/vm-tools/uvt
5@@ -3626,7 +3626,9 @@ def load_uvt_config():
6 path = config[d]
7 while path != "/":
8 rc, out = runcmd(["getfacl", "-e", path])
9- if (not os.stat(path).st_mode & 0o001) and (re.search("user:libvirt-qemu:..x", out) is None):
10+ if (not os.stat(path).st_mode & 0o001) and \
11+ ((re.search("user:libvirt-qemu:..x", out) is None) or
12+ (re.search("mask::..x", out) is None)):
13
14 print("Missing permissions found while creating '%s' directory. libvirt-qemu user "
15 "requires search permission all the way up the path, but it seems to be"

Subscribers

People subscribed via source and target branches