From 391df454d1713a8e61098231422151165f3e01ea Mon Sep 17 00:00:00 2001 From: Aditya R Date: Sat, 1 Oct 2022 17:05:01 +0530 Subject: [PATCH] linux: reject sysctl kernel.domainname when OCI knob domainname is set Setting sysctl `kernel.domainname` directly by user is not environment agnostic, it shows either incorrect ( on non-working ) behaviour in `rootless` environment. It was decided to make this part of `runtime-spec` so the OCI runtime can itself handle this behaviour correctly. As a result a new field `domainname` was added to `runtime-spec`. Since crun already implementes this field therefore `sysctl` configured by user conflicts with the behaviour expected by the OCI runtime. Runtime-spec PR: https://github.com/opencontainers/runtime-spec/pull/1156 Furthermore a similar `sysctl` `kernal.hostname` is blocked by crun explicitly to prevent this conflicting behaviour. https://github.com/containers/crun/blob/main/src/libcrun/linux.c#L3203 Following commit ensures that crun rejects sysctl `kernel.domainname` when OCI field `domainname` is already set. Signed-off-by: Aditya R --- src/libcrun/linux.c | 17 ++++++++++----- tests/test_domainname.py | 46 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/src/libcrun/linux.c b/src/libcrun/linux.c index f413f46445..878b6837c6 100644 --- a/src/libcrun/linux.c +++ b/src/libcrun/linux.c @@ -3161,7 +3161,7 @@ const char *sysctlRequiringIPC[] = { }; static int -validate_sysctl (const char *original_value, const char *name, unsigned long namespaces_created, libcrun_error_t *err) +validate_sysctl (const char *original_key, const char *original_value, const char *name, unsigned long namespaces_created, runtime_spec_schema_config_schema *def, libcrun_error_t *err) { const char *namespace = ""; @@ -3192,6 +3192,13 @@ validate_sysctl (const char *original_value, const char *name, unsigned long nam if (strcmp (name, "kernel/domainname") == 0) { + // Value of sysctl `kernel/domainname` is going to + // conflict with already set field `domainname` in + // OCI spec, in such scenario crun will fail to prevent + // unexpected behaviour for end user. + if (! is_empty_string (def->domainname) && (strcmp (original_value, def->domainname) != 0)) + return crun_make_error (err, 0, "the sysctl `%s` conflicts with OCI field `domainname`", original_key); + if (namespaces_created & CLONE_NEWUTS) return 0; @@ -3200,7 +3207,7 @@ validate_sysctl (const char *original_value, const char *name, unsigned long nam } if (strcmp (name, "kernel/hostname") == 0) - return crun_make_error (err, 0, "the sysctl `%s` conflicts with OCI field `hostname`", original_value); + return crun_make_error (err, 0, "the sysctl `%s` conflicts with OCI field `hostname`", original_key); } if (has_prefix (name, "net/")) { @@ -3211,10 +3218,10 @@ validate_sysctl (const char *original_value, const char *name, unsigned long nam goto fail; } - return crun_make_error (err, 0, "the sysctl `%s` is not namespaced", original_value); + return crun_make_error (err, 0, "the sysctl `%s` is not namespaced", original_key); fail: - return crun_make_error (err, 0, "the sysctl `%s` requires a new %s namespace", original_value, namespace); + return crun_make_error (err, 0, "the sysctl `%s` requires a new %s namespace", original_key, namespace); } int @@ -3256,7 +3263,7 @@ libcrun_set_sysctl (libcrun_container_t *container, libcrun_error_t *err) if (*it == '.') *it = '/'; - ret = validate_sysctl (def->linux->sysctl->keys[i], name, namespaces_created, err); + ret = validate_sysctl (def->linux->sysctl->keys[i], def->linux->sysctl->values[i], name, namespaces_created, def, err); if (UNLIKELY (ret < 0)) return ret; diff --git a/tests/test_domainname.py b/tests/test_domainname.py index f204a9c9eb..829b779aa3 100644 --- a/tests/test_domainname.py +++ b/tests/test_domainname.py @@ -43,9 +43,53 @@ def test_domainname(): if cid is not None: run_crun_command(["delete", "-f", cid]) - +def test_domainname_conflict_sysctl(): + # Setting sysctl `kernel.domainname` and OCI field `domainname` must fail + # since it produces unexpected behaviour for the end-users. + conf = base_config() + conf['process']['args'] = ['/init', 'getdomainname'] + conf['domainname'] = "foomachine" + add_all_namespaces(conf) + conf['linux']['sysctl'] = {'kernel.domainname' : 'foo'} + cid = None + try: + out, cid = run_and_get_output(conf) + if out == "(none)\n": + return 0 + sys.stderr.write("unexpected success\n") + return -1 + except: + return 0 + finally: + if cid is not None: + run_crun_command(["delete", "-f", cid]) + return 0 + +def test_domainname_with_sysctl(): + # Setting sysctl `kernel.domainname` and OCI field `domainname` must pass + # when both have exact same value + conf = base_config() + conf['process']['args'] = ['/init', 'getdomainname'] + conf['domainname'] = "foo" + add_all_namespaces(conf) + conf['linux']['sysctl'] = {'kernel.domainname' : 'foo'} + cid = None + try: + out, cid = run_and_get_output(conf) + if out == "(none)\n": + return 0 + return 0 + except: + return -1 + finally: + if cid is not None: + run_crun_command(["delete", "-f", cid]) + return 0 + all_tests = { "domainname" : test_domainname, + "domainname conflict with syctl" : test_domainname_conflict_sysctl, + "domainname with syctl" : test_domainname_with_sysctl, } if __name__ == "__main__":