Skip to content

Keyring base API#161

Merged
mattsb42-aws merged 18 commits intoaws:keyringfrom
MeghaShetty:mmegs-keyring
Jul 12, 2019
Merged

Keyring base API#161
mattsb42-aws merged 18 commits intoaws:keyringfrom
MeghaShetty:mmegs-keyring

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"?

MeghaShetty and others added 5 commits June 28, 2019 12:26
Co-Authored-By: Matt Bullock <bullocm@amazon.com>
Co-Authored-By: Matt Bullock <bullocm@amazon.com>
Co-Authored-By: Matt Bullock <bullocm@amazon.com>
Co-Authored-By: Matt Bullock <bullocm@amazon.com>
@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