From bd80a56ebd1addd9de84bb3db954eb8bbd2f79c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 28 Oct 2026 01:34:34 +0100 Subject: [PATCH] hostnamed: allow removal of tags by glob Previously, we would process additions first and removal second. Since only exact patterns were allowed, this was mostly useless, since the caller could just omit the tags that shouldn't be added from the list. When globs are allowed, removing first and adding second becomes very useful, so let's just do that. Also, we previously verified that list of tags to remove using the same rules as when adding tags. But removal is not symmetrical: we can only have one value for a tag, but it makes sense to remove multiple (possible) values for a tag. So the whole check validity check is simplified to only check the length of the array (to limit excessive processing in the service). It's not even useful to check for validity of removed tags: if they are not valid, they will simply not match anything. With those minimal changes, the existing code allows globs to be used in removal patterns. --- man/org.freedesktop.hostname1.xml | 14 ++++--- src/hostname/hostnamed.c | 18 ++++----- src/shared/machine-tags.c | 49 ++++++++++++++++++------ src/shared/machine-tags.h | 3 +- src/shared/varlink-io.systemd.Hostname.c | 4 +- src/test/test-machine-tags.c | 48 ++++++++++++++++------- test/units/TEST-71-HOSTNAME.sh | 16 ++++++++ 7 files changed, 107 insertions(+), 45 deletions(-) diff --git a/man/org.freedesktop.hostname1.xml b/man/org.freedesktop.hostname1.xml index 09097145a1d..2ce3e87bee1 100644 --- a/man/org.freedesktop.hostname1.xml +++ b/man/org.freedesktop.hostname1.xml @@ -390,13 +390,15 @@ node /org/freedesktop/hostname1 { the TAGS= field from /etc/machine-info. AddAndRemoveTags() incrementally updates the Tags - property instead of replacing it. It takes two lists of machine tags: the tags in add - are added to the current list, and the tags in remove are dropped from it (the - resulting list is (current ∪ add) \ remove, sorted and deduplicated). A + property instead of replacing it. It takes two lists of machine tags: the tags in + add are added to the current list, and any tags matching the patterns in + remove are dropped from it (the resulting list is (current ∪ add) \ + remove, sorted and deduplicated). A key=value tag in - add replaces any assignment of the same key in the current list. This is useful - when several independent agents each manage a subset of the tags without racing on a read-modify-write - of the full list. Each tag must be a valid machine tag, as for SetTags(). + add replaces any assignment of the same key in the current list. This is useful when + several independent agents each manage a subset of the tags without racing on a read-modify-write of + the full list. Each tag to add must be a valid machine tag, as for + SetTags(). PrettyHostname, IconName, Chassis, Deployment, Location, and Tags are stored in diff --git a/src/hostname/hostnamed.c b/src/hostname/hostnamed.c index 5ad8c0f0862..5093484eed1 100644 --- a/src/hostname/hostnamed.c +++ b/src/hostname/hostnamed.c @@ -1677,7 +1677,7 @@ static int method_set_tags(sd_bus_message *m, void *userdata, sd_bus_error *erro if (!j) return log_oom(); - r = machine_tag_list_is_valid(tags); + r = machine_tag_list_is_valid(tags, /* to_add= */ true); if (r < 0) return r; if (r == 0) @@ -1728,7 +1728,7 @@ static int method_add_and_remove_tags(sd_bus_message *m, void *userdata, sd_bus_ if (r < 0) return r; - r = machine_tag_list_is_valid(add); + r = machine_tag_list_is_valid(add, /* to_add= */ true); if (r < 0) return r; if (r == 0) { @@ -1736,13 +1736,11 @@ static int method_add_and_remove_tags(sd_bus_message *m, void *userdata, sd_bus_ return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid tags to add '%s'", strna(j)); } - r = machine_tag_list_is_valid(remove); + r = machine_tag_list_is_valid(remove, /* to_add= */ false); if (r < 0) return r; - if (r == 0) { - _cleanup_free_ char *j = strv_join(remove, ":"); - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid tags to remove '%s'", strna(j)); - } + if (r == 0) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Too many tags patterns to remove."); context_read_machine_info(c); @@ -2534,20 +2532,20 @@ static int vl_method_set_tags(sd_varlink *link, sd_json_variant *parameters, sd_ bool reset = sd_json_variant_by_key(parameters, "set"); if (reset) { - r = machine_tag_list_is_valid(p.set); + r = machine_tag_list_is_valid(p.set, /* to_add= */ true); if (r < 0) return r; if (r == 0) return sd_varlink_error_invalid_parameter_name(link, "set"); } - r = machine_tag_list_is_valid(p.add); + r = machine_tag_list_is_valid(p.add, /* to_add= */ true); if (r < 0) return r; if (r == 0) return sd_varlink_error_invalid_parameter_name(link, "add"); - r = machine_tag_list_is_valid(p.remove); + r = machine_tag_list_is_valid(p.remove, /* to_add= */ false); if (r < 0) return r; if (r == 0) diff --git a/src/shared/machine-tags.c b/src/shared/machine-tags.c index 1e7d5446e91..b0883b76b54 100644 --- a/src/shared/machine-tags.c +++ b/src/shared/machine-tags.c @@ -59,18 +59,40 @@ static int machine_tag_key(const char *tag, char **ret) { return 0; } -int machine_tag_list_is_valid(char **l) { +int machine_tag_list_is_valid(char **l, bool to_add) { int r; - r = machine_tags_from_strv(l, /* graceful= */ false, /* ret= */ NULL); - switch (r) { - case -EINVAL: - case -E2BIG: - return false; - case 0: + if (to_add) { + r = machine_tags_from_strv(l, /* graceful= */ false, /* ret= */ NULL); + switch (r) { + case -EINVAL: + case -E2BIG: + return false; + case 0: + return true; + default: + return r; + } + } else { + size_t n = 0, n_globs = 0; + + STRV_FOREACH(i, l) { + if (++n > MACHINE_TAGS_MAX) + return false; + if (!in_charset(*i, ALPHANUMERICAL "-.=[]?*")) + return false; + + size_t len = strlen(*i); + if (len <= 0 || len >= 256) + return false; + + /* Globs are potentially quite expensive to evaluate. Use a stricter limit for them. */ + n_globs += string_is_glob(*i); + if (n_globs > MACHINE_TAG_GLOBS_MAX) + return false; + } + return true; - default: - return r; } } @@ -242,21 +264,24 @@ int machine_tags_edit(char* const *base, char* const *add, char* const *remove, _cleanup_hashmap_free_ Hashmap *h = NULL; int r; + /* The removal is done first. It can contain globs which are used to remove + * existing entries. The additions are done after the removals. */ + r = tags_make_hashmap(base, &h); if (r < 0) { assert(r == -ENOMEM); return log_oom(); } + STRV_FOREACH(i, remove) + tags_remove_matching(h, *i); + STRV_FOREACH(i, add) { r = tags_add(&h, *i); if (r < 0) return r; } - STRV_FOREACH(i, remove) - tags_remove_matching(h, *i); - return tags_hashmap_to_strv(h, ret); } diff --git a/src/shared/machine-tags.h b/src/shared/machine-tags.h index 6215bb59541..9ccdeac1c69 100644 --- a/src/shared/machine-tags.h +++ b/src/shared/machine-tags.h @@ -4,9 +4,10 @@ #include "forward.h" #define MACHINE_TAGS_MAX 1024U +#define MACHINE_TAG_GLOBS_MAX 64U bool machine_tag_is_valid(const char *s); -int machine_tag_list_is_valid(char **l); +int machine_tag_list_is_valid(char **l, bool to_add); int machine_tags_from_string(const char *s, bool graceful, char ***ret); int machine_tags_from_strv(char **l, bool graceful, char ***ret); diff --git a/src/shared/varlink-io.systemd.Hostname.c b/src/shared/varlink-io.systemd.Hostname.c index b3323215ac8..f0c72c230ee 100644 --- a/src/shared/varlink-io.systemd.Hostname.c +++ b/src/shared/varlink-io.systemd.Hostname.c @@ -121,11 +121,11 @@ static SD_VARLINK_DEFINE_METHOD( static SD_VARLINK_DEFINE_METHOD( SetTags, - SD_VARLINK_FIELD_COMMENT("If specified, the machine tag list is first reset to exactly these tags, before the 'add' and 'remove' fields are applied. If null, the current tag list is used as basis instead."), + SD_VARLINK_FIELD_COMMENT("If specified, the machine tag list is first reset to exactly these tags, before the 'remove' and 'add' fields are applied. If null, the current tag list is used as basis instead."), SD_VARLINK_DEFINE_INPUT(set, SD_VARLINK_STRING, SD_VARLINK_NULLABLE|SD_VARLINK_ARRAY), SD_VARLINK_FIELD_COMMENT("Machine tags to add to the list. A key=value tag replaces any assignment of the same key already in the list."), SD_VARLINK_DEFINE_INPUT(add, SD_VARLINK_STRING, SD_VARLINK_NULLABLE|SD_VARLINK_ARRAY), - SD_VARLINK_FIELD_COMMENT("Machine tags to remove from the list."), + SD_VARLINK_FIELD_COMMENT("Machine tags to remove from the list. The removals are processed before the additions. Removals may include globs."), SD_VARLINK_DEFINE_INPUT(remove, SD_VARLINK_STRING, SD_VARLINK_NULLABLE|SD_VARLINK_ARRAY), VARLINK_DEFINE_POLKIT_INPUT); diff --git a/src/test/test-machine-tags.c b/src/test/test-machine-tags.c index 628c5cd7f3c..9573cb2e10d 100644 --- a/src/test/test-machine-tags.c +++ b/src/test/test-machine-tags.c @@ -58,21 +58,41 @@ TEST(machine_tag_is_valid) { } TEST(machine_tag_list_is_valid) { - ASSERT_OK_POSITIVE(machine_tag_list_is_valid(NULL)); /* empty list is valid */ - ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("a"))); - ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo", "bar", "c-d.e"))); - ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=uuu", "bar=qqqq", "c-d.e"))); - ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo", "foo=aa"))); /* bare key and assignment coexist */ - ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=1", "foobar=2"))); /* one key is a prefix of the other */ - ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("ab=1", "a=2"))); /* ... and the other way around */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(NULL, /* to_add= */ true)); /* empty list is valid */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("a"), true)); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo", "bar", "c-d.e"), true)); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=uuu", "bar=qqqq", "c-d.e"), true)); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo", "foo=aa"), true)); /* bare key and assignment coexist */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=1", "foobar=2"), true)); /* one key is a prefix of the other */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("ab=1", "a=2"), true)); /* ... and the other way around */ - ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "foo=aa"))); /* same key + same value is OK */ - ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo", "b:c"))); - ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo", ""))); - ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "foo=b"))); /* same key, different value */ - ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("a=1", "b=2", "a=3"))); /* ... also when not adjacent */ - ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "bar", "foo=aa", "foo=b"))); - ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("=aa"))); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "foo=aa"), true)); /* same key + same value is OK */ + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo", "b:c"), true)); + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo", ""), true)); + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "foo=b"), true)); /* same key, different value */ + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("a=1", "b=2", "a=3"), true)); /* ... also when not adjacent */ + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "bar", "foo=aa", "foo=b"), true)); + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("=aa"), true)); + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("ab=1*"), true)); /* globs */ + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("ab=2[345]"), true)); + + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(NULL, /* to_add= */ false)); /* empty list is valid */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("a"), false)); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo", "bar", "c-d.e"), false)); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=uuu", "bar=qqqq", "c-d.e"), false)); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo", "foo=aa"), false)); /* bare key and assignment coexist */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=1", "foobar=2"), false)); /* one key is a prefix of the other */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("ab=1", "a=2"), false)); /* ... and the other way around */ + + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "foo=aa"), false)); /* same key + same value is OK */ + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo", "b:c"), false)); + ASSERT_OK_ZERO(machine_tag_list_is_valid(STRV_MAKE("foo", ""), false)); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "foo=b"), false)); /* same key, different value */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("a=1", "b=2", "a=3"), false)); /* ... also when not adjacent */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("foo=aa", "bar", "foo=aa", "foo=b"), false)); + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("=aa"), false)); /* this is not a valid pattern, but we don't check */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("ab=1*"), false)); /* globs */ + ASSERT_OK_POSITIVE(machine_tag_list_is_valid(STRV_MAKE("ab=2[345]"), false)); } TEST(machine_tags_from_string) { diff --git a/test/units/TEST-71-HOSTNAME.sh b/test/units/TEST-71-HOSTNAME.sh index 3c3bcd98401..414f3ff6011 100755 --- a/test/units/TEST-71-HOSTNAME.sh +++ b/test/units/TEST-71-HOSTNAME.sh @@ -492,6 +492,11 @@ testcase_tags() { AddAndRemoveTags asas 1 three 1 one assert_eq "$(hostnamectl tags)" "three:two" + # Check removal by glob + busctl call org.freedesktop.hostname1 /org/freedesktop/hostname1 org.freedesktop.hostname1 \ + AddAndRemoveTags asas 2 three one 1 'tw*' + assert_eq "$(hostnamectl tags)" "one:three" + # The SetTags() Varlink method: 'set' resets the list, then 'add'/'remove' are applied on top. varlinkctl call /run/systemd/io.systemd.Hostname io.systemd.Hostname.SetTags \ '{"set":["x","y"],"add":["z"],"remove":["x"]}' @@ -511,6 +516,17 @@ testcase_tags() { varlinkctl call /run/systemd/io.systemd.Hostname io.systemd.Hostname.SetTags '{"set":["bbb"]}' # Invalid tags are refused. (! varlinkctl call /run/systemd/io.systemd.Hostname io.systemd.Hostname.SetTags '{"add":["in valid"]}') + assert_eq "$(hostnamectl tags)" "bbb" + + # Remove by glob + varlinkctl call /run/systemd/io.systemd.Hostname io.systemd.Hostname.SetTags '{"add":["bbb=33"],"remove":["bb*"]}' + assert_eq "$(hostnamectl tags)" "bbb=33" + varlinkctl call /run/systemd/io.systemd.Hostname io.systemd.Hostname.SetTags '{"add":["aaa=33"],"remove":["bbb","bbb=34"]}' + assert_eq "$(hostnamectl tags)" "aaa=33:bbb=33" + + # Removals precede additions + varlinkctl call /run/systemd/io.systemd.Hostname io.systemd.Hostname.SetTags '{"add":["aaa=34","bbb=34"],"remove":["aaa=*","bbb=*"]}' + assert_eq "$(hostnamectl tags)" "aaa=34:bbb=34" # Describe() exposes the parsed tag list as MachineTags, on both transports. varlinkctl call /run/systemd/io.systemd.Hostname io.systemd.Hostname.SetTags '{"set":["foo","bar"]}'