Merge pull request #102 from mattsb42-aws/metastore

Fix MostRecentProvider.encryption_materials() caching bug

Author: mattsb42-aws

CHANGELOG.rst

@@ -2,6 +2,14 @@
 Changelog
 *********
 
+1.0.6 -- 2018-01-xx
+===================
+
+Bugfixes
+--------
+* Fix :class:`MostRecentProvider` bug in providing invalid cached results.
+  `#102 <https://github.com/aws/aws-dynamodb-encryption-python/pull/102>`_
+
 1.0.5 -- 2018-08-01
 ===================
 * Move the ``aws-dynamodb-encryption-python`` repository from ``awslabs`` to ``aws``.

setup.cfg

@@ -28,7 +28,7 @@ markers =
     veryslow: mark a test as being known to take a very long time to complete (order t > 60s)
     nope: mark a test as being so slow that it should only be very infrequently (order t > 30m)
     travis_isolation: mark a test that crashes Travis CI when run with other tests
-log_level=NOTSET
+log_level=DEBUG
 
 # Flake8 Configuration
 [flake8]

src/dynamodb_encryption_sdk/material_providers/aws_kms.py

@@ -214,7 +214,9 @@ def __attrs_post_init__(self):
             default_algorithm=_DEFAULT_SIGNING_ALGORITHM,
             default_key_length=_DEFAULT_SIGNING_KEY_LENGTH,
         )
-        self._regional_clients = {}  # type: Dict[Text, botocore.client.BaseClient]  # noqa pylint: disable=attribute-defined-outside-init
+        self._regional_clients = (
+            {}
+        )  # type: Dict[Text, botocore.client.BaseClient]  # noqa pylint: disable=attribute-defined-outside-init
 
     def _add_regional_client(self, region_name):
         # type: (Text) -> None

src/dynamodb_encryption_sdk/material_providers/most_recent.py

@@ -134,10 +134,8 @@ class MostRecentProvider(CryptographicMaterialsProvider):
     _material_name = attr.ib(validator=attr.validators.instance_of(six.string_types))
     _version_ttl = attr.ib(validator=attr.validators.instance_of(float))
 
-    def __init__(
-        self, provider_store, material_name, version_ttl  # type: ProviderStore  # type: Text  # type: float
-    ):  # noqa=D107
-        # type: (...) -> None
+    def __init__(self, provider_store, material_name, version_ttl):  # noqa=D107
+        # type: (ProviderStore, Text, float) -> None
         # Workaround pending resolution of attrs/mypy interaction.
         # https://github.com/python/mypy/issues/2088
         # https://github.com/python-attrs/attrs/issues/215
@@ -242,6 +240,7 @@ def _get_most_recent_version(self, allow_local):
             # We failed to acquire the lock.
             # If blocking, we will never reach this point.
             # If not blocking, we want whatever the latest local version is.
+            _LOGGER.debug("Failed to acquire lock. Returning the last cached version.")
             version = self._version
             return self._cache.get(version)
 
@@ -254,6 +253,7 @@ def _get_most_recent_version(self, allow_local):
             received_version = self._provider_store.version_from_material_description(provider._material_description)
             # TODO: ^ should we promote material description from hidden?
 
+            _LOGGER.debug("Caching materials provider version %d", received_version)
             self._version = received_version
             self._last_updated = time.time()
             self._cache.put(received_version, provider)
@@ -271,17 +271,22 @@ def encryption_materials(self, encryption_context):
         """
         ttl_action = self._ttl_action()
 
+        provider = None
+
         if ttl_action is TtlActions.LIVE:
             try:
-                return self._cache.get(self._version)
+                _LOGGER.debug("Looking in cache for materials provider version %d", self._version)
+                provider = self._cache.get(self._version)
             except KeyError:
+                _LOGGER.debug("Provider not found in cache")
                 ttl_action = TtlActions.EXPIRED
 
-        # Just get the latest local version if we cannot acquire the lock.
-        # Otherwise, block until we can acquire the lock.
-        allow_local = bool(ttl_action is TtlActions.GRACE_PERIOD)
+        if provider is None:
+            # Just get the latest local version if we cannot acquire the lock.
+            # Otherwise, block until we can acquire the lock.
+            allow_local = bool(ttl_action is TtlActions.GRACE_PERIOD)
 
-        provider = self._get_most_recent_version(allow_local)
+            provider = self._get_most_recent_version(allow_local)
 
         return provider.encryption_materials(encryption_context)
 

src/dynamodb_encryption_sdk/material_providers/store/__init__.py

@@ -17,7 +17,7 @@
 
 from dynamodb_encryption_sdk.exceptions import NoKnownVersionError
 from dynamodb_encryption_sdk.material_providers import (  # noqa pylint: disable=unused-import
-    CryptographicMaterialsProvider
+    CryptographicMaterialsProvider,
 )
 
 try:  # Python 3.5.0 and 3.5.1 have incompatible typing modules

test/functional/functional_test_utils.py

@@ -13,6 +13,7 @@
 """Helper tools for use in tests."""
 from __future__ import division
 
+import base64
 import copy
 import itertools
 import logging
@@ -34,6 +35,7 @@
 from dynamodb_encryption_sdk.identifiers import CryptoAction
 from dynamodb_encryption_sdk.internal.identifiers import ReservedAttributes
 from dynamodb_encryption_sdk.material_providers.static import StaticCryptographicMaterialsProvider
+from dynamodb_encryption_sdk.material_providers.store.meta import MetaStore
 from dynamodb_encryption_sdk.material_providers.wrapped import WrappedCryptographicMaterialsProvider
 from dynamodb_encryption_sdk.materials.raw import RawDecryptionMaterials, RawEncryptionMaterials
 from dynamodb_encryption_sdk.structures import AttributeActions
@@ -645,3 +647,25 @@ def client_cycle_batch_items_check_paginators(
     e_scan_result = e_client.scan(TableName=table_name, ConsistentRead=True)
     assert not raw_scan_result["Items"]
     assert not e_scan_result["Items"]
+
+
+def build_metastore():
+    client = boto3.client("dynamodb", region_name="us-west-2")
+    table_name = base64.urlsafe_b64encode(os.urandom(32)).decode("utf-8").replace("=", ".")
+
+    MetaStore.create_table(client, table_name, 1, 1)
+    waiter = client.get_waiter("table_exists")
+    waiter.wait(TableName=table_name)
+
+    table = boto3.resource("dynamodb", region_name="us-west-2").Table(table_name)
+    yield MetaStore(table, build_static_jce_cmp("AES", 256, "HmacSHA256", 256))
+
+    client.delete_table(TableName=table_name)
+    waiter = client.get_waiter("table_not_exists")
+    waiter.wait(TableName=table_name)
+
+
+@pytest.fixture
+def mock_metastore():
+    with mock_dynamodb2():
+        yield next(build_metastore())

test/functional/material_providers/store/test_meta.py

@@ -18,20 +18,52 @@
 import pytest
 from moto import mock_dynamodb2
 
-from dynamodb_encryption_sdk.material_providers.store.meta import MetaStore
+from dynamodb_encryption_sdk.exceptions import NoKnownVersionError
+from dynamodb_encryption_sdk.material_providers.store.meta import MetaStore, MetaStoreAttributeNames
+from dynamodb_encryption_sdk.material_providers.wrapped import WrappedCryptographicMaterialsProvider
+
+from ...functional_test_utils import build_static_jce_cmp, mock_metastore
 
 pytestmark = [pytest.mark.functional, pytest.mark.local]
 
 
-@mock_dynamodb2
-def test_create_table():
-    client = boto3.client("dynamodb", region_name="us-west-2")
-    table_name = base64.b64encode(os.urandom(32)).decode("utf-8")
+def test_create_table(mock_metastore):
+    # type: (MetaStore) -> None
+    assert mock_metastore._table.key_schema == [
+        {"AttributeName": MetaStoreAttributeNames.PARTITION.value, "KeyType": "HASH"},
+        {"AttributeName": MetaStoreAttributeNames.SORT.value, "KeyType": "RANGE"},
+    ]
+    assert mock_metastore._table.attribute_definitions == [
+        {"AttributeName": MetaStoreAttributeNames.PARTITION.value, "AttributeType": "S"},
+        {"AttributeName": MetaStoreAttributeNames.SORT.value, "AttributeType": "N"},
+    ]
+
+
+def test_max_version_empty(mock_metastore):
+    # type: (MetaStore) -> None
+    with pytest.raises(NoKnownVersionError) as excinfo:
+        mock_metastore.max_version("example_name")
+
+    excinfo.match(r"No known version for name: ")
+
+
+def test_max_version_exists(mock_metastore):
+    # type: (MetaStore) -> None
+    mock_metastore.get_or_create_provider("example_name", 2)
+    mock_metastore.get_or_create_provider("example_name", 5)
+
+    test = mock_metastore.max_version("example_name")
+
+    assert test == 5
+
+
+@pytest.mark.xfail(strict=True)
+def test_version_from_material_description():
+    assert False
+
 
-    MetaStore.create_table(client, table_name, 1, 1)
-    waiter = client.get_waiter("table_exists")
-    waiter.wait(TableName=table_name)
+def test_provider(mock_metastore):
+    # type: (MetaStore) -> None
+    test = mock_metastore.provider("example_name")
 
-    client.delete_table(TableName=table_name)
-    waiter = client.get_waiter("table_not_exists")
-    waiter.wait(TableName=table_name)
+    assert isinstance(test, WrappedCryptographicMaterialsProvider)

test/functional/material_providers/test_most_recent.py

@@ -11,15 +11,60 @@
 # ANY KIND, either express or implied. See the License for the specific
 # language governing permissions and limitations under the License.
 """Functional tests for ``dynamodb_encryption_sdk.material_providers.most_recent``."""
+from collections import defaultdict
+
 import pytest
 from mock import MagicMock, sentinel
 
+from dynamodb_encryption_sdk.exceptions import NoKnownVersionError
+from dynamodb_encryption_sdk.material_providers import CryptographicMaterialsProvider
 from dynamodb_encryption_sdk.material_providers.most_recent import MostRecentProvider
 from dynamodb_encryption_sdk.material_providers.store import ProviderStore
 
 pytestmark = [pytest.mark.functional, pytest.mark.local]
 
 
+class SentinelCryptoMaterialsProvider(CryptographicMaterialsProvider):
+    def __init__(self, name, version):
+        self.name = name
+        self.version = version
+        self._material_description = version
+        self.provider_calls = []
+
+    def encryption_materials(self, encryption_context):
+        self.provider_calls.append(("encryption_materials", encryption_context))
+        return getattr(sentinel, "{name}_{version}_encryption".format(name=self.name, version=self.version))
+
+    def decryption_materials(self, encryption_context):
+        self.provider_calls.append(("decryption_materials", encryption_context))
+        return getattr(sentinel, "{name}_{version}_decryption".format(name=self.name, version=self.version))
+
+
+class MockProviderStore(ProviderStore):
+    def __init__(self):
+        self.provider_calls = []
+        self._providers = defaultdict(dict)
+
+    def get_or_create_provider(self, material_name, version):
+        self.provider_calls.append(("get_or_create_provider", material_name, version))
+        try:
+            return self._providers[material_name][version]
+        except KeyError:
+            self._providers[material_name][version] = SentinelCryptoMaterialsProvider(material_name, version)
+        return self._providers[material_name][version]
+
+    def max_version(self, material_name):
+        self.provider_calls.append(("max_version", material_name))
+        try:
+            return sorted(self._providers[material_name].keys())[-1]
+        except IndexError:
+            raise NoKnownVersionError('No known version for name: "{}"'.format(material_name))
+
+    def version_from_material_description(self, material_description):
+        self.provider_calls.append(("version_from_material_description", material_description))
+        return material_description
+
+
 def test_failed_lock_acquisition():
     store = MagicMock(__class__=ProviderStore)
     provider = MostRecentProvider(provider_store=store, material_name="my material", version_ttl=10.0)
@@ -31,3 +76,57 @@ def test_failed_lock_acquisition():
 
     assert test is sentinel.nine
     assert not store.mock_calls
+
+
+def test_encryption_materials_cache_use():
+    store = MockProviderStore()
+    name = "material"
+    provider = MostRecentProvider(provider_store=store, material_name=name, version_ttl=10.0)
+
+    test1 = provider.encryption_materials(sentinel.encryption_context_1)
+    assert test1 is sentinel.material_0_encryption
+
+    assert provider._version == 0
+    assert len(provider._cache._cache) == 1
+
+    expected_calls = [
+        ("max_version", name),
+        ("get_or_create_provider", name, 0),
+        ("version_from_material_description", 0),
+    ]
+
+    assert store.provider_calls == expected_calls
+
+    test2 = provider.encryption_materials(sentinel.encryption_context_1)
+    assert test2 is sentinel.material_0_encryption
+
+    assert provider._version == 0
+    assert len(provider._cache._cache) == 1
+
+    assert store.provider_calls == expected_calls
+
+
+def test_decryption_materials_cache_use():
+    store = MockProviderStore()
+    name = "material"
+    provider = MostRecentProvider(provider_store=store, material_name=name, version_ttl=10.0)
+
+    context = MagicMock(material_description=0)
+
+    test1 = provider.decryption_materials(context)
+    assert test1 is sentinel.material_0_decryption
+
+    assert len(provider._cache._cache) == 1
+
+    expected_calls = [("version_from_material_description", 0), ("get_or_create_provider", name, 0)]
+
+    assert store.provider_calls == expected_calls
+
+    test2 = provider.decryption_materials(context)
+    assert test2 is sentinel.material_0_decryption
+
+    assert len(provider._cache._cache) == 1
+
+    expected_calls.append(("version_from_material_description", 0))
+
+    assert store.provider_calls == expected_calls

test/integration/integration_test_utils.py

@@ -65,3 +65,8 @@ def ddb_table_name():
                 " for integration tests to run"
             ).format(DDB_TABLE_NAME)
         )
+
+
+@pytest.fixture
+def temp_metastore():
+    yield next(functional_test_utils.build_metastore())