From 6394e5cd34a157fc8c1281babef7bd3c8ab7e03f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 18:51:47 +0200 Subject: [PATCH 01/16] execute: use ASSERT_PTR where appropriate --- src/core/execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index 3cd63846b50..73531210556 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2716,7 +2716,7 @@ static int load_cred_recurse_dir_cb( void *userdata) { _cleanup_free_ char *credname = NULL, *sub_id = NULL; - struct load_cred_args *args = userdata; + struct load_cred_args *args = ASSERT_PTR(userdata); int r; if (event != RECURSE_DIR_ENTRY) From 9a6994e9718f9d79caeb3441df541d45ad97d7b8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 18:52:10 +0200 Subject: [PATCH 02/16] execute: drop double empty line --- src/core/execute.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index 73531210556..128ed6471ff 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2859,7 +2859,6 @@ static int acquire_credentials( if (r < 0) return r; - left -= add; } From 10b44e1dc1e4263468cda5f8e05008fc737286fc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 18:43:02 +0200 Subject: [PATCH 03/16] execute: rework load_credential() not to take an ExecLoadCredential object we must synthesize Let's just simplify the logic and pass the fields we need as regular arguments, even if that means the function now has a lot. It's otherwise really weird that we have to fake a local ExecLoadCredential from the real one. --- src/core/execute.c | 87 +++++++++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 31 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 128ed6471ff..437d517b718 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2598,7 +2598,9 @@ static int write_credential( static int load_credential( const ExecContext *context, const ExecParameters *params, - ExecLoadCredential *lc, + const char *id, + const char *path, + bool encrypted, const char *unit, int read_dfd, int write_dfd, @@ -2606,12 +2608,6 @@ static int load_credential( bool ownership_ok, uint64_t *left) { - assert(context); - assert(lc); - assert(unit); - assert(write_dfd >= 0); - assert(left); - ReadFullFileFlags flags = READ_FULL_FILE_SECURE|READ_FULL_FILE_FAIL_WHEN_LARGER; _cleanup_(erase_and_freep) char *data = NULL; _cleanup_free_ char *j = NULL, *bindname = NULL; @@ -2620,14 +2616,22 @@ static int load_credential( size_t size, add; int r; - if (path_is_absolute(lc->path) || read_dfd >= 0) { + assert(context); + assert(params); + assert(id); + assert(path); + assert(unit); + assert(write_dfd >= 0); + assert(left); + + if (path_is_absolute(path) || read_dfd >= 0) { /* If this is an absolute path, read the data directly from it, and support AF_UNIX sockets */ - source = lc->path; + source = path; flags |= READ_FULL_FILE_CONNECT_SOCKET; /* Pass some minimal info about the unit and the credential name we are looking to acquire * via the source socket address in case we read off an AF_UNIX socket. */ - if (asprintf(&bindname, "@%" PRIx64"/unit/%s/%s", random_u64(), unit, lc->id) < 0) + if (asprintf(&bindname, "@%" PRIx64"/unit/%s/%s", random_u64(), unit, id) < 0) return -ENOMEM; missing_ok = false; @@ -2636,7 +2640,7 @@ static int load_credential( /* If this is a relative path, take it relative to the credentials we received * ourselves. We don't support the AF_UNIX stuff in this mode, since we are operating * on a credential store, i.e. this is guaranteed to be regular files. */ - j = path_join(params->received_credentials, lc->path); + j = path_join(params->received_credentials, path); if (!j) return -ENOMEM; @@ -2648,14 +2652,14 @@ static int load_credential( r = read_full_file_full( read_dfd, source, UINT64_MAX, - lc->encrypted ? CREDENTIAL_ENCRYPTED_SIZE_MAX : CREDENTIAL_SIZE_MAX, - flags | (lc->encrypted ? READ_FULL_FILE_UNBASE64 : 0), + encrypted ? CREDENTIAL_ENCRYPTED_SIZE_MAX : CREDENTIAL_SIZE_MAX, + flags | (encrypted ? READ_FULL_FILE_UNBASE64 : 0), bindname, &data, &size); else r = -ENOENT; - if (r == -ENOENT && (missing_ok || hashmap_contains(context->set_credentials, lc->id))) { + if (r == -ENOENT && (missing_ok || hashmap_contains(context->set_credentials, id))) { /* Make a missing inherited credential non-fatal, let's just continue. After all apps * will get clear errors if we don't pass such a missing credential on as they * themselves will get ENOENT when trying to read them, which should not be much @@ -2663,17 +2667,17 @@ static int load_credential( * * Also, if the source file doesn't exist, but a fallback is set via SetCredentials= * we are fine, too. */ - log_debug_errno(r, "Couldn't read inherited credential '%s', skipping: %m", lc->path); + log_debug_errno(r, "Couldn't read inherited credential '%s', skipping: %m", path); return 0; } if (r < 0) - return log_debug_errno(r, "Failed to read credential '%s': %m", lc->path); + return log_debug_errno(r, "Failed to read credential '%s': %m", path); - if (lc->encrypted) { + if (encrypted) { _cleanup_free_ void *plaintext = NULL; size_t plaintext_size = 0; - r = decrypt_credential_and_warn(lc->id, now(CLOCK_REALTIME), NULL, data, size, &plaintext, &plaintext_size); + r = decrypt_credential_and_warn(id, now(CLOCK_REALTIME), NULL, data, size, &plaintext, &plaintext_size); if (r < 0) return r; @@ -2681,11 +2685,11 @@ static int load_credential( size = plaintext_size; } - add = strlen(lc->id) + size; + add = strlen(id) + size; if (add > *left) return -E2BIG; - r = write_credential(write_dfd, lc->id, data, size, uid, ownership_ok); + r = write_credential(write_dfd, id, data, size, uid, ownership_ok); if (r < 0) return r; @@ -2695,10 +2699,9 @@ static int load_credential( struct load_cred_args { Set *seen_creds; - const ExecContext *context; const ExecParameters *params; - ExecLoadCredential *parent_local_credential; + ExecLoadCredential *parent_load_credential; const char *unit; int dfd; uid_t uid; @@ -2729,7 +2732,7 @@ static int load_cred_recurse_dir_cb( if (!credname) return -ENOMEM; - sub_id = strjoin(args->parent_local_credential->id, "_", credname); + sub_id = strjoin(args->parent_load_credential->id, "_", credname); if (!sub_id) return -ENOMEM; @@ -2745,12 +2748,18 @@ static int load_cred_recurse_dir_cb( if (r < 0) return r; - r = load_credential(args->context, args->params, - &(ExecLoadCredential) { - .id = sub_id, - .path = (char *) de->d_name, - .encrypted = args->parent_local_credential->encrypted, - }, args->unit, dir_fd, args->dfd, args->uid, args->ownership_ok, args->left); + r = load_credential( + args->context, + args->params, + sub_id, + de->d_name, + args->parent_load_credential->encrypted, + args->unit, + dir_fd, + args->dfd, + args->uid, + args->ownership_ok, + args->left); if (r < 0) return r; @@ -2797,14 +2806,30 @@ static int acquire_credentials( return -errno; if (sub_fd < 0) { + /* Regular file */ + r = set_put_strdup(&seen_creds, lc->id); if (r < 0) return r; - r = load_credential(context, params, lc, unit, -1, dfd, uid, ownership_ok, &left); + + r = load_credential( + context, + params, + lc->id, + lc->path, + lc->encrypted, + unit, + -1, + dfd, + uid, + ownership_ok, + &left); if (r < 0) return r; } else { + /* Directory */ + r = recurse_dir( sub_fd, /* path= */ "", @@ -2816,7 +2841,7 @@ static int acquire_credentials( .seen_creds = seen_creds, .context = context, .params = params, - .parent_local_credential = lc, + .parent_load_credential = lc, .unit = unit, .dfd = dfd, .uid = uid, From 1451435ca5d43c2e632b7448d45e8b04dc727d62 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 18:51:56 +0200 Subject: [PATCH 04/16] execute: debug log if a generated recursive cred name is too long --- src/core/execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index 437d517b718..4cc24466753 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2737,7 +2737,7 @@ static int load_cred_recurse_dir_cb( return -ENOMEM; if (!credential_name_valid(sub_id)) - return -EINVAL; + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Credential would get ID %s, which is not valid, refusing", sub_id); if (set_contains(args->seen_creds, sub_id)) { log_debug("Skipping credential with duplicated ID %s at %s", sub_id, path); From 3691083ce5e12e0290f0c8a83983cb0eefb6c126 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 18:51:02 +0200 Subject: [PATCH 05/16] execute: passing NULL as second argument for recurse_dir() is equivalent to "" --- src/core/execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index 4cc24466753..580589eab82 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2832,7 +2832,7 @@ static int acquire_credentials( r = recurse_dir( sub_fd, - /* path= */ "", + /* path= */ NULL, /* statx_mask= */ 0, /* n_depth_max= */ UINT_MAX, RECURSE_DIR_IGNORE_DOT|RECURSE_DIR_ENSURE_TYPE, From 11348386158ae51af7d3866f814b5468877fa60b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 22:48:30 +0200 Subject: [PATCH 06/16] execute: let recurse_dir() concate the cred name for us recurse_dir() allows specifiying a freely choosable initial path to which to append the subdirs as it descends into the tree. If we pass the configured id there, recurse_dir() will suffix the subdir to that for us, so that we don't have to do that manually anymore in the callback, simplifying things a bit. --- src/core/execute.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 580589eab82..eb06215266c 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2718,8 +2718,8 @@ static int load_cred_recurse_dir_cb( const struct statx *sx, void *userdata) { - _cleanup_free_ char *credname = NULL, *sub_id = NULL; struct load_cred_args *args = ASSERT_PTR(userdata); + _cleanup_free_ char *sub_id = NULL; int r; if (event != RECURSE_DIR_ENTRY) @@ -2728,11 +2728,7 @@ static int load_cred_recurse_dir_cb( if (!IN_SET(de->d_type, DT_REG, DT_SOCK)) return RECURSE_DIR_CONTINUE; - credname = strreplace(path, "/", "_"); - if (!credname) - return -ENOMEM; - - sub_id = strjoin(args->parent_load_credential->id, "_", credname); + sub_id = strreplace(path, "/", "_"); if (!sub_id) return -ENOMEM; @@ -2832,7 +2828,7 @@ static int acquire_credentials( r = recurse_dir( sub_fd, - /* path= */ NULL, + /* 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, RECURSE_DIR_IGNORE_DOT|RECURSE_DIR_ENSURE_TYPE, From 461345a1640043f8b138f4707097685568cd6376 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 22:51:13 +0200 Subject: [PATCH 07/16] execute: simplify 'load_creds_args' struct a bit Given we only need a single field off the ExecLoadCredential structure we don't have to link it as a whole, but just copy that one bit over directly, simplifying the struct a bit. --- src/core/execute.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index eb06215266c..de6ea283665 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2701,7 +2701,7 @@ struct load_cred_args { Set *seen_creds; const ExecContext *context; const ExecParameters *params; - ExecLoadCredential *parent_load_credential; + bool encrypted; const char *unit; int dfd; uid_t uid; @@ -2749,7 +2749,7 @@ static int load_cred_recurse_dir_cb( args->params, sub_id, de->d_name, - args->parent_load_credential->encrypted, + args->encrypted, args->unit, dir_fd, args->dfd, @@ -2837,7 +2837,7 @@ static int acquire_credentials( .seen_creds = seen_creds, .context = context, .params = params, - .parent_load_credential = lc, + .encrypted = lc->encrypted, .unit = unit, .dfd = dfd, .uid = uid, From 5bec447afb9e0a72e28be5e3f7f03b370b486fc9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 23:01:16 +0200 Subject: [PATCH 08/16] execute: drop 'seen_creds' set When checking whether we already loaded a credential before, let's just use faccessat() in the credential dir we are populating. First of all, we already do it exactly that way when appliying SetCredential= settings later. Secondly, this is not performance relevant, and by using faccessat() things simply become a lot simpler. --- src/core/execute.c | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index de6ea283665..a0c58ac255e 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2698,7 +2698,6 @@ static int load_credential( } struct load_cred_args { - Set *seen_creds; const ExecContext *context; const ExecParameters *params; bool encrypted; @@ -2735,14 +2734,12 @@ static int load_cred_recurse_dir_cb( if (!credential_name_valid(sub_id)) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Credential would get ID %s, which is not valid, refusing", sub_id); - if (set_contains(args->seen_creds, sub_id)) { + if (faccessat(args->dfd, sub_id, F_OK, AT_SYMLINK_NOFOLLOW) >= 0) { log_debug("Skipping credential with duplicated ID %s at %s", sub_id, path); return RECURSE_DIR_CONTINUE; } - - r = set_put_strdup(&args->seen_creds, sub_id); - if (r < 0) - return r; + if (errno != ENOENT) + return log_debug_errno(errno, "Failed to test if credential %s exists: %m", sub_id); r = load_credential( args->context, @@ -2772,7 +2769,6 @@ static int acquire_credentials( uint64_t left = CREDENTIALS_TOTAL_SIZE_MAX; _cleanup_close_ int dfd = -1; - _cleanup_set_free_ Set *seen_creds = NULL; ExecLoadCredential *lc; ExecSetCredential *sc; int r; @@ -2784,10 +2780,6 @@ static int acquire_credentials( if (dfd < 0) return -errno; - seen_creds = set_new(&string_hash_ops_free); - if (!seen_creds) - return -ENOMEM; - /* First, load credentials off disk (or acquire via AF_UNIX socket) */ HASHMAP_FOREACH(lc, context->load_credentials) { _cleanup_close_ int sub_fd = -1; @@ -2804,10 +2796,6 @@ static int acquire_credentials( if (sub_fd < 0) { /* Regular file */ - r = set_put_strdup(&seen_creds, lc->id); - if (r < 0) - return r; - r = load_credential( context, params, @@ -2834,7 +2822,6 @@ static int acquire_credentials( RECURSE_DIR_IGNORE_DOT|RECURSE_DIR_ENSURE_TYPE, load_cred_recurse_dir_cb, &(struct load_cred_args) { - .seen_creds = seen_creds, .context = context, .params = params, .encrypted = lc->encrypted, From 9883cbb203b1648cc8c49038a03dd7fe30a24761 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 23:03:22 +0200 Subject: [PATCH 09/16] execute: sort directory entries when loading credentials recursively Given that the recusive credential loading allows two ways to load the same credentials, it's important to define a clear order so that it is always the same one that wins. i.e. if you use LoadCredential=foobar:/tmp/xyz and there are two files /tmp/xyz/abc/cde and /tmp/xyz/abc_cde these would both result in a credential foobar_abc_cde being set, hence it is important to make clear which one shall win, and that it is always the same one. --- src/core/execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index a0c58ac255e..c85ca2a3564 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2819,7 +2819,7 @@ static int acquire_credentials( /* 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, - RECURSE_DIR_IGNORE_DOT|RECURSE_DIR_ENSURE_TYPE, + RECURSE_DIR_SORT|RECURSE_DIR_IGNORE_DOT|RECURSE_DIR_ENSURE_TYPE, load_cred_recurse_dir_cb, &(struct load_cred_args) { .context = context, From 9e6e9d61bda7430fc03b48bbc605a9d0b521d7ba Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 23:05:38 +0200 Subject: [PATCH 10/16] execute: correct comments This is not done first, but second. Also, while we are at it, explain why faccessat() is OK here. --- src/core/execute.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index c85ca2a3564..f93c7b79927 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2836,13 +2836,17 @@ static int acquire_credentials( } } - /* First we use the literally specified credentials. Note that they might be overridden again below, - * and thus act as a "default" if the same credential is specified multiple times */ + /* Second, we add in literally specified credentials. If the credentials already exist, we'll not add + * them, so that they can act as a "default" if the same credential is specified multiple times. */ HASHMAP_FOREACH(sc, context->set_credentials) { _cleanup_(erase_and_freep) void *plaintext = NULL; const char *data; size_t size, add; + /* Note that we check ahead of time here instead of relying on O_EXCL|O_CREAT later to return + * EEXIST if the credential already exists. That's because the TPM2-based decryption is kinda + * slow and involved, hence it's nice to be able to skip that if the credential already + * exists anyway. */ if (faccessat(dfd, sc->id, F_OK, AT_SYMLINK_NOFOLLOW) >= 0) continue; if (errno != ENOENT) From 61c5a49eb251264a875a23527346698d6390445b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 23:07:26 +0200 Subject: [PATCH 11/16] execute: share error path between reg file/dir credential loading --- src/core/execute.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index f93c7b79927..a16dbdd0c78 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2793,9 +2793,8 @@ static int acquire_credentials( if (sub_fd < 0 && errno != ENOTDIR) return -errno; - if (sub_fd < 0) { + if (sub_fd < 0) /* Regular file */ - r = load_credential( context, params, @@ -2808,12 +2807,8 @@ static int acquire_credentials( uid, ownership_ok, &left); - if (r < 0) - return r; - - } else { + else /* Directory */ - r = recurse_dir( sub_fd, /* path= */ lc->id, /* recurse_dir() will suffix the subdir paths from here to the top-level id */ @@ -2831,9 +2826,8 @@ static int acquire_credentials( .ownership_ok = ownership_ok, .left = &left, }); - if (r < 0) - return r; - } + if (r < 0) + return r; } /* Second, we add in literally specified credentials. If the credentials already exist, we'll not add From f344f7fdca63ca90cc3b4ea7d30f32c43dd3d1a6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 13 Apr 2022 23:35:15 +0200 Subject: [PATCH 12/16] execute: restore ability to propagate creds from further up (i.e. container manager and such) This was broken in 3989bdc1ad7cca4d75c06cdf601fea2cb37ba337 let's restore the functionality. Basically, we want that if a relative name is specified as source to load from we take it relative to the credentials dir the service manager itself got passed. --- src/core/execute.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index a16dbdd0c78..5e6b1131e47 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2625,7 +2625,8 @@ static int load_credential( assert(left); if (path_is_absolute(path) || read_dfd >= 0) { - /* If this is an absolute path, read the data directly from it, and support AF_UNIX sockets */ + /* If this is an absolute path (or a directory fd is specifier relative which to read), read + * the data directly from it, and support AF_UNIX sockets */ source = path; flags |= READ_FULL_FILE_CONNECT_SOCKET; @@ -2784,17 +2785,19 @@ static int acquire_credentials( HASHMAP_FOREACH(lc, context->load_credentials) { _cleanup_close_ int sub_fd = -1; - /* Skip over credentials with unspecified paths. These are received by the - * service manager via the $CREDENTIALS_DIRECTORY environment variable. */ - if (!is_path(lc->path) && streq(lc->id, lc->path)) - continue; + /* If this is an absolute path, then try to open it as a directory. If that works, then we'll + * recurse into it. If it is an absolute path but it isn't a directory, then we'll open it as + * a regular file. Finally, if it's a relative path we will use it as a credential name to + * propagate a credential passed to us from further up. */ - sub_fd = open(lc->path, O_DIRECTORY|O_CLOEXEC|O_RDONLY); - if (sub_fd < 0 && errno != ENOTDIR) - return -errno; + if (path_is_absolute(lc->path)) { + sub_fd = open(lc->path, O_DIRECTORY|O_CLOEXEC|O_RDONLY); + if (sub_fd < 0 && errno != ENOTDIR) + return -errno; + } if (sub_fd < 0) - /* Regular file */ + /* Regular file (incl. a credential passed in from higher up) */ r = load_credential( context, params, From 1d68a2e1684f14c885449569c6b2d95d34ee965d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Apr 2022 18:01:28 +0200 Subject: [PATCH 13/16] execute: restore ability that SetCredential= can act as fallback for LoadCredential= If SetCredential= and LoadCredentials= are combined for the same credential name, then the former shall act as fallback for the latter in case the source file does not exist. That's documented, but didn't work. Let's fix that. --- src/core/execute.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 5e6b1131e47..fac3be8e29f 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2792,8 +2792,10 @@ static int acquire_credentials( if (path_is_absolute(lc->path)) { sub_fd = open(lc->path, O_DIRECTORY|O_CLOEXEC|O_RDONLY); - if (sub_fd < 0 && errno != ENOTDIR) - return -errno; + if (sub_fd < 0 && !IN_SET(errno, + ENOTDIR, /* Not a directory */ + ENOENT)) /* Doesn't exist? */ + return log_debug_errno(errno, "Failed to open '%s': %m", lc->path); } if (sub_fd < 0) From 94602bff1f61cdb928843e4aa09e03b8d54e5ab7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 14 Apr 2022 18:08:36 +0200 Subject: [PATCH 14/16] execute: add more debug logging --- src/core/execute.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/execute.c b/src/core/execute.c index fac3be8e29f..61bfedfcd3d 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2692,7 +2692,7 @@ static int load_credential( r = write_credential(write_dfd, id, data, size, uid, ownership_ok); if (r < 0) - return r; + return log_debug_errno(r, "Failed to write credential '%s': %m", id); *left -= add; return 0; From 6d085447fa63579a77387585b097e2558ea0f9da Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 21 Apr 2022 17:35:38 +0200 Subject: [PATCH 15/16] test: make sure that SetCredential=/LoadCredential fallback won#t regress --- test/units/testsuite-54.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/units/testsuite-54.sh b/test/units/testsuite-54.sh index c8b685b675c..15b0d5c2db1 100755 --- a/test/units/testsuite-54.sh +++ b/test/units/testsuite-54.sh @@ -16,6 +16,12 @@ systemd-run -p LoadCredential=passwd:/etc/passwd \ ( cat /etc/passwd /etc/shadow && echo -n wuff ) | cmp /tmp/ts54-concat rm /tmp/ts54-concat +# Test that SetCredential= acts as fallback for LoadCredential= +echo piff > /tmp/ts54-fallback +[ "$(systemd-run -p LoadCredential=paff:/tmp/ts54-fallback -p SetCredential=paff:poff --pipe --wait systemd-creds cat paff)" = "piff" ] +rm /tmp/ts54-fallback +[ "$(systemd-run -p LoadCredential=paff:/tmp/ts54-fallback -p SetCredential=paff:poff --pipe --wait systemd-creds cat paff)" = "poff" ] + # Verify that the creds are immutable systemd-run -p LoadCredential=passwd:/etc/passwd \ -p DynamicUser=1 \ From 42a3f23cc1548eff521468cb908be6c2c1160379 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 22 Apr 2022 11:31:00 +0200 Subject: [PATCH 16/16] =?UTF-8?q?test:=20also=20test=20nspawn=20system?= =?UTF-8?q?=E2=86=92service=20inheritance=20of=20creds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/TEST-54-CREDS/test.sh | 1 + test/units/testsuite-54.sh | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/test/TEST-54-CREDS/test.sh b/test/TEST-54-CREDS/test.sh index 3689be42032..d045d2391f2 100755 --- a/test/TEST-54-CREDS/test.sh +++ b/test/TEST-54-CREDS/test.sh @@ -3,6 +3,7 @@ set -e TEST_DESCRIPTION="test credentials" +NSPAWN_ARGUMENTS="--set-credential=mynspawncredential:strangevalue" # shellcheck source=test/test-functions . "${TEST_BASE_DIR:?}/test-functions" diff --git a/test/units/testsuite-54.sh b/test/units/testsuite-54.sh index 15b0d5c2db1..bf43205cbd2 100755 --- a/test/units/testsuite-54.sh +++ b/test/units/testsuite-54.sh @@ -22,6 +22,20 @@ echo piff > /tmp/ts54-fallback rm /tmp/ts54-fallback [ "$(systemd-run -p LoadCredential=paff:/tmp/ts54-fallback -p SetCredential=paff:poff --pipe --wait systemd-creds cat paff)" = "poff" ] +if systemd-detect-virt -q -c ; then + # If this test is run in nspawn a credential should have been passed to us. See test/TEST-54-CREDS/test.sh + [ "$(systemd-creds --system cat mynspawncredential)" = "strangevalue" ] + + # Test that propagation from system credential to service credential works + [ "$(systemd-run -p LoadCredential=mynspawncredential --pipe --wait systemd-creds cat mynspawncredential)" = "strangevalue" ] + + # Check it also works, if we rename it while propagating it + [ "$(systemd-run -p LoadCredential=miau:mynspawncredential --pipe --wait systemd-creds cat miau)" = "strangevalue" ] + + # Combine it with a fallback (which should have no effect, given the cred should be passed down) + [ "$(systemd-run -p LoadCredential=mynspawncredential -p SetCredential=mynspawncredential:zzz --pipe --wait systemd-creds cat mynspawncredential)" = "strangevalue" ] +fi + # Verify that the creds are immutable systemd-run -p LoadCredential=passwd:/etc/passwd \ -p DynamicUser=1 \