-
Notifications
You must be signed in to change notification settings - Fork 86
docs: add keyring examples #221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
doh; looks like I need to seed the environment with a default AWS region :/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just had a few questions/ np's below
examples/src/keyring/aws_kms/discovery_decrypt_in_region_only.py
Outdated
Show resolved
Hide resolved
examples/src/keyring/aws_kms/discovery_decrypt_with_preferred_regions.py
Show resolved
Hide resolved
What is the value of doing that? The way I see it, all current customers are already using MKPs and don't really need example code to follow for doing something with MKPs, and if they want to try out a new use case they should just use Keyrings anyway? I could see an argument to do it for completeness, but it still seems like the lowest priority to me. |
examples/src/keyring/aws_kms/discovery_decrypt_with_preferred_regions.py
Outdated
Show resolved
Hide resolved
examples/src/keyring/aws_kms/discovery_decrypt_with_preferred_regions.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review in progress (back later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked over all diffs since my last review. Looks good! Would be great to also get an example of how to handle errors/ failing to encrypt/decrypt, but that can be out of scope for this PR.
examples/src/keyring/aws_kms/discovery_decrypt_in_region_only.py
Outdated
Show resolved
Hide resolved
examples/src/keyring/aws_kms/discovery_decrypt_in_region_only.py
Outdated
Show resolved
Hide resolved
examples/src/keyring/aws_kms/discovery_decrypt_in_region_only.py
Outdated
Show resolved
Hide resolved
} | ||
|
||
# Create the keyring that determines how your data keys are protected. | ||
encrypt_keyring = KmsKeyring(generator_key_id=aws_kms_cmk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
examples/src/keyring/aws_kms/discovery_decrypt_in_region_only.py
Outdated
Show resolved
Hide resolved
Co-Authored-By: June Blender <[email protected]>
dd59a36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of general comments.
-
The example readme is a nice description of the ESDK. But it needs a table with the name and description of each example.
-
The examples all use (one?) CMK that's passed to the function. Please show at least one example with multiple CMKs (generator, additional) and different keyrings for encrypt and decrypt.
-
Show examples for discovery keyring and client supplier variants. (I suspect that's coming.)
Let's look at that separately from this PR. I opened #232 to track this.
|
Issue #, if available: #156 #146
Description of changes:
This adds several examples that show how to use keyrings to solve some common problems.
These new examples all follow the format set in #219.
Who reviews what
Everyone is of course welcome to review anything else, but these are the things that I'm specifically looking for from each tagged person.
@SalusaSecondus : The example scenario description and framing for the multi-keyring escrow example.
@WesleyRosenblum : Keyring use and any additional example use-cases you can think of. We'll also be translating these examples to Java once these are merged.
@acioc : Python good-ness and any thoughts on example use-cases.
@juneb : Descriptions of example use-cases and the in-line comments where they diverge from the template.
What this is not
Things I intentionally left out of this 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: