From bb1cf4d54b2b7e95dd3f37d4c3f0cd1b0045ce7b Mon Sep 17 00:00:00 2001 From: Thierry Carrez Date: Mon, 20 Apr 2015 18:26:05 +0200 Subject: [PATCH 1/7] Update .gitreview to match stable/kilo Change-Id: I6d6a396924b338fe7cca3fc381feda8752491f56 --- .gitreview | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitreview b/.gitreview index 955af452f..c7ec36537 100644 --- a/.gitreview +++ b/.gitreview @@ -2,3 +2,4 @@ host=review.openstack.org port=29418 project=openstack/barbican.git +defaultbranch=stable/kilo From 93718aaa70d3f4523e636bfa6d602470e0d26b26 Mon Sep 17 00:00:00 2001 From: OpenStack Proposal Bot Date: Mon, 20 Apr 2015 17:54:00 +0000 Subject: [PATCH 2/7] Updated from global requirements Change-Id: Ife99d56a70c0ebd10a9ea47b06f969cd1e74b984 --- requirements.txt | 2 +- test-requirements.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements.txt b/requirements.txt index b97ce7a26..f671ef1fc 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,7 +22,7 @@ pecan>=0.8.0 pycrypto>=2.6 pyOpenSSL>=0.11 python-ldap>=2.4 -keystonemiddleware>=1.5.0 +keystonemiddleware>=1.5.0,<1.6.0 six>=1.9.0 SQLAlchemy>=0.9.7,<=0.9.99 stevedore>=1.3.0,<1.4.0 # Apache-2.0 diff --git a/test-requirements.txt b/test-requirements.txt index 3d3a60e57..0f65d3291 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -11,7 +11,7 @@ testrepository>=0.0.18 testtools>=0.9.36,!=1.2.0 fixtures>=0.3.14 requests>=2.2.0,!=2.4.0 -python-keystoneclient>=1.1.0 +python-keystoneclient>=1.1.0,<1.4.0 tempest-lib>=0.4.0 # Documentation build requirements From b37c35c9229dab43e3d77e5061d06f34c787bc2b Mon Sep 17 00:00:00 2001 From: Dave McCowan Date: Fri, 24 Apr 2015 08:50:09 -0400 Subject: [PATCH 3/7] Fix call to load_privatekey() when passphrase is None The original code worked, but breaks with PyOpenSSL 0.15.1, the version currently used by the gate. Closes-Bug: #1448193 Change-Id: Iae44f08fa6442e3463e6b552955229f3fd36fbde --- barbican/tasks/certificate_resources.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/barbican/tasks/certificate_resources.py b/barbican/tasks/certificate_resources.py index 65ad03d4b..7d9abfcbe 100644 --- a/barbican/tasks/certificate_resources.py +++ b/barbican/tasks/certificate_resources.py @@ -321,11 +321,17 @@ def _generate_csr(order_model, project_model): if not private_key: raise excep.StoredKeyPrivateKeyNotFound(container_id) - pkey = crypto.load_privatekey( - crypto.FILETYPE_PEM, - private_key, - passphrase - ) + if passphrase is None: + pkey = crypto.load_privatekey( + crypto.FILETYPE_PEM, + private_key + ) + else: + pkey = crypto.load_privatekey( + crypto.FILETYPE_PEM, + private_key, + passphrase + ) subject_name = order_model.meta.get('subject_dn') subject_name_dns = ldap.dn.str2dn(subject_name) From 4861932b51e491d217276f07f52e116179dc0d15 Mon Sep 17 00:00:00 2001 From: Dave McCowan Date: Tue, 21 Apr 2015 17:59:41 -0400 Subject: [PATCH 4/7] Fix failure with get on dict that was None When calling get_acl_dict_for_user() in the RBAC feature, the user list may be empty. In this case, make sure an empty list (not None) is returned so the receiving code won't fail. Change-Id: I6aeb94e03aa7898823ec408807180f7eeb2d2916 Closes-bug: #1446826 --- barbican/api/controllers/__init__.py | 2 +- barbican/tests/api/test_resources_policy.py | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/barbican/api/controllers/__init__.py b/barbican/api/controllers/__init__.py index a1eb82839..2c10e77b4 100644 --- a/barbican/api/controllers/__init__.py +++ b/barbican/api/controllers/__init__.py @@ -222,7 +222,7 @@ class ACLMixin(object): if not ctxt: return None acl_dict = {acl.operation: acl.operation for acl in acl_list - if ctxt.user in acl.to_dict_fields().get('users')} + if ctxt.user in acl.to_dict_fields().get('users', [])} co_dict = {'%s_creator_only' % acl.operation: acl.creator_only for acl in acl_list if acl.creator_only is not None} acl_dict.update(co_dict) diff --git a/barbican/tests/api/test_resources_policy.py b/barbican/tests/api/test_resources_policy.py index 9413546c5..5c75bc3f1 100644 --- a/barbican/tests/api/test_resources_policy.py +++ b/barbican/tests/api/test_resources_policy.py @@ -377,6 +377,25 @@ class WhenTestingSecretResource(BaseTestCase): user_id=self.user_id, project_id=self.external_project_id) + def test_should_raise_decrypt_secret_for_with_creator_only_nolist(self): + """Should raise authz error as secret is marked private. + + As secret is private so project users should not be able to access + the secret. This test passes user_ids as empty list, which is a + valid and common case. + """ + self.acl_list.pop() # remove read acl from default setup + acl_read = models.SecretACL(secret_id=self.secret_id, operation='read', + creator_only=True, + user_ids=[]) + self.acl_list.append(acl_read) + self._assert_fail_rbac(['admin', 'observer', 'creator', 'audit'], + self._invoke_on_get, + accept='notjsonaccepttype', + content_type='application/json', + user_id=self.user_id, + project_id=self.external_project_id) + def test_should_pass_decrypt_secret_private_enabled_with_read_acl(self): """Should pass authz as user has read acl for private secret. From 46184bb4b3a81e503a9e4aff4ba9ea0a66061a16 Mon Sep 17 00:00:00 2001 From: Charles Neill Date: Tue, 21 Apr 2015 15:49:20 -0500 Subject: [PATCH 5/7] Removing signing_dir directive from config The signing_dir directive defined in barbican-api-paste.ini explicitly stores Keystone's signing certificates in a known /tmp directory. This could be exploited by populating the directory with bogus certificates, potentially allowing a malicious user to generate valid tokens. Added comment explaining signing_dir, and a reasonable (commented) default. Change-Id: I15fda6863e888e3881694ab47a836eee2fb578ee Closes-Bug: #1446406 --- etc/barbican/barbican-api-paste.ini | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/etc/barbican/barbican-api-paste.ini b/etc/barbican/barbican-api-paste.ini index a8a8b512c..204f87a21 100644 --- a/etc/barbican/barbican-api-paste.ini +++ b/etc/barbican/barbican-api-paste.ini @@ -39,7 +39,6 @@ paste.filter_factory = barbican.api.middleware.context:ContextMiddleware.factory [filter:keystone_authtoken] paste.filter_factory = keystonemiddleware.auth_token:filter_factory -signing_dir = /tmp/barbican/cache #need ability to re-auth a token, thus admin url identity_uri = http://localhost:35357 admin_tenant_name = service @@ -48,6 +47,11 @@ admin_password = orange auth_version = v3.0 #delay failing perhaps to log the unauthorized request in barbican .. #delay_auth_decision = true +# signing_dir is configurable, but the default behavior of the authtoken +# middleware should be sufficient. It will create a temporary directory +# for the user the barbican process is running as. +#signing_dir = /var/barbican/keystone-signing + [filter:profile] use = egg:repoze.profile From 604c402be0e50aaa305154dc1c39fda08b7566d9 Mon Sep 17 00:00:00 2001 From: Arun Kant Date: Fri, 24 Apr 2015 09:19:25 -0700 Subject: [PATCH 6/7] Fix for missing id check in ACL count query. Fixing issue and adding unit test to cover this API specifically. It may need to be backported to Kilo as well. Closes-Bug: #1447868 Change-Id: I1d6cc4ea59ea767d08112b148fb6b085bb2c4859 --- barbican/model/repositories.py | 4 +- .../repositories/test_repositories_acls.py | 64 ++++++++++++++++--- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/barbican/model/repositories.py b/barbican/model/repositories.py index ec555b7de..1eff03d4f 100755 --- a/barbican/model/repositories.py +++ b/barbican/model/repositories.py @@ -1757,6 +1757,7 @@ class SecretACLRepo(BaseRepo): """Gets count of existing secret ACL(s) for a given secret.""" session = self.get_session(session) query = session.query(sa_func.count(models.SecretACL.id)) + query = query.filter(models.SecretACL.secret_id == secret_id) return query.scalar() def delete_acls_for_secret(self, secret, session=None): @@ -1847,11 +1848,12 @@ class ContainerACLRepo(BaseRepo): container_acl.save(session=session) - def get_count(self, secret_id, session=None): + def get_count(self, container_id, session=None): """Gets count of existing container ACL(s) for a given container.""" session = self.get_session(session) query = session.query(sa_func.count(models.ContainerACL.id)) + query = query.filter(models.ContainerACL.container_id == container_id) return query.scalar() def delete_acls_for_container(self, container, session=None): diff --git a/barbican/tests/model/repositories/test_repositories_acls.py b/barbican/tests/model/repositories/test_repositories_acls.py index f46fa7bf9..da189095c 100644 --- a/barbican/tests/model/repositories/test_repositories_acls.py +++ b/barbican/tests/model/repositories/test_repositories_acls.py @@ -45,19 +45,21 @@ class WhenTestingSecretACLRepository(database_utils.RepositoryTestCase, super(WhenTestingSecretACLRepository, self).setUp() self.acl_repo = repositories.get_secret_acl_repository() - def _create_base_secret(self): + def _create_base_secret(self, project_id=None): # Setup the secret and needed base relationship secret_repo = repositories.get_secret_repository() session = secret_repo.get_session() secret = secret_repo.create_from(models.Secret(), session=session) - project = models.Project() - project.external_id = "keystone_project_id" - project.save(session=session) + if project_id is None: # don't re-create project if it created earlier + project = models.Project() + project.external_id = "keystone_project_id" + project.save(session=session) + project_id = project.id project_secret = models.ProjectSecret() project_secret.secret_id = secret.id - project_secret.project_id = project.id + project_secret.project_id = project_id project_secret.save(session=session) session.commit() @@ -216,6 +218,28 @@ class WhenTestingSecretACLRepository(database_utils.RepositoryTestCase, self._assert_acl_users(['u1', 'u2', 'u3', 'u4'], acls, acl2.id) self._assert_acl_users(['u1', 'u2', 'u4'], acls, acl3.id) + def test_get_count(self): + session = self.acl_repo.get_session() + secret1 = self._create_base_secret() + acl1 = self.acl_repo.create_from(models.SecretACL(secret1.id, 'read', + None, ['u1', 'u2']), + session) + self.acl_repo.create_or_replace_from(secret1, acl1) + + secret2 = self._create_base_secret( + secret1.project_assocs[0].project_id) + acl21 = self.acl_repo.create_from(models.SecretACL(secret2.id, 'read', + None, ['u3', 'u4']), + session) + self.acl_repo.create_or_replace_from(secret2, acl21) + acl22 = self.acl_repo.create_from(models.SecretACL(secret2.id, 'write', + None, ['u5', 'u6']), + session) + self.acl_repo.create_or_replace_from(secret2, acl22) + + self.assertEqual(1, self.acl_repo.get_count(secret1.id)) + self.assertEqual(2, self.acl_repo.get_count(secret2.id)) + def test_delete_single_acl_and_count(self): session = self.acl_repo.get_session() @@ -273,18 +297,20 @@ class WhenTestingContainerACLRepository(database_utils.RepositoryTestCase, super(WhenTestingContainerACLRepository, self).setUp() self.acl_repo = repositories.get_container_acl_repository() - def _create_base_container(self): + def _create_base_container(self, project_id=None): # Setup the container and needed base relationship container_repo = repositories.get_container_repository() session = container_repo.get_session() - project = models.Project() - project.external_id = "keystone_project_id" - project.save(session=session) + if project_id is None: + project = models.Project() + project.external_id = "keystone_project_id" + project.save(session=session) + project_id = project.id container = models.Container() - container.project_id = project.id + container.project_id = project_id container.save(session=session) session.commit() @@ -444,6 +470,24 @@ class WhenTestingContainerACLRepository(database_utils.RepositoryTestCase, self._assert_acl_users(['u1', 'u2', 'u3', 'u4'], acls, acl2.id) self._assert_acl_users(['u1', 'u2', 'u4'], acls, acl3.id) + def test_get_count(self): + session = self.acl_repo.get_session() + container1 = self._create_base_container() + acl1 = self.acl_repo.create_from(models.ContainerACL( + container1.id, 'read', None, ['u1', 'u2']), session) + self.acl_repo.create_or_replace_from(container1, acl1) + + container2 = self._create_base_container(container1.project_id) + acl21 = self.acl_repo.create_from(models.ContainerACL( + container2.id, 'read', None, ['u3', 'u4']), session) + self.acl_repo.create_or_replace_from(container2, acl21) + acl22 = self.acl_repo.create_from(models.ContainerACL( + container2.id, 'write', None, ['u5', 'u6']), session) + self.acl_repo.create_or_replace_from(container2, acl22) + + self.assertEqual(1, self.acl_repo.get_count(container1.id)) + self.assertEqual(2, self.acl_repo.get_count(container2.id)) + def test_delete_single_acl_and_count(self): session = self.acl_repo.get_session() From e6f05febbe18a86e4e6b05acc5f4868fa3beb291 Mon Sep 17 00:00:00 2001 From: Nathan Reller Date: Tue, 28 Apr 2015 08:54:25 -0400 Subject: [PATCH 7/7] Fixed Bug for KMIP Secret Storage The KMIP secret store was incorrectly storing secrets. In some cases this resulted in extra information being stored with the keys and in other cases the key storage would fail with a 500 internal server error. This patch fixes the KMIP secret store to correctly store secrets. Change-Id: I94944a05776d366bd33d46ddb25f7129425405d0 Co-authored-by: Kaitlin Farr Closes-Bug: #1449234 (cherry picked from commit 597869880f186ce951809fe85d5d7d0610f35c4f) --- barbican/plugin/kmip_secret_store.py | 4 +--- barbican/tests/plugin/test_kmip.py | 28 ++++++++++++++++++++++++++-- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/barbican/plugin/kmip_secret_store.py b/barbican/plugin/kmip_secret_store.py index 8e0c5ca61..b040d6c86 100644 --- a/barbican/plugin/kmip_secret_store.py +++ b/barbican/plugin/kmip_secret_store.py @@ -319,9 +319,7 @@ class KMIPSecretStore(ss.SecretStoreBase): secret_features = { 'key_format_type': key_format_type, - 'key_value': { - 'bytes': normalized_secret - }, + 'key_value': normalized_secret, 'cryptographic_algorithm': algorithm_value, 'cryptographic_length': secret_dto.key_spec.bit_length } diff --git a/barbican/tests/plugin/test_kmip.py b/barbican/tests/plugin/test_kmip.py index 8e42970d6..374fca27a 100644 --- a/barbican/tests/plugin/test_kmip.py +++ b/barbican/tests/plugin/test_kmip.py @@ -349,8 +349,9 @@ class WhenTestingKMIPSecretStore(utils.BaseTestCase): def test_store_symmetric_secret_assert_called(self): key_spec = secret_store.KeySpec(secret_store.KeyAlgorithm.AES, 128, 'mode') + sym_key = utils.get_symmetric_key() secret_dto = secret_store.SecretDTO(secret_store.SecretType.SYMMETRIC, - "AAAA", + sym_key, key_spec, 'content_type', transport_key=None) @@ -360,12 +361,24 @@ class WhenTestingKMIPSecretStore(utils.BaseTestCase): template_attribute=mock.ANY, secret=mock.ANY, credential=self.credential) + _, register_call_kwargs = self.secret_store.client.register.call_args + actual_secret = register_call_kwargs.get('secret') + self.assertEqual( + 128, + actual_secret.key_block.cryptographic_length.value) + self.assertEqual( + attr.CryptographicAlgorithm(enums.CryptographicAlgorithm.AES), + actual_secret.key_block.cryptographic_algorithm) + self.assertEqual( + base64.b64decode(sym_key), + actual_secret.key_block.key_value.key_material.value) def test_store_symmetric_secret_return_value(self): key_spec = secret_store.KeySpec(secret_store.KeyAlgorithm.AES, 128, 'mode') + sym_key = utils.get_symmetric_key() secret_dto = secret_store.SecretDTO(secret_store.SecretType.SYMMETRIC, - "AAAA", + sym_key, key_spec, 'content_type', transport_key=None) @@ -387,6 +400,17 @@ class WhenTestingKMIPSecretStore(utils.BaseTestCase): template_attribute=mock.ANY, secret=mock.ANY, credential=self.credential) + _, register_call_kwargs = self.secret_store.client.register.call_args + actual_secret = register_call_kwargs.get('secret') + self.assertEqual( + 2048, + actual_secret.key_block.cryptographic_length.value) + self.assertEqual( + attr.CryptographicAlgorithm(enums.CryptographicAlgorithm.RSA), + actual_secret.key_block.cryptographic_algorithm) + self.assertEqual( + keys.get_private_key_der(), + actual_secret.key_block.key_value.key_material.value) def test_store_private_key_secret_return_value(self): key_spec = secret_store.KeySpec(secret_store.KeyAlgorithm.RSA, 2048)