feat: immutable cryptographic materials (for keyrings) #231
Conversation
* adopt immutable materials * add encryption context entries to keyring trace awslabs/aws-encryption-sdk-specification#32 * use the key name, not the provider info, in the keyring trace * fix the keyring trace tests to accurately fail when trace is wrong
| @@ -180,25 +181,26 @@ class _AwsKmsSingleCmkKeyring(Keyring): | |||
| def on_encrypt(self, encryption_materials): | |||
| # type: (EncryptionMaterials) -> EncryptionMaterials | |||
| trace_info = MasterKeyInfo(provider_id=_PROVIDER_ID, key_info=self._key_id) | |||
| new_materials = copy.copy(encryption_materials) | |||
There was a problem hiding this comment.
Why is it necessary to copy the materials?
There was a problem hiding this comment.
It's not strictly necessary, but I think that having access to the original materials inside that scope could help with debugging if things aren't doing what people thing they should.
There was a problem hiding this comment.
Maybe this is a Python subtlety I'm missing, but aren't the original materials already available through the "encryption_materials" input?
There was a problem hiding this comment.
The flow is:
if no data key:
data key, EDK = generate()
new materials = materials(with data key)
else:
EDK = encrypt(data key)
new materials = materials with data key(with EDK)
I went with that flow so that we could single-source the "with EDK" bit, but because of that, if we want to retain an available reference to the materials in the request we have a choice: we can either copy the request materials first:
copied materials = copy(request materials)
if no data key:
data key, EDK = generate()
copied materials = copied materials(with data key)
else:
EDK = encrypt(data key)
copied materials = copied materials(with EDK)
or we can balance a null value
new materials = None
if no data key:
data key, EDK = generate()
new materials = request materials(with data key)
else:
EDK = encrypt(data key)
if new materials is None:
new materials = request materials(with EDK)
else:
new materials = new materials(with EDK)
The copy flow seems cleaner to me.
There was a problem hiding this comment.
This doesn't work?
new materials = request materials
if no data key:
data key, EDK = generate()
new materials = new materials(with data key)
else:
EDK = encrypt(data key)
new materials = new materials(with EDK)There was a problem hiding this comment.
Yeah, I guess that would be fine. It feels less explicit, but since we're only ever using the immutable APIs it would effectively be doing the same thing.
There was a problem hiding this comment.
That's how I did it in Java, but I'll defer to your judgement on what you prefer for Python
Co-Authored-By: Wesley Rosenblum <[email protected]>
| @@ -5,6 +5,7 @@ | |||
| .. versionadded:: 1.5.0 | |||
|
|
|||
| """ | |||
| import copy | |||
There was a problem hiding this comment.
Do you still need this?
There was a problem hiding this comment.
Nope! That'll be why CI is failing. :)
| :rtype: EncryptionMaterials | ||
| """ | ||
| if encryption_materials.data_encryption_key is None: | ||
| _generate_data_key(encryption_materials=encryption_materials, key_provider=self._key_provider) | ||
| new_materials = copy.copy(encryption_materials) |
There was a problem hiding this comment.
| new_materials = copy.copy(encryption_materials) | |
| new_materials = encryption_materials |
| @@ -1,6 +1,7 @@ | |||
| # Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
| """Resources required for Multi Keyrings.""" | |||
| import copy | |||
Issue #, if available:
Description of changes:
The primary focus of this change is to move keyrings to immutable cryptographic materials. Rather than providing APIs that let you modify materials, we now provide APIs that let you get a new copy of those materials that also include additional contents.
While I was working on that, I found a few other points where the keyrings were not compliant with the spec, particularly around behavior when failing on encrypt and decrypt, and in the specific values set in the keyring trace. Looking back through the changes, I would have a hard time extracting any single piece, so unfortunately they're all just getting lumped into this one PR.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: