launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/omshell-bug-1062040 into lp:maas.

Commit message:
Repeatedly generate omapi keys if a known bad one comes up.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1062040 in MAAS: "Setting host maps in omshell fails"
  https://bugs.launchpad.net/maas/+bug/1062040

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/omshell-bug-1062040/+merge/128203
-- 
https://code.launchpad.net/~julian-edwards/maas/omshell-bug-1062040/+merge/128203
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/omshell-bug-1062040 into lp:maas.

=== modified file 'src/provisioningserver/omshell.py'
--- src/provisioningserver/omshell.py	2012-09-10 02:50:48 +0000
+++ src/provisioningserver/omshell.py	2012-10-05 10:05:25 +0000
@@ -17,6 +17,7 @@
     "Omshell",
     ]
 
+import re
 import os
 import shutil
 from subprocess import (
@@ -41,19 +42,15 @@
         env=env)
 
 
-def generate_omapi_key():
-    """Generate a HMAC-MD5 key by calling out to the dnssec-keygen tool.
-
-    :return: The shared key suitable for OMAPI access.
-    :type: string
-    """
-    # dnssec-keygen writes out files to a specified directory, so we
-    # need to make a temp directory for that.
-
-    # mkdtemp() says it will return a directory that is readable,
-    # writable, and searchable only by the creating user ID.
-    tmpdir = mkdtemp(prefix="%s." % os.path.basename(__file__))
-    try:
+def run_repeated_keygen(tmpdir):
+    # omshell has a bug where if the chars '/' or '+' appear either
+    # side of the word 'no' (in any case), it throws an error like
+    # "partial base64 value left over".  We check for that here and
+    # repeatedly generate a new key until a good one is generated.
+
+    key = None
+    pattern = re.compile(".*[+/][Nn][Oo][+/].*")
+    while key is None:
         key_id = call_dnssec_keygen(tmpdir)
 
         # Locate the file that was written and strip out the Key: field in
@@ -70,8 +67,31 @@
         if parsing_error or 'Key' not in config:
             raise AssertionError(
                 "Key field not found in output from dnssec-keygen")
-        else:
-            return config['Key']
+
+        key = config['Key']
+        if pattern.match(key) is not None:
+            # Force a retry.
+            os.remove(key_file_name)
+            key = None
+
+    return key
+
+
+def generate_omapi_key():
+    """Generate a HMAC-MD5 key by calling out to the dnssec-keygen tool.
+
+    :return: The shared key suitable for OMAPI access.
+    :type: string
+    """
+    # dnssec-keygen writes out files to a specified directory, so we
+    # need to make a temp directory for that.
+
+    # mkdtemp() says it will return a directory that is readable,
+    # writable, and searchable only by the creating user ID.
+    tmpdir = mkdtemp(prefix="%s." % os.path.basename(__file__))
+    try:
+        key = run_repeated_keygen(tmpdir)
+        return key
     finally:
         shutil.rmtree(tmpdir)
 

=== modified file 'src/provisioningserver/tests/test_omshell.py'
--- src/provisioningserver/tests/test_omshell.py	2012-09-10 02:50:48 +0000
+++ src/provisioningserver/tests/test_omshell.py	2012-10-05 10:05:25 +0000
@@ -23,6 +23,7 @@
 from maastesting.testcase import TestCase
 from mock import Mock
 from provisioningserver import omshell
+import provisioningserver.omshell
 from provisioningserver.omshell import (
     generate_omapi_key,
     Omshell,
@@ -207,3 +208,27 @@
 
         self.patch(omshell, 'call_dnssec_keygen', returns_junk)
         self.assertRaises(AssertionError, generate_omapi_key)
+
+    def test_run_repeated_keygen(self):
+        # Test that a known bad key is ignored and we generate a new one
+        # to replace it.
+        key_that_fails_in_omshell = (
+            "YXY5pr+No/8NZeodSd27wWbI8N6kIjMF/nrnFIlPwVLuByJKkQcBRtfDrD"
+            "LLG2U9/ND7/bIlJxEGTUnyipffHQ==")
+        # Set up a Mock with a side effect that returns a known bad key
+        # on the first pass, and the real function on the second.
+        def bad_key(*args):
+            return {'Key': key_that_fails_in_omshell}
+        side_effects = [
+            bad_key, provisioningserver.omshell.parse_key_value_file]
+        def side_effect(foo):
+            func = side_effects.pop(0)
+            return func(foo)
+
+        mock = self.patch(provisioningserver.omshell, 'parse_key_value_file')
+        mock.side_effect = side_effect
+
+        new_key = generate_omapi_key()
+        self.assertNotEqual(new_key, key_that_fails_in_omshell)
+
+