From e88748c17e58aad6818e64fd3071de011808165e Mon Sep 17 00:00:00 2001 From: Quentin Deslandes Date: Fri, 22 Jul 2022 17:18:15 +0200 Subject: [PATCH] sysctl: add --strict option to fail if sysctl does not exists systemd-sysctl currently fails silently under any of these conditions: - Missing permission to write a sysctl. - Invalid sysctl (path doesn't exists). - Ignore failure flag ('-' in front of the sysctl name). Because of this behaviour, configuration issues can go unnoticed as there is no way to detect those unless going through the logs. --strict option forces systemd-sysctl to fail if a sysctl is invalid or if permission are insufficient. Errors on sysctl marked as "ignore failure" will still be ignored. --- man/systemd-sysctl.service.xml | 8 ++++++++ src/sysctl/sysctl.c | 22 ++++++++++++++++------ test/TEST-76-SYSCTL/Makefile | 6 ++++++ test/TEST-76-SYSCTL/test.sh | 10 ++++++++++ test/units/testsuite-76.service | 8 ++++++++ test/units/testsuite-76.sh | 16 ++++++++++++++++ 6 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 test/TEST-76-SYSCTL/Makefile create mode 100755 test/TEST-76-SYSCTL/test.sh create mode 100644 test/units/testsuite-76.service create mode 100755 test/units/testsuite-76.sh diff --git a/man/systemd-sysctl.service.xml b/man/systemd-sysctl.service.xml index 312bc3ba43f..fede8b092d7 100644 --- a/man/systemd-sysctl.service.xml +++ b/man/systemd-sysctl.service.xml @@ -64,6 +64,14 @@ Only apply rules with the specified prefix. + + + + Always return non-zero exit code on failure (including invalid sysctl variable + name and insufficient permissions), unless the sysctl variable name is prefixed with a "-" + character. + + diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c index e92640d9489..de0e03ec95b 100644 --- a/src/sysctl/sysctl.c +++ b/src/sysctl/sysctl.c @@ -28,6 +28,7 @@ static char **arg_prefixes = NULL; static bool arg_cat_config = false; +static bool arg_strict = false; static PagerFlags arg_pager_flags = 0; STATIC_DESTRUCTOR_REGISTER(arg_prefixes, strv_freep); @@ -101,13 +102,16 @@ static int sysctl_write_or_warn(const char *key, const char *value, bool ignore_ r = sysctl_write(key, value); if (r < 0) { - /* If the sysctl is not available in the kernel or we are running with reduced privileges and - * cannot write it, then log about the issue, and proceed without failing. (EROFS is treated - * as a permission problem here, since that's how container managers usually protected their - * sysctls.) In all other cases log an error and make the tool fail. */ - if (ignore_failure || r == -EROFS || ERRNO_IS_PRIVILEGE(r)) + /* Proceed without failing if ignore_failure is true. + * If the sysctl is not available in the kernel or we are running with reduced privileges and + * cannot write it, then log about the issue, and proceed without failing. Unless strict mode + * (arg_strict = true) is enabled, in which case we should fail. (EROFS is treated as a + * permission problem here, since that's how container managers usually protected their + * sysctls.) + * In all other cases log an error and make the tool fail. */ + if (ignore_failure || (!arg_strict && (r == -EROFS || ERRNO_IS_PRIVILEGE(r)))) log_debug_errno(r, "Couldn't write '%s' to '%s', ignoring: %m", value, key); - else if (r == -ENOENT) + else if (!arg_strict && r == -ENOENT) log_warning_errno(r, "Couldn't write '%s' to '%s', ignoring: %m", value, key); else return log_error_errno(r, "Couldn't write '%s' to '%s': %m", value, key); @@ -326,6 +330,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_CAT_CONFIG, ARG_PREFIX, ARG_NO_PAGER, + ARG_STRICT, }; static const struct option options[] = { @@ -334,6 +339,7 @@ static int parse_argv(int argc, char *argv[]) { { "cat-config", no_argument, NULL, ARG_CAT_CONFIG }, { "prefix", required_argument, NULL, ARG_PREFIX }, { "no-pager", no_argument, NULL, ARG_NO_PAGER }, + { "strict", no_argument, NULL, ARG_STRICT }, {} }; @@ -382,6 +388,10 @@ static int parse_argv(int argc, char *argv[]) { arg_pager_flags |= PAGER_DISABLE; break; + case ARG_STRICT: + arg_strict = true; + break; + case '?': return -EINVAL; diff --git a/test/TEST-76-SYSCTL/Makefile b/test/TEST-76-SYSCTL/Makefile new file mode 100644 index 00000000000..9f65d4ca4fd --- /dev/null +++ b/test/TEST-76-SYSCTL/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later + +all setup run clean clean-again: + @TEST_BASE_DIR=../ ./test.sh --$@ + +.PHONY: all setup run clean clean-again diff --git a/test/TEST-76-SYSCTL/test.sh b/test/TEST-76-SYSCTL/test.sh new file mode 100755 index 00000000000..11a44afaf53 --- /dev/null +++ b/test/TEST-76-SYSCTL/test.sh @@ -0,0 +1,10 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -e + +TEST_DESCRIPTION="Sysctl-related tests" + +# shellcheck source=test/test-functions +. "${TEST_BASE_DIR:?}/test-functions" + +do_test "$@" diff --git a/test/units/testsuite-76.service b/test/units/testsuite-76.service new file mode 100644 index 00000000000..3c8a9e867fa --- /dev/null +++ b/test/units/testsuite-76.service @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Unit] +Description=TEST-76-SYSCTL + +[Service] +ExecStartPre=rm -f /failed /testok +ExecStart=/usr/lib/systemd/tests/testdata/units/%N.sh +Type=oneshot diff --git a/test/units/testsuite-76.sh b/test/units/testsuite-76.sh new file mode 100755 index 00000000000..6ebdbc6a549 --- /dev/null +++ b/test/units/testsuite-76.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -eux +set -o pipefail + +export SYSTEMD_LOG_LEVEL=debug + +echo "foo.bar=42" > /etc/sysctl.d/foo.conf +[[ $(/usr/lib/systemd/systemd-sysctl /etc/sysctl.d/foo.conf)$? -eq 0 ]] +[[ $(/usr/lib/systemd/systemd-sysctl --strict /etc/sysctl.d/foo.conf)$? -ne 0 ]] + +echo "-foo.foo=42" > /etc/sysctl.d/foo.conf +[[ $(/usr/lib/systemd/systemd-sysctl /etc/sysctl.d/foo.conf)$? -eq 0 ]] +[[ $(/usr/lib/systemd/systemd-sysctl --strict /etc/sysctl.d/foo.conf)$? -eq 0 ]] + +touch /testok -- 2.47.3