Skip to content

Keyring materials #163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jul 11, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
boto3>=1.4.4
cryptography>=1.8.1
attrs>=17.4.0
attrs>=19.1.0
wrapt>=1.10.11
10 changes: 10 additions & 0 deletions src/aws_encryption_sdk/identifiers.py
Original file line number Diff line number Diff line change
Expand Up @@ -328,3 +328,13 @@ class ContentAADString(Enum):
FRAME_STRING_ID = b"AWSKMSEncryptionClient Frame"
FINAL_FRAME_STRING_ID = b"AWSKMSEncryptionClient Final Frame"
NON_FRAMED_STRING_ID = b"AWSKMSEncryptionClient Single Block"


class KeyringTraceFlag(Enum):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My spidey sense is tingling that we're going to hit an assumption impedance mismatch down the road between this being an incrementing integer enum here and bit fields in C https://github.com/aws/aws-encryption-sdk-c/blob/969c71d7b48a9c48e00a3ec8bc420c245681bab9/include/aws/cryptosdk/keyring_trace.h#L56

Have we hashed this out in the specification yet? Is there a good reason not to keep consistent keyring trace definition and value mappings across implementations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS does the bit flag thing because I copied C. I don't much like the bit flag because people will not understand it.
While I can get behind your "specification" point, I'm not sure that this is strictly a specification thing. Knowing what wrapping key did what should be, but how this is recorded?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't touched on this yet in the spec, no.

My general take on this is that we have not really been defining this as a thing that should be passed around between implementations, but instead have been defining it as a thing that is used as a unique reference within an implementation.

Talking with the folks who worked on the C client, the reason for using the bitshift values was for simple storage and comparison in a world where you lack things like sets.

I guess it wouldn't hurt anything to set the values to those bit-shift values just in case we want to use them later, and make the decision on if we do or not when we get to that point with the spec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can get behind your "specification" point, I'm not sure that this is strictly a specification thing. Knowing what wrapping key did what should be, but how this is recorded?

I think the question is more: "is this a value that we want to expose in any artifact that might cross implementations?"

We have precedent for values like this in, for example, the cache entry identifier and encryption context sorting order.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have precedent for values like this...

Then I vote bit flags all the way down.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"""KeyRing Trace actions."""

WRAPPING_KEY_GENERATED_DATA_KEY = 1
WRAPPING_KEY_ENCRYPTED_DATA_KEY = 2
WRAPPING_KEY_DECRYPTED_DATA_KEY = 3
WRAPPING_KEY_SIGNED_ENC_CTX = 4
WRAPPING_KEY_VERIFIED_ENC_CTX = 5
150 changes: 129 additions & 21 deletions src/aws_encryption_sdk/materials_managers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@
"""
import attr
import six
from attr.validators import deep_iterable, deep_mapping, instance_of, optional

from ..identifiers import Algorithm
from ..identifiers import Algorithm, KeyringTraceFlag
from ..internal.utils.streams import ROStream
from ..structures import DataKey
from ..structures import DataKey, EncryptedDataKey, KeyringTrace


@attr.s(hash=False)
Expand Down Expand Up @@ -51,28 +52,87 @@ class EncryptionMaterialsRequest(object):
)


@attr.s(hash=False)
class EncryptionMaterials(object):
@attr.s
class CryptographicMaterials(object):
"""Cryptographic materials core.

.. versionadded:: 1.5.0

:param Algorithm algorithm: Algorithm to use for encrypting message
:param dict encryption_context: Encryption context tied to `encrypted_data_keys`
:param DataKey data_encryption_key: Plaintext data key to use for encrypting message
:param encrypted_data_keys: List of encrypted data keys
:type encrypted_data_keys: list of :class:`EncryptedDataKey`
:param keyring_trace: Any KeyRing trace entries
:type keyring_trace: list of :class:`KeyringTrace`
"""

algorithm = attr.ib(validator=optional(instance_of(Algorithm)))
encryption_context = attr.ib(
validator=optional(
deep_mapping(key_validator=instance_of(six.string_types), value_validator=instance_of(six.string_types))
)
)
data_encryption_key = attr.ib(default=None, validator=optional(instance_of(DataKey)))
encrypted_data_keys = attr.ib(
default=attr.Factory(list), validator=optional(deep_iterable(member_validator=instance_of(EncryptedDataKey)))
)
keyring_trace = attr.ib(
default=attr.Factory(list), validator=optional(deep_iterable(member_validator=instance_of(KeyringTrace)))
)


@attr.s(hash=False, init=False)
class EncryptionMaterials(CryptographicMaterials):
"""Encryption materials returned by a crypto material manager's `get_encryption_materials` method.

.. versionadded:: 1.3.0

:param algorithm: Algorithm to use for encrypting message
:type algorithm: aws_encryption_sdk.identifiers.Algorithm
:param data_encryption_key: Plaintext data key to use for encrypting message
:type data_encryption_key: aws_encryption_sdk.structures.DataKey
:param encrypted_data_keys: List of encrypted data keys
:type encrypted_data_keys: list of `aws_encryption_sdk.structures.EncryptedDataKey`
.. versionadded:: 1.5.0

The **keyring_trace** parameter.

.. versionadded:: 1.5.0

Most parameters are now optional.

:param Algorithm algorithm: Algorithm to use for encrypting message
:param DataKey data_encryption_key: Plaintext data key to use for encrypting message (optional)
:param encrypted_data_keys: List of encrypted data keys (optional)
:type encrypted_data_keys: list of :class:`EncryptedDataKey`
:param dict encryption_context: Encryption context tied to `encrypted_data_keys`
:param bytes signing_key: Encoded signing key
:param bytes signing_key: Encoded signing key (optional)
:param keyring_trace: Any KeyRing trace entries (optional)
:type keyring_trace: list of :class:`KeyringTrace`
"""

algorithm = attr.ib(validator=attr.validators.instance_of(Algorithm))
data_encryption_key = attr.ib(validator=attr.validators.instance_of(DataKey))
encrypted_data_keys = attr.ib(validator=attr.validators.instance_of(set))
encryption_context = attr.ib(validator=attr.validators.instance_of(dict))
signing_key = attr.ib(default=None, validator=attr.validators.optional(attr.validators.instance_of(bytes)))

def __init__(
self,
algorithm=None,
data_encryption_key=None,
encrypted_data_keys=None,
encryption_context=None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose not to include the Encryption Context in the material. I did this because the top level caller must have the encryption context. I'm not saying we should remove it, just that it is slightly different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should include the encryption context so that it is more directly bound.

Specifically, the encryption context that needs to be used as part of these encryption materials, as possibly distinct from the encryption context that was written in the encrypted message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encryption context that needs to be used as part of these encryption materials
That is the part that convinces me :)

I can get down with this. The part I don't like is that the material is created by the CMM, when is the encryption context set? The best time would be when the material is created. Otherwise I have to work to distinguish between "no encryption context" and "encryption context has not yet been set so these material are not yet valid"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part I don't like is that the material is created by the CMM

This is explicitly part of the CMM contract: any CMM MAY modify the encryption context in either direction.

when is the encryption context set?

On initial creation of materials. The keyring MUST not modify the encryption context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, I'm cool with "initial creation of materials". But once a CMM creates materials, downstream CMM's can ~not change the Encryption Context... Or they have to re-create materials and clone things over.

i.e. this make is tedious to send an encryption context to the KMS keyring, but not return that encryption context to be serialized into the message format.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or they have to re-create materials and clone things over.

This. CMMs MAY modify the encryption context. They also MAY return whatever encryption/decryption materials that they deem appropriate. That MAY include a CMM layer that creates new materials based in part, whole, or not at all, on the materials returned from an underlying CMM.

signing_key=None,
**kwargs
):
if algorithm is None:
raise TypeError("algorithm must not be None")

if encryption_context is None:
raise TypeError("encryption_context must not be None")

super(EncryptionMaterials, self).__init__(
algorithm=algorithm,
encryption_context=encryption_context,
data_encryption_key=data_encryption_key,
encrypted_data_keys=encrypted_data_keys,
**kwargs
)
self.signing_key = signing_key
attr.validate(self)


@attr.s(hash=False)
class DecryptionMaterialsRequest(object):
Expand All @@ -92,16 +152,64 @@ class DecryptionMaterialsRequest(object):
encryption_context = attr.ib(validator=attr.validators.instance_of(dict))


@attr.s(hash=False)
class DecryptionMaterials(object):
_DEFAULT_SENTINEL = object()


@attr.s(hash=False, init=False)
class DecryptionMaterials(CryptographicMaterials):
"""Decryption materials returned by a crypto material manager's `decrypt_materials` method.

.. versionadded:: 1.3.0

:param data_key: Plaintext data key to use with message decryption
:type data_key: aws_encryption_sdk.structures.DataKey
:param bytes verification_key: Raw signature verification key
.. versionadded:: 1.5.0

The **algorithm**, **data_encryption_key**, **encrypted_data_keys**,
**encryption_context**, and **keyring_trace** parameters.

.. versionadded:: 1.5.0

All parameters are now optional.

:param Algorithm algorithm: Algorithm to use for encrypting message (optional)
:param DataKey data_encryption_key: Plaintext data key to use for encrypting message (optional)
:param encrypted_data_keys: List of encrypted data keys (optional)
:type encrypted_data_keys: list of :class:`EncryptedDataKey`
:param dict encryption_context: Encryption context tied to `encrypted_data_keys` (optional)
:param bytes verification_key: Raw signature verification key (optional)
:param keyring_trace: Any KeyRing trace entries (optional)
:type keyring_trace: list of :class:`KeyringTrace`
"""

data_key = attr.ib(validator=attr.validators.instance_of(DataKey))
verification_key = attr.ib(default=None, validator=attr.validators.optional(attr.validators.instance_of(bytes)))

def __init__(self, data_key=_DEFAULT_SENTINEL, verification_key=None, **kwargs):
if any(
(
data_key is _DEFAULT_SENTINEL and "data_encryption_key" not in kwargs,
data_key is not _DEFAULT_SENTINEL and "data_encryption_key" in kwargs,
)
):
raise TypeError("Exactly one of data_key or data_encryption_key must be set")

if data_key is not _DEFAULT_SENTINEL and "data_encryption_key" not in kwargs:
kwargs["data_encryption_key"] = data_key

for legacy_missing in ("algorithm", "encryption_context"):
if legacy_missing not in kwargs:
kwargs[legacy_missing] = None

super(DecryptionMaterials, self).__init__(**kwargs)

self.verification_key = verification_key
attr.validate(self)

@property
def data_key(self):
"""Backwards-compatible shim."""
return self.data_encryption_key

@data_key.setter
def data_key(self, value):
# type: (DataKey) -> None
"""Backwards-compatible shim."""
self.data_encryption_key = value
16 changes: 16 additions & 0 deletions src/aws_encryption_sdk/structures.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"""Public data structures for aws_encryption_sdk."""
import attr
import six
from attr.validators import deep_iterable, instance_of

import aws_encryption_sdk.identifiers
from aws_encryption_sdk.internal.str_ops import to_bytes, to_str
Expand Down Expand Up @@ -104,3 +105,18 @@ class EncryptedDataKey(object):

key_provider = attr.ib(hash=True, validator=attr.validators.instance_of(MasterKeyInfo))
encrypted_data_key = attr.ib(hash=True, validator=attr.validators.instance_of(bytes))


@attr.s
class KeyringTrace(object):
"""Record of all actions that a KeyRing performed with a wrapping key.

:param MasterKeyInfo wrapping_key: Wrapping key used
:param flags: Actions performed
:type flags: set of :class:`KeyringTraceFlag`
"""

wrapping_key = attr.ib(validator=instance_of(MasterKeyInfo))
flags = attr.ib(
validator=deep_iterable(member_validator=instance_of(aws_encryption_sdk.identifiers.KeyringTraceFlag))
)
76 changes: 59 additions & 17 deletions test/unit/test_material_managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,66 +12,93 @@
# language governing permissions and limitations under the License.
"""Test suite for aws_encryption_sdk.materials_managers"""
import pytest
from mock import MagicMock
from mock import MagicMock, sentinel
from pytest_mock import mocker # noqa pylint: disable=unused-import

from aws_encryption_sdk.identifiers import Algorithm
from aws_encryption_sdk.identifiers import KeyringTraceFlag
from aws_encryption_sdk.internal.defaults import ALGORITHM
from aws_encryption_sdk.internal.utils.streams import ROStream
from aws_encryption_sdk.materials_managers import (
CryptographicMaterials,
DecryptionMaterials,
DecryptionMaterialsRequest,
EncryptionMaterials,
EncryptionMaterialsRequest,
)
from aws_encryption_sdk.structures import DataKey
from aws_encryption_sdk.structures import DataKey, KeyringTrace, MasterKeyInfo

pytestmark = [pytest.mark.unit, pytest.mark.local]

_DATA_KEY = DataKey(
key_provider=MasterKeyInfo(provider_id="Provider", key_info=b"Info"),
data_key=b"1234567890123456789012",
encrypted_data_key=b"asdf",
)

_VALID_KWARGS = {
"CryptographicMaterials": dict(
algorithm=ALGORITHM,
encryption_context={"additional": "data"},
data_encryption_key=_DATA_KEY,
encrypted_data_keys=[],
keyring_trace=[
KeyringTrace(
wrapping_key=MasterKeyInfo(provider_id="Provider", key_info=b"Info"),
flags={KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY},
)
],
),
"EncryptionMaterialsRequest": dict(
encryption_context={},
plaintext_rostream=MagicMock(__class__=ROStream),
frame_length=5,
algorithm=MagicMock(__class__=Algorithm),
algorithm=ALGORITHM,
plaintext_length=5,
),
"EncryptionMaterials": dict(
algorithm=MagicMock(__class__=Algorithm),
data_encryption_key=MagicMock(__class__=DataKey),
algorithm=ALGORITHM,
data_encryption_key=_DATA_KEY,
encrypted_data_keys=set([]),
encryption_context={},
signing_key=b"",
),
"DecryptionMaterialsRequest": dict(
algorithm=MagicMock(__class__=Algorithm), encrypted_data_keys=set([]), encryption_context={}
"DecryptionMaterialsRequest": dict(algorithm=ALGORITHM, encrypted_data_keys=set([]), encryption_context={}),
"DecryptionMaterials": dict(
data_key=_DATA_KEY, verification_key=b"ex_verification_key", algorithm=ALGORITHM, encryption_context={}
),
"DecryptionMaterials": dict(data_key=MagicMock(__class__=DataKey), verification_key=b"ex_verification_key"),
}
_REMOVE = object()


@pytest.mark.parametrize(
"attr_class, invalid_kwargs",
(
(CryptographicMaterials, dict(algorithm=1234)),
(CryptographicMaterials, dict(encryption_context=1234)),
(CryptographicMaterials, dict(data_encryption_key=1234)),
(CryptographicMaterials, dict(encrypted_data_keys=1234)),
(CryptographicMaterials, dict(keyring_trace=1234)),
(EncryptionMaterialsRequest, dict(encryption_context=None)),
(EncryptionMaterialsRequest, dict(frame_length="not an int")),
(EncryptionMaterialsRequest, dict(algorithm="not an Algorithm or None")),
(EncryptionMaterialsRequest, dict(plaintext_length="not an int or None")),
(EncryptionMaterials, dict(algorithm=None)),
(EncryptionMaterials, dict(data_encryption_key=None)),
(EncryptionMaterials, dict(encrypted_data_keys=None)),
(EncryptionMaterials, dict(encryption_context=None)),
(EncryptionMaterials, dict(signing_key=u"not bytes or None")),
(DecryptionMaterialsRequest, dict(algorithm=None)),
(DecryptionMaterialsRequest, dict(encrypted_data_keys=None)),
(DecryptionMaterialsRequest, dict(encryption_context=None)),
(DecryptionMaterials, dict(data_key=None)),
(DecryptionMaterials, dict(verification_key=5555)),
(DecryptionMaterials, dict(data_key=_DATA_KEY, data_encryption_key=_DATA_KEY)),
(DecryptionMaterials, dict(data_key=_REMOVE, data_encryption_key=_REMOVE)),
),
)
def test_attributes_fails(attr_class, invalid_kwargs):
kwargs = _VALID_KWARGS[attr_class.__name__].copy()
kwargs.update(invalid_kwargs)
purge_keys = [key for key, val in kwargs.items() if val is _REMOVE]
for key in purge_keys:
del kwargs[key]
with pytest.raises(TypeError):
attr_class(**kwargs)

Expand All @@ -85,14 +112,29 @@ def test_encryption_materials_request_attributes_defaults():

def test_encryption_materials_defaults():
test = EncryptionMaterials(
algorithm=MagicMock(__class__=Algorithm),
data_encryption_key=MagicMock(__class__=DataKey),
encrypted_data_keys=set([]),
encryption_context={},
algorithm=ALGORITHM, data_encryption_key=_DATA_KEY, encrypted_data_keys=set([]), encryption_context={}
)
assert test.signing_key is None


def test_decryption_materials_defaults():
test = DecryptionMaterials(data_key=MagicMock(__class__=DataKey))
test = DecryptionMaterials(data_key=_DATA_KEY)
assert test.verification_key is None
assert test.algorithm is None
assert test.encryption_context is None


def test_decryption_materials_legacy_data_key_get():
test = DecryptionMaterials(data_encryption_key=_DATA_KEY)

assert test.data_encryption_key is _DATA_KEY
assert test.data_key is _DATA_KEY


def test_decryption_materials_legacy_data_key_set():
test = DecryptionMaterials(data_encryption_key=_DATA_KEY)

test.data_key = sentinel.data_key

assert test.data_encryption_key is sentinel.data_key
assert test.data_key is sentinel.data_key
Loading