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

Credstash - Upstream changes #144

Open
wants to merge 50 commits into
base: master
Choose a base branch
from

Conversation

nathan-muir
Copy link
Contributor

@nathan-muir nathan-muir commented Mar 13, 2017

Hi there,

I started a fork of credstash a few months ago, so I could quickly iterate on a few concepts.

I've been using it in production for a while now, and thought it would be good to contribute back some of the features that were developed.

The main goal for the fork:

  • Allow the KMS & DynamoDB config to persist in a config file; eg. /etc/credsmash.cfg
  • Add support for template interpolation with secret data (see credsmash render-templates)

On the way: (See History.md)

  • Add support for binary data
  • Switch from pycrypto to cryptography & fix a few issues in aes_ctr (already upstreamed in Cryptography - Switch from pycrypto to cryptography #116 )
  • Added support for aes-gcm
  • Converted the CLI from argparse to click
  • Pluggable Key Services
  • Pluggable Storage Services
  • Created a better API for using directly in python

Obviously there was a big departure from the credstash codebase, so I did start work on re-using the argparse implementation with the new internal api's - 3stack-software#4


So there's a lot going on here, it would be great to discuss and see what can be made suitable to upstream.

My biggest fear would be for a change like #136 to cause unnecessary divergence.

The goal was to clean up some of the CLI, and to modularise
the api for calling kms/dynamodb.

This will let us add new features like setting default options
from a configuration file, and writing configuration templates.

Fixes fugue#93 allowing boto3/botocore to configure itself.

Fixes fugue#79 as file input is the default for put/put-many

Closes fugue#88 as `put` only allows for files , so you
 must use `printf`/`echo` to put any value.
Providing `--table-name` or `--key-id` will take
precedence over the `credsmash.cfg`.
Manually adding `--context key value` will only append each key-value pair.
This was necessary so we can add support for other encryption
schemes, and fix-up support for binary files; without breaking
existing stored secrets.
Fixes fugue#96

Also, we now expose the `nonce` being used for CTR
mode- see fugue#75. To be resolved in a future update.
…dom nonces.

Fixes fugue#73 by no longer treating all values as unicode.

Fixes fugue#75 by generating & storing a unique nonce.
Also move configuration of encryption to `credsmash.cfg`,
so the `--digest` parameter has been removed from `put`.
…sh` data.

Adds the new `render-template` and `render-templates` commands.
…has changed.

You can avoid this behaviour by using `--version`, or `--no-compare`.
The new naming makes it's purpose clearer.
You can now have full control over the output of your templates.
@davbo
Copy link

davbo commented Mar 22, 2017

@nathan-muir Thanks for flagging up the risk of backward incompatibility from #136.

Let me know if I can contribute anything to your fork to allow for such a change. I'm keen to see some default authenticated data included in credstash, either directly contributed to credstash or by way adding it to your fork.

@nathan-muir
Copy link
Contributor Author

nathan-muir commented Mar 23, 2017

@davbo Initially I was a little hesitant to rely on another AWS/KMS feature - in favour of supporting AEAD in AES-GCM directly, but after some thought there isn't any substantial reasons to suggest that change.

After a bit of review, I would only suggest the following changes:

  1. Don't use a padded-int for the version field. (In this fork) it is an implementation detail of DynamoDB storage, and the rest of the code sees a regular int.

  2. Rather than using try/except to detect the format of secrets, consider adding an attribute to the object, eg. {"aead": "v1"}/ {"protocol": "2"} which tells us how to properly decode it.

    Relying on exceptions in a specific component becomes hard to refactor later into clean/separate components.

@collisdigital
Copy link

We're looking to use credstash and support for AES-GCM-256 would be very welcome along with some of the other changes from this fork. @alex-luminal this is quite a large PR; is it likely to merge in the near future or would we be better splitting out some specific things (like GCM support) into a smaller PR to get those things in sooner?

`pip` can behave unexpectedly with some version constraints.
  1. It will abort when constraints aren't satisfiable using the highest version
  2. It will not consider multiple constraints on a single dependency

In general, it is safer to remove constraints from setup.py, and allow
users to ensure install which-ever dependencies work.
@gene1wood
Copy link
Contributor

@collisdigital I would suspect the new features in this PR would be much more likely to get merged if they were pulled out into their own PRs. If there's a feature in this PR that you'd like to see, breaking it into it's own PR is probably a good path.

@collisdigital
Copy link

@gene1wood thanks, to be honest this has fallen off our radar now as we decided to go with a different solution in the end.

@wayne-luminal
Copy link
Contributor

@nathan-muir Apologies for taking so long to get back to you. Thanks for the awesome work! I'm in the process of reviewing all these PRs and my gameplan for all these PRs is basically to bring everything into an integration branch (assuming it's suitable to merge upstream) and ultimately release a new major version of credstash. I am documenting breaking changes like you correctly pointed out for #136 and intend to provide upgrade instructions.

Overall your PR looks like it's something we can pull most of as it has a few things I intended to do anyway.

Consolidate api/cli into a single core module.

It's now much easier to load & call credsmash via python.

Core - Perform version/compare before sealing new secret.

DynamoDB - Create separate `get_one` and `get_latest`.

This also removes the `name` & `version` from the ciphertext output,
as it is an implementation detail of how DynamoDB stores data.
Crypto - Support additional authenticated data on key service
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.

5 participants