master key provider keyring #209
Conversation
…cally wrap MKPs in keyrings
mattsb42-aws
requested review from
MatthewBennington,
WesleyRosenblum and
tawdry-audrey
| @@ -0,0 +1,222 @@ | |||
| # Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
This date is wrong, but according to yesterday's communication we can remove the date anyway
There was a problem hiding this comment.
Good catch. I think what I'll do here is update the date here, then in a separate PR update all the copyright notices per aws/crypto-tools#15
| :raises NotImplementedError: if method is not implemented | ||
| """ | ||
| primary_master_key, master_keys = self._master_key_provider.master_keys_for_encryption( | ||
| encryption_context=encryption_materials.encryption_context, plaintext_rostream=None, plaintext_length=None, |
There was a problem hiding this comment.
Isn't this the same issue that prevented the Java ESDK from using a MasterKeyProviderKeyring? You aren't providing the plaintext_rostream or plaintext_length to the MKP, but the Default CMM used to. I think we may have discussed this, but why isn't that a potential breaking change for customers who have written custom MKPs and used the plaintext_rostream or plaintext_length inputs?
| "Unable to use master keys with encryption materials that already contain a data key." | ||
| " You are probably trying to mix master key providers and keyrings." | ||
| " If you want to do that, the master key provider MUST be the generator." |
There was a problem hiding this comment.
Should the multi keyring validate that the children keyrings are not MKP keyrings upon creation? This behavior is different than all the other keyrings, so it might be confusing
| # not the provider info, which is what is in the returned data key. | ||
| try: | ||
| decrypting_master_key = list(self._master_key_provider.master_keys_for_data_key(decrypted_data_key))[0] | ||
| except IndexError: |
There was a problem hiding this comment.
What would cause this to happen, a buggy MKP?
| encryption_context=encryption_context, | ||
| plaintext_rostream=request.plaintext_rostream, | ||
| plaintext_length=request.plaintext_length, |
There was a problem hiding this comment.
See comment on master_key.py, its ok for plaintext_rostream and plaintext_length to not be passed to master_keys_for_encryption()?
|
I've been thinking about this and discussing the problem offline with @WesleyRosenblum. I think that the best answer we can get here is the same as we arrived at for Java; the master key provider and keyring APIs are just too different, in too many subtle ways, for this sort of abstraction layer to work without breaking people. :( I'm going to close this and change tack to instead have the default CMM directly handle both keyrings and MKPs. |
Issue #, if available: #146
Description of changes:
This adds
MasterKeyProviderKeyringas an abstraction layer on top of master key providers (MKPs). This allows us to isolate MKP logic within this specific keyring so that everything upstream only ever needs to deal in keyrings and the keyring interface.This gives us two benefits over leaving this logic in the default CMM:
DefaultCryptoMaterialsManagerandCachingCryptoMaterialsManageralso now accept a keyring in addition to a MKP (and a CMM in the case of the caching CMM).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: