From 91de02be3497223cbe8f6f2963d43aae50b276be Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Thu, 22 Mar 2018 14:45:01 +0000 Subject: [PATCH] Fix secret_uuid used to query Barbican's Secrets API This is to fix secrets_manager.SecretsManager.get method which is passing in the secret reference to Barbican directly for GET /secrets/{uuid} [0] causing Barbican to raise a ValueError exception when it attempts to validate that {secret_uuid} is in fact a UUID. The fix is to extract the secret_uuid from the secret_ref returned by Barbican before querying the GET /secrets/{uuid} API. [0] https://docs.openstack.org/barbican/latest/api/reference/secrets.html#get-v1-secrets-uuid Change-Id: I4db317e3ba12b4268df5b84b79be8da1da5ac2ba --- deckhand/barbican/driver.py | 3 +- deckhand/conf/config.py | 9 +- deckhand/control/revision_documents.py | 3 +- deckhand/db/sqlalchemy/api.py | 4 +- deckhand/engine/secrets_manager.py | 114 +++++++++++++----- .../tests/unit/engine/test_secrets_manager.py | 60 +++++++-- 6 files changed, 145 insertions(+), 48 deletions(-) diff --git a/deckhand/barbican/driver.py b/deckhand/barbican/driver.py index 6c3bec7f..8b611d5d 100644 --- a/deckhand/barbican/driver.py +++ b/deckhand/barbican/driver.py @@ -54,6 +54,7 @@ class BarbicanDriver(object): return self.barbicanclient.call("secrets.get", secret_ref) except (barbicanclient.exceptions.HTTPAuthError, barbicanclient.exceptions.HTTPClientError, - barbicanclient.exceptions.HTTPServerError) as e: + barbicanclient.exceptions.HTTPServerError, + ValueError) as e: LOG.exception(str(e)) raise errors.BarbicanException(details=str(e)) diff --git a/deckhand/conf/config.py b/deckhand/conf/config.py index d58f79d9..6eb8aa27 100644 --- a/deckhand/conf/config.py +++ b/deckhand/conf/config.py @@ -26,15 +26,16 @@ Barbican options for allowing Deckhand to communicate with Barbican. """) barbican_opts = [ + # TODO(fmontei): Drop these options and related group once Keystone + # endpoint lookup is used instead. cfg.StrOpt( 'api_endpoint', - default='http://127.0.0.1/key-manager', sample_default='http://barbican.example.org:9311/', help='URL override for the Barbican API endpoint.'), ] -context_opts = [ +default_opts = [ cfg.BoolOpt('allow_anonymous_access', default=False, help=""" Allow limited access to unauthenticated users. @@ -58,13 +59,13 @@ Possible values: def register_opts(conf): conf.register_group(barbican_group) conf.register_opts(barbican_opts, group=barbican_group) - conf.register_opts(context_opts) + conf.register_opts(default_opts) ks_loading.register_auth_conf_options(conf, group=barbican_group.name) ks_loading.register_session_conf_options(conf, group=barbican_group.name) def list_opts(): - opts = {None: context_opts, + opts = {None: default_opts, barbican_group: barbican_opts + ks_loading.get_session_conf_options() + ks_loading.get_auth_common_conf_options() + diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index 98d2680f..7df9e90a 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -123,7 +123,8 @@ class RenderedDocumentsResource(api_base.BaseResource): except (errors.LayeringPolicyNotFound, errors.SubstitutionSourceNotFound) as e: raise falcon.HTTPConflict(description=e.format_message()) - except errors.UnknownSubstitutionError as e: + except (errors.DeckhandException, + errors.UnknownSubstitutionError) as e: raise falcon.HTTPInternalServerError( description=e.format_message()) diff --git a/deckhand/db/sqlalchemy/api.py b/deckhand/db/sqlalchemy/api.py index 47b502f7..5f789167 100644 --- a/deckhand/db/sqlalchemy/api.py +++ b/deckhand/db/sqlalchemy/api.py @@ -1029,8 +1029,8 @@ def _get_validation_policies_for_revision(revision_id, session=None): schema=types.VALIDATION_POLICY_SCHEMA) if not validation_policies: # Otherwise return early. - LOG.info('Failed to find a ValidationPolicy for revision ID %s.' - 'Only the "%s" results will be included in the response.', + LOG.debug('Failed to find a ValidationPolicy for revision ID %s. ' + 'Only the "%s" results will be included in the response.', revision_id, types.DECKHAND_SCHEMA_VALIDATION) validation_policies = [] diff --git a/deckhand/engine/secrets_manager.py b/deckhand/engine/secrets_manager.py index 9101a955..a3a62d65 100644 --- a/deckhand/engine/secrets_manager.py +++ b/deckhand/engine/secrets_manager.py @@ -17,6 +17,7 @@ import re from oslo_config import cfg from oslo_log import log as logging +from oslo_utils import uuidutils import six from deckhand.barbican import driver @@ -39,6 +40,9 @@ class SecretsManager(object): barbican_driver = driver.BarbicanDriver() + _url_re = re.compile(r'http[s]?://(?:[a-zA-Z]|[0-9]|[$-_@.&+]|[!*\(\),]|' + '(?:%[0-9a-fA-F][0-9a-fA-F]))+') + @classmethod def create(cls, secret_doc): """Securely store secrets contained in ``secret_doc``. @@ -84,10 +88,52 @@ class SecretsManager(object): return created_secret + @classmethod + def _is_barbican_ref(cls, secret_ref): + return ( + isinstance(secret_ref, six.string_types) and + cls._url_re.match(secret_ref) and 'secrets' in secret_ref + ) + @classmethod def get(cls, secret_ref): - secret = cls.barbican_driver.get_secret(secret_ref=secret_ref) + """Return a secret payload from Barbican if ``secret_ref`` is a + Barbican secret reference or else return ``secret_ref``. + + Extracts {secret_uuid} from a secret reference and queries Barbican's + Secrets API with it. + + :param str secret_ref: + + * String formatted like: + "https://{barbican_host}/v1/secrets/{secret_uuid}" + which results in a Barbican query. + * Any other string which results in a pass-through. + + :returns: Secret payload from Barbican or ``secret_ref``. + """ + + # TODO(fmontei): Query Keystone service catalog for Barbican endpoint + # and cache it if Keystone is enabled. For now, it should be enough + # to check that ``secret_ref`` is a valid URL, contains 'secrets' + # substring, ends in a UUID and that the source document from which + # the reference is extracted is encrypted. + if cls._is_barbican_ref(secret_ref): + LOG.debug('Resolving Barbican secret using source document ' + 'reference...') + try: + secret_uuid = secret_ref.split('/')[-1] + except Exception: + secret_uuid = None + if not uuidutils.is_uuid_like(secret_uuid): + return secret_ref + else: + return secret_ref + + # TODO(fmontei): Need to avoid this call if Keystone is disabled. + secret = cls.barbican_driver.get_secret(secret_ref=secret_uuid) payload = secret.payload + LOG.debug('Successfully retrieved Barbican secret using reference.') return payload @classmethod @@ -118,6 +164,10 @@ class SecretsSubstitution(object): __slots__ = ('_fail_on_missing_sub_src', '_substitution_sources') + _insecure_reg_exps = ( + re.compile(r'^.* is not of type .+$'), + ) + @staticmethod def sanitize_potential_secrets(error, document): """Sanitize all secret data that may have been substituted into the @@ -133,15 +183,13 @@ class SecretsSubstitution(object): if not document.substitutions and not document.is_encrypted: return document - insecure_reg_exps = [ - re.compile(r'^.* is not of type .+$') - ] to_sanitize = copy.deepcopy(document) safe_message = 'Sanitized to avoid exposing secret.' # Sanitize any secrets contained in `error.message` referentially. if error.message and any( - r.match(error.message) for r in insecure_reg_exps): + r.match(error.message) + for r in SecretsSubstitution._insecure_reg_exps): error.message = safe_message # Sanitize any secrets extracted from the document itself. @@ -178,10 +226,6 @@ class SecretsSubstitution(object): self._substitution_sources.setdefault( (document.schema, document.name), document) - def _is_barbican_ref(self, src_secret): - return (isinstance(src_secret, six.string_types) and - src_secret.startswith(CONF.barbican.api_endpoint)) - def substitute_all(self, documents): """Substitute all documents that have a `metadata.substitutions` field. @@ -250,11 +294,23 @@ class SecretsSubstitution(object): else: src_secret = src_doc.get('data') - # Check if src_secret is Barbican secret reference. - if self._is_barbican_ref(src_secret): - LOG.debug('Resolving Barbican reference for %s.', - src_secret) - src_secret = SecretsManager.get(src_secret) + # If the document has storagePolicy == encrypted then resolve + # the Barbican reference into the actual secret. + if src_doc.is_encrypted: + try: + src_secret = SecretsManager.get(src_secret) + except errors.BarbicanException as e: + LOG.error( + 'Failed to resolve a Barbican reference for ' + 'substitution source document [%s] %s referenced ' + 'in document [%s] %s. Details: %s', src_schema, + src_name, document.schema, document.name, + e.format_message()) + raise errors.UnknownSubstitutionError( + src_schema=src_schema, src_name=src_name, + document_name=document.name, + document_schema=document.schema, + details=e.format_message()) dest_path = sub['dest']['path'] dest_pattern = sub['dest'].get('pattern', None) @@ -263,6 +319,7 @@ class SecretsSubstitution(object): 'into dest_path=%s, dest_pattern=%s', src_schema, src_name, src_path, dest_path, dest_pattern) try: + exc_message = '' substituted_data = utils.jsonpath_replace( document['data'], src_secret, dest_path, dest_pattern) if (isinstance(document['data'], dict) @@ -271,35 +328,26 @@ class SecretsSubstitution(object): elif substituted_data: document['data'] = substituted_data else: - message = ( + exc_message = ( 'Failed to create JSON path "%s" in the ' 'destination document [%s] %s. No data was ' 'substituted.', dest_path, document.schema, document.name) - LOG.error(message) - raise errors.UnknownSubstitutionError(details=message) - except errors.BarbicanException as e: - LOG.error('Failed to resolve a Barbican reference for ' - 'substitution source document [%s] %s referenced' - ' in document [%s] %s. Details: %s', src_schema, - src_name, document.schema, document.name, - e.format_message()) - raise errors.UnknownSubstitutionError( - src_schema=src_schema, src_name=src_name, - document_name=document.name, - document_schema=document.schema, - details=e.format_message()) + LOG.error(exc_message) except Exception as e: LOG.error('Unexpected exception occurred while attempting ' 'substitution using source document [%s] %s ' 'referenced in [%s] %s. Details: %s', src_schema, src_name, document.schema, document.name, six.text_type(e)) - raise errors.UnknownSubstitutionError( - src_schema=src_schema, src_name=src_name, - document_name=document.name, - document_schema=document.schema, - details=six.text_type(e)) + exc_message = six.text_type(e) + finally: + if exc_message: + raise errors.UnknownSubstitutionError( + src_schema=src_schema, src_name=src_name, + document_name=document.name, + document_schema=document.schema, + details=exc_message) yield document diff --git a/deckhand/tests/unit/engine/test_secrets_manager.py b/deckhand/tests/unit/engine/test_secrets_manager.py index 07c276dd..e6915f44 100644 --- a/deckhand/tests/unit/engine/test_secrets_manager.py +++ b/deckhand/tests/unit/engine/test_secrets_manager.py @@ -16,6 +16,7 @@ import copy import yaml import mock +from oslo_utils import uuidutils from deckhand.db.sqlalchemy import api as db_api from deckhand.engine import secrets_manager @@ -30,51 +31,96 @@ class TestSecretsManager(test_base.TestDbBase): super(TestSecretsManager, self).setUp() self.mock_barbican_driver = self.patchobject( secrets_manager.SecretsManager, 'barbican_driver') - self.secret_ref = 'https://path/to/fake_secret' + self.secret_ref = "https://barbican_host/v1/secrets/{secret_uuid}"\ + .format(**{'secret_uuid': uuidutils.generate_uuid()}) self.mock_barbican_driver.create_secret.return_value = ( {'secret_ref': self.secret_ref}) - - self.secrets_manager = secrets_manager.SecretsManager() self.factory = factories.DocumentSecretFactory() def _test_create_secret(self, encryption_type, secret_type): secret_data = test_utils.rand_password() secret_doc = self.factory.gen_test( secret_type, encryption_type, secret_data) + payload = secret_doc['data'] + self.mock_barbican_driver.get_secret.return_value = ( + mock.Mock(payload=payload)) - created_secret = self.secrets_manager.create(secret_doc) + created_secret = secrets_manager.SecretsManager.create(secret_doc) if encryption_type == 'cleartext': self.assertEqual(secret_data, created_secret) elif encryption_type == 'encrypted': expected_kwargs = { 'name': secret_doc['metadata']['name'], - 'secret_type': ('private' if secret_type == 'CertificateKey' - else secret_type.lower()), - 'payload': secret_doc['data'] + 'secret_type': secrets_manager.SecretsManager._get_secret_type( + 'deckhand/' + secret_type), + 'payload': payload } self.mock_barbican_driver.create_secret.assert_called_once_with( **expected_kwargs) self.assertEqual(self.secret_ref, created_secret) + return created_secret, payload + def test_create_cleartext_certificate(self): self._test_create_secret('cleartext', 'Certificate') + def test_create_cleartext_certificate_authority(self): + self._test_create_secret('cleartext', 'CertificateAuthority') + + def test_create_cleartext_certificate_authority_key(self): + self._test_create_secret('cleartext', 'CertificateAuthorityKey') + def test_create_cleartext_certificate_key(self): self._test_create_secret('cleartext', 'CertificateKey') def test_create_cleartext_passphrase(self): self._test_create_secret('cleartext', 'Passphrase') + def test_create_cleartext_private_key(self): + self._test_create_secret('cleartext', 'PrivateKey') + def test_create_encrypted_certificate(self): self._test_create_secret('encrypted', 'Certificate') + def test_create_encrypted_certificate_authority(self): + self._test_create_secret('encrypted', 'CertificateAuthority') + + def test_create_encrypted_certificate_authority_key(self): + self._test_create_secret('encrypted', 'CertificateAuthorityKey') + def test_create_encrypted_certificate_key(self): self._test_create_secret('encrypted', 'CertificateKey') def test_create_encrypted_passphrase(self): self._test_create_secret('encrypted', 'Passphrase') + def test_create_encrypted_private_key(self): + self._test_create_secret('encrypted', 'PrivateKey') + + def test_retrieve_barbican_secret(self): + secret_ref, expected_secret = self._test_create_secret( + 'encrypted', 'Certificate') + secret_uuid = secret_ref.split('/')[-1] + secret_payload = secrets_manager.SecretsManager.get(secret_ref) + + self.assertEqual(expected_secret, secret_payload) + self.mock_barbican_driver.get_secret.assert_called_once_with( + secret_ref=secret_uuid) + + +class TestSecretsManagerNegative(test_base.TestDbBase): + + def test_retrieve_barbican_secret_bad_reference_raises_exc(self): + """Verify that passing in an invalid reference to + ``SecretsManager.get`` returns gracefully with the original argument. + """ + bad_refs = ('', 'a/b', 'a/12345', 'a/%s/b' % uuidutils.generate_uuid(), + 12345, False, None, {}, []) + for bad_ref in bad_refs: + self.assertEqual( + bad_ref, secrets_manager.SecretsManager.get(bad_ref)) + class TestSecretsSubstitution(test_base.TestDbBase):