feat: enable use of keyrings #216
Conversation
…urn incomplete or broken materials
|
reminder for the record: Appveyor failing on 3.4 is a known issue and not caused by this PR |
|
My questions have been resolved. @ajw-aws :) |
| provided_count = len([is_set for is_set in options_provided if is_set]) | ||
|
|
||
| if provided_count != 1: | ||
| raise TypeError("Exactly one of 'backing_materials_manager', 'keyring', or 'key_provider' must be provided") |
There was a problem hiding this comment.
Is TypeError the best exception we can throw here? Is there a better custom exception class, or should we create one?
There was a problem hiding this comment.
TypeError is consistent with what built-ins and attrs throw if you provide incorrect parameters or types.
>>> def foo(b):
... print("foo")
...
>>> foo(3)
foo
>>> foo(b=3)
foo
>>> foo(f=3)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: foo() got an unexpected keyword argument 'f'
>>> foo(3, 4)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: foo() takes 1 positional argument but 2 were given|
|
||
| materials_are_valid = ( | ||
| final_materials.algorithm is request.algorithm, | ||
| final_materials.encryption_context == request.encryption_context, |
There was a problem hiding this comment.
Is there a reason we have to use == here instead of is ?
There was a problem hiding this comment.
Because we send a copy into the keyring rather than the original to avoid any potential side effects from misbehaving keyrings modifying it.
| test = CachingCryptoMaterialsManager( | ||
| cache=MagicMock(__class__=CryptoMaterialsCache), max_age=10.0, master_key_provider=mock_mkp | ||
| cache=LocalCryptoMaterialsCache(capacity=10), max_age=10.0, master_key_provider=mkp |
There was a problem hiding this comment.
Where does the magic number 10 come from? Should this instead be a declared constant elsewhere?
There was a problem hiding this comment.
It's not magic, it's just a required parameter. It wasn't there before because I was using mocks before and now I'm actually using the actual class. IMO making it a constant would give it undue gravity; it really doesn't matter what number it is.
Issue #, if available: #211
Description of changes:
This adds support for using keyrings to
DefaultCryptographicMaterialsManageras well as the appropriate passthroughs viaCachingCryptographicMaterialsManagerand the encrypt/decrypt entry points.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: