From 3a0f6c217d178ff97c0fb7182604f852569be580 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 6 Feb 2024 22:17:34 +0800 Subject: [PATCH 1/5] core/exec-credential: add missing assertions --- src/core/exec-credential.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index 048cccf56e..3b017972a7 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -172,6 +172,10 @@ static int write_credential( _cleanup_close_ int fd = -EBADF; int r; + assert(dfd >= 0); + assert(id); + assert(data || size == 0); + r = tempfn_random_child("", "cred", &tmp); if (r < 0) return r; @@ -224,7 +228,6 @@ typedef enum CredentialSearchPath { } CredentialSearchPath; static char **credential_search_path(const ExecParameters *params, CredentialSearchPath path) { - _cleanup_strv_free_ char **l = NULL; assert(params); @@ -274,6 +277,10 @@ static int maybe_decrypt_and_write_credential( size_t add; int r; + assert(dir_fd >= 0); + assert(id); + assert(left); + if (encrypted) { r = decrypt_credential_and_warn( id, @@ -306,7 +313,7 @@ static int maybe_decrypt_and_write_credential( static int load_credential_glob( const char *path, bool encrypted, - char **search_path, + char * const *search_path, ReadFullFileFlags flags, int write_dfd, uid_t uid, @@ -316,6 +323,11 @@ static int load_credential_glob( int r; + assert(path); + assert(search_path); + assert(write_dfd >= 0); + assert(left); + STRV_FOREACH(d, search_path) { _cleanup_globfree_ glob_t pglob = {}; _cleanup_free_ char *j = NULL; @@ -522,6 +534,9 @@ static int load_cred_recurse_dir_cb( _cleanup_free_ char *sub_id = NULL; int r; + assert(path); + assert(de); + if (event != RECURSE_DIR_ENTRY) return RECURSE_DIR_CONTINUE; @@ -578,6 +593,8 @@ static int acquire_credentials( int r; assert(context); + assert(params); + assert(unit); assert(p); dfd = open(p, O_DIRECTORY|O_CLOEXEC); @@ -622,8 +639,7 @@ static int acquire_credentials( &left); else /* Directory */ - r = recurse_dir( - sub_fd, + r = recurse_dir(sub_fd, /* path= */ lc->id, /* recurse_dir() will suffix the subdir paths from here to the top-level id */ /* statx_mask= */ 0, /* n_depth_max= */ UINT_MAX, @@ -773,6 +789,8 @@ static int setup_credentials_internal( const char *where; assert(context); + assert(params); + assert(unit); assert(final); assert(workspace); @@ -918,6 +936,7 @@ int exec_setup_credentials( assert(context); assert(params); + assert(unit); if (!exec_context_has_credentials(context)) return 0; From 0f781376c033c8c8d38157a810e45cf226bc8d72 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 6 Feb 2024 22:20:49 +0800 Subject: [PATCH 2/5] core/exec-credential: use FOREACH_ARRAY at one more place --- src/core/exec-credential.c | 40 ++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index 3b017972a7..7248ad0348 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -342,38 +342,36 @@ static int load_credential_glob( if (r < 0) return r; - for (size_t n = 0; n < pglob.gl_pathc; n++) { + FOREACH_ARRAY(p, pglob.gl_pathv, pglob.gl_pathc) { _cleanup_free_ char *fn = NULL; _cleanup_(erase_and_freep) char *data = NULL; size_t size; /* path is absolute, hence pass AT_FDCWD as nop dir fd here */ r = read_full_file_full( - AT_FDCWD, - pglob.gl_pathv[n], - UINT64_MAX, - encrypted ? CREDENTIAL_ENCRYPTED_SIZE_MAX : CREDENTIAL_SIZE_MAX, - flags, - NULL, - &data, &size); + AT_FDCWD, + *p, + UINT64_MAX, + encrypted ? CREDENTIAL_ENCRYPTED_SIZE_MAX : CREDENTIAL_SIZE_MAX, + flags, + NULL, + &data, &size); if (r < 0) - return log_debug_errno(r, "Failed to read credential '%s': %m", - pglob.gl_pathv[n]); + return log_debug_errno(r, "Failed to read credential '%s': %m", *p); - r = path_extract_filename(pglob.gl_pathv[n], &fn); + r = path_extract_filename(*p, &fn); if (r < 0) - return log_debug_errno(r, "Failed to extract filename from '%s': %m", - pglob.gl_pathv[n]); + return log_debug_errno(r, "Failed to extract filename from '%s': %m", *p); r = maybe_decrypt_and_write_credential( - write_dfd, - fn, - encrypted, - uid, - gid, - ownership_ok, - data, size, - left); + write_dfd, + fn, + encrypted, + uid, + gid, + ownership_ok, + data, size, + left); if (r == -EEXIST) continue; if (r < 0) From 54c3546188e4c9f2dd1b9ae2d752f145f3125710 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 6 Feb 2024 17:10:49 +0800 Subject: [PATCH 3/5] TEST-54-CREDS: add test for ExecStartPost= (#31194) --- test/units/testsuite-54.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/units/testsuite-54.sh b/test/units/testsuite-54.sh index 3c92994b14..914f986545 100755 --- a/test/units/testsuite-54.sh +++ b/test/units/testsuite-54.sh @@ -297,11 +297,18 @@ fi systemd-run -p DynamicUser=yes -p 'LoadCredential=os:/etc/os-release' \ -p 'ExecStartPre=true' \ -p 'ExecStartPre=systemd-creds cat os' \ - --unit=test-54-exec-start.service \ + --unit=test-54-exec-start-pre.service \ --wait \ --pipe \ true | cmp /etc/os-release +# https://github.com/systemd/systemd/issues/31194 +systemd-run -p DynamicUser=yes -p 'LoadCredential=os:/etc/os-release' \ + -p 'ExecStartPost=systemd-creds cat os' \ + --unit=test-54-exec-start-post.service \ + --service-type=oneshot --wait --pipe \ + true | cmp /etc/os-release + if ! systemd-detect-virt -q -c ; then # Validate that the credential we inserted via the initrd logic arrived test "$(systemd-creds cat --system myinitrdcred)" = "guatemala" From 1221ba0f6f2438035a601ef55cb651bb030456f1 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 6 Feb 2024 21:53:35 +0800 Subject: [PATCH 4/5] core: introduce exec_params_need_credentials Also rename EXEC_WRITE_CREDENTIALS to EXEC_SETUP_CREDENTIALS. --- src/core/exec-credential.c | 10 ++++++++-- src/core/exec-credential.h | 2 ++ src/core/exec-invoke.c | 18 +++++++----------- src/core/execute.h | 2 +- src/core/service.c | 2 +- 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index 7248ad0348..9b76c5a324 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -49,6 +49,12 @@ DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR( char, string_hash_func, string_compare_func, ExecLoadCredential, exec_load_credential_free); +bool exec_params_need_credentials(const ExecParameters *p) { + assert(p); + + return FLAGS_SET(p->flags, EXEC_SETUP_CREDENTIALS); +} + bool exec_context_has_credentials(const ExecContext *c) { assert(c); @@ -106,7 +112,7 @@ int exec_context_get_credential_directory( assert(unit); assert(ret); - if (!exec_context_has_credentials(context)) { + if (!exec_params_need_credentials(params) || !exec_context_has_credentials(context)) { *ret = NULL; return 0; } @@ -936,7 +942,7 @@ int exec_setup_credentials( assert(params); assert(unit); - if (!exec_context_has_credentials(context)) + if (!exec_params_need_credentials(params) || !exec_context_has_credentials(context)) return 0; if (!params->prefix[EXEC_DIRECTORY_RUNTIME]) diff --git a/src/core/exec-credential.h b/src/core/exec-credential.h index 9e4770d0b8..70bb46bdde 100644 --- a/src/core/exec-credential.h +++ b/src/core/exec-credential.h @@ -34,6 +34,8 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(ExecLoadCredential*, exec_load_credential_free); extern const struct hash_ops exec_set_credential_hash_ops; extern const struct hash_ops exec_load_credential_hash_ops; +bool exec_params_need_credentials(const ExecParameters *p); + bool exec_context_has_credentials(const ExecContext *c); bool exec_context_has_encrypted_credentials(const ExecContext *c); diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 81d243c5b2..c62b7385cb 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3175,11 +3175,9 @@ static int apply_mount_namespace( params, "shared mount propagation hidden by other fs namespacing unit settings: ignoring"); - if (FLAGS_SET(params->flags, EXEC_WRITE_CREDENTIALS)) { - r = exec_context_get_credential_directory(context, params, params->unit_id, &creds_path); - if (r < 0) - return r; - } + r = exec_context_get_credential_directory(context, params, params->unit_id, &creds_path); + if (r < 0) + return r; if (params->runtime_scope == RUNTIME_SCOPE_SYSTEM) { propagate_dir = path_join("/run/systemd/propagate/", params->unit_id); @@ -4534,12 +4532,10 @@ int exec_invoke( return log_exec_error_errno(context, params, r, "Failed to set up special execution directory in %s: %m", params->prefix[dt]); } - if (FLAGS_SET(params->flags, EXEC_WRITE_CREDENTIALS)) { - r = exec_setup_credentials(context, params, params->unit_id, uid, gid); - if (r < 0) { - *exit_status = EXIT_CREDENTIALS; - return log_exec_error_errno(context, params, r, "Failed to set up credentials: %m"); - } + r = exec_setup_credentials(context, params, params->unit_id, uid, gid); + if (r < 0) { + *exit_status = EXIT_CREDENTIALS; + return log_exec_error_errno(context, params, r, "Failed to set up credentials: %m"); } r = build_environment( diff --git a/src/core/execute.h b/src/core/execute.h index e226654c6a..916e66adcd 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -399,7 +399,7 @@ typedef enum ExecFlags { EXEC_CGROUP_DELEGATE = 1 << 6, EXEC_IS_CONTROL = 1 << 7, EXEC_CONTROL_CGROUP = 1 << 8, /* Place the process not in the indicated cgroup but in a subcgroup '/.control', but only EXEC_CGROUP_DELEGATE and EXEC_IS_CONTROL is set, too */ - EXEC_WRITE_CREDENTIALS = 1 << 9, /* Set up the credential store logic */ + EXEC_SETUP_CREDENTIALS = 1 << 9, /* Set up the credential store logic */ /* The following are not used by execute.c, but by consumers internally */ EXEC_PASS_FDS = 1 << 10, diff --git a/src/core/service.c b/src/core/service.c index 38934390f0..8553e28e1d 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1609,7 +1609,7 @@ static ExecFlags service_exec_flags(ServiceExecCommand command_id) { /* All start phases get access to credentials */ if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START, SERVICE_EXEC_START_POST)) - flags |= EXEC_WRITE_CREDENTIALS; + flags |= EXEC_SETUP_CREDENTIALS; if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START)) flags |= EXEC_SETENV_MONITOR_RESULT; From cfbf7538d87023840c5574fa5b0452e5b0f42149 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 4 Feb 2024 23:22:46 +0800 Subject: [PATCH 5/5] core: reuse credential dir across start and start-post if populated, fresh otherwise Currently, exec_setup_credential() always rewrite all credentials upon exec_invoke(), i.e. invocation of each ExecCommand, and within a single tmpfs instance. This is problematic though: * When writing each tmp cred file, we essentially double the size of the credential. Therefore, if one cred is bigger than half of CREDENTIALS_TOTAL_SIZE_MAX, confusing ENOSPC occurs (see also https://github.com/systemd/systemd/pull/24734#issuecomment-1925440546) * Credential is a unit-wide thing and thus should not change during the whole lifetime of main process. However, if e.g. a on-disk credential or SetCredential= in unit file changes between ExecStart= and ExecStartPost=, the credentials are overwritten when the latter gets to run, and the already-running main process is suddenly seeing completely different creds. So, let's try to reuse final cred dir if the main process has started and the tmpfs has been populated, so that the creds used is stable across all ExecStart= and ExecStartPost=-s. We still want to retain the ability of updating creds through ExecStartPre= though, therefore we forcibly use a fresh cred dir for those. 'Fresh' means to actually unmount the old tmpfs first, so the first problem goes away, too. --- src/core/exec-credential.c | 119 +++++++++++++++++++------------------ src/core/execute.h | 29 ++++----- src/core/service.c | 37 +++++++----- test/units/testsuite-54.sh | 25 ++++++++ 4 files changed, 121 insertions(+), 89 deletions(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index 9b76c5a324..ad43980216 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -52,7 +52,7 @@ DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR( bool exec_params_need_credentials(const ExecParameters *p) { assert(p); - return FLAGS_SET(p->flags, EXEC_SETUP_CREDENTIALS); + return p->flags & (EXEC_SETUP_CREDENTIALS|EXEC_SETUP_CREDENTIALS_FRESH); } bool exec_context_has_credentials(const ExecContext *c) { @@ -787,10 +787,9 @@ static int setup_credentials_internal( uid_t uid, gid_t gid) { + bool final_mounted; int r, workspace_mounted; /* negative if we don't know yet whether we have/can mount something; true * if we mounted something; false if we definitely can't mount anything */ - bool final_mounted; - const char *where; assert(context); assert(params); @@ -798,6 +797,30 @@ static int setup_credentials_internal( assert(final); assert(workspace); + r = path_is_mount_point(final); + if (r < 0) + return r; + final_mounted = r > 0; + + if (final_mounted) { + if (FLAGS_SET(params->flags, EXEC_SETUP_CREDENTIALS_FRESH)) { + r = umount_verbose(LOG_DEBUG, final, MNT_DETACH|UMOUNT_NOFOLLOW); + if (r < 0) + return r; + + final_mounted = false; + } else { + /* We can reuse the previous credential dir */ + r = dir_is_empty(final, /* ignore_hidden_or_backup = */ false); + if (r < 0) + return r; + if (r == 0) { + log_debug("Credential dir for unit '%s' already set up, skipping.", unit); + return 0; + } + } + } + if (reuse_workspace) { r = path_is_mount_point(workspace); if (r < 0) @@ -810,40 +833,19 @@ static int setup_credentials_internal( } else workspace_mounted = -1; /* ditto */ - r = path_is_mount_point(final); - if (r < 0) - return r; - if (r > 0) { - /* If the final place already has something mounted, we use that. If the workspace also has - * something mounted we assume it's actually the same mount (but with MS_RDONLY - * different). */ - final_mounted = true; - - if (workspace_mounted < 0) { - /* If the final place is mounted, but the workspace isn't, then let's bind mount - * the final version to the workspace, and make it writable, so that we can make - * changes */ - - r = mount_nofollow_verbose(LOG_DEBUG, final, workspace, NULL, MS_BIND|MS_REC, NULL); - if (r < 0) - return r; - - r = mount_nofollow_verbose(LOG_DEBUG, NULL, workspace, NULL, MS_BIND|MS_REMOUNT|credentials_fs_mount_flags(/* ro= */ false), NULL); - if (r < 0) - return r; - - workspace_mounted = true; - } - } else - final_mounted = false; + /* If both the final place and the workspace are mounted, we have no mounts to set up, based on + * the assumption that they're actually the same tmpfs (but the latter with MS_RDONLY different). + * If the workspace is not mounted, we just bind the final place over and make it writable. */ + must_mount = must_mount || final_mounted; if (workspace_mounted < 0) { - /* Nothing is mounted on the workspace yet, let's try to mount something now */ - - r = mount_credentials_fs(workspace, CREDENTIALS_TOTAL_SIZE_MAX, /* ro= */ false); - if (r < 0) { - /* If that didn't work, try to make a bind mount from the final to the workspace, so - * that we can make it writable there. */ + if (!final_mounted) + /* Nothing is mounted on the workspace yet, let's try to mount a new tmpfs if + * not using the final place. */ + r = mount_credentials_fs(workspace, CREDENTIALS_TOTAL_SIZE_MAX, /* ro= */ false); + if (final_mounted || r < 0) { + /* If using final place or failed to mount new tmpfs, make a bind mount from + * the final to the workspace, so that we can make it writable there. */ r = mount_nofollow_verbose(LOG_DEBUG, final, workspace, NULL, MS_BIND|MS_REC, NULL); if (r < 0) { if (!ERRNO_IS_PRIVILEGE(r)) @@ -856,12 +858,19 @@ static int setup_credentials_internal( return r; /* If we lack privileges to bind mount stuff, then let's gracefully proceed - * for compat with container envs, and just use the final dir as is. */ + * for compat with container envs, and just use the final dir as is. + * Final place must not be mounted in this case (refused by must_mount + * above) */ workspace_mounted = false; } else { /* Make the new bind mount writable (i.e. drop MS_RDONLY) */ - r = mount_nofollow_verbose(LOG_DEBUG, NULL, workspace, NULL, MS_BIND|MS_REMOUNT|credentials_fs_mount_flags(/* ro= */ false), NULL); + r = mount_nofollow_verbose(LOG_DEBUG, + NULL, + workspace, + NULL, + MS_BIND|MS_REMOUNT|credentials_fs_mount_flags(/* ro= */ false), + NULL); if (r < 0) return r; @@ -871,34 +880,26 @@ static int setup_credentials_internal( workspace_mounted = true; } - assert(!must_mount || workspace_mounted > 0); - where = workspace_mounted ? workspace : final; + assert(workspace_mounted >= 0); + assert(!must_mount || workspace_mounted); + + const char *where = workspace_mounted ? workspace : final; (void) label_fix_full(AT_FDCWD, where, final, 0); r = acquire_credentials(context, params, unit, where, uid, gid, workspace_mounted); - if (r < 0) + if (r < 0) { + /* If we're using final place as workspace, and failed to acquire credentials, we might + * have left half-written creds there. Let's get rid of the whole mount, so future + * calls won't reuse it. */ + if (final_mounted) + (void) umount_verbose(LOG_DEBUG, final, MNT_DETACH|UMOUNT_NOFOLLOW); + return r; + } if (workspace_mounted) { - bool install; - - /* Determine if we should actually install the prepared mount in the final location by bind - * mounting it there. We do so only if the mount is not established there already, and if the - * mount is actually non-empty (i.e. carries at least one credential). Not that in the best - * case we are doing all this in a mount namespace, thus no one else will see that we - * allocated a file system we are getting rid of again here. */ - if (final_mounted) - install = false; /* already installed */ - else { - r = dir_is_empty(where, /* ignore_hidden_or_backup= */ false); - if (r < 0) - return r; - - install = r == 0; /* install only if non-empty */ - } - - if (install) { + if (!final_mounted) { /* Make workspace read-only now, so that any bind mount we make from it defaults to * read-only too */ r = mount_nofollow_verbose(LOG_DEBUG, NULL, workspace, NULL, MS_BIND|MS_REMOUNT|credentials_fs_mount_flags(/* ro= */ true), NULL); @@ -908,7 +909,7 @@ static int setup_credentials_internal( /* And mount it to the final place, read-only */ r = mount_nofollow_verbose(LOG_DEBUG, workspace, final, NULL, MS_MOVE, NULL); } else - /* Otherwise get rid of it */ + /* Otherwise we just get rid of the bind mount of final place */ r = umount_verbose(LOG_DEBUG, workspace, MNT_DETACH|UMOUNT_NOFOLLOW); if (r < 0) return r; diff --git a/src/core/execute.h b/src/core/execute.h index 916e66adcd..eea6b7518a 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -390,22 +390,23 @@ static inline bool exec_context_with_rootfs(const ExecContext *c) { } typedef enum ExecFlags { - EXEC_APPLY_SANDBOXING = 1 << 0, - EXEC_APPLY_CHROOT = 1 << 1, - EXEC_APPLY_TTY_STDIN = 1 << 2, - EXEC_PASS_LOG_UNIT = 1 << 3, /* Whether to pass the unit name to the service's journal stream connection */ - EXEC_CHOWN_DIRECTORIES = 1 << 4, /* chown() the runtime/state/cache/log directories to the user we run as, under all conditions */ - EXEC_NSS_DYNAMIC_BYPASS = 1 << 5, /* Set the SYSTEMD_NSS_DYNAMIC_BYPASS environment variable, to disable nss-systemd blocking on PID 1, for use by dbus-daemon */ - EXEC_CGROUP_DELEGATE = 1 << 6, - EXEC_IS_CONTROL = 1 << 7, - EXEC_CONTROL_CGROUP = 1 << 8, /* Place the process not in the indicated cgroup but in a subcgroup '/.control', but only EXEC_CGROUP_DELEGATE and EXEC_IS_CONTROL is set, too */ - EXEC_SETUP_CREDENTIALS = 1 << 9, /* Set up the credential store logic */ + EXEC_APPLY_SANDBOXING = 1 << 0, + EXEC_APPLY_CHROOT = 1 << 1, + EXEC_APPLY_TTY_STDIN = 1 << 2, + EXEC_PASS_LOG_UNIT = 1 << 3, /* Whether to pass the unit name to the service's journal stream connection */ + EXEC_CHOWN_DIRECTORIES = 1 << 4, /* chown() the runtime/state/cache/log directories to the user we run as, under all conditions */ + EXEC_NSS_DYNAMIC_BYPASS = 1 << 5, /* Set the SYSTEMD_NSS_DYNAMIC_BYPASS environment variable, to disable nss-systemd blocking on PID 1, for use by dbus-daemon */ + EXEC_CGROUP_DELEGATE = 1 << 6, + EXEC_IS_CONTROL = 1 << 7, + EXEC_CONTROL_CGROUP = 1 << 8, /* Place the process not in the indicated cgroup but in a subcgroup '/.control', but only EXEC_CGROUP_DELEGATE and EXEC_IS_CONTROL is set, too */ + EXEC_SETUP_CREDENTIALS = 1 << 9, /* Set up the credential store logic */ + EXEC_SETUP_CREDENTIALS_FRESH = 1 << 10, /* Set up a new credential store (disable reuse) */ /* The following are not used by execute.c, but by consumers internally */ - EXEC_PASS_FDS = 1 << 10, - EXEC_SETENV_RESULT = 1 << 11, - EXEC_SET_WATCHDOG = 1 << 12, - EXEC_SETENV_MONITOR_RESULT = 1 << 13, /* Pass exit status to OnFailure= and OnSuccess= dependencies. */ + EXEC_PASS_FDS = 1 << 11, + EXEC_SETENV_RESULT = 1 << 12, + EXEC_SET_WATCHDOG = 1 << 13, + EXEC_SETENV_MONITOR_RESULT = 1 << 14, /* Pass exit status to OnFailure= and OnSuccess= dependencies. */ } ExecFlags; /* Parameters for a specific invocation of a command. This structure is put together right before a command is diff --git a/src/core/service.c b/src/core/service.c index 8553e28e1d..2b760eb7aa 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1595,27 +1595,33 @@ static Service *service_get_triggering_service(Service *s) { return NULL; } -static ExecFlags service_exec_flags(ServiceExecCommand command_id) { +static ExecFlags service_exec_flags(ServiceExecCommand command_id, ExecFlags cred_flag) { /* All service main/control processes honor sandboxing and namespacing options (except those explicitly excluded in service_spawn()) */ ExecFlags flags = EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT; assert(command_id >= 0); assert(command_id < _SERVICE_EXEC_COMMAND_MAX); + assert((cred_flag & ~(EXEC_SETUP_CREDENTIALS_FRESH|EXEC_SETUP_CREDENTIALS)) == 0); + assert((cred_flag != 0) == (command_id == SERVICE_EXEC_START)); /* Control processes spawned before main process also get tty access */ if (IN_SET(command_id, SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START)) flags |= EXEC_APPLY_TTY_STDIN; - /* All start phases get access to credentials */ - if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START, SERVICE_EXEC_START_POST)) + /* All start phases get access to credentials. ExecStartPre= gets a new credential store upon + * every invocation, so that updating credential files through it works. When the first main process + * starts, passed creds become stable. Also see 'cred_flag'. */ + if (command_id == SERVICE_EXEC_START_PRE) + flags |= EXEC_SETUP_CREDENTIALS_FRESH; + if (command_id == SERVICE_EXEC_START_POST) flags |= EXEC_SETUP_CREDENTIALS; if (IN_SET(command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START)) flags |= EXEC_SETENV_MONITOR_RESULT; if (command_id == SERVICE_EXEC_START) - return flags|EXEC_PASS_FDS|EXEC_SET_WATCHDOG; + return flags|cred_flag|EXEC_PASS_FDS|EXEC_SET_WATCHDOG; flags |= EXEC_IS_CONTROL; @@ -1632,13 +1638,12 @@ static ExecFlags service_exec_flags(ServiceExecCommand command_id) { static int service_spawn_internal( const char *caller, Service *s, - ServiceExecCommand command_id, ExecCommand *c, + ExecFlags flags, usec_t timeout, PidRef *ret_pid) { - _cleanup_(exec_params_shallow_clear) ExecParameters exec_params = - EXEC_PARAMETERS_INIT(service_exec_flags(command_id)); + _cleanup_(exec_params_shallow_clear) ExecParameters exec_params = EXEC_PARAMETERS_INIT(flags); _cleanup_(sd_event_source_unrefp) sd_event_source *exec_fd_source = NULL; _cleanup_strv_free_ char **final_env = NULL, **our_env = NULL; _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; @@ -2083,8 +2088,8 @@ static void service_enter_stop_post(Service *s, ServiceResult f) { pidref_done(&s->control_pid); r = service_spawn(s, - s->control_command_id, s->control_command, + service_exec_flags(s->control_command_id, /* cred_flag = */ 0), s->timeout_stop_usec, &s->control_pid); if (r < 0) { @@ -2206,8 +2211,8 @@ static void service_enter_stop(Service *s, ServiceResult f) { pidref_done(&s->control_pid); r = service_spawn(s, - s->control_command_id, s->control_command, + service_exec_flags(s->control_command_id, /* cred_flag = */ 0), s->timeout_stop_usec, &s->control_pid); if (r < 0) { @@ -2290,8 +2295,8 @@ static void service_enter_start_post(Service *s) { pidref_done(&s->control_pid); r = service_spawn(s, - s->control_command_id, s->control_command, + service_exec_flags(s->control_command_id, /* cred_flag = */ 0), s->timeout_start_usec, &s->control_pid); if (r < 0) { @@ -2400,8 +2405,8 @@ static void service_enter_start(Service *s) { timeout = s->timeout_start_usec; r = service_spawn(s, - SERVICE_EXEC_START, c, + service_exec_flags(SERVICE_EXEC_START, EXEC_SETUP_CREDENTIALS_FRESH), timeout, &pidref); if (r < 0) { @@ -2460,8 +2465,8 @@ static void service_enter_start_pre(Service *s) { s->control_command_id = SERVICE_EXEC_START_PRE; r = service_spawn(s, - s->control_command_id, s->control_command, + service_exec_flags(s->control_command_id, /* cred_flag = */ 0), s->timeout_start_usec, &s->control_pid); if (r < 0) { @@ -2497,8 +2502,8 @@ static void service_enter_condition(Service *s) { pidref_done(&s->control_pid); r = service_spawn(s, - s->control_command_id, s->control_command, + service_exec_flags(s->control_command_id, /* cred_flag = */ 0), s->timeout_start_usec, &s->control_pid); @@ -2608,8 +2613,8 @@ static void service_enter_reload(Service *s) { pidref_done(&s->control_pid); r = service_spawn(s, - s->control_command_id, s->control_command, + service_exec_flags(s->control_command_id, /* cred_flag = */ 0), s->timeout_start_usec, &s->control_pid); if (r < 0) { @@ -2664,8 +2669,8 @@ static void service_run_next_control(Service *s) { pidref_done(&s->control_pid); r = service_spawn(s, - s->control_command_id, s->control_command, + service_exec_flags(s->control_command_id, /* cred_flag = */ 0), timeout, &s->control_pid); if (r < 0) { @@ -2696,8 +2701,8 @@ static void service_run_next_main(Service *s) { service_unwatch_main_pid(s); r = service_spawn(s, - SERVICE_EXEC_START, s->main_command, + service_exec_flags(SERVICE_EXEC_START, EXEC_SETUP_CREDENTIALS), s->timeout_start_usec, &pidref); if (r < 0) { diff --git a/test/units/testsuite-54.sh b/test/units/testsuite-54.sh index 914f986545..8f20ac4328 100755 --- a/test/units/testsuite-54.sh +++ b/test/units/testsuite-54.sh @@ -3,6 +3,9 @@ # shellcheck disable=SC2016 set -eux +# shellcheck source=test/units/util.sh +. "$(dirname "$0")"/util.sh + systemd-analyze log-level debug run_with_cred_compare() { @@ -309,6 +312,28 @@ systemd-run -p DynamicUser=yes -p 'LoadCredential=os:/etc/os-release' \ --service-type=oneshot --wait --pipe \ true | cmp /etc/os-release +# https://github.com/systemd/systemd/pull/24734#issuecomment-1925440546 +# Also ExecStartPre= should be able to update creds +dd if=/dev/urandom of=/tmp/cred-huge bs=600K count=1 +chmod 777 /tmp/cred-huge +systemd-run -p ProtectSystem=full \ + -p 'LoadCredential=huge:/tmp/cred-huge' \ + -p 'ExecStartPre=true' \ + -p 'ExecStartPre=bash -c "echo fresh >/tmp/cred-huge"' \ + --unit=test-54-huge-cred.service \ + --wait --pipe \ + systemd-creds cat huge | cmp - <(echo "fresh") +rm /tmp/cred-huge + +echo stable >/tmp/cred-stable +systemd-run -p 'LoadCredential=stable:/tmp/cred-stable' \ + -p 'ExecStartPost=systemd-creds cat stable' \ + --unit=test-54-stable.service \ + --service-type=oneshot --wait --pipe \ + bash -c "echo bogus >/tmp/cred-stable" | cmp - <(echo "stable") +assert_eq "$(cat /tmp/cred-stable)" "bogus" +rm /tmp/cred-stable + if ! systemd-detect-virt -q -c ; then # Validate that the credential we inserted via the initrd logic arrived test "$(systemd-creds cat --system myinitrdcred)" = "guatemala"