Skip to content

Auth only CMP #35

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

Merged
merged 3 commits into from
May 1, 2018
Merged

Auth only CMP #35

merged 3 commits into from
May 1, 2018

Conversation

mattsb42-aws
Copy link
Member

@mattsb42-aws mattsb42-aws commented Apr 28, 2018

Enable CMPs that only provide authentication materials and no encryption materials.

@mattsb42-aws mattsb42-aws changed the base branch from DELETE-5 to master May 1, 2018 17:37
MaterialDescriptionKeys.ATTRIBUTE_ENCRYPTION_MODE.value
] = encryption_mode
try:
encryption_materials.encryption_key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be cleaner to do this with a conditional rather than a try/except/else clause, particularly because you aren't actually trying to do anything in the try clause. Start with:
if hasattr(encryption_materials, 'encryption_key'):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Asking for forgiveness instead of permission" is a general Python convention.

http://docs.quantifiedcode.com/python-anti-patterns/readability/asking_for_permission_instead_of_forgiveness_when_working_with_files.html

It's also generally faster, though that's not really going to have any affect here.

for name, attribute in item.items():
if crypto_config.attribute_actions.action(name) is not CryptoAction.ENCRYPT_AND_SIGN:
encrypted_item[name] = attribute.copy()
continue
Copy link
Contributor

@david-koenig david-koenig May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I'm reading this correctly, you can just use else: here one indent level higher instead of continue, which is more natural. I'd also probably switch the order of the two clauses, so that you can do the more natural test of is instead of is not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough; updating

@@ -162,12 +172,22 @@ def decrypt_dynamodb_item(item, crypto_config):

decryption_materials = inner_crypto_config.decryption_materials()

verify_item_signature(signature_attribute, item, decryption_materials.verification_key, inner_crypto_config)

try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might also do this as a hasattr test as above.

@david-koenig david-koenig merged commit 6515bbf into aws:master May 1, 2018
@mattsb42-aws mattsb42-aws deleted the auth-only-cmp branch May 1, 2018 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants