From 4ccb4368cefb4339247e8e6e3aed91bbf90f6cb2 Mon Sep 17 00:00:00 2001 From: Phil Sphicas Date: Thu, 9 Jan 2020 06:55:16 +0000 Subject: [PATCH] Barbican driver simplification Under some circumstances, the payloads retrieved from Barbican do not match what was stored. This primarily affects surrounding whitespace[0], but the implications for passphrases are significant, and even for PEM encoded data, a difference in whitespace in a configmap is enough to trigger a chart upgrade. In general, the effort to align Deckhand document types with Barbican secret types adds complexity without tangible benefit. Barbican does no enforcement of the contents of the data, and if it did, that could lead to further incompatibilities. This change uses the 'opaque' secret type for all secret document types. Before storage (or caching), the payload is serialized using `repr`, and base64 encoded. Upon retrieval, the payload is base64 decoded and parsed back into an object with `ast.literal_eval`. [0]: https://storyboard.openstack.org/#!/story/2007017 Change-Id: I9c2f3427f52a87aad718f95160cf688db35e1b83 --- deckhand/barbican/driver.py | 82 +++---------------- .../gabbits/document-crud-secret.yaml | 3 +- .../document-substitution-secret-generic.yaml | 4 +- .../gabbits/document-substitution-secret.yaml | 14 ++-- .../gabbits/revision-delete-all-secrets.yaml | 3 +- .../tests/unit/engine/test_secrets_manager.py | 11 ++- 6 files changed, 30 insertions(+), 87 deletions(-) diff --git a/deckhand/barbican/driver.py b/deckhand/barbican/driver.py index 8a133d71..a3fde93a 100644 --- a/deckhand/barbican/driver.py +++ b/deckhand/barbican/driver.py @@ -20,7 +20,6 @@ from oslo_config import cfg from oslo_log import log as logging from oslo_serialization import base64 from oslo_utils import excutils -import six from deckhand.barbican import cache from deckhand.barbican import client_wrapper @@ -36,67 +35,15 @@ class BarbicanDriver(object): def __init__(self): self.barbicanclient = client_wrapper.BarbicanClientWrapper() - @staticmethod - def _get_secret_type(schema): - """Get the Barbican secret type based on the following mapping: - - ``deckhand/Certificate/v1`` => certificate - ``deckhand/CertificateKey/v1`` => private - ``deckhand/CertificateAuthority/v1`` => certificate - ``deckhand/CertificateAuthorityKey/v1`` => private - ``deckhand/Passphrase/v1`` => passphrase - ``deckhand/PrivateKey/v1`` => private - ``deckhand/PublicKey/v1`` => public - Other => passphrase - - :param schema: The document's schema. - :returns: The value corresponding to the mapping above. - """ - parts = schema.split('/') - if len(parts) == 3: - namespace, kind, _ = parts - elif len(parts) == 2: - namespace, kind = parts - else: - raise ValueError( - 'Schema %s must consist of namespace/kind/version.' % schema) - - is_generic = ( - '/'.join([namespace, kind]) not in types.DOCUMENT_SECRET_TYPES - ) - - # If the document kind is not a built-in secret type, then default to - # 'passphrase'. - if is_generic: - LOG.debug('Defaulting to secret_type="passphrase" for generic ' - 'document schema %s.', schema) - return 'passphrase' - - kind = kind.lower() - - if kind in [ - 'certificateauthoritykey', 'certificatekey', 'privatekey' - ]: - return 'private' - elif kind == 'certificateauthority': - return 'certificate' - elif kind == 'publickey': - return 'public' - # NOTE(felipemonteiro): This branch below handles certificate and - # passphrase, both of which are supported secret types in Barbican. - return kind - def _base64_encode_payload(self, secret_doc): """Ensures secret document payload is compatible with Barbican.""" payload = secret_doc.data - secret_type = self._get_secret_type(secret_doc.schema) - - # NOTE(felipemonteiro): The logic for the 2 conditions below is - # enforced from Barbican's Python client. Some pre-processing and - # transformation is needed to make Barbican work with non-compatible - # formats. - if not payload and payload is not False: + secret_type = None + # Explicitly list the "empty" payloads we are refusing to store. + # We don't use ``if not payload`` because that would not encrypt + # and store something like ``data: !!int 0`` + if payload in ('', {}, [], None): # There is no point in even bothering to encrypt an empty # body, which just leads to needless overhead, so return # early. @@ -104,19 +51,14 @@ class BarbicanDriver(object): 'Deckhand will not encrypt document [%s, %s] %s.', secret_doc.schema, secret_doc.layer, secret_doc.name) secret_doc.storage_policy = types.CLEARTEXT - elif not isinstance( - payload, (six.text_type, six.binary_type)): - LOG.debug('Forcibly setting secret_type=opaque and ' - 'base64-encoding non-string payload for ' - 'document [%s, %s] %s.', secret_doc.schema, - secret_doc.layer, secret_doc.name) - # NOTE(felipemonteiro): base64-encoding the non-string payload is - # done for serialization purposes, not for security purposes. - # 'opaque' is used to avoid Barbican doing any further - # serialization server-side. + else: + LOG.debug('Setting secret_type=opaque and ' + 'base64-encoding payload of type %s for ' + 'document [%s, %s] %s.', type(payload), + secret_doc.schema, secret_doc.layer, secret_doc.name) secret_type = 'opaque' try: - payload = base64.encode_as_text(six.text_type(payload)) + payload = base64.encode_as_text(repr(payload)) except Exception: message = ('Failed to base64-encode payload of type %s ' 'for Barbican storage.', type(payload)) @@ -222,7 +164,7 @@ class BarbicanDriver(object): payload = secret.payload if secret.secret_type == 'opaque': - LOG.debug('Forcibly base64-decoding original non-string payload ' + LOG.debug('Base64-decoding original payload ' 'for document [%s, %s] %s.', *src_doc.meta) secret = self._base64_decode_payload(payload) else: diff --git a/deckhand/tests/integration/gabbits/document-crud-secret.yaml b/deckhand/tests/integration/gabbits/document-crud-secret.yaml index 5fd6be20..c94a4de2 100644 --- a/deckhand/tests/integration/gabbits/document-crud-secret.yaml +++ b/deckhand/tests/integration/gabbits/document-crud-secret.yaml @@ -65,7 +65,8 @@ tests: response_headers: content-type: application/octet-stream; charset=UTF-8 response_strings: - - not-a-real-password + # base64.b64encode(repr("not-a-real-password")) + - J25vdC1hLXJlYWwtcGFzc3dvcmQn - name: verify_revision_documents_returns_secret_ref desc: Verify that the documents for the created revision returns the secret ref. diff --git a/deckhand/tests/integration/gabbits/document-substitution-secret-generic.yaml b/deckhand/tests/integration/gabbits/document-substitution-secret-generic.yaml index f5ffb2c1..b317a9df 100644 --- a/deckhand/tests/integration/gabbits/document-substitution-secret-generic.yaml +++ b/deckhand/tests/integration/gabbits/document-substitution-secret-generic.yaml @@ -82,7 +82,7 @@ tests: $.[0].data.`split(:, 0, 1)` + "://" + $.[0].data.`split(/, 2, 3)`: $ENVIRON['TEST_BARBICAN_URL'] - name: verify_generic_secret_created_in_barbican - desc: Validate that the generic secret gets stored with secret_type passphrase. + desc: Validate that the generic secret gets stored with secret_type opaque. GET: $ENVIRON['TEST_BARBICAN_URL']/v1/secrets/$RESPONSE['$.[0].data.`split(/, 5, -1)`'] status: 200 request_headers: @@ -93,7 +93,7 @@ tests: $.status: ACTIVE $.name: example-armada-cert # Default type for documents with generic schema. - $.secret_type: passphrase + $.secret_type: opaque - name: verify_secret_payload_in_destination_document desc: Verify secret payload is injected in destination document as well as example-armada-cert. diff --git a/deckhand/tests/integration/gabbits/document-substitution-secret.yaml b/deckhand/tests/integration/gabbits/document-substitution-secret.yaml index 44946535..fcc62a38 100644 --- a/deckhand/tests/integration/gabbits/document-substitution-secret.yaml +++ b/deckhand/tests/integration/gabbits/document-substitution-secret.yaml @@ -222,13 +222,13 @@ tests: - example-private-key - example-public-key $.secrets[*].secret_type: - - certificate - - private - - certificate - - private - - passphrase - - private - - public + - opaque + - opaque + - opaque + - opaque + - opaque + - opaque + - opaque - name: verify_secret_payload_in_destination_document desc: | diff --git a/deckhand/tests/integration/gabbits/revision-delete-all-secrets.yaml b/deckhand/tests/integration/gabbits/revision-delete-all-secrets.yaml index 2b9b1768..81dbdb8f 100644 --- a/deckhand/tests/integration/gabbits/revision-delete-all-secrets.yaml +++ b/deckhand/tests/integration/gabbits/revision-delete-all-secrets.yaml @@ -62,7 +62,8 @@ tests: response_headers: content-type: application/octet-stream; charset=UTF-8 response_strings: - - not-a-real-password + # base64.b64encode(repr("not-a-real-password")) + - J25vdC1hLXJlYWwtcGFzc3dvcmQn - name: delete_all_revisions desc: Delete all revisions from Deckhand, which should delete all secrets. diff --git a/deckhand/tests/unit/engine/test_secrets_manager.py b/deckhand/tests/unit/engine/test_secrets_manager.py index 11306a8f..6f1e45a3 100644 --- a/deckhand/tests/unit/engine/test_secrets_manager.py +++ b/deckhand/tests/unit/engine/test_secrets_manager.py @@ -18,10 +18,8 @@ import yaml import mock from oslo_serialization import base64 from oslo_utils import uuidutils -import six import testtools -from deckhand.barbican import driver from deckhand.common import document as document_wrapper from deckhand.engine import secrets_manager from deckhand import errors @@ -64,9 +62,8 @@ class TestSecretsManager(test_base.DeckhandWithDBTestCase): elif encryption_type == 'encrypted': expected_kwargs = { 'name': secret_doc['metadata']['name'], - 'secret_type': driver.BarbicanDriver._get_secret_type( - 'deckhand/' + secret_type), - 'payload': payload + 'secret_type': 'opaque', + 'payload': base64.encode_as_text(repr(payload)) } self.assertEqual(self.secret_ref, secret_ref) self.mock_barbicanclient.call.assert_called_once_with( @@ -123,6 +120,8 @@ class TestSecretsManager(test_base.DeckhandWithDBTestCase): 'secrets.get', secret_ref) def test_empty_payload_skips_encryption(self): + # NOTE: Not testing for the `None` case here, because gen_test + # factory method interprets `None` as "generate a passphrase" for empty_payload in ('', {}, []): secret_doc = self.factory.gen_test( 'Certificate', 'encrypted', empty_payload) @@ -142,7 +141,7 @@ class TestSecretsManager(test_base.DeckhandWithDBTestCase): secret_doc = self.factory.gen_test( 'Certificate', 'encrypted', payload) - expected_payload = base64.encode_as_text(six.text_type({'foo': 'bar'})) + expected_payload = base64.encode_as_text(repr({'foo': 'bar'})) expected_kwargs = { 'name': secret_doc['metadata']['name'], 'secret_type': 'opaque',