Merge lp:~itohm/nova/enable-rightaws into lp:~hudson-openstack/nova/trunk

[Masanori Itoh]

Enable RightAWS style signature checking using server_string without port number, add test cases for authenticate() and a new helper routine, and fix lp753660.

Enable RightAWS style signing on server_string without port number and fixes lp753660.
In short, when nova detected signature unmatch failure, try one more signature generated
using server_string without port number. e.g., drop ':8773' of '10.1.2.3:8773'
For more details, I wrote explanation why this patch is needed and pasted some evidences in the bug report.
I hope this fix makes sense for people.

[Rick Harris (rconradharris)]

Looks good, nice work Masanori.

Just some *really* small style points:

> 4 + LOG.debug('host_only_signature: %s', host_only_signature)

Needs i18n treatment.

> 50 + [address, sep, port] = server_str.partition(':')

Might be cleaner to use `split` since the separator is ignored as well as unpacking to a tuple since that's more conventional:

    (address, port) = server_str.split(':')

Side note: I think when/if we move to 2.7 we can eventually replace the host-extraction code with urlparse (which supports IPv6 addresses and differentiating the host and port portion of the netloc).

review: Needs Fixing

[Masanori Itoh (itohm)]

Hi Rick,

Thanks for the review!

I updated the branch according to your comments.
Actually, I found some more lines which need I18N treatment
in the same method which I modified and fixed them.

Also, I rebased to trunk rev 987, ran run_tests.sh and did some other checking.
I hope this looks goot for folks.

Regards,
Masanori

From: Rick Harris <email address hidden>
Subject: Re: [Merge] lp:~itoumsn/nova/enable-rightaws into lp:nova
Date: Thu, 14 Apr 2011 05:55:46 +0900

> Review: Needs Fixing
> Looks good, nice work Masanori.
>
> Just some *really* small style points:
>
> > 4 + LOG.debug('host_only_signature: %s', host_only_signature)
>
> Needs i18n treatment.
>
> > 50 + [address, sep, port] = server_str.partition(':')
>
> Might be cleaner to use `split` since the separator is ignored as well as unpacking to a tuple since that's more conventional:
>
> (address, port) = server_str.split(':')
>
> Side note: I think when/if we move to 2.7 we can eventually replace the host-extraction code with urlparse (which supports IPv6 addresses and differentiating the host and port portion of the netloc).
>
>
>
> --
> https://code.launchpad.net/~itoumsn/nova/enable-rightaws/+merge/57370
> You are the owner of lp:~itoumsn/nova/enable-rightaws.

[Rick Harris (rconradharris)]

lgtm, thanks for the fixups!

review: Approve

[Matt Dietz (cerberus)]

Looks good, but can we get some unit tests for this, as well as the helper method in utils?

review: Needs Fixing

[Masanori Itoh (itohm)]

Thanks Matt!

OK.
I'll consider some test cases for both auth code and the helper method.
I set the status "Work In Progress", and give me time a bit.

Regards,
Masanori

From: Matt Dietz <email address hidden>
Subject: Re: [Merge] lp:~itoumsn/nova/enable-rightaws into lp:nova
Date: Wed, 20 Apr 2011 07:41:18 +0900

> Review: Needs Fixing
> Looks good, but can we get some unit tests for this, as well as the helper method in utils?
> --
> https://code.launchpad.net/~itoumsn/nova/enable-rightaws/+merge/57370
> You are the owner of lp:~itoumsn/nova/enable-rightaws.

[Masanori Itoh (itohm)]

Hi,

Following Matt's comment, I added test cases for the new helper routine which I added and the authenticate() method in nova/auth/manager.py. Actually, the latter test case, test_004_signature_is_valid(), did nothing so far. I implemented it this time for two patterns of signature generation, using server_string with/without a port number.

One more thing is that I renamed the new helper function to make it more generic one
from get_host_only_server_string() to parse_server_string(). Now, it returns a list of
host and port strings.

One possible issue is that in the test case for authenticate() I used signatures and query strings captured on my development pc. They work anyway, but I'm not sure if it looks good for nova-core people.

-Masanori

[Vish Ishaya (vishvananda)]

Wow, nice job on the tests. We really should remove the numbers from those tests. 004 and 005 are left over from old tests and 005 doesn't even do anything, but i'm ok with this patch as is.

review: Approve

[Masanori Itoh (itohm)]

Hi,

I rebased my branch to trunk rev 1057, and did run_tests.sh.
No code change.

-Masanori

[lp:~itohm/nova/enable-rightaws]

965. By Masanori Itoh on 2011-04-25

Rebased to trunk rev 1030.

966. By Masanori Itoh on 2011-04-29

Rebased to trunk rev 1035.

967. By Masanori Itoh on 2011-05-07

Rebased to trunk rev 1057.

[Dan Prince (dan-prince)]

This works great. Been waiting for this one. My script:

---
require 'rubygems'
require 'right_aws'

ACCESS_KEY=ENV['EC2_ACCESS_KEY']
SECRET_KEY=ENV['EC2_SECRET_KEY']
EC2_URL=ENV['EC2_URL']

@ec2 = RightAws::Ec2.new(ACCESS_KEY, SECRET_KEY, { :endpoint_url => EC2_URL})
images = @ec2.describe_images
puts images.inspect
---

Approve.

review: Approve

[Vish Ishaya (vishvananda)]

looks like matts review has been handled. Approving.

=== modified file 'nova/auth/manager.py'
--- nova/auth/manager.py	2011-04-30 02:31:29 +0000
+++ nova/auth/manager.py	2011-05-07 15:42:37 +0000
@@ -305,9 +305,9 @@
         if check_type == 's3':
             sign = signer.Signer(user.secret.encode())
             expected_signature = sign.s3_authorization(headers, verb, path)
-            LOG.debug('user.secret: %s', user.secret)
-            LOG.debug('expected_signature: %s', expected_signature)
-            LOG.debug('signature: %s', signature)
+            LOG.debug(_('user.secret: %s'), user.secret)
+            LOG.debug(_('expected_signature: %s'), expected_signature)
+            LOG.debug(_('signature: %s'), signature)
             if signature != expected_signature:
                 LOG.audit(_("Invalid signature for user %s"), user.name)
                 raise exception.InvalidSignature(signature=signature,
@@ -317,10 +317,20 @@
             #             secret isn't unicode
             expected_signature = signer.Signer(user.secret.encode()).generate(
                     params, verb, server_string, path)
-            LOG.debug('user.secret: %s', user.secret)
-            LOG.debug('expected_signature: %s', expected_signature)
-            LOG.debug('signature: %s', signature)
+            LOG.debug(_('user.secret: %s'), user.secret)
+            LOG.debug(_('expected_signature: %s'), expected_signature)
+            LOG.debug(_('signature: %s'), signature)
             if signature != expected_signature:
+                (addr_str, port_str) = utils.parse_server_string(server_string)
+                # If the given server_string contains port num, try without it.
+                if port_str != '':
+                    host_only_signature = signer.Signer(
+                        user.secret.encode()).generate(params, verb,
+                                                       addr_str, path)
+                    LOG.debug(_('host_only_signature: %s'),
+                              host_only_signature)
+                    if signature == host_only_signature:
+                        return (user, project)
                 LOG.audit(_("Invalid signature for user %s"), user.name)
                 raise exception.InvalidSignature(signature=signature,
                                                  user=user)

=== modified file 'nova/tests/test_auth.py'
--- nova/tests/test_auth.py	2011-03-28 15:05:28 +0000
+++ nova/tests/test_auth.py	2011-05-07 15:42:37 +0000
@@ -101,9 +101,43 @@
             self.assertEqual('private-party', u.access)
 
     def test_004_signature_is_valid(self):
-        #self.assertTrue(self.manager.authenticate(**boto.generate_url ...? ))
-        pass
-        #raise NotImplementedError
+        with user_generator(self.manager, name='admin', secret='admin',
+                            access='admin'):
+            with project_generator(self.manager, name="admin",
+                                   manager_user='admin'):
+                accesskey = 'admin:admin'
+                expected_result = (self.manager.get_user('admin'),
+                                   self.manager.get_project('admin'))
+                # captured sig and query string using boto 1.9b/euca2ools 1.2
+                sig = 'd67Wzd9Bwz8xid9QU+lzWXcF2Y3tRicYABPJgrqfrwM='
+                auth_params = {'AWSAccessKeyId': 'admin:admin',
+                               'Action': 'DescribeAvailabilityZones',
+                               'SignatureMethod': 'HmacSHA256',
+                               'SignatureVersion': '2',
+                               'Timestamp': '2011-04-22T11:29:29',
+                               'Version': '2009-11-30'}
+                self.assertTrue(expected_result, self.manager.authenticate(
+                        accesskey,
+                        sig,
+                        auth_params,
+                        'GET',
+                        '127.0.0.1:8773',
+                        '/services/Cloud/'))
+                # captured sig and query string using RightAWS 1.10.0
+                sig = 'ECYLU6xdFG0ZqRVhQybPJQNJ5W4B9n8fGs6+/fuGD2c='
+                auth_params = {'AWSAccessKeyId': 'admin:admin',
+                               'Action': 'DescribeAvailabilityZones',
+                               'SignatureMethod': 'HmacSHA256',
+                               'SignatureVersion': '2',
+                               'Timestamp': '2011-04-22T11:29:49.000Z',
+                               'Version': '2008-12-01'}
+                self.assertTrue(expected_result, self.manager.authenticate(
+                        accesskey,
+                        sig,
+                        auth_params,
+                        'GET',
+                        '127.0.0.1',
+                        '/services/Cloud'))
 
     def test_005_can_get_credentials(self):
         return

=== modified file 'nova/tests/test_utils.py'
--- nova/tests/test_utils.py	2011-03-17 20:43:22 +0000
+++ nova/tests/test_utils.py	2011-05-07 15:42:37 +0000
@@ -250,3 +250,28 @@
         input = {'a': [1, 2, {'b': 'b_1'}]}
         self.assertEquals([1, 2, {'b': 'b_1'}], f(input, "a"))
         self.assertEquals(['b_1'], f(input, "a/b"))
+
+
+class GenericUtilsTestCase(test.TestCase):
+    def test_parse_server_string(self):
+        result = utils.parse_server_string('::1')
+        self.assertEqual(('::1', ''), result)
+        result = utils.parse_server_string('[::1]:8773')
+        self.assertEqual(('::1', '8773'), result)
+        result = utils.parse_server_string('2001:db8::192.168.1.1')
+        self.assertEqual(('2001:db8::192.168.1.1', ''), result)
+        result = utils.parse_server_string('[2001:db8::192.168.1.1]:8773')
+        self.assertEqual(('2001:db8::192.168.1.1', '8773'), result)
+        result = utils.parse_server_string('192.168.1.1')
+        self.assertEqual(('192.168.1.1', ''), result)
+        result = utils.parse_server_string('192.168.1.2:8773')
+        self.assertEqual(('192.168.1.2', '8773'), result)
+        result = utils.parse_server_string('192.168.1.3')
+        self.assertEqual(('192.168.1.3', ''), result)
+        result = utils.parse_server_string('www.example.com:8443')
+        self.assertEqual(('www.example.com', '8443'), result)
+        result = utils.parse_server_string('www.example.com')
+        self.assertEqual(('www.example.com', ''), result)
+        # error case
+        result = utils.parse_server_string('www.exa:mple.com:8443')
+        self.assertEqual(('', ''), result)

=== modified file 'nova/utils.py'
--- nova/utils.py	2011-04-22 19:44:24 +0000
+++ nova/utils.py	2011-05-07 15:42:37 +0000
@@ -709,3 +709,33 @@
     raise Exception(_('Expected object of type: %s') % (str(cls)))
     # TODO(justinsb): Can we make this better??
     return cls()  # Ugly PyLint hack
+
+
+def parse_server_string(server_str):
+    """
+    Parses the given server_string and returns a list of host and port.
+    If it's not a combination of host part and port, the port element
+    is a null string. If the input is invalid expression, return a null
+    list.
+    """
+    try:
+        # First of all, exclude pure IPv6 address (w/o port).
+        if netaddr.valid_ipv6(server_str):
+            return (server_str, '')
+
+        # Next, check if this is IPv6 address with a port number combination.
+        if server_str.find("]:") != -1:
+            (address, port) = server_str.replace('[', '', 1).split(']:')
+            return (address, port)
+
+        # Third, check if this is a combination of an address and a port
+        if server_str.find(':') == -1:
+            return (server_str, '')
+
+        # This must be a combination of an address and a port
+        (address, port) = server_str.split(':')
+        return (address, port)
+
+    except:
+        LOG.debug(_('Invalid server_string: %s' % server_str))
+        return ('', '')