Merge pull request #1469 from c-po/macsec-equuleus

MACsec: T4537: fix AES-GCM-256 support

5 files changed, 112 insertions, 62 deletions

diff --git a/data/templates/macsec/wpa_supplicant.conf.tmpl b/data/templates/macsec/wpa_supplicant.conf.tmpl
index 5b353def8..65747ea6f 100644
--- a/data/templates/macsec/wpa_supplicant.conf.tmpl
+++ b/data/templates/macsec/wpa_supplicant.conf.tmpl
@@ -47,6 +47,12 @@ network={
     # 1: Integrity only
     macsec_integ_only={{ '0' if security is defined and security.encrypt is defined else '1' }}
 
+    # macsec_csindex: IEEE 802.1X/MACsec cipher suite
+    # 0 = GCM-AES-128
+    # 1 = GCM-AES-256
+{# security.cipher is a mandatory key #}
+    macsec_csindex={{ '1' if security.cipher is defined and security.cipher == 'gcm-aes-256' else '0' }}
+
 {% if security is defined %}
 {%   if security.encrypt is defined %}
     # mka_cak, mka_ckn, and mka_priority: IEEE 802.1X/MACsec pre-shared key mode
@@ -63,7 +69,7 @@ network={
     # mka_priority (Priority of MKA Actor) is in 0..255 range with 255 being
     # default priority
     mka_priority={{ security.mka.priority }}
-{%   endif %}
+{% endif %}
 
 {%   if security.replay_window is defined %}
     # macsec_replay_protect: IEEE 802.1X/MACsec replay protection
@@ -85,5 +91,9 @@ network={
     macsec_replay_window={{ security.replay_window }}
 {%   endif %}
 {% endif %}
+
+    # macsec_port: IEEE 802.1X/MACsec port - Port component of the SCI
+    # Range: 1-65534 (default: 1)
+    macsec_port=1
 }
 
diff --git a/interface-definitions/interfaces-macsec.xml.in b/interface-definitions/interfaces-macsec.xml.in
index 96dcf3ca0..fa54aedc1 100644
--- a/interface-definitions/interfaces-macsec.xml.in
+++ b/interface-definitions/interfaces-macsec.xml.in
@@ -60,11 +60,12 @@
                     <properties>
                       <help>Secure Connectivity Association Key</help>
                       <valueHelp>
-                        <format>key</format>
-                        <description>16-byte (128-bit) hex-string (32 hex-digits)</description>
+                        <format>txt</format>
+                        <description>16-byte (128-bit) hex-string (32 hex-digits) for gcm-aes-128 or 32-byte (256-bit) hex-string (64 hex-digits) for gcm-aes-256</description>
                       </valueHelp>
                       <constraint>
-                        <regex>^[A-Fa-f0-9]{32}$</regex>
+                        <regex>[A-Fa-f0-9]{32}</regex>
+                        <regex>[A-Fa-f0-9]{64}</regex>
                       </constraint>
                     </properties>
                   </leafNode>
@@ -72,7 +73,7 @@
                     <properties>
                       <help>Secure Connectivity Association Key Name</help>
                       <valueHelp>
-                        <format>key</format>
+                        <format>txt</format>
                         <description>32-byte (256-bit) hex-string (64 hex-digits)</description>
                       </valueHelp>
                       <constraint>
diff --git a/smoketest/scripts/cli/test_interfaces_macsec.py b/smoketest/scripts/cli/test_interfaces_macsec.py
index 5b10bfa44..64bfa0dc9 100755
--- a/smoketest/scripts/cli/test_interfaces_macsec.py
+++ b/smoketest/scripts/cli/test_interfaces_macsec.py
@@ -28,6 +28,8 @@ from vyos.util import read_file
 from vyos.util import get_interface_config
 from vyos.util import process_named_running
 
+PROCESS_NAME = 'wpa_supplicant'
+
 def get_config_value(interface, key):
     tmp = read_file(f'/run/wpa_supplicant/{interface}.conf')
     tmp = re.findall(r'\n?{}=(.*)'.format(key), tmp)
@@ -55,6 +57,10 @@ class MACsecInterfaceTest(BasicInterfaceTest.TestCase):
         # call base-classes classmethod
         super(cls, cls).setUpClass()
 
+    def tearDown(self):
+        super().tearDown()
+        self.assertFalse(process_named_running(PROCESS_NAME))
+
     def test_macsec_encryption(self):
         # MACsec can be operating in authentication and encryption mode - both
         # using different mandatory settings, lets test encryption as the basic
@@ -96,28 +102,29 @@ class MACsecInterfaceTest(BasicInterfaceTest.TestCase):
             self.cli_commit()
 
             tmp = get_config_value(src_interface, 'macsec_integ_only')
-            self.assertTrue("0" in tmp)
+            self.assertIn("0", tmp)
 
             tmp = get_config_value(src_interface, 'mka_cak')
-            self.assertTrue(mak_cak in tmp)
+            self.assertIn(mak_cak, tmp)
 
             tmp = get_config_value(src_interface, 'mka_ckn')
-            self.assertTrue(mak_ckn in tmp)
+            self.assertIn(mak_ckn, tmp)
 
             # check that the default priority of 255 is programmed
             tmp = get_config_value(src_interface, 'mka_priority')
-            self.assertTrue("255" in tmp)
+            self.assertIn("255", tmp)
 
             tmp = get_config_value(src_interface, 'macsec_replay_window')
-            self.assertTrue(replay_window in tmp)
+            self.assertIn(replay_window, tmp)
 
             tmp = read_file(f'/sys/class/net/{interface}/mtu')
             self.assertEqual(tmp, '1460')
 
-            # Check for running process
-            self.assertTrue(process_named_running('wpa_supplicant'))
+        # Check for running process
+        self.assertTrue(process_named_running(PROCESS_NAME))
 
     def test_macsec_gcm_aes_128(self):
+        src_interface = 'eth0'
         interface = 'macsec1'
         cipher = 'gcm-aes-128'
         self.cli_set(self._base_path + [interface])
@@ -125,7 +132,7 @@ class MACsecInterfaceTest(BasicInterfaceTest.TestCase):
         # check validate() - source interface is mandatory
         with self.assertRaises(ConfigSessionError):
             self.cli_commit()
-        self.cli_set(self._base_path + [interface, 'source-interface', 'eth0'])
+        self.cli_set(self._base_path + [interface, 'source-interface', src_interface])
 
         # check validate() - cipher is mandatory
         with self.assertRaises(ConfigSessionError):
@@ -138,7 +145,15 @@ class MACsecInterfaceTest(BasicInterfaceTest.TestCase):
         self.assertIn(interface, interfaces())
         self.assertEqual(cipher, get_cipher(interface))
 
+        # check that we use the new macsec_csindex option (T4537)
+        tmp = get_config_value(src_interface, 'macsec_csindex')
+        self.assertIn("0", tmp)
+
+        # Check for running process
+        self.assertTrue(process_named_running(PROCESS_NAME))
+
     def test_macsec_gcm_aes_256(self):
+        src_interface = 'eth0'
         interface = 'macsec4'
         cipher = 'gcm-aes-256'
         self.cli_set(self._base_path + [interface])
@@ -146,7 +161,7 @@ class MACsecInterfaceTest(BasicInterfaceTest.TestCase):
         # check validate() - source interface is mandatory
         with self.assertRaises(ConfigSessionError):
             self.cli_commit()
-        self.cli_set(self._base_path + [interface, 'source-interface', 'eth0'])
+        self.cli_set(self._base_path + [interface, 'source-interface', src_interface])
 
         # check validate() - cipher is mandatory
         with self.assertRaises(ConfigSessionError):
@@ -158,6 +173,13 @@ class MACsecInterfaceTest(BasicInterfaceTest.TestCase):
         self.assertIn(interface, interfaces())
         self.assertEqual(cipher, get_cipher(interface))
 
+        # check that we use the new macsec_csindex option (T4537)
+        tmp = get_config_value(src_interface, 'macsec_csindex')
+        self.assertIn("1", tmp)
+
+        # Check for running process
+        self.assertTrue(process_named_running(PROCESS_NAME))
+
     def test_macsec_source_interface(self):
         # Ensure source-interface can bot be part of any other bond or bridge
 
@@ -186,6 +208,9 @@ class MACsecInterfaceTest(BasicInterfaceTest.TestCase):
             self.cli_commit()
             self.assertIn(interface, interfaces())
 
+        # Check for running process
+        self.assertTrue(process_named_running(PROCESS_NAME))
+
 if __name__ == '__main__':
-    unittest.main(verbosity=2)
+    unittest.main(verbosity=2, failfast=True)
 
diff --git a/src/conf_mode/interfaces-macsec.py b/src/conf_mode/interfaces-macsec.py
index e3527a366..8076a27b6 100755
--- a/src/conf_mode/interfaces-macsec.py
+++ b/src/conf_mode/interfaces-macsec.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2020 VyOS maintainers and contributors
+# Copyright (C) 2020-2022 VyOS maintainers and contributors
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 or later as
@@ -21,17 +21,20 @@ from sys import exit
 
 from vyos.config import Config
 from vyos.configdict import get_interface_dict
-from vyos.ifconfig import MACsecIf
-from vyos.ifconfig import Interface
-from vyos.template import render
-from vyos.util import call
-from vyos.util import dict_search
+from vyos.configdict import is_node_changed
+from vyos.configdict import is_source_interface
 from vyos.configverify import verify_vrf
 from vyos.configverify import verify_address
 from vyos.configverify import verify_bridge_delete
 from vyos.configverify import verify_mtu_ipv6
 from vyos.configverify import verify_source_interface
 from vyos.configverify import verify_bond_bridge_member
+from vyos.ifconfig import MACsecIf
+from vyos.ifconfig import Interface
+from vyos.template import render
+from vyos.util import call
+from vyos.util import dict_search
+from vyos.util import is_systemd_service_running
 from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
@@ -56,6 +59,17 @@ def get_config(config=None):
         source_interface = conf.return_effective_value(['source-interface'])
         macsec.update({'source_interface': source_interface})
 
+    ifname = macsec['ifname']
+    if is_node_changed(conf, base + [ifname, 'security']):
+        macsec.update({'shutdown_required': {}})
+
+    if is_node_changed(conf, base + [ifname, 'source_interface']):
+        macsec.update({'shutdown_required': {}})
+
+    if 'source_interface' in macsec:
+        tmp = is_source_interface(conf, macsec['source_interface'], 'macsec')
+        if tmp and tmp != ifname: macsec.update({'is_source_interface' : tmp})
+
     return macsec
 
 
@@ -70,20 +84,12 @@ def verify(macsec):
     verify_address(macsec)
     verify_bond_bridge_member(macsec)
 
-    if not (('security' in macsec) and
-            ('cipher' in macsec['security'])):
-        raise ConfigError(
-            'Cipher suite must be set for MACsec "{ifname}"'.format(**macsec))
-
-    if (('security' in macsec) and
-        ('encrypt' in macsec['security'])):
-        tmp = macsec.get('security')
+    if dict_search('security.cipher', macsec) == None:
+        raise ConfigError('Cipher suite must be set for MACsec "{ifname}"'.format(**macsec))
 
-        if not (('mka' in tmp) and
-                ('cak' in tmp['mka']) and
-                ('ckn' in tmp['mka'])):
-            raise ConfigError('Missing mandatory MACsec security '
-                              'keys as encryption is enabled!')
+    if dict_search('security.encrypt', macsec) != None:
+        if dict_search('security.mka.cak', macsec) == None or dict_search('security.mka.ckn', macsec) == None:
+            raise ConfigError('Missing mandatory MACsec security keys as encryption is enabled!')
 
         cak_len = len(dict_search('security.mka.cak', macsec))
 
@@ -95,6 +101,12 @@ def verify(macsec):
             # gcm-aes-128 requires a 128bit long key - 64 characters (string) = 32byte = 256bit
             raise ConfigError('gcm-aes-128 requires a 256bit long key!')
 
+    if 'is_source_interface' in macsec:
+        tmp = macsec['is_source_interface']
+        src_ifname = macsec['source_interface']
+        raise ConfigError(f'Can not use source-interface "{src_ifname}", it already ' \
+                          f'belongs to interface "{tmp}"!')
+
     if 'source_interface' in macsec:
         # MACsec adds a 40 byte overhead (32 byte MACsec + 8 bytes VLAN 802.1ad
         # and 802.1q) - we need to check the underlaying MTU if our configured
@@ -114,35 +126,37 @@ def generate(macsec):
 
 
 def apply(macsec):
-    # Remove macsec interface
-    if 'deleted' in macsec:
-        call('systemctl stop wpa_supplicant-macsec@{source_interface}'
-             .format(**macsec))
+    systemd_service = 'wpa_supplicant-macsec@{source_interface}'.format(**macsec)
+
+    # Remove macsec interface on deletion or mandatory parameter change
+    if 'deleted' in macsec or 'shutdown_required' in macsec:
+        call(f'systemctl stop {systemd_service}')
 
         if macsec['ifname'] in interfaces():
             tmp = MACsecIf(macsec['ifname'])
             tmp.remove()
 
-        # delete configuration on interface removal
-        if os.path.isfile(wpa_suppl_conf.format(**macsec)):
-            os.unlink(wpa_suppl_conf.format(**macsec))
-
-    else:
-        # This is a special type of interface which needs additional parameters
-        # when created using iproute2. Instead of passing a ton of arguments,
-        # use a dictionary provided by the interface class which holds all the
-        # options necessary.
-        conf = MACsecIf.get_config()
-        conf['source_interface'] = macsec['source_interface']
-        conf['security_cipher'] = macsec['security']['cipher']
-
-        # It is safe to "re-create" the interface always, there is a sanity
-        # check that the interface will only be create if its non existent
-        i = MACsecIf(macsec['ifname'], **conf)
-        i.update(macsec)
-
-        call('systemctl restart wpa_supplicant-macsec@{source_interface}'
-             .format(**macsec))
+        if 'deleted' in macsec:
+            # delete configuration on interface removal
+            if os.path.isfile(wpa_suppl_conf.format(**macsec)):
+                os.unlink(wpa_suppl_conf.format(**macsec))
+            return None
+
+    # This is a special type of interface which needs additional parameters
+    # when created using iproute2. Instead of passing a ton of arguments,
+    # use a dictionary provided by the interface class which holds all the
+    # options necessary.
+    conf = MACsecIf.get_config()
+    conf['source_interface'] = macsec['source_interface']
+    conf['security_cipher'] = macsec['security']['cipher']
+
+    # It is safe to "re-create" the interface always, there is a sanity
+    # check that the interface will only be create if its non existent
+    i = MACsecIf(macsec['ifname'], **conf)
+    i.update(macsec)
+
+    if not is_systemd_service_running(systemd_service) or 'shutdown_required' in macsec:
+        call(f'systemctl reload-or-restart {systemd_service}')
 
     return None
 
diff --git a/src/systemd/wpa_supplicant-macsec@.service b/src/systemd/wpa_supplicant-macsec@.service
index 7e0bee8e1..d5739583e 100644
--- a/src/systemd/wpa_supplicant-macsec@.service
+++ b/src/systemd/wpa_supplicant-macsec@.service
@@ -1,17 +1,17 @@
 [Unit]
-Description=WPA supplicant daemon (macsec-specific version)
+Description=WPA supplicant daemon (MACsec-specific version)
 Requires=sys-subsystem-net-devices-%i.device
 ConditionPathExists=/run/wpa_supplicant/%I.conf
 After=vyos-router.service
 RequiresMountsFor=/run
 
-# NetworkManager users will probably want the dbus version instead.
-
 [Service]
 Type=simple
 WorkingDirectory=/run/wpa_supplicant
 PIDFile=/run/wpa_supplicant/%I.pid
-ExecStart=/sbin/wpa_supplicant -c/run/wpa_supplicant/%I.conf -Dmacsec_linux -i%I
+ExecStart=/sbin/wpa_supplicant -c/run/wpa_supplicant/%I.conf -Dmacsec_linux -P/run/wpa_supplicant/%I.pid -i%I
+Restart=always
+RestartSec=2
 
 [Install]
 WantedBy=multi-user.target