Bug 985856

+++ This bug was initially created as a clone of Bug #985269 +++

Description of problem:
Command 'acl-set-file' failed for a specified user


Version-Release number of selected component (if applicable):
libguestfs-1.20.9-6.el6.x86_64


How reproducible:
always


Steps to reproduce:
1. Prepare a linux guest(rhel6.4), and add a user named "acltest" in guest
2. Create a file and set acl value for user 'acltest'
    # setfacl -m u:acltest:r-x /home/test

3. lauch guestfish to check
><fs> guestfish -a RHEL-Server-6.4-64-hvm.raw
><fs> run
><fs> mount-options acl /dev/VolGroup/lv_root /
><fs> acl-get-file /home/test access
user::rwx
user:500:r-x        # we can see it uses UID here
group::rwx
mask::rwx
other::rwx

><fs> cat /etc/passwd
...
acltest:500:500::/home/acltest:/bin/bash

# set acl value with user uid
><fs> acl-set-file /home/test access 'u:500:rwx,g::rwx,o::rwx'
libguestfs: error: acl_set_file: /home/test: Invalid argument

# set acl value with user name
><fs> acl-set-file /home/test access 'u:acltest:rwx,g::rwx,o::rwx'
libguestfs: error: acl_set_file: could not parse acl string: u:acltest:rwx,g::rwx,o::rwx: acl_from_text: Success


Actual results:
set acl value for a specified user failed


Expected results:
can set acl value for a specified user


Additional info:
Same issue is in rhel7

--- Additional comment from Richard W.M. Jones on 2013-07-17 04:59:39 EDT ---

We don't resolve UIDs against the guest database.  The caller
has to resolve usernames to UIDs (eg. using Augeas).  Compare
it to the 'chown' call which uses UIDs, not user names.

> ><fs> acl-set-file /home/test access 'u:500:rwx,g::rwx,o::rwx'
> libguestfs: error: acl_set_file: /home/test: Invalid argument

This failed presumably because you used the wrong path?

> ><fs> acl-set-file /home/test access 'u:acltest:rwx,g::rwx,o::rwx'
> libguestfs: error: acl_set_file: could not parse acl string: u:acltest:rwx,g::rwx,o::rwx: acl_from_text: Success

It shouldn't say "Success" here.  It looks like there's
a bug in acl_from_text -- it doesn't set errno.  According
to the documentation it should set errno.

--- Additional comment from Richard W.M. Jones on 2013-07-17 05:05:00 EDT ---

It's actually a bug in how acl_from_text uses getpwnam:

/* libacl/acl_from_text.c */

static int
get_uid(const char *token, uid_t *uid_p)
{
        struct passwd *passwd;

        if (get_id(token, uid_p) == 0)
                return 0;
        passwd = getpwnam(token);
        if (passwd) {
                *uid_p = passwd->pw_uid;
                return 0;
        }
        return -1;
}

It expects that errno is always set, but getpwnam only sets
errno if there was an error.  It can return NULL if the user
is not found, in which case errno is not touched.

This code has other failures too.

--- Additional comment from  on 2013-07-17 06:07:21 EDT ---

(In reply to Richard W.M. Jones from comment #1)
> We don't resolve UIDs against the guest database.  The caller
> has to resolve usernames to UIDs (eg. using Augeas).  Compare
> it to the 'chown' call which uses UIDs, not user names.
> 
> > ><fs> acl-set-file /home/test access 'u:500:rwx,g::rwx,o::rwx'
> > libguestfs: error: acl_set_file: /home/test: Invalid argument
> 
> This failed presumably because you used the wrong path?
> 
No, it's a correct path.

> > ><fs> acl-set-file /home/test access 'u:acltest:rwx,g::rwx,o::rwx'
> > libguestfs: error: acl_set_file: could not parse acl string: u:acltest:rwx,g::rwx,o::rwx: acl_from_text: Success
> 
> It shouldn't say "Success" here.  It looks like there's
> a bug in acl_from_text -- it doesn't set errno.  According
> to the documentation it should set errno.

<fs> acl-set-file /home/test access 'u:root:rwx,g::rwx,o::rwx'
libguestfs: error: acl_set_file: /home/test: Invalid argument

if user is root, it will give this error message

Does this command can set independent access rights for a specified user, if not, what it can do?

--- Additional comment from Richard W.M. Jones on 2013-07-17 07:01:45 EDT ---

Here's my self-contained test script:

guestfish -x -N fs:ext4 -m /dev/sda1:/:acl <<EOF                                
mkdir /test                                                                     
-acl-set-file /test access u:500:rwx,g::rwx,o::rwx                              
-acl-set-file /test access u:root:rwx,g::rwx,o::rwx                             
-acl-set-file /test access u:notauser:rwx,g::rwx,o::rwx                         
EOF                                                                             

This does appear to fail.  Here is the output:

libguestfs: trace: mkdir "/test"
libguestfs: trace: mkdir = 0
libguestfs: trace: acl_set_file "/test" "access" "u:500:rwx,g::rwx,o::rwx"
libguestfs: trace: acl_set_file = -1 (error)
*stdin*:4: libguestfs: error: acl_set_file: /test: Invalid argument
libguestfs: trace: acl_set_file "/test" "access" "u:root:rwx,g::rwx,o::rwx"
libguestfs: trace: acl_set_file = -1 (error)
*stdin*:6: libguestfs: error: acl_set_file: /test: Invalid argument
libguestfs: trace: acl_set_file "/test" "access" "u:notauser:rwx,g::rwx,o::rwx"
libguestfs: trace: acl_set_file = -1 (error)
*stdin*:8: libguestfs: error: acl_set_file: could not parse acl string: u:notauser:rwx,g::rwx,o::rwx: acl_from_text: Success

--- Additional comment from Richard W.M. Jones on 2013-07-17 07:23:49 EDT ---

(In reply to Richard W.M. Jones from comment #4)
> Here's my self-contained test script:

Actually that test is incorrect.  You have to set regular
permissions before setting extra ACLs.  A corrected test is:

guestfish -x -N fs:ext4 -m /dev/sda1:/:acl <<EOF                                
mkdir /testdir                                                                  
touch /testfile                                                                 
                                                                                
acl-set-file /testdir access u::rwx,g::rwx,o::rwx,u:500:rwx                     
acl-set-file /testfile access u::rwx,g::rwx,o::rwx,u:500:rwx                    
                                                                                
acl-set-file /testdir access u::rwx,g::rwx,o::rwx,u:root:rwx                    
acl-set-file /testfile access u::rwx,g::rwx,o::rwx,u:root:rwx                   
                                                                                
# notauser doesn't exist, so we expect a real error.                            
-acl-set-file /testdir access u:notauser:rwx,g::rwx,o::rwx                      
EOF                                                                             

Note the corrected test is still failing, so it does seem
as if there's a real error here.  (The equivalent setfacl
command does not fail).

--- Additional comment from Richard W.M. Jones on 2013-07-17 07:41:55 EDT ---

Apparently you have to set the 'mask' entry.  The setfacl
program does some trickery behind the scenes, automatically
setting mask, so it's not clear that you have to do this
if you use setfacl.

Here's a corrected, and working program:

guestfish -x -N fs:ext4 -m /dev/sda1:/:acl <<EOF
mkdir /testdir
touch /testfile

acl-set-file /testdir access u::rwx,g::rwx,o::rwx,m::rwx,u:500:rwx
acl-set-file /testfile access u::rwx,g::rwx,o::rwx,m::rwx,u:500:rwx

acl-set-file /testdir access u::rwx,g::rwx,o::rwx,m::rwx,u:root:rwx
acl-set-file /testfile access u::rwx,g::rwx,o::rwx,m::rwx,u:root:rwx

# notauser doesn't exist, so we expect a real error.
-acl-set-file /testdir access u:notauser:rwx,g::rwx,o::rwx
EOF

Output:

libguestfs: trace: mkdir "/testdir"
libguestfs: trace: mkdir = 0
libguestfs: trace: touch "/testfile"
libguestfs: trace: touch = 0
libguestfs: trace: acl_set_file "/testdir" "access" "u::rwx,g::rwx,o::rwx,m::rwx,u:500:rwx"
libguestfs: trace: acl_set_file = 0
libguestfs: trace: acl_set_file "/testfile" "access" "u::rwx,g::rwx,o::rwx,m::rwx,u:500:rwx"
libguestfs: trace: acl_set_file = 0
libguestfs: trace: acl_set_file "/testdir" "access" "u::rwx,g::rwx,o::rwx,m::rwx,u:root:rwx"
libguestfs: trace: acl_set_file = 0
libguestfs: trace: acl_set_file "/testfile" "access" "u::rwx,g::rwx,o::rwx,m::rwx,u:root:rwx"
libguestfs: trace: acl_set_file = 0
# the following error is expected:
libguestfs: trace: acl_set_file "/testdir" "access" "u:notauser:rwx,g::rwx,o::rwx"
libguestfs: trace: acl_set_file = -1 (error)
*stdin*:11: libguestfs: error: acl_set_file: could not parse acl string: u:notauser:rwx,g::rwx,o::rwx: acl_from_text: Success

--- Additional comment from  on 2013-07-17 22:43:34 EDT ---

Jones,
Thanks for your help, it worked with your approach. I think we'd better to update helpout and gives the usage for customer.

What do you think?

--- Additional comment from Richard W.M. Jones on 2013-07-18 05:04:57 EDT ---

Yup, this should be documented since it's very obscure.

Added upstream:

https://github.com/libguestfs/libguestfs/commit/758a2262f5f4033de3400bbe00fac2f85310b4ff