Keyring materials #163

mattsb42-aws · 2019-06-21T00:40:43Z

Description of changes:

Update Encryption/Decryption materials and create Keyring traces in preparation for Keyring work.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…erials

src/aws_encryption_sdk/materials_managers/__init__.py

mattsb42-aws · 2019-06-28T18:41:02Z

@seebees @lizroth Something that occurred to me working through this:

The way I am maintaining the keyring trace currently is via a list containing keyring identifier / keyring trace flags pairs. However, if a keyring generates the data key and adds an encrypted data key, then we end up with two entries in that list.

I'm inclined to say now that it would be better to instead maintain this as a map of keyring identifier to keyring trace flags. That way it would be more straightforward to locate all of the flags added by a given keyring.

Thoughts?

src/aws_encryption_sdk/materials_managers/__init__.py

seebees · 2019-06-28T19:08:11Z

src/aws_encryption_sdk/materials_managers/__init__.py

+        algorithm=None,
+        data_encryption_key=None,
+        encrypted_data_keys=None,
+        encryption_context=None,


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.

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.

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"

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.

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.

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.

src/aws_encryption_sdk/materials_managers/__init__.py

…ed to EncryptionMaterials

src/aws_encryption_sdk/materials_managers/__init__.py

lizroth · 2019-07-01T23:37:04Z

src/aws_encryption_sdk/identifiers.py

@@ -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):


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?

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?

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.

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.

We have precedent for values like this...

Then I vote bit flags all the way down.

src/aws_encryption_sdk/materials_managers/__init__.py

lizroth · 2019-07-01T23:47:38Z

src/aws_encryption_sdk/materials_managers/__init__.py

+    def keyring_trace(self):
+        """Return a read-only version of the keyring trace.
+
+        :rtype: tuple


Is this for immutability only? What are we trying to protect against? The return below is a shallow copy, right?

What I'm trying to do here (and with Encryptionmaterials.encrypted_data_keys) is to remove the easy path for people to modify the encrypted data keys or keyring trace directly without going through the add_ methods.

The reason for this is to make it harder for people to add one without the other.

src/aws_encryption_sdk/materials_managers/__init__.py

…add_data_encryption_key can be called

…but no data_encryption_key

…ials

seebees · 2019-07-09T16:23:58Z

I'm inclined to say now that it would be better to instead maintain this as a map of keyring identifier to keyring trace flags. That way it would be more straightforward to locate all of the flags added by a given keyring.

I agree.

https://github.com/awslabs/aws-encryption-sdk-javascript/blob/master/modules/material-management/src/cryptographic_material.ts#L371

seebees · 2019-07-09T16:38:00Z

src/aws_encryption_sdk/materials_managers/__init__.py

+
+        return True
+
+    def add_data_encryption_key(self, data_encryption_key, keyring_trace):


Re: #163 (comment)

I think a comment about why the keyring trace is here? Maybe it will resolve the other convo?

mattsb42-aws · 2019-07-09T20:42:28Z

I'm inclined to say now that it would be better to instead maintain this as a map of keyring identifier to keyring trace flags. That way it would be more straightforward to locate all of the flags added by a given keyring.

I agree.

https://github.com/awslabs/aws-encryption-sdk-javascript/blob/master/modules/material-management/src/cryptographic_material.ts#L371

I've actually been oscillating the other way on this. Mainly because: if a keyring does the same action twice, how should that be recorded in the trace?

We can leave this for resolution after this PR; the write API on the materials will not change, just the read API, which will not affect the other ongoing keyring development.

…ementations

mattsb42-aws · 2019-07-09T21:19:22Z

Looks like something in pydocstyle 4.0.0 broke flake8-docstrings. For the moment I'm going to put in a temporary pin to <4.0.0. We can deal with that separately.

…ted_data_keys to tuple

…ocstrings

mattsb42-aws · 2019-07-09T21:22:23Z

Per offline conversation, changing EncryptionMaterials.encrypted_data_keys away from set/frozenset to list/tuple.

mattsb42-aws · 2019-07-11T21:47:07Z

AppVeyor is failing install on 2.7 and 3.4, but that looks unrelated to these changes to me so since this is not going into master yet anyway I'm inclined to merge this now to unblock the rest of the keyring work.

mattsb42-aws added 3 commits June 20, 2019 17:36

bump attrs to 19.1.0

a3d947d

add keyring trace and integrate into updated encrytion/decryption mat…

daaacba

…erials

s/KeyRing/Keyring/g

f254e73

mattsb42-aws requested a review from seebees June 21, 2019 00:40

seebees reviewed Jun 21, 2019

View reviewed changes

src/aws_encryption_sdk/materials_managers/__init__.py Outdated Show resolved Hide resolved

seebees reviewed Jun 21, 2019

View reviewed changes

src/aws_encryption_sdk/materials_managers/__init__.py Outdated Show resolved Hide resolved

mattsb42-aws added 3 commits June 26, 2019 20:10

align cryptographic materials and add write-only interface

f9aa29d

encrypted_data_keys must only contain EncryptedDataKey

01759b9

fix test to be Python 2 compatible

e8e5b82

mattsb42-aws mentioned this pull request Jun 27, 2019

Keyring base API #161

Merged

MeghaShetty mentioned this pull request Jun 28, 2019

Raw keyrings #165

Merged

mattsb42-aws requested a review from lizroth June 28, 2019 18:41

seebees reviewed Jun 28, 2019

View reviewed changes

data encryption key must be set before encrypted data keys can be add…

469600c

…ed to EncryptionMaterials

seebees reviewed Jul 1, 2019

View reviewed changes

src/aws_encryption_sdk/materials_managers/__init__.py Show resolved Hide resolved

seebees reviewed Jul 1, 2019

View reviewed changes

src/aws_encryption_sdk/materials_managers/__init__.py Show resolved Hide resolved

seebees reviewed Jul 1, 2019

View reviewed changes

src/aws_encryption_sdk/materials_managers/__init__.py Show resolved Hide resolved

add signing/verification key checks to Encryption/DecryptionMaterials

a27ff74

lizroth reviewed Jul 2, 2019

View reviewed changes

mattsb42-aws added 4 commits July 1, 2019 19:03

DecryptionMaterials.algorithm must be set before DecryptionMaterials.…

b311cda

…add_data_encryption_key can be called

update materials docs and typehints

10ded57

EncryptionMaterials must not be initialized with encrypted_data_keys …

4f95e53

…but no data_encryption_key

add is_complete properties to EncryptionMaterials and DecryptionMater…

7302775

…ials

seebees reviewed Jul 9, 2019

View reviewed changes

change KeyringTraceFlag values to bitshifted ints to match other impl…

f1e7f2f

…ementations

normalize EncryptionMaterials._encrypted_data_keys to list and encryp…

524d847

…ted_data_keys to tuple

temporarily pin pydocstyle at <4.0.0 to avoid issue breaking flake8-d…

d786409

…ocstrings

seebees approved these changes Jul 10, 2019

View reviewed changes

temporarily cap pydocstyle at <4.0.0 for decrypt oracle

888fc17

mattsb42-aws merged commit 54dfc23 into aws:keyring Jul 11, 2019

mattsb42-aws mentioned this pull request Jul 12, 2019

add keyrings #146

Closed

10 tasks

mattsb42-aws added this to the keyrings milestone Feb 21, 2020

mattsb42-aws deleted the keyring-materials branch March 8, 2020 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyring materials #163

Keyring materials #163

mattsb42-aws commented Jun 21, 2019

mattsb42-aws commented Jun 28, 2019

seebees Jun 28, 2019

mattsb42-aws Jul 3, 2019

seebees Jul 9, 2019

mattsb42-aws Jul 9, 2019

seebees Jul 9, 2019

mattsb42-aws Jul 10, 2019

lizroth Jul 1, 2019

seebees Jul 2, 2019

mattsb42-aws Jul 2, 2019

mattsb42-aws Jul 2, 2019

seebees Jul 9, 2019

mattsb42-aws Jul 9, 2019

lizroth Jul 1, 2019

mattsb42-aws Jul 2, 2019

seebees commented Jul 9, 2019

seebees Jul 9, 2019

mattsb42-aws commented Jul 9, 2019

mattsb42-aws commented Jul 9, 2019

mattsb42-aws commented Jul 9, 2019

mattsb42-aws commented Jul 11, 2019


		return True

		def add_data_encryption_key(self, data_encryption_key, keyring_trace):

Keyring materials #163

Keyring materials #163

Conversation

mattsb42-aws commented Jun 21, 2019

mattsb42-aws commented Jun 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seebees commented Jul 9, 2019

Choose a reason for hiding this comment

mattsb42-aws commented Jul 9, 2019

mattsb42-aws commented Jul 9, 2019

mattsb42-aws commented Jul 9, 2019

mattsb42-aws commented Jul 11, 2019