Merge ~ddstreet/maas:lp1916860-v2 into maas:master

[Dan Streetman]

[MAAS Lander (maas-lander)]

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

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9400/console
COMMIT: 83b230ee1d49b89a703e9d1086f7f1acd603e3b5

review: Needs Fixing

[Lee Trager (ltrager)]

Thanks for the patch and noticing I used an '=' instead of a '+=' :). I think the root cause of the bug is MAAS isn't accounting for gaps in ciphers. See my comment in the bug for a full explanation. I'll create a patch for testing.

review: Needs Fixing

[Dan Streetman (ddstreet)]

> Thanks for the patch and noticing I used an '=' instead of a '+=' :). I think
> the root cause of the bug is MAAS isn't accounting for gaps in ciphers. See my
> comment in the bug for a full explanation. I'll create a patch for testing.

no, i think you need to look at my patch again, please - the problem wasn't (just) = instead of +=

[MAAS Lander (maas-lander)]

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

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9401/console
COMMIT: 94245fe639b23ff1fe191db1ec8d3e3f5c1380da

review: Needs Fixing

[Lee Trager (ltrager)]

I agree there were other things wrong with the original code. While this fixes one of them it doesn't check for gaps in supported ciphers. Look at the unit test I added in my branch[1]. Lets say ipmitool returns the following for supported ciphers and their settings

RMCP+ Cipher Suites : 1,2,3,6,7,8,11,12,15,16,17
Cipher Suite Priv Max : aaaXXXXXXXXXXaa

This means the BMC doesn't IPMI ciphers 0, 4, 5, 9, 10, 13, and 14. It 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.

This branch will result push the settings string "XXXXXXXXXXXXXXX" which disables all ciphers making remote access impossible. The correct cipher string is "XXaXXXXXXXaXXaa" which means disable the insecure ciphers 1 and 2, keep 3 and the two vendor ciphers as is, and enable cipher 17 for MAAS to use.

[1] https://code.launchpad.net/~ltrager/maas/+git/maas/+merge/399134

[Dan Streetman (ddstreet)]

> While this fixes one of them it doesn't check for gaps in supported ciphers.

again, you're misunderstanding the api of ipmitool. the commit you landed to fix this is wrong.

> This branch will result push the settings string "XXXXXXXXXXXXXXX" which disables all ciphers making remote access impossible.

no, that isn't what this commit will use...

Unmerged commits

94245fe... by Dan Streetman on 2021-03-03

BMC config: fix usage of ipmi cipher_privs

LP: #1916860

Signed-off-by: Dan Streetman <email address hidden>

diff --git a/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py b/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
index 69cc4c6..b6b31f7 100755
--- a/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
+++ b/src/metadataserver/builtin_scripts/commissioning_scripts/bmc_config.py
@@ -624,29 +624,21 @@ class IPMI(BMCConfig):
     ):
         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"
+            if i in [17, 3, 8, 12]:
+                if i == max_cipher_suite_id and v != "a":
+                    print(
+                        'INFO: Enabling IPMI cipher suite id "%s" '
+                        "for MAAS use..." % i
+                    )
+                    v = "a"
             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
+                if v != "X":
+                    print(
+                        "INFO: Disabling insecure IPMI cipher suite id "
+                        '"%s"' % i
+                    )
+                v = "X"
+            new_cipher_suite_privs += v
 
         if cipher_suite_privs == new_cipher_suite_privs:
             # Cipher suites are already properly configured, nothing