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

@@ -390,13 +390,15 @@ node /org/freedesktop/hostname1 {
       the <varname>TAGS=</varname> field from <filename>/etc/machine-info</filename>.</para>
 
       <para><function>AddAndRemoveTags()</function> incrementally updates the <varname>Tags</varname>
-      property instead of replacing it. It takes two lists of machine tags: the tags in <varname>add</varname>
-      are added to the current list, and the tags in <varname>remove</varname> are dropped from it (the
-      resulting list is <varname>(current ∪ add) \ remove</varname>, sorted and deduplicated). A
+      property instead of replacing it. It takes two lists of machine tags: the tags in
+      <varname>add</varname> are added to the current list, and any tags matching the patterns in
+      <varname>remove</varname> are dropped from it (the resulting list is <varname>(current ∪ add) \
+      remove</varname>, sorted and deduplicated). A
       <literal><replaceable>key</replaceable>=<replaceable>value</replaceable></literal> tag in
-      <varname>add</varname> 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 <function>SetTags()</function>.</para>
+      <varname>add</varname> 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
+      <function>SetTags()</function>.</para>
 
       <para><varname>PrettyHostname</varname>, <varname>IconName</varname>, <varname>Chassis</varname>,
       <varname>Deployment</varname>, <varname>Location</varname>, and <varname>Tags</varname> are stored in

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)

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);
 }
 

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);
 

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);
 

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) {

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"]}'