Merge ~ajkavanagh/simplestreams:bug/1802407 into simplestreams:master

[Alex Kavanagh]

Add SSL support to simplestreams/openstack.py

This change enables the openstack integration to connect to https
OpenStack endpoints by using the OS_CACERT environement variable.

LP: #1802407

[Scott Moser (smoser)]

Hey, Thanks
This does look good.

I know there isnt much unit tests in place, but it would be good to add some here.

It seems you could reasonably easily add some tests to get_ksclient that the session it created was right. I'm not sure if you end up needing to mock session.Session or not for that, but either way it shoudlnt be too bad.

thanks.

review: Needs Fixing

[Alex Kavanagh (ajkavanagh)]

Sure, I'll see if I can rustle up some tests on it. I kind of avoided it as there weren't any for that part of the code!

5d1cda9... by Alex Kavanagh on 2018-12-31

Add .tox to .gitignore

c9d83b3... by Alex Kavanagh on 2018-12-31

Add tests for the changes made to add SSL to the keystone auth

[Alex Kavanagh (ajkavanagh)]

So I've now added tests to the branch to test the new code, and fixed a bug in the existing code (by writing the new tests!). I guess the slow addition of tests is a good idea.

[Scott Moser (smoser)]

Some small comments inline.
Thank you for this though, it looks really good.

What bug was it that you found? Mostly just curious.

[Alex Kavanagh (ajkavanagh)]

Thanks for the comments; I've replied and will update the merge proposal.

Re: the bug; it's a feature additional to add SSL support (and the ability to provide the cert), which was requested in the linked bug on the MP: https://bugs.launchpad.net/charm-glance-simplestreams-sync/+bug/1802407

Essentially, this change is needed by: https://review.openstack.org/#/c/623488/

Hope that's okay!?

2423d4d... by Alex Kavanagh on 2019-01-07

Update tests following review comments

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:2423d4d
https://jenkins.ubuntu.com/server/job/simplestreams-ci/83/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/simplestreams-ci/nodes=metal-amd64/83
    SUCCESS: https://jenkins.ubuntu.com/server/job/simplestreams-ci/nodes=metal-ppc64el/83
    SUCCESS: https://jenkins.ubuntu.com/server/job/simplestreams-ci/nodes=metal-s390x/83

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/simplestreams-ci/83/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

review: Approve

[Alex Kavanagh (ajkavanagh)]

Thanks for the merging the code!

diff --git a/.gitignore b/.gitignore
index 14c2107..b1af602 100644
--- a/.gitignore
+++ b/.gitignore
@@ -6,3 +6,6 @@ exdata
 exdata-query
 *.sjson
 *.gpg
+MANIFEST
+.coverage
+.tox
diff --git a/simplestreams/openstack.py b/simplestreams/openstack.py
index 4899df7..ebf63c4 100644
--- a/simplestreams/openstack.py
+++ b/simplestreams/openstack.py
@@ -40,20 +40,34 @@ OS_ENV_VARS = (
 )
 
 
+# only used for legacy client connection
 PT_V2 = ('username', 'password', 'tenant_id', 'tenant_name', 'auth_url',
          'cacert', 'insecure', )
-PT_V3 = ('username', 'password', 'project_id', 'project_name', 'auth_url',
-         'cacert', 'insecure', 'user_domain_name', 'project_domain_name',
-         'user_domain_id', 'project_domain_id', )
+
+# annoyingly the 'insecure' option in the old client constructor is now called
+# the 'verify' option in the session.Session() constructor
+PASSWORD_V2 = ('auth_url', 'username', 'password', 'user_id', 'trust_id',
+               'tenant_id', 'tenant_name', 'reauthenticate')
+PASSWORD_V3 = ('auth_url', 'password', 'username',
+               'user_id', 'user_domain_id', 'user_domain_name',
+               'trust_id', 'system_scope',
+               'domain_id', 'domain_name',
+               'project_id', 'project_name',
+               'project_domain_id', 'project_domain_name',
+               'reauthenticate')
+SESSION_ARGS = ('cert', 'timeout', 'verify', 'original_ip', 'redirect',
+                'addition_headers', 'app_name', 'app_version',
+                'additional_user_agent',
+                'discovery_cache', 'split_loggers', 'collect_timing')
 
 
 Settings = collections.namedtuple('Settings', 'mod ident arg_set')
 KS_VERSION_RESOLVER = {2: Settings(mod=ksclient_v2,
                                    ident=v2,
-                                   arg_set=PT_V2),
+                                   arg_set=PASSWORD_V2),
                        3: Settings(mod=ksclient_v3,
                                    ident=v3,
-                                   arg_set=PT_V3), }
+                                   arg_set=PASSWORD_V3)}
 
 
 def load_keystone_creds(**kwargs):
@@ -68,7 +82,7 @@ def load_keystone_creds(**kwargs):
         # take off 'os_'
         short = lc[3:]
         if short in kwargs:
-            ret[lc] = kwargs.get(lc)
+            ret[short] = kwargs.get(short)
         elif name in os.environ:
             # take off 'os_'
             ret[short] = os.environ[name]
@@ -76,13 +90,18 @@ def load_keystone_creds(**kwargs):
     if 'insecure' in ret:
         if isinstance(ret['insecure'], str):
             ret['insecure'] = (ret['insecure'].lower() not in
-                               ("", "0", "no", "off"))
+                               ("", "0", "no", "off", 'false'))
         else:
             ret['insecure'] = bool(ret['insecure'])
 
+    # verify is the key that is used by requests, and thus the Session object.
+    # i.e. verify is either False or a certificate path or file.
+    if not ret.get('insecure', False) and 'cacert' in ret:
+        ret['verify'] = ret['cacert']
+
     missing = []
     for req in ('username', 'auth_url'):
-        if not ret.get(req):
+        if not ret.get(req, None):
             missing.append(req)
 
     if not (ret.get('auth_token') or ret.get('password')):
@@ -91,14 +110,11 @@ def load_keystone_creds(**kwargs):
     api_version = get_ks_api_version(ret.get('auth_url', '')) or 2
     if (api_version == 2 and
             not (ret.get('tenant_id') or ret.get('tenant_name'))):
-        raise ValueError("(tenant_id or tenant_name)")
-
-    if (api_version == 3 and
-            not (ret.get('user_domain_name') and
-                 ret.get('project_domain_name') and
-                 ret.get('project_name'))):
-        raise ValueError("(user_domain_name, project_domain_name "
-                         "or project_name)")
+        missing.append("(tenant_id or tenant_name)")
+    if api_version == 3:
+        for k in ('user_domain_name', 'project_domain_name', 'project_name'):
+            if not ret.get(k, None):
+                missing.append(k)
 
     if missing:
         raise ValueError("Need values for: %s" % missing)
@@ -167,7 +183,9 @@ def get_ksclient(**kwargs):
     # Filter/select the args for the api version from the kwargs dictionary
     kskw = {k: v for k, v in kwargs.items() if k in arg_set}
     auth = KS_VERSION_RESOLVER[api_version].ident.Password(**kskw)
-    sess = session.Session(auth=auth)
+    authkw = {k: v for k, v in kwargs.items() if k in SESSION_ARGS}
+    authkw['auth'] = auth
+    sess = session.Session(**authkw)
     client = KS_VERSION_RESOLVER[api_version].mod.Client(session=sess)
     client.auth_ref = auth.get_access(sess)
     return client
diff --git a/tests/unittests/test_openstack.py b/tests/unittests/test_openstack.py
new file mode 100644
index 0000000..beec9a2
--- /dev/null
+++ b/tests/unittests/test_openstack.py
@@ -0,0 +1,265 @@
+import mock
+import unittest
+
+import simplestreams.openstack as s_openstack
+
+
+class TestOpenStack(unittest.TestCase):
+    MOCK_OS_VARS = {
+        'OS_AUTH_TOKEN': 'a-token',
+        'OS_AUTH_URL': 'http://0.0.0.0/v2.0',
+        'OS_CACERT': 'some-cert',
+        'OS_IMAGE_API_VERSION': '2',
+        'OS_IMAGE_URL': 'http://1.2.3.4:9292/',
+        'OS_PASSWORD': 'some-password',
+        'OS_REGION_NAME': 'region1',
+        'OS_STORAGE_URL': 'http://1.2.3.4:8080/v1/AUTH_123456',
+        'OS_TENANT_ID': '123456789',
+        'OS_TENANT_NAME': 'a-project',
+        'OS_USERNAME': 'openstack',
+        'OS_INSECURE': 'true',
+        'OS_USER_DOMAIN_NAME': 'default',
+        'OS_PROJECT_DOMAIN_NAME': 'Default',
+        'OS_USER_DOMAIN_ID': 'default',
+        'OS_PROJECT_DOMAIN_ID': 'default',
+        'OS_PROJECT_NAME': 'some-project',
+        'OS_PROJECT_ID': 'project-id',
+    }
+
+    def test_load_keystone_creds_V2_from_osvars(self):
+
+        with mock.patch('os.environ', new=self.MOCK_OS_VARS.copy()):
+            creds = s_openstack.load_keystone_creds()
+
+        self.assertEquals(creds,
+                          {'auth_token': 'a-token',
+                           'auth_url': 'http://0.0.0.0/v2.0',
+                           'cacert': 'some-cert',
+                           'image_api_version': '2',
+                           'image_url': 'http://1.2.3.4:9292/',
+                           'insecure': True,
+                           'password': 'some-password',
+                           'project_domain_id': 'default',
+                           'project_domain_name': 'Default',
+                           'project_id': 'project-id',
+                           'project_name': 'some-project',
+                           'region_name': 'region1',
+                           'storage_url': 'http://1.2.3.4:8080/v1/AUTH_123456',
+                           'tenant_id': '123456789',
+                           'tenant_name': 'a-project',
+                           'user_domain_id': 'default',
+                           'user_domain_name': 'default',
+                           'username': 'openstack'})
+
+    def test_load_keystone_creds_V2_from_kwargs(self):
+
+        with mock.patch('os.environ', new=self.MOCK_OS_VARS.copy()):
+            creds = s_openstack.load_keystone_creds(
+                password='the-password',
+                username='myuser')
+
+        self.assertEquals(creds,
+                          {'auth_token': 'a-token',
+                           'auth_url': 'http://0.0.0.0/v2.0',
+                           'cacert': 'some-cert',
+                           'image_api_version': '2',
+                           'image_url': 'http://1.2.3.4:9292/',
+                           'insecure': True,
+                           'password': 'the-password',
+                           'project_domain_id': 'default',
+                           'project_domain_name': 'Default',
+                           'project_id': 'project-id',
+                           'project_name': 'some-project',
+                           'region_name': 'region1',
+                           'storage_url': 'http://1.2.3.4:8080/v1/AUTH_123456',
+                           'tenant_id': '123456789',
+                           'tenant_name': 'a-project',
+                           'user_domain_id': 'default',
+                           'user_domain_name': 'default',
+                           'username': 'myuser'})
+
+    def test_load_keystone_creds_V3_from_osvars(self):
+        v3kwargs = self.MOCK_OS_VARS.copy()
+        v3kwargs['OS_AUTH_URL'] = 'http://0.0.0.0/v3'
+
+        with mock.patch('os.environ', new=v3kwargs):
+            creds = s_openstack.load_keystone_creds()
+
+        self.assertEquals(creds,
+                          {'auth_token': 'a-token',
+                           'auth_url': 'http://0.0.0.0/v3',
+                           'cacert': 'some-cert',
+                           'image_api_version': '2',
+                           'image_url': 'http://1.2.3.4:9292/',
+                           'insecure': True,
+                           'password': 'some-password',
+                           'project_domain_id': 'default',
+                           'project_domain_name': 'Default',
+                           'project_id': 'project-id',
+                           'project_name': 'some-project',
+                           'region_name': 'region1',
+                           'storage_url': 'http://1.2.3.4:8080/v1/AUTH_123456',
+                           'tenant_id': '123456789',
+                           'tenant_name': 'a-project',
+                           'user_domain_id': 'default',
+                           'user_domain_name': 'default',
+                           'username': 'openstack'})
+
+    def test_load_keystone_creds_insecure(self):
+        """test load_keystone_creds behaves correctly for OS_INSECURE values.
+        """
+        kwargs = self.MOCK_OS_VARS.copy()
+        test_pairs = (('off', False),
+                      ('no', False),
+                      ('false', False),
+                      ('', False),
+                      ('anything-else', True))
+        for val, expected in test_pairs:
+            kwargs['OS_INSECURE'] = val
+            with mock.patch('os.environ', new=kwargs):
+                creds = s_openstack.load_keystone_creds()
+            self.assertEqual(creds['insecure'], expected)
+
+    def test_load_keystone_creds_verify(self):
+        """Test that cacert comes across as verify."""
+        kwargs = self.MOCK_OS_VARS.copy()
+
+        with mock.patch('os.environ', new=kwargs):
+            creds = s_openstack.load_keystone_creds()
+        self.assertNotIn('verify', creds)
+
+        kwargs['OS_INSECURE'] = 'false'
+        with mock.patch('os.environ', new=kwargs):
+            creds = s_openstack.load_keystone_creds()
+        self.assertEqual(creds['verify'], kwargs['OS_CACERT'])
+
+        kwargs['OS_INSECURE'] = 'false'
+        del kwargs['OS_CACERT']
+        with mock.patch('os.environ', new=kwargs):
+            creds = s_openstack.load_keystone_creds()
+        self.assertNotIn('verify', creds)
+
+    def test_load_keystone_creds_missing(self):
+        kwargs = self.MOCK_OS_VARS.copy()
+        del kwargs['OS_USERNAME']
+        with mock.patch('os.environ', new=kwargs):
+            with self.assertRaises(ValueError):
+                s_openstack.load_keystone_creds()
+
+        kwargs = self.MOCK_OS_VARS.copy()
+        del kwargs['OS_AUTH_URL']
+        with mock.patch('os.environ', new=kwargs):
+            with self.assertRaises(ValueError):
+                s_openstack.load_keystone_creds()
+
+        # either auth_token or password needs to be exist, but if both are
+        # missing then raise an exception
+        kwargs = self.MOCK_OS_VARS.copy()
+        del kwargs['OS_AUTH_TOKEN']
+        with mock.patch('os.environ', new=kwargs):
+            s_openstack.load_keystone_creds()
+        kwargs = self.MOCK_OS_VARS.copy()
+        del kwargs['OS_PASSWORD']
+        with mock.patch('os.environ', new=kwargs):
+            s_openstack.load_keystone_creds()
+        kwargs = self.MOCK_OS_VARS.copy()
+        del kwargs['OS_AUTH_TOKEN']
+        del kwargs['OS_PASSWORD']
+        with mock.patch('os.environ', new=kwargs):
+            with self.assertRaises(ValueError):
+                s_openstack.load_keystone_creds()
+
+        # API version 3
+        for k in ('OS_USER_DOMAIN_NAME',
+                  'OS_PROJECT_DOMAIN_NAME',
+                  'OS_PROJECT_NAME'):
+            kwargs = self.MOCK_OS_VARS.copy()
+            kwargs['OS_AUTH_URL'] = 'http://0.0.0.0/v3'
+            del kwargs[k]
+            with self.assertRaises(ValueError):
+                s_openstack.load_keystone_creds()
+
+    @mock.patch.object(s_openstack, '_LEGACY_CLIENTS', new=False)
+    @mock.patch.object(s_openstack.session, 'Session')
+    def test_get_ksclient_v2(self, m_session):
+        kwargs = self.MOCK_OS_VARS.copy()
+        mock_ksclient_v2 = mock.Mock()
+        mock_ident_v2 = mock.Mock()
+        with mock.patch.object(
+                s_openstack, 'KS_VERSION_RESOLVER', new={}) as m:
+            m[2] = s_openstack.Settings(mod=mock_ksclient_v2,
+                                        ident=mock_ident_v2,
+                                        arg_set=s_openstack.PASSWORD_V2)
+
+            # test openstack ks 2
+            m_auth = mock.Mock()
+            mock_ident_v2.Password.return_value = m_auth
+            m_get_access = mock.Mock()
+            m_auth.get_access.return_value = m_get_access
+            m_session.return_value = mock.sentinel.session
+
+            with mock.patch('os.environ', new=kwargs):
+                creds = s_openstack.load_keystone_creds()
+
+            c = s_openstack.get_ksclient(**creds)
+            # verify that mock_ident_v2 is called with password
+            mock_ident_v2.Password.assert_has_calls([
+                mock.call(auth_url='http://0.0.0.0/v2.0',
+                          password='some-password',
+                          tenant_id='123456789',
+                          tenant_name='a-project',
+                          username='openstack')])
+            # verify that the session was called with the v2 password
+            m_session.assert_called_once_with(auth=m_auth)
+            # verify that the client was called with the session
+            mock_ksclient_v2.Client.assert_called_once_with(
+                session=mock.sentinel.session)
+            # finally check that the client as an auth_ref and that it contains
+            # the get_access() call
+            self.assertEqual(c.auth_ref, m_get_access)
+            m_auth.get_access.assert_called_once_with(mock.sentinel.session)
+
+    @mock.patch.object(s_openstack, '_LEGACY_CLIENTS', new=False)
+    @mock.patch.object(s_openstack.session, 'Session')
+    def test_get_ksclient_v3(self, m_session):
+        kwargs = self.MOCK_OS_VARS.copy()
+        kwargs['OS_AUTH_URL'] = 'http://0.0.0.0/v3'
+        mock_ksclient_v3 = mock.Mock()
+        mock_ident_v3 = mock.Mock()
+        with mock.patch.object(
+                s_openstack, 'KS_VERSION_RESOLVER', new={}) as m:
+            m[3] = s_openstack.Settings(mod=mock_ksclient_v3,
+                                        ident=mock_ident_v3,
+                                        arg_set=s_openstack.PASSWORD_V3)
+
+            # test openstack ks 3
+            m_auth = mock.Mock()
+            mock_ident_v3.Password.return_value = m_auth
+            m_get_access = mock.Mock()
+            m_auth.get_access.return_value = m_get_access
+            m_session.return_value = mock.sentinel.session
+
+            with mock.patch('os.environ', new=kwargs):
+                creds = s_openstack.load_keystone_creds()
+
+            c = s_openstack.get_ksclient(**creds)
+            # verify that mock_ident_v2 is called with password
+            mock_ident_v3.Password.assert_has_calls([
+                mock.call(auth_url='http://0.0.0.0/v3',
+                          password='some-password',
+                          project_domain_id='default',
+                          project_domain_name='Default',
+                          project_id='project-id',
+                          project_name='some-project',
+                          user_domain_id='default',
+                          user_domain_name='default',
+                          username='openstack')])
+            # verify that the session was called with the v2 password
+            m_session.assert_called_once_with(auth=m_auth)
+            # verify that the client was called with the session
+            mock_ksclient_v3.Client.assert_called_once_with(
+                session=mock.sentinel.session)
+            # finally check that the client as an auth_ref and that it contains
+            # the get_access() call
+            self.assertEqual(c.auth_ref, m_get_access)
+            m_auth.get_access.assert_called_once_with(mock.sentinel.session)