Merge ~ltrager/maas:lp1916860 into maas:master

[Lee Trager]

LP: #1916860 - Check for gaps in supported ciphers when using ipmitool

When ipmitool was being used to configure IPMI ciphers MAAS wasn't
accounting for BMCs which omit support for certain ciphers. In this
case the setting string MAAS uses to enable ciphers was offset which
resulted in the wrong ciphers being enabled and disabled.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1916860 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9402/console
COMMIT: 403734fb90155f1fac808b4aa98f18819045143d

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1916860 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: c81a66a887094ee336bd26c5e06f3a07efca99dd

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1916860 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 60d33a59c1ee5aac6f2e4cc2b64963341495605f

review: Approve

[Björn Tillenius (bjornt)]

+1

review: Approve

diff --git a/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py b/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
index 69cc4c6..2c2777c 100755
--- a/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
+++ b/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
@@ -622,32 +622,32 @@ class IPMI(BMCConfig):
         supported_cipher_suite_ids,
         cipher_suite_privs,
     ):
-        new_cipher_suite_privs = ""
-        for i, v in enumerate(cipher_suite_privs):
-            if i < len(supported_cipher_suite_ids):
-                cipher_suite_id = supported_cipher_suite_ids[i]
-                if cipher_suite_id in [17, 3, 8, 12]:
-                    if cipher_suite_id == max_cipher_suite_id and v != "a":
-                        print(
-                            'INFO: Enabling IPMI cipher suite id "%s" '
-                            "for MAAS use..." % cipher_suite_id
-                        )
-                        new_cipher_suite_privs += "a"
-                    else:
-                        new_cipher_suite_privs += v
-                else:
-                    if v != "X":
-                        print(
-                            "INFO: Disabling insecure IPMI cipher suite id "
-                            '"%s"' % cipher_suite_id
-                        )
-                    new_cipher_suite_privs = "X"
-            else:
-                # 15 characters are usually given even if there
-                # aren't 15 ciphers supported. Copy the current value
-                # incase there is some OEM use for them.
-                new_cipher_suite_privs += v
+        cipher_suite_privs_list = list(cipher_suite_privs)
+        for i, cipher_suite_id in enumerate(supported_cipher_suite_ids):
+            if cipher_suite_id > 17:
+                # The IPMI spec reserves ciphers > 17 for vendor use.
+                # Don't touch them
+                continue
+            elif cipher_suite_id == max_cipher_suite_id:
+                if cipher_suite_privs_list[i] != "a":
+                    # Make sure the maximum supported cipher suite is enabled,
+                    # this is the cipher suite MAAS will use.
+                    cipher_suite_privs_list[i] = "a"
+                    print(
+                        'INFO: Enabling IPMI cipher suite id "%s" '
+                        "for MAAS use..." % cipher_suite_id
+                    )
+            elif cipher_suite_id not in [17, 3, 8, 12]:
+                # Disable any cipher suite which isn't a known secure
+                # cipher suite.
+                if cipher_suite_privs_list[i] != "X":
+                    cipher_suite_privs_list[i] = "X"
+                    print(
+                        "INFO: Disabling insecure IPMI cipher suite id "
+                        '"%s"' % cipher_suite_id
+                    )
 
+        new_cipher_suite_privs = "".join(cipher_suite_privs_list)
         if cipher_suite_privs == new_cipher_suite_privs:
             # Cipher suites are already properly configured, nothing
             # to do.
diff --git a/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py b/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
index 6065608..af571d1 100644
--- a/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
+++ b/src/metadataserver/builtin_scripts/commissioning_scripts/tests/test_bmc_config.py
@@ -1,4 +1,4 @@
-# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2020-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test bmc_config functions."""
@@ -796,6 +796,41 @@ EndSection
         )
         self.assertEqual("17", self.ipmi._cipher_suite_id)
 
+    def test_config_ipmitool_cipher_suite_ids_with_gaps(self):
+        mock_get_ipmitool_lan_print = self.patch(
+            self.ipmi, "_get_ipmitool_lan_print"
+        )
+        mock_get_ipmitool_lan_print.return_value = (
+            "2",
+            factory.make_name("output"),
+        )
+
+        # This tests a BMC which has IPMI ciphers 0, 4, 5, 9, 10, 13, and
+        # 14. The BMC has options for all supported ciphers plus 4 vendor
+        # ciphers which are not listed. ciphers 1, 2, 3 and two vendor
+        # ciphers are currently enabled while the others are disabled.
+        self.ipmi._config_ipmitool_cipher_suite_ids(
+            17, [1, 2, 3, 6, 7, 8, 11, 12, 15, 16, 17], "aaaXXXXXXXXXXaa"
+        )
+
+        # This verifies MAAS would keep cipher 3 and the two vendor ciphers
+        # as is while enabling cipher 17 for MAAS to use.
+        self.assertThat(
+            self.mock_check_call,
+            MockCalledOnceWith(
+                [
+                    "ipmitool",
+                    "lan",
+                    "set",
+                    "2",
+                    "cipher_privs",
+                    "XXaXXXXXXXaXXaa",
+                ],
+                timeout=60,
+            ),
+        )
+        self.assertEqual("17", self.ipmi._cipher_suite_id)
+
     def test_config_ipmitool_cipher_suite_ids_does_nothing(self):
         mock_get_ipmitool_lan_print = self.patch(
             self.ipmi, "_get_ipmitool_lan_print"