Skip to content

Keyring base API #161

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 18 commits into from
Jul 12, 2019
Merged

Keyring base API #161

merged 18 commits into from
Jul 12, 2019

Conversation

MeghaShetty
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mattsb42-aws
Copy link
Member

mattsb42-aws commented Jun 18, 2019

I'm not sure why this is showing all the files added in #158 as new, but would you please rebase on top of aws/keyring? This is pulling in more changes than should be here.

@mattsb42-aws mattsb42-aws self-assigned this Jun 18, 2019
"""
raise NotImplementedError("Keyring does not implement on_encrypt function")

def on_decrypt(self, decryption_materials):
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed offline, per discussion in #163 we need to change this interface. encrypted_data_keys should be an iterable of EncryptedDataKey.

Suggested change
def on_decrypt(self, decryption_materials):
def on_decrypt(self, decryption_materials, encrypted_data_keys):

@MeghaShetty MeghaShetty mentioned this pull request Jun 28, 2019
def on_decrypt(self, decryption_materials, encrypted_data_keys):
"""Attempt to decrypt the encrypted data keys.

:param decryption_materials: May contain verification key, algorithm, encryption context and keyring trace.
Copy link
Member

Choose a reason for hiding this comment

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

Let's be consistent with the encryption materials.

I'm inclined to say that we should punt any description of the contents of encryption/decryption materials to the definitions of those structures (which Sphinx will link to when the docs are generated). Maybe just say "encryption/decryption materials for the keyring to modify" and "optionally modified encryption/decryption materials"?

@mattsb42-aws mattsb42-aws mentioned this pull request Jul 12, 2019
10 tasks
@mattsb42-aws
Copy link
Member

We still need tests for this, but this is simple enough that I'm ok leaving that for another PR since this is not going into master yet anyway. I broke this out in #146 to make sure we don't forget.

@mattsb42-aws mattsb42-aws merged commit 1615d63 into aws:keyring Jul 12, 2019
@MeghaShetty MeghaShetty deleted the mmegs-keyring branch July 12, 2019 20:07
@mattsb42-aws mattsb42-aws added this to the keyrings milestone Feb 21, 2020
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.

3 participants