Skip to content
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

Pass a default EncryptionContext on calls to KMS #136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davbo
Copy link

@davbo davbo commented Feb 16, 2017

This addresses #134. As discussed there: setting an EncryptionContext gives some nice benefits such as preventing credentials being swapped in the storage backend.

Credentials put with this version of credstash cannot be retrieved by older versions without specifying the context explicitly. This backward incompatibility is why I bumped the version to 2.0.0. For now it is possible for this version to get credentials put by older versions. This way people can upgrade the version of credstash in use then using the migrate script update all credentials to have an EncryptionContext attached.

I'm not sure what a change like this would mean for other "credstash compatible" libraries either.

Any suggestions for changes or documentation to help existing users upgrade would be appreciated.

Using the credential name and version as Additional Authentication Data (AAD)
can mitigate ciphertext replacement attacks on the DynamoDB backend.

Bumps the version to 2.0.0 as it is backwards incompatible with older versions
of credstash which didn't set an EncryptionContext by default.

Upgrading from older versions is possible by running

  ./migrations/credstash-migrate-encryption-context.py

This will re-encrypt secrets with the default EncryptionContext.
@davbo
Copy link
Author

davbo commented Mar 1, 2017

Any thoughts on what might need to be added to get this merged? cc @alex-luminal

@alex-luminal
Copy link
Contributor

Took a cursory look at it looks good. Will take a closer look tonight/tomorrow and should be able to get it merged.

@davbo
Copy link
Author

davbo commented Mar 22, 2017

Hi @alex-luminal sorry for pestering you on this.

Anything I can do to help get this merged?

@wayne-luminal
Copy link
Contributor

@davbo Apologies for taking so long to get back to you. I'm working on the next version of credstash and I'm planning to pull these changes in. Thank you for the PR. Stay tuned!

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