Merge lp:~cbehrens/nova/lp788979 into lp:~hudson-openstack/nova/trunk

[Chris Behrens]

When encrypting passwords in xenapi's SimpleDH(), we shouldn't send a final newline to openssl, as it'll use that as encryption data. However, we do need to make sure there's a newline on the end when we write the base64 string for decoding.. Made these changes and updated the test.

[Mark Washenberger (markwash)]

If you put newlines at the end of your test message, you will see that there are still some problems here.

I was also kind of frustrated by the fact that I could run _run_ssl(text, 'DECAPITATE!') and it would still happily decrypt, rather than decapitating as I asked. But rather than put the burden on you I have a merge prop for this branch that will fix the problems I saw above and make me slightly happier :-)

review: Needs Fixing

[Chris Behrens (cbehrens)]

Love the DECAPITATE comment. :) Thanks for the additional tests... those should all have been there in the first place!

I've merged your branch and I have all of the tests passing now, reverting to a slightly modified version of what I had initially. I moved most of the openssl command line back into _run_ssl, but I made the 2nd parameter be optional extra arguments, vs just a 'd'. DECAPITATE should return an error now. :)

[Mark Washenberger (markwash)]

Looks good.

I will say one last thing in favor of adding '-A'. If you were to print the result of encryption in 'test_encrypt_really_long_message', it would be a multiline base64 string. This seems potentially undesirable and there is no check which prevents client code from passing in a sufficiently long text. I believe the text would only need to be 61 characters to trigger the first line break.

With that off my chest, I defer to your judgment.

review: Approve

[Chris Behrens (cbehrens)]

Ah. I'll go ahead and put it back and re-test.

[Chris Behrens (cbehrens)]

Ok.. works with -A in.

[Mark Washenberger (markwash)]

Double approve! :-)

review: Approve

[Chris Behrens (cbehrens)]

Thanks man :)

[Chris Behrens (cbehrens)]

No need for the \n with -A. Last fix, hopefully.

[Vish Ishaya (vishvananda)]

lgtm

review: Approve

=== modified file 'nova/tests/test_xenapi.py'
--- nova/tests/test_xenapi.py	2011-05-26 19:27:27 +0000
+++ nova/tests/test_xenapi.py	2011-05-27 20:49:13 +0000
@@ -592,11 +592,29 @@
         bob_shared = self.bob.compute_shared(alice_pub)
         self.assertEquals(alice_shared, bob_shared)
 
-    def test_encryption(self):
-        msg = "This is a top-secret message"
-        enc = self.alice.encrypt(msg)
+    def _test_encryption(self, message):
+        enc = self.alice.encrypt(message)
+        self.assertFalse(enc.endswith('\n'))
         dec = self.bob.decrypt(enc)
-        self.assertEquals(dec, msg)
+        self.assertEquals(dec, message)
+
+    def test_encrypt_simple_message(self):
+        self._test_encryption('This is a simple message.')
+
+    def test_encrypt_message_with_newlines_at_end(self):
+        self._test_encryption('This message has a newline at the end.\n')
+
+    def test_encrypt_many_newlines_at_end(self):
+        self._test_encryption('Message with lotsa newlines.\n\n\n')
+
+    def test_encrypt_newlines_inside_message(self):
+        self._test_encryption('Message\nwith\ninterior\nnewlines.')
+
+    def test_encrypt_with_leading_newlines(self):
+        self._test_encryption('\n\nMessage with leading newlines.')
+
+    def test_encrypt_really_long_message(self):
+        self._test_encryption(''.join(['abcd' for i in xrange(1024)]))
 
     def tearDown(self):
         super(XenAPIDiffieHellmanTestCase, self).tearDown()

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-05-26 19:30:23 +0000
+++ nova/virt/xenapi/vmops.py	2011-05-27 20:49:13 +0000
@@ -1190,26 +1190,22 @@
         mpi = M2Crypto.m2.bn_to_mpi(bn)
         return mpi
 
-    def _run_ssl(self, text, which):
-        base_cmd = ('openssl enc -aes-128-cbc -a -pass pass:%(shared)s '
-                '-nosalt %(dec_flag)s')
-        if which.lower()[0] == 'd':
-            dec_flag = ' -d'
-        else:
-            dec_flag = ''
-        shared = self._shared
-        cmd = base_cmd % locals()
-        proc = _runproc(cmd)
-        proc.stdin.write(text + '\n')
+    def _run_ssl(self, text, extra_args=None):
+        if not extra_args:
+            extra_args = ''
+        cmd = 'enc -aes-128-cbc -A -a -pass pass:%s -nosalt %s' % (
+                self._shared, extra_args)
+        proc = _runproc('openssl %s' % cmd)
+        proc.stdin.write(text)
         proc.stdin.close()
         proc.wait()
         err = proc.stderr.read()
         if err:
             raise RuntimeError(_('OpenSSL error: %s') % err)
-        return proc.stdout.read().strip('\n')
+        return proc.stdout.read()
 
     def encrypt(self, text):
-        return self._run_ssl(text, 'enc')
+        return self._run_ssl(text).strip('\n')
 
     def decrypt(self, text):
-        return self._run_ssl(text, 'dec')
+        return self._run_ssl(text, '-d')