track file mode and ownership in files.plist

[classabbyamp]

fixes #514

Questions

• should the checking of the mode/uid/gid be put into the libxbps api somewhere like the file hash check?
• should mutable files be considered in the mode/uid/gid pkgdb checks?
• surely there's a better way to do print_mode()?
• should the --long -f output include the mtime?
• should the --long -f output look more like ls -l? (e.g. single space between columns, right justify size, etc)
• should there be an option to humanize the filesize in --long -f?
• what tests should be created for this?
• do the mode/uid/gid need to be tracked anywhere else?
• is there anywhere else where this metadata would be useful?

Examples

$ xbps-query --long -f testpkg            
-rw-r-----    nobody    wheel    0    /opt/foo/owned_by_nobody
-rw-r-----    root    nogroup    0    /opt/foo/owned_by_nogroup
-rw-r-Sr--    root    root    0    /opt/foo/sgid_nox_file
-rwxr-sr-x    root    root    0    /opt/foo/sgidfile
-rw-r--r--    root    root    0    /opt/foo/some_other_file
-rwxr-x---    root    root    0    /opt/foo/somefile
-rwSr--r--    root    root    0    /opt/foo/suid_nox_file
-rwsr-xr-x    root    root    0    /opt/foo/suidfile
lrwxrwxrwx    root    root    0    /opt/foo/symlink -> /opt/foo/foo
$ ls -l /opt/foo 
total 0
-rw-r----- 1 nobody wheel   0 Feb 19 20:22 owned_by_nobody
-rw-r----- 1 root   nogroup 0 Feb 19 20:22 owned_by_nogroup
-rw-r-Sr-- 1 root   root    0 Feb 19 20:22 sgid_nox_file
-rwxr-sr-x 1 root   root    0 Feb 19 20:22 sgidfile
-rw-r--r-- 1 root   root    0 Feb 19 20:22 some_other_file
-rwxr-x--- 1 root   root    0 Feb 19 20:22 somefile
-rwSr--r-- 1 root   root    0 Feb 19 20:22 suid_nox_file
-rwsr-xr-x 1 root   root    0 Feb 19 20:22 suidfile
lrwxrwxrwx 1 root   root    3 Feb 19 20:22 symlink -> foo
$ doas chmod 644 /opt/foo/owned_by_nobody
$ doas chgrp audio /opt/foo/somefile
$ doas chown chrony:root /opt/foo/some_other_file
$ ls -l /opt/foo
total 0
-rw-r--r-- 1 nobody wheel   0 Feb 19 20:22 owned_by_nobody
-rw-r----- 1 root   nogroup 0 Feb 19 20:22 owned_by_nogroup
-rw-r-Sr-- 1 root   root    0 Feb 19 20:22 sgid_nox_file
-rwxr-sr-x 1 root   root    0 Feb 19 20:22 sgidfile
-rw-r--r-- 1 chrony root    0 Feb 19 20:22 some_other_file
-rwxr-x--- 1 root   audio   0 Feb 19 20:22 somefile
-rwSr--r-- 1 root   root    0 Feb 19 20:22 suid_nox_file
-rwsr-xr-x 1 root   root    0 Feb 19 20:22 suidfile
lrwxrwxrwx 1 root   root    3 Feb 19 20:22 symlink -> foo
$ xbps-pkgdb testpkg
ERROR: testpkg: mode mismatch for /opt/foo/owned_by_nobody.
ERROR: testpkg: owner mismatch for /opt/foo/some_other_file.
ERROR: testpkg: group mismatch for /opt/foo/somefile.
ERROR: testpkg: files check FAILED.
ERROR: Failed to check `testpkg': Operation not permitted

another example of xbps-query --long -f, with files that actually have sizes:

$ xbps-query --repository=Desktop/berkeley-mono --long -f font-berkeley-mono
-rw-rw-r--    root    root    53096    /usr/share/fonts/berkeley-mono/OTF/BerkeleyMono-Bold.otf
-rw-rw-r--    root    root    54288    /usr/share/fonts/berkeley-mono/OTF/BerkeleyMono-BoldItalic.otf
-rw-rw-r--    root    root    53748    /usr/share/fonts/berkeley-mono/OTF/BerkeleyMono-Italic.otf
-rw-rw-r--    root    root    512512   /usr/share/fonts/berkeley-mono/OTF/BerkeleyMono-Regular.otf
...

[Duncaen]

• should the checking of the mode/uid/gid be put into the libxbps api somewhere like the file hash check?

Yes see last point "is there anywhere else where this metadata would be useful?".

• should mutable files be considered in the mode/uid/gid pkgdb checks?

I guess that wouldn't hurt could track that information but don't error if it changes maybe.

• surely there's a better way to do print_mode()?

BSDs have strmode(3), you could maybe use the same api and make it a compat function. Your could import it from some BSD, but your implementation seems good, its basically the same.

Might be nice to have in libxbps to also use in debug or error messages for file conflicts.

• should the --long -f output include the mtime?
• should the --long -f output look more like ls -l? (e.g. single space between columns, right justify size, etc)
• should there be an option to humanize the filesize in --long -f?

I've written some code for -F/--format for xbps-query, for the various package lists. At the moment this just allows to print data from xbps_dictionary_t with the keys as format specifiers %pkgver\n %{pkgver}\n. I think that would be a good way to customize the output for whatever the user requires.

• what tests should be created for this?

I would just add a simple basic test for both xbps-pkgdb and xbps-query for now.

For xbps-query I would just create a basic package with the common files and check that the format output is right.

For xbps-pkgdb I would create a similar/the same basic package, install it, run xbps-pkgdb, modify some things and then make sure xbps-pkgdb complains.

• do the mode/uid/gid need to be tracked anywhere else?
• is there anywhere else where this metadata would be useful?

I think it would be good to extend this to directories, then also track the mode in transaction_files.c since multiple packages could own the same directory and conflicting modes might be an issue.

Regarding UID/GIDs, I think it would be better to default to root and optionally allow to set a user and group entry in files .plist with user and group names. This would allow us to replace a lot of scriptlets generated by xbps-src's make_dirs.

The PR looks good and I think we can merge this fast so that we don't have this sitting around for too long.

[classabbyamp]

I see what you're getting at with tracking the owner/group name rather than id: I was assuming they would be uniform across systems, which is ofc not correct.

I think it would be good to extend this to directories, then also track the mode in transaction_files.c since multiple packages could own the same directory and conflicting modes might be an issue.
...
This would allow us to replace a lot of scriptlets generated by xbps-src's make_dirs.

I'm not sure I see where the directory handling code is? from what I read xbps-create doesn't bundle empty dirs. are you talking about non-empty dirs here?

[Duncaen]

from what I read xbps-create doesn't bundle empty dirs. are you talking about non-empty dirs here?

This would need new code to first store empty directories in files.plist and then also create them when extracting the package.

But I think this can wait for a second PR, getting the metadata in and having the basic support for file checks is good.

[Duncaen]

Also I'm generally moving into the direction of making functions return -errno on issues and checking for errors with if (r < 0) so that when needed functions can return other things than error and just success with 0.

[classabbyamp]

needed for strsep(). is this the right spot to put it?

[classabbyamp]

this doesn't handle paths with spaces... maybe accepting the argument multiple times would be better, or is this not a concern?

[classabbyamp]

something else to consider: should chown -R be representable?

[classabbyamp]

ok pkgdb is updated to use the idtree cache. I think all that remains is writing some tests.

[classabbyamp]

@Duncaen ready for further review

[Duncaen]

General code style changes:

Braces of functions on new line:

void
funcname()
{
}

Switch cases not indented:

switch (x) {
case 0:
case 1:
default:
}

[Duncaen]

I feel like the file_{mode,user,group} functions are unnecessary, duplicate a lot of error handling and system calls:

while ((obj = xbps_object_iterator_next(iter))) {
    struct stat st;
    const char *name;
    if (lstat(path, &st) == -1) {
        // ...
        continue;
    }
    if (xbps_dictionary_get_cstring_nocopy(obj, "user", &name)) {
        const char *user  = idtree_username(users, st.st_uid);
        if (strcmp(user, name) != 0)
           // ...
    }
    if (xbps_dictionary_get_cstring_nocopy(obj, "group", &name)) {
        const char *group = idtree_username(groups, st.st_gid);
        if (strcmp(group, name) != 0)
            // ...
    }
}

You'll also have to use different id trees for users and groups since its not required that there is a primary group with the gid being the same as the uid.

Maybe also don't check if user and/or group are not set since the metadata will not be there for some time and would produce errors.
We can later enable the check for when the default is known to be always root.

[Duncaen]

All comments are generally old c-style /* */ so probably nicer to match that.

[Duncaen]

Looking over this, the idtree was probably not the right choice what we generally need is a map from names to uids, that makes the code a lot simpler. The idtree makes sense if we need to find the name from ids, using the idtree to compare strings after search the tree doesn't make much sense.

[classabbyamp]

style issues fixed (except where refactor will happen)

what data structure should be used for a mapping then? could xbps_dictionary be used?

[Duncaen]

I would just use uthash, we using it already and I don't really like the proplib api, it tends to hide errors.

Not sure about the name of the functions/api, generally combining users and groups behind the same api makes sense I think.
I'll use xbps_users as an example name, but might be nice to find a better one.

Not sure if we should read the whole passwd or groups file when creating the cache or looking them up on demand.

xbps.h.in:

struct xbps_users;
struct xbps_users *xbps_users_new(const char *rootdir);
uid_t xbps_users_get_uid(struct xbps_users *users, const char *username);
gid_t xbps_users_get_gid(struct xbps_users *users, const char *groupname);

lib/users.c:

struct xbps_users {
    struct user *users;
    struct group *groups;
};
struct user {
   char name[MAX?];
   uid_t uid;
   // or a cached pwent?
   UT_hash_handle hh;
};
struct group {
   char name[MAX?];
   gid_t gid;
   // or a cached grpent?
  UT_hash_handle hh;
};

uid_t
xbps_users_get_uid(struct xbps_users *users, const char *username)
{
    struct user *user;
    HASH_FIND_STR(users->users, username, &user)
    if (!user) {
        user = calloc(1, sizeof(*user));
        getpwent(...);
        // maybe cache if getpwent failed so that we don't try to look up the same failing user the whole time.
        HASH_ADD_STR(users->users, username, user);
    }
    return user->uid;
}

Damn also noticing this now while thinking about the api we can't really using the default libc api, because we need to take rootdir into account.

[Duncaen]

With having to take rootdir into account, how would we avoid nss with glibc. Anyways, we probably should read /etc/passwd and /etc/groups manually and in the future also write manually.

With that in mind I guess we do should split the api:

struct xbps_user {
   char name[MAX?];
   uid_t uid;
   // other fields we are interested in maybe, at least comment is set by xbps hooks to track disabled accounts iirc.
};
struct xbps_users;
struct xbps_users *xbps_users_open(const char *rootdir);
const struct xbps_user *xbps_users_get(struct xbps_users *users, const char *username);

struct xbps_groups;
struct xbps_group {
   char name[MAX?];
   gid_t gid;
};
struct xbps_groups *xbps_groups_open(const char *rootdir);
const struct xbps_group *xbps_groups_get(struct xbps_groups *groups, const char *groupname);