core/exec-credential: fix credentials plain dir exchanging (#40108)

Follow-ups for #39637

Split out from #40093

src/core/exec-credential.c

@@ -980,7 +980,13 @@ static int setup_credentials_plain_dir(
         r = RET_NERRNO(rename(workspace, cred_dir));
         if (r >= 0)
                 workspace_rm = NULL;
-        if (r == -EEXIST) {
+        if (IN_SET(r, -ENOTEMPTY, -EEXIST)) {
+                _cleanup_close_ int old_dfd = open(cred_dir, O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW);
+                if (old_dfd < 0)
+                        return log_debug_errno(errno, "Failed to open credentials dir '%s': %m", cred_dir);
+
+                (void) fd_acl_make_writable(old_dfd);
+
                 log_debug_errno(r, "Credential dir '%s' already populated, exchanging with workspace.", cred_dir);
                 r = RET_NERRNO(renameat2(AT_FDCWD, workspace, AT_FDCWD, cred_dir, RENAME_EXCHANGE));
         }

src/shared/acl-util.c

@@ -668,15 +668,34 @@ int fd_add_uid_acl_permission(
 
         return 0;
 }
+#endif
+
+static int fd_acl_make_read_only_fallback(int fd) {
+        struct stat st;
+
+        assert(fd >= 0);
+
+        if (fstat(fd, &st) < 0)
+                return -errno;
+
+        if ((st.st_mode & 0222) == 0)
+                return 0;
+
+        if (fchmod(fd, st.st_mode & 0555) < 0)
+                return -errno;
+
+        return 1;
+}
 
 int fd_acl_make_read_only(int fd) {
+        assert(fd >= 0);
+
+#if HAVE_ACL
         _cleanup_(acl_freep) acl_t acl = NULL;
         bool changed = false;
         acl_entry_t i;
         int r;
 
-        assert(fd >= 0);
-
         /* Safely drops all W bits from all relevant ACL entries of the file, without changing entries which
          * are masked by the ACL mask */
 
@@ -734,13 +753,13 @@ int fd_acl_make_read_only(int fd) {
 maybe_fallback:
         if (!ERRNO_IS_NEG_NOT_SUPPORTED(r))
                 return r;
+#endif
 
         /* No ACLs? Then just update the regular mode_t */
         return fd_acl_make_read_only_fallback(fd);
 }
-#endif
 
-int fd_acl_make_read_only_fallback(int fd) {
+static int fd_acl_make_writable_fallback(int fd) {
         struct stat st;
 
         assert(fd >= 0);
@@ -748,15 +767,83 @@ int fd_acl_make_read_only_fallback(int fd) {
         if (fstat(fd, &st) < 0)
                 return -errno;
 
-        if ((st.st_mode & 0222) == 0)
+        if ((st.st_mode & 0200) != 0) /* already set */
                 return 0;
 
-        if (fchmod(fd, st.st_mode & 0555) < 0)
+        if (fchmod(fd, (st.st_mode & 07777) | 0200) < 0)
                 return -errno;
 
         return 1;
 }
 
+int fd_acl_make_writable(int fd) {
+        assert(fd >= 0);
+
+#if HAVE_ACL
+        _cleanup_(acl_freep) acl_t acl = NULL;
+        acl_entry_t i;
+        int r;
+
+        /* Safely adds the writable bit to the owner's ACL entry of this inode. (And only the owner's! – This
+         * not the obvious inverse of fd_acl_make_read_only() hence!) */
+
+        r = dlopen_libacl();
+        if (r < 0)
+                goto maybe_fallback;
+
+        acl = sym_acl_get_fd(fd);
+        if (!acl) {
+                r = -errno;
+                goto maybe_fallback;
+        }
+
+        for (r = sym_acl_get_entry(acl, ACL_FIRST_ENTRY, &i);
+             r > 0;
+             r = sym_acl_get_entry(acl, ACL_NEXT_ENTRY, &i)) {
+                acl_permset_t permset;
+                acl_tag_t tag;
+                int b;
+
+                if (sym_acl_get_tag_type(i, &tag) < 0)
+                        return -errno;
+
+                if (tag != ACL_USER_OBJ)
+                        continue;
+
+                if (sym_acl_get_permset(i, &permset) < 0)
+                        return -errno;
+
+                b = sym_acl_get_perm(permset, ACL_WRITE);
+                if (b < 0)
+                        return -errno;
+
+                if (b)
+                        return 0; /* Already set? Then there's nothing to do. */
+
+                if (sym_acl_add_perm(permset, ACL_WRITE) < 0)
+                        return -errno;
+
+                break;
+        }
+        if (r < 0)
+                return -errno;
+
+        if (sym_acl_set_fd(fd, acl) < 0) {
+                r = -errno;
+                goto maybe_fallback;
+        }
+
+        return 1;
+
+maybe_fallback:
+        if (!ERRNO_IS_NEG_NOT_SUPPORTED(r))
+                return r;
+#endif
+
+        /* No ACLs? Then just update the regular mode_t */
+        return fd_acl_make_writable_fallback(fd);
+}
+
 int inode_type_can_acl(mode_t mode) {
         return IN_SET(mode & S_IFMT, S_IFSOCK, S_IFREG, S_IFBLK, S_IFCHR, S_IFDIR, S_IFIFO);
 }

src/shared/acl-util.h

@@ -3,8 +3,6 @@
 
 #include "shared-forward.h"
 
-int fd_acl_make_read_only_fallback(int fd);
-
 #if HAVE_ACL
 #include <acl/libacl.h> /* IWYU pragma: export */
 #include <sys/acl.h>    /* IWYU pragma: export */
@@ -54,8 +52,6 @@ int parse_acl(
 int acls_for_file(const char *path, acl_type_t type, acl_t acl, acl_t *ret);
 int fd_add_uid_acl_permission(int fd, uid_t uid, unsigned mask);
 
-int fd_acl_make_read_only(int fd);
-
 /* acl_free() takes multiple argument types. Multiple cleanup functions are necessary. */
 DEFINE_TRIVIAL_CLEANUP_FUNC_FULL_RENAME(acl_t, sym_acl_free, acl_freep, NULL);
 DEFINE_TRIVIAL_CLEANUP_FUNC_FULL_RENAME(char*, sym_acl_free, acl_free_charpp, NULL);
@@ -68,7 +64,7 @@ static inline int acl_set_perm(acl_permset_t ps, acl_perm_t p, bool b) {
 
 #else
 
-typedef void *acl_t;
+typedef void* acl_t;
 typedef int acl_tag_t;
 typedef unsigned acl_type_t;
 
@@ -100,10 +96,9 @@ static inline int devnode_acl(int fd, uid_t uid) {
 static inline int fd_add_uid_acl_permission(int fd, uid_t uid, unsigned mask) {
         return -EOPNOTSUPP;
 }
-
-static inline int fd_acl_make_read_only(int fd) {
-        return fd_acl_make_read_only_fallback(fd);
-}
 #endif
 
+int fd_acl_make_read_only(int fd);
+int fd_acl_make_writable(int fd);
+
 int inode_type_can_acl(mode_t mode);

src/test/test-acl-util.c

@@ -112,6 +112,30 @@ TEST_RET(fd_acl_make_read_only) {
         cmd = strjoina("stat ", fn);
         ASSERT_OK_ZERO_ERRNO(system(cmd));
 
+        log_info("writable");
+        ASSERT_OK_POSITIVE(fd_acl_make_writable(fd));
+
+        ASSERT_OK_ERRNO(fstat(fd, &st));
+        ASSERT_EQ(st.st_mode & 0222, 0200u);
+
+        cmd = strjoina("getfacl -p ", fn);
+        ASSERT_OK_ZERO_ERRNO(system(cmd));
+
+        cmd = strjoina("stat ", fn);
+        ASSERT_OK_ZERO_ERRNO(system(cmd));
+
+        log_info("read-only");
+        ASSERT_OK_POSITIVE(fd_acl_make_read_only(fd));
+
+        ASSERT_OK_ERRNO(fstat(fd, &st));
+        ASSERT_EQ(st.st_mode & 0222, 0000u);
+
+        cmd = strjoina("getfacl -p ", fn);
+        ASSERT_OK_ZERO_ERRNO(system(cmd));
+
+        cmd = strjoina("stat ", fn);
+        ASSERT_OK_ZERO_ERRNO(system(cmd));
+
         return 0;
 }
 

test/units/TEST-54-CREDS.sh

@@ -554,6 +554,9 @@ run0 -u testuser --pipe mkdir -p /home/testuser/.config/credstore.encrypted
 run0 -u testuser --pipe systemd-creds encrypt --user --name=brummbaer - /home/testuser/.config/credstore.encrypted/brummbaer < /tmp/brummbaer.data
 run0 -u testuser --pipe systemd-run --user --pipe -p ImportCredential=brummbaer systemd-creds cat brummbaer | cmp /tmp/brummbaer.data
 
+# https://github.com/systemd/systemd/pull/40108
+run0 -u testuser --pipe systemd-run --user --wait -p ImportCredential=brummbaer -p ExecStartPre='ls -l $CREDENTIALS_DIRECTORY' bash -c 'systemd-creds cat brummbaer | cmp /tmp/brummbaer.data'
+
 # https://github.com/systemd/systemd/pull/39651
 TESTUSER_CRED_DIR="/run/user/$(id -u testuser)/credentials"