fix: KeyringTraceFlag requirements and data key caching #210
Conversation
cloneMaterial incorectly required WRAPPING_KEY_GENERATED_DATA_KEY. in the case of DecryptionMaterials, this should never be the case. When I wrote the tests, I did not take this case into consideration and simply set the generate and decrypt flags on the same keyringTrace. The organization of the traces has also changed. There is now a single trace for each “operation”. This mean that a trace will exists for the set operation as well as the encrypted data key. This lets me require only valid trace flags for each operation. I simplified the cloneMaterial function to rely on the set methods `setUnencryptedDataKey` and `addEncryptedDataKey` to deny invalid fields. I also, organize the KeyringTraceFlags to determin which flags are valid. The KMS Keyring needed to be updated to make sure that the trace contains the WRAPPING_KEY_SIGNED_ENC_CTX flag. Tests are updated, and added.
| expect(material.keyringTrace).to.have.lengthOf(2) | ||
| const [traceGenerate, traceEncrypt] = material.keyringTrace | ||
| expect(material.keyringTrace).to.have.lengthOf(3) | ||
| const [traceGenerate, traceEncrypt1, traceEncrypt2] = material.keyringTrace | ||
| expect(traceGenerate.keyNamespace).to.equal('aws-kms') | ||
| expect(traceGenerate.keyName).to.equal(generatorKeyId) | ||
| expect(traceGenerate.flags & KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY).to.equal(KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY) | ||
| expect(traceGenerate.flags & KeyringTraceFlag.WRAPPING_KEY_ENCRYPTED_DATA_KEY).to.equal(KeyringTraceFlag.WRAPPING_KEY_ENCRYPTED_DATA_KEY) |
There was a problem hiding this comment.
Why do WRAPPING_KEY_ENCRYPTED_DATA_KEY and WRAPPING_KEY_SIGNED_ENC_CTX exist on the generate trace? Should they if the wrapping key hasn't been used to encrypt yet?
There was a problem hiding this comment.
In the KMS case. On generate, the encrypted data key is also returned....
It is just so that people can see the relationship between
the generate trace and the encrypt trace.
| source.encryptedDataKeys.forEach(edk => { | ||
| const encryptedDataKeys = source.encryptedDataKeys | ||
| /* Precondition: For each encrypted data key, there must be a trace. */ | ||
| needs(encryptedDataKeys.length === traces.length, 'KeyringTrace length does not match encrypted data keys.') |
There was a problem hiding this comment.
How is this true if there is a separate trace for generate?
There was a problem hiding this comment.
const [generate, ...others] = traces
If you see above :)
|
|
||
| ENCRYPT_FLAGS = WRAPPING_KEY_ENCRYPTED_DATA_KEY | WRAPPING_KEY_SIGNED_ENC_CTX, // eslint-disable-line no-unused-vars | ||
|
|
||
| SET_FLAGS = WRAPPING_KEY_GENERATED_DATA_KEY | WRAPPING_KEY_DECRYPTED_DATA_KEY, // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
After going over the PR a couple times the inclusion of WRAPPING_KEY_DECRYPTED_DATA_KEY here finally makes sense, but is initially very confusing. When I think of the decrypt flag I think of the decrypt operation. The fact that the output was of that set in the materials is something that seems less salient. I'm not sure if there is another way to name this/organize these fields that is more intuitive. 🤔
There was a problem hiding this comment.
I also am thinking about it. That is why I called it SET because the function setUnencryptedDataKey
| * it must be valid for the type of material. | ||
| * It is invalid to claim that EncryptionMaterial were decrypted. | ||
| */ | ||
| const deniedSetFlags = (KeyringTraceFlag.SET_FLAGS ^ setFlag) | ( |
There was a problem hiding this comment.
Going back to the SET_FLAG naming, it feels weird to organize the SET_FLAGS but then also have this logic here say "if you're encrypting actually we only care about the generate flag and if you're decrypting we only care about the decrypt flag". If this function requires this logic anyway, why try to reason in terms of SET_FLAGS at all and instead just think in terms of the generate and decrypt flags?
|
This PR deals with KeyringTraces. Ideally the general interface would be improved. |
cloneMaterial incorectly required WRAPPING_KEY_GENERATED_DATA_KEY.
in the case of DecryptionMaterials, this should never be the case.
When I wrote the tests, I did not take this case into consideration
and simply set the generate and decrypt flags on the same keyringTrace.
The organization of the traces has also changed.
There is now a single trace for each “operation”.
This mean that a trace will exists for the set operation
as well as the encrypted data key.
This lets me require only valid trace flags for each operation.
I simplified the cloneMaterial function to rely on the set methods
setUnencryptedDataKeyandaddEncryptedDataKeyto deny invalid fields.
I also, organize the KeyringTraceFlags to determin which flags are valid.
The KMS Keyring needed to be updated
to make sure that the trace contains the WRAPPING_KEY_SIGNED_ENC_CTX
flag.
Tests are updated, and added.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: