-
Notifications
You must be signed in to change notification settings - Fork 86
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
Keyring materials #163
Changes from 3 commits
a3d947d
daaacba
f254e73
f9aa29d
01759b9
e8e5b82
469600c
a27ff74
b311cda
10ded57
4f95e53
7302775
f1e7f2f
524d847
d786409
888fc17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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 | ||
seebees marked this conversation as resolved.
Show resolved
Hide resolved
|
||
: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 | ||
seebees marked this conversation as resolved.
Show resolved
Hide resolved
|
||
: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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is explicitly part of the CMM contract: any CMM MAY modify the encryption context in either direction.
On initial creation of materials. The keyring MUST not modify the encryption context. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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): | ||
|
@@ -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, | ||
seebees marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I vote bit flags all the way down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done