From 6045958babfa2321d949c5940d5755959ae00431 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 22 Nov 2023 11:52:39 +0100 Subject: [PATCH 1/5] machine-credential: fix error logging Remove duplicate logging: let exclusively machine_credential_load()/machine_credential_set() log, and not the caller again. --- src/nspawn/nspawn.c | 9 +++------ src/shared/machine-credential.c | 10 +++------- src/vmspawn/vmspawn.c | 8 ++------ 3 files changed, 8 insertions(+), 19 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 38196ef3d6..8ac86eeb55 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1569,19 +1569,16 @@ static int parse_argv(int argc, char *argv[]) { case ARG_SET_CREDENTIAL: r = machine_credential_set(&arg_credentials, &arg_n_credentials, optarg); - if (r == -ENOMEM) - return log_oom(); if (r < 0) - return log_error_errno(r, "Failed to set credential from %s: %m", optarg); + return r; + arg_settings_mask |= SETTING_CREDENTIALS; break; case ARG_LOAD_CREDENTIAL: r = machine_credential_load(&arg_credentials, &arg_n_credentials, optarg); - if (r == -ENOMEM) - return log_oom(); if (r < 0) - return log_error_errno(r, "Failed to load credential from %s: %m", optarg); + return r; arg_settings_mask |= SETTING_CREDENTIALS; break; diff --git a/src/shared/machine-credential.c b/src/shared/machine-credential.c index 752bf0da9c..9c3a352e94 100644 --- a/src/shared/machine-credential.c +++ b/src/shared/machine-credential.c @@ -39,8 +39,6 @@ int machine_credential_set(MachineCredential **credentials, size_t *n_credential assert(creds || n_creds == 0); r = extract_first_word(&p, &word, ":", EXTRACT_DONT_COALESCE_SEPARATORS); - if (r == -ENOMEM) - return r; if (r < 0) return log_error_errno(r, "Failed to parse --set-credential= parameter: %m"); if (r == 0 || !p) @@ -59,7 +57,7 @@ int machine_credential_set(MachineCredential **credentials, size_t *n_credential GREEDY_REALLOC(creds, n_creds + 1); if (!creds) - return -ENOMEM; + return log_oom(); creds[n_creds++] = (MachineCredential) { .id = TAKE_PTR(word), @@ -85,8 +83,6 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia assert(creds || n_creds == 0); r = extract_first_word(&p, &word, ":", EXTRACT_DONT_COALESCE_SEPARATORS); - if (r == -ENOMEM) - return -ENOMEM; if (r < 0) return log_error_errno(r, "Failed to parse --load-credential= parameter: %m"); if (r == 0 || !p) @@ -110,7 +106,7 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia j = path_join(e, p); if (!j) - return -ENOMEM; + return log_oom(); } r = read_full_file_full(AT_FDCWD, j ?: p, UINT64_MAX, SIZE_MAX, @@ -122,7 +118,7 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia GREEDY_REALLOC(creds, n_creds + 1); if (!creds) - return -ENOMEM; + return log_oom(); creds[n_creds++] = (MachineCredential) { .id = TAKE_PTR(word), diff --git a/src/vmspawn/vmspawn.c b/src/vmspawn/vmspawn.c index 6cfaed97f0..ebae681893 100644 --- a/src/vmspawn/vmspawn.c +++ b/src/vmspawn/vmspawn.c @@ -225,20 +225,16 @@ static int parse_argv(int argc, char *argv[]) { case ARG_SET_CREDENTIAL: { r = machine_credential_set(&arg_credentials, &arg_n_credentials, optarg); - if (r == -ENOMEM) - return log_oom(); if (r < 0) - return log_error_errno(r, "Failed to set credential from %s: %m", optarg); + return r; arg_settings_mask |= SETTING_CREDENTIALS; break; } case ARG_LOAD_CREDENTIAL: { r = machine_credential_load(&arg_credentials, &arg_n_credentials, optarg); - if (r == -ENOMEM) - return log_oom(); if (r < 0) - return log_error_errno(r, "Failed to load credential from %s: %m", optarg); + return r; arg_settings_mask |= SETTING_CREDENTIALS; break; From d66c4d101e9d0da1c3b4a13fade2ec9e5f4b0ed8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 22 Nov 2023 11:53:45 +0100 Subject: [PATCH 2/5] machine-credential: fix GREEDY_REALLOC error handling We must check the return value of GREEDY_REALLOC for OOM, and the pointer are updated already on success, hence it's a bad idea to make a copy of the pointer beforehand. --- src/shared/machine-credential.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/shared/machine-credential.c b/src/shared/machine-credential.c index 9c3a352e94..0e6db381b6 100644 --- a/src/shared/machine-credential.c +++ b/src/shared/machine-credential.c @@ -30,13 +30,12 @@ void machine_credential_free_all(MachineCredential *creds, size_t n) { int machine_credential_set(MachineCredential **credentials, size_t *n_credentials, const char *cred_string) { _cleanup_free_ char *word = NULL, *data = NULL; - MachineCredential *creds = *ASSERT_PTR(credentials); ssize_t l; - size_t n_creds = *ASSERT_PTR(n_credentials); int r; const char *p = ASSERT_PTR(cred_string); - assert(creds || n_creds == 0); + assert(credentials && n_credentials); + assert(*credentials || *n_credentials == 0); r = extract_first_word(&p, &word, ":", EXTRACT_DONT_COALESCE_SEPARATORS); if (r < 0) @@ -47,7 +46,7 @@ int machine_credential_set(MachineCredential **credentials, size_t *n_credential if (!credential_name_valid(word)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "MachineCredential name is not valid: %s", word); - FOREACH_ARRAY(cred, creds, n_creds) + FOREACH_ARRAY(cred, *credentials, *n_credentials) if (streq(cred->id, word)) return log_error_errno(SYNTHETIC_ERRNO(EEXIST), "Duplicate credential '%s', refusing.", word); @@ -55,19 +54,15 @@ int machine_credential_set(MachineCredential **credentials, size_t *n_credential if (l < 0) return log_error_errno(l, "Failed to unescape credential data: %s", p); - GREEDY_REALLOC(creds, n_creds + 1); - if (!creds) + if (!GREEDY_REALLOC(*credentials, *n_credentials + 1)) return log_oom(); - creds[n_creds++] = (MachineCredential) { + (*credentials)[(*n_credentials)++] = (MachineCredential) { .id = TAKE_PTR(word), .data = TAKE_PTR(data), .size = l, }; - *credentials = creds; - *n_credentials = n_creds; - return 0; } @@ -75,12 +70,12 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia ReadFullFileFlags flags = READ_FULL_FILE_SECURE; _cleanup_(erase_and_freep) char *data = NULL; _cleanup_free_ char *word = NULL, *j = NULL; - MachineCredential *creds = *ASSERT_PTR(credentials); - size_t size, n_creds = *ASSERT_PTR(n_credentials); - int r; const char *p = ASSERT_PTR(cred_path); + size_t size; + int r; - assert(creds || n_creds == 0); + assert(credentials && n_credentials); + assert(*credentials || *n_credentials == 0); r = extract_first_word(&p, &word, ":", EXTRACT_DONT_COALESCE_SEPARATORS); if (r < 0) @@ -91,7 +86,7 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia if (!credential_name_valid(word)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "MachineCredential name is not valid: %s", word); - FOREACH_ARRAY(cred, creds, n_creds) + FOREACH_ARRAY(cred, *credentials, *n_credentials) if (streq(cred->id, word)) return log_error_errno(SYNTHETIC_ERRNO(EEXIST), "Duplicate credential '%s', refusing.", word); @@ -116,18 +111,14 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia if (r < 0) return log_error_errno(r, "Failed to read credential '%s': %m", j ?: p); - GREEDY_REALLOC(creds, n_creds + 1); - if (!creds) + if (!GREEDY_REALLOC(*credentials, *n_credentials + 1)) return log_oom(); - creds[n_creds++] = (MachineCredential) { + (*credentials)[(*n_credentials)++] = (MachineCredential) { .id = TAKE_PTR(word), .data = TAKE_PTR(data), .size = size, }; - *credentials = creds; - *n_credentials = n_creds; - return 0; } From 437f3e156d3df0ce0e12dfd76877bf986e044c0d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 22 Nov 2023 11:55:24 +0100 Subject: [PATCH 3/5] machine-credential: use word 'Credential' rather than 'MachineCredential' Probably some search/replace mistake. --- src/shared/machine-credential.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/shared/machine-credential.c b/src/shared/machine-credential.c index 0e6db381b6..992ac47c11 100644 --- a/src/shared/machine-credential.c +++ b/src/shared/machine-credential.c @@ -44,7 +44,7 @@ int machine_credential_set(MachineCredential **credentials, size_t *n_credential return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Missing value for --set-credential=: %s", cred_string); if (!credential_name_valid(word)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "MachineCredential name is not valid: %s", word); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Credential name is not valid: %s", word); FOREACH_ARRAY(cred, *credentials, *n_credentials) if (streq(cred->id, word)) @@ -84,7 +84,7 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Missing value for --load-credential=: %s", cred_path); if (!credential_name_valid(word)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "MachineCredential name is not valid: %s", word); + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Credential name is not valid: %s", word); FOREACH_ARRAY(cred, *credentials, *n_credentials) if (streq(cred->id, word)) @@ -97,7 +97,7 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia r = get_credentials_dir(&e); if (r < 0) - return log_error_errno(r, "MachineCredential not available (no credentials passed at all): %s", word); + return log_error_errno(r, "Credential not available (no credentials passed at all): %s", word); j = path_join(e, p); if (!j) From 1311946233c72ce6e9d555a6b365c5b2e5268585 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 22 Nov 2023 11:55:46 +0100 Subject: [PATCH 4/5] machine-credential: simplify code a bit --- src/shared/machine-credential.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/shared/machine-credential.c b/src/shared/machine-credential.c index 992ac47c11..7df3604977 100644 --- a/src/shared/machine-credential.c +++ b/src/shared/machine-credential.c @@ -102,14 +102,16 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia j = path_join(e, p); if (!j) return log_oom(); + + p = j; } - r = read_full_file_full(AT_FDCWD, j ?: p, UINT64_MAX, SIZE_MAX, + r = read_full_file_full(AT_FDCWD, p, UINT64_MAX, SIZE_MAX, flags, NULL, &data, &size); if (r < 0) - return log_error_errno(r, "Failed to read credential '%s': %m", j ?: p); + return log_error_errno(r, "Failed to read credential '%s': %m", p); if (!GREEDY_REALLOC(*credentials, *n_credentials + 1)) return log_oom(); From 2105f79ed33b08b054f3d5e50445ea5e7e75a2e3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 22 Nov 2023 12:02:07 +0100 Subject: [PATCH 5/5] machine-credential: tweak --load-credential= use a bit Let's try to recognize paths (i.e. those with a "/") as source for credentials to load, and then read them from the file system. Also, only read credentials from an inbound credentials directory if the source qualifies as valid credential name. Otherwise print a nice error. --- src/shared/machine-credential.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/shared/machine-credential.c b/src/shared/machine-credential.c index 7df3604977..17f7afc4a0 100644 --- a/src/shared/machine-credential.c +++ b/src/shared/machine-credential.c @@ -90,9 +90,9 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia if (streq(cred->id, word)) return log_error_errno(SYNTHETIC_ERRNO(EEXIST), "Duplicate credential '%s', refusing.", word); - if (path_is_absolute(p)) + if (is_path(p) && path_is_valid(p)) flags |= READ_FULL_FILE_CONNECT_SOCKET; - else { + else if (credential_name_valid(p)) { const char *e; r = get_credentials_dir(&e); @@ -104,7 +104,8 @@ int machine_credential_load(MachineCredential **credentials, size_t *n_credentia return log_oom(); p = j; - } + } else + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Credential source appears to be neither a valid path nor a credential name: %s", p); r = read_full_file_full(AT_FDCWD, p, UINT64_MAX, SIZE_MAX, flags,