Check if the interface name is too long (LP: #1988749) (#313)

* tests: fix a keyfile test logic for bridges For bridges, the emitter will use the interface name as ID and not the NetworkManager UUID. * parser: warning if the interface name is too long Interfaces names are limited to 15 characters (see IF_NAMESIZE in net/if.h). Currently Netplan will allow the user to use invalid names without a warning. In this case the operation will be ignored by the backends. With this change, Netplan will show a warning but still complete successfully. As there might be users with invalid interface names in their configuration without knowing it, changing the behavior to fail would probably cause problems. * validation: check if netdef is NULL Also, fix apply.py code style to make the linter happy. * ctests: add tests for the new validation function Also add a utility to load the yaml content from strings. * validation: add a comment about IF_NAMSIZE warning * nm: replace undocumented constant with IF_NAMESIZE * ctest:utils: adopt to netplan_error_clear rename Co-authored-by: Lukas Märdian <[email protected]>
canonical · Jan 19, 2023 · d53d96c · d53d96c
1 parent 22de574
commit d53d96c
Show file tree

Hide file tree

Showing 7 changed files with 221 additions and 6 deletions.
diff --git a/netplan/cli/commands/apply.py b/netplan/cli/commands/apply.py
@@ -36,6 +36,8 @@
 
 OVS_CLEANUP_SERVICE = 'netplan-ovs-cleanup.service'
 
+IF_NAMESIZE = 16
+
 
 class NetplanApply(utils.NetplanCommand):
 
@@ -234,6 +236,9 @@ def command_apply(self, run_generate=True, sync=False, exit_on_error=True, state
             # rename non-critical network interfaces
             new_name = settings.get('name')
             if new_name:
+                if len(new_name) >= IF_NAMESIZE:
+                    logging.warning('Interface name {} is too long. {} will not be renamed'.format(new_name, iface))
+                    continue
                 if iface in devices and new_name in devices_after_udev:
                     logging.debug('Interface rename {} -> {} already happened.'.format(iface, new_name))
                     continue  # re-name already happened via 'udevadm test'

diff --git a/src/nm.c b/src/nm.c
@@ -21,6 +21,7 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <arpa/inet.h>
+#include <net/if.h>
 
 #include <glib.h>
 #include <glib/gprintf.h>
@@ -657,8 +658,8 @@ write_nm_conf_access_point(const NetplanNetDefinition* def, const char* rootdir,
         /* else matches on something other than the name, do not restrict interface-name */
     } else {
         /* virtual (created) devices set a name */
-        if (strlen(def->id) > 15)
-            g_debug("interface-name longer than 15 characters is not supported");
+        if (strnlen(def->id, IF_NAMESIZE) == IF_NAMESIZE)
+            g_debug("interface-name %s is too long. Ignoring.", def->id);
         else
             g_key_file_set_string(kf, "connection", "interface-name", def->id);
 

diff --git a/src/validation.c b/src/validation.c
@@ -19,6 +19,7 @@
 #include <glib/gstdio.h>
 #include <gio/gio.h>
 #include <arpa/inet.h>
+#include <net/if.h>
 #include <regex.h>
 
 #include <yaml.h>
@@ -151,6 +152,32 @@ validate_ovs_target(gboolean host_first, gchar* s) {
     return FALSE;
 }
 
+static gboolean
+validate_interface_name_length(const NetplanNetDefinition* netdef)
+{
+    gboolean validation = TRUE;
+    char* iface = NULL;
+
+    if (netdef->type >= NETPLAN_DEF_TYPE_VIRTUAL && netdef->type < NETPLAN_DEF_TYPE_NM) {
+        if (strnlen(netdef->id, IF_NAMESIZE) == IF_NAMESIZE) {
+            iface = netdef->id;
+            validation = FALSE;
+        }
+    } else if (netdef->set_name) {
+        if (strnlen(netdef->set_name, IF_NAMESIZE) == IF_NAMESIZE) {
+            iface = netdef->set_name;
+            validation = FALSE;
+        }
+    }
+
+    /* TODO: make this a hard failure in the future, but keep it as a warning
+     *       for now, to not break netplan generate at boot. */
+    if (iface)
+        g_warning("Interface name '%s' is too long. It will be ignored by the backend.", iface);
+
+    return validation;
+}
+
 /************************************************
  * Validation for grammar and backend rules.
  ************************************************/
@@ -378,6 +405,9 @@ validate_netdef_grammar(const NetplanParser* npp, NetplanNetDefinition* nd, yaml
     if (nd->type == NETPLAN_DEF_TYPE_NM && (!nd->backend_settings.nm.passthrough || !g_datalist_get_data(&nd->backend_settings.nm.passthrough, "connection.type")))
         return yaml_error(npp, node, error, "%s: network type 'nm-devices:' needs to provide a 'connection.type' via passthrough", nd->id);
 
+    if (npp->current.netdef)
+        validate_interface_name_length(npp->current.netdef);
+
     valid = TRUE;
 
 netdef_grammar_error:

diff --git a/tests/ctests/meson.build b/tests/ctests/meson.build
@@ -4,6 +4,7 @@ tests = {
   'test_netplan_error': false,
   'test_netplan_misc': false,
   'test_netplan_deprecated': false,
+  'test_netplan_validation': false,
 }
 
 cmocka = dependency('cmocka', required: true)

diff --git a/tests/ctests/test_netplan_validation.c b/tests/ctests/test_netplan_validation.c
@@ -0,0 +1,138 @@
+#include <stdio.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+
+#include <cmocka.h>
+
+#include "netplan.h"
+#include "parse.h"
+
+#undef __USE_MISC
+#include "error.c"
+#include "names.c"
+#include "validation.c"
+#include "types.c"
+#include "util.c"
+#include "parse.c"
+
+#include "test_utils.h"
+
+void
+test_validate_interface_name_length(void** state)
+{
+    const char* yaml =
+        "network:\n"
+        "  version: 2\n"
+        "  bridges:\n"
+        "    ashortname:\n"
+        "      dhcp4: no\n";
+
+    NetplanState* np_state = load_string_to_netplan_state(yaml);
+    NetplanStateIterator iter;
+    NetplanNetDefinition* netdef = NULL;
+    netplan_state_iterator_init(np_state, &iter);
+
+    netdef = netplan_state_iterator_next(&iter);
+
+    assert_true(validate_interface_name_length(netdef));
+
+    netplan_state_clear(&np_state);
+}
+
+void
+test_validate_interface_name_length_set_name(void** state)
+{
+    const char* yaml =
+        "network:\n"
+        "  version: 2\n"
+        "  ethernets:\n"
+        "    eth0:\n"
+        "      match:\n"
+        "        macaddress: aa:bb:cc:dd:ee:ff\n"
+        "      set-name: ashortname\n";
+
+    NetplanState* np_state = load_string_to_netplan_state(yaml);
+    NetplanStateIterator iter;
+    NetplanNetDefinition* netdef = NULL;
+    netplan_state_iterator_init(np_state, &iter);
+
+    netdef = netplan_state_iterator_next(&iter);
+
+    assert_true(validate_interface_name_length(netdef));
+
+    netplan_state_clear(&np_state);
+}
+
+void
+test_validate_interface_name_length_too_long(void** state)
+{
+    const char* yaml =
+        "network:\n"
+        "  version: 2\n"
+        "  bridges:\n"
+        "    averylongnameforaninterface:\n"
+        "      dhcp4: no\n";
+
+    NetplanState* np_state = load_string_to_netplan_state(yaml);
+    NetplanStateIterator iter;
+    NetplanNetDefinition* netdef = NULL;
+    netplan_state_iterator_init(np_state, &iter);
+
+    netdef = netplan_state_iterator_next(&iter);
+
+    assert_false(validate_interface_name_length(netdef));
+
+    netplan_state_clear(&np_state);
+}
+
+void
+test_validate_interface_name_length_set_name_too_long(void** state)
+{
+    const char* yaml =
+        "network:\n"
+        "  version: 2\n"
+        "  ethernets:\n"
+        "    eth0:\n"
+        "      match:\n"
+        "        macaddress: aa:bb:cc:dd:ee:ff\n"
+        "      set-name: averylongnameforaninterface\n";
+
+    NetplanState* np_state = load_string_to_netplan_state(yaml);
+    NetplanStateIterator iter;
+    NetplanNetDefinition* netdef = NULL;
+    netplan_state_iterator_init(np_state, &iter);
+
+    netdef = netplan_state_iterator_next(&iter);
+
+    assert_false(validate_interface_name_length(netdef));
+
+    netplan_state_clear(&np_state);
+}
+
+int
+setup(void** state)
+{
+    return 0;
+}
+
+int
+tear_down(void** state)
+{
+    return 0;
+}
+
+int
+main()
+{
+
+    const struct CMUnitTest tests[] = {
+        cmocka_unit_test(test_validate_interface_name_length),
+        cmocka_unit_test(test_validate_interface_name_length_too_long),
+        cmocka_unit_test(test_validate_interface_name_length_set_name),
+        cmocka_unit_test(test_validate_interface_name_length_set_name_too_long),
+    };
+
+    return cmocka_run_group_tests(tests, setup, tear_down);
+
+}
diff --git a/tests/ctests/test_utils.h b/tests/ctests/test_utils.h
@@ -5,7 +5,10 @@
 #include "types.h"
 #include "netplan.h"
 #include "parse.h"
+#include "util.h"
+#include "types-internal.h"
 
+// LCOV_EXCL_START
 NetplanState *
 load_fixture_to_netplan_state(const char* filename)
 {
@@ -25,3 +28,41 @@ load_fixture_to_netplan_state(const char* filename)
 
     return np_state;
 }
+
+NetplanState*
+load_string_to_netplan_state(const char* yaml)
+{
+    yaml_parser_t parser;
+    yaml_document_t* doc;
+    NetplanError** error = NULL;
+    NetplanState* np_state = NULL;
+
+    NetplanParser* npp = netplan_parser_new();
+
+    doc = &npp->doc;
+
+    yaml_parser_initialize(&parser);
+    yaml_parser_set_input_string(&parser, (const unsigned char*) yaml, strlen(yaml));
+    yaml_parser_load(&parser, doc);
+
+    process_document(npp, error);
+
+    if (error && *error) {
+        netplan_error_clear(error);
+    } else {
+        np_state = netplan_state_new();
+        netplan_state_import_parser_results(np_state, npp, error);
+    }
+
+    yaml_parser_delete(&parser);
+    yaml_document_delete(doc);
+    netplan_parser_clear(&npp);
+
+    if (error && *error) {
+        netplan_state_clear(&np_state);
+    }
+
+    return np_state;
+}
+
+// LCOV_EXCL_STOP
diff --git a/tests/parser/test_keyfile.py b/tests/parser/test_keyfile.py
@@ -1371,11 +1371,11 @@ def test_keyfile_nm_140_default_ethernet_group(self):
 addr-gen-mode=default
 method=auto
 
-[proxy]\n'''.format(UUID))
+[proxy]\n'''.format(UUID), netdef_id='br0')
         self.assert_netplan({UUID: '''network:
   version: 2
   bridges:
-    NM-{}:
+    br0:
       renderer: NetworkManager
       addresses:
       - "1.2.3.4/24"
@@ -1384,12 +1384,11 @@ def test_keyfile_nm_140_default_ethernet_group(self):
         uuid: "{}"
         name: "Test Write Bridge Main"
         passthrough:
-          connection.interface-name: "br0"
           ethernet._: ""
           bridge._: ""
           ipv4.address1: "1.2.3.4/24,1.1.1.1"
           ipv4.method: "manual"
           ipv6.addr-gen-mode: "default"
           ipv6.ip6-privacy: "-1"
           proxy._: ""
-'''.format(UUID, UUID)})
+'''.format(UUID)})