chore(duvet): annotate Keyring, S3Keyring, and KmsKeyring classes#143
Conversation
texastony
left a comment
texastony
left a comment
There was a problem hiding this comment.
I have a few nits, but in general, LGTM.
| ##= specification/s3-encryption/materials/s3-kms-keyring.md#encryptdatakey | ||
| ##= type=exception | ||
| ##% The KmsKeyring MUST implement the EncryptDataKey method. | ||
| ##% The keyring MUST call [AWS KMS Encrypt](https://docs.aws.amazon.com/kms/latest/ | ||
| ##% APIReference/API_Encrypt.html) using the configured AWS KMS client. |
There was a problem hiding this comment.
Is this duplicated from the exceptions file?
| ##% When decrypting using Kms+Context mode, the KmsKeyring MUST validate the provided (request) encryption context with the stored (materials) encryption context. | ||
| ##= specification/s3-encryption/materials/s3-kms-keyring.md#decryptdatakey | ||
| ##= type=test | ||
| ##% If the Key Provider Info of the Encrypted Data Key is "kms+context", the KmsKeyring MUST attempt to decrypt using Kms+Context mode. |
There was a problem hiding this comment.
Nit: this is not really asserted directly by this test.
There was a problem hiding this comment.
good point, i'll move it to the below test
| encryption_context_stored={ | ||
| "aws:x-amz-cek-alg": "AES/GCM/NoPadding", | ||
| "custom-key": "stored-value", | ||
| }, |
There was a problem hiding this comment.
The spec says two reserved keys. But I only count 1.
There was a problem hiding this comment.
Aside: the spec should include a line to specify that the presence of a reserved key MUST throw an exception.
Also, yes, this is a bug in the existing Python code. I've cut an issue to track this: #146
| with pytest.raises(Exception): | ||
| keyring.on_decrypt(dec_materials) |
There was a problem hiding this comment.
Can we ensure that this the exception defined on line 420?
There was a problem hiding this comment.
Good shout, this test was fine but test_on_decrypt_fails_when_kms_v1_fails was throwing a different exception.
2 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.