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

Add a KMS Envelope-Encryption Renderer #46155

Merged
merged 4 commits into from Mar 12, 2018

Conversation

Projects
None yet
4 participants
@kojiromike
Copy link
Contributor

commented Feb 23, 2018

What does this PR do?

This provides a renderer to decrypt values stored using a KMS data key as per KMS envelope encryption. It is pretty similar to the gpg renderer -- it just uses a different approach to do the actual decryption. This is something that is useful to me. I hope it's useful to others, too.

New Behavior

  1. Provide a encrypted KMS data key in your master config under the name aws_kms_data_key.
  2. Ensure that the salt master has appropriate iam permissions and configuration to access the KMS key needed to decrypt the data key.
  3. Encrypt some values using the plaintext value of the data key with cryptography.fernet and put them in a sls file to be rendered via the kms renderer provided in this PR.

Notes and sales pitch

This is most useful for salt users already in an AWS environment who need shared secrets stored in pillars and prefer to put them e.g. in Git rather than in S3. With GPG you would need each salt user to have their own key, but encrypted PGP messages get linearly larger with each recipient key. Of course, you could use a single shared GPG key, but then you'd have to figure out how to securely share that key. KMS just handles the securely-sharing keys bit for us.

Tests written?

Yes

Commits signed with GPG?

Yes

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

Please rename to aws_kms as kms is to generic.
kms_data_key to something like aws_kms_data

@kojiromike

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2018

OK, I did that.

@kojiromike

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2018

It occurs to me that the main difference between GPG and AWS_KMS is the configuration of a particular decrypt function. While I don't have time to work on a generic decrypt renderer just now, I'm curious what kind of interest there would be in such a thing.

@damon-atkins

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

The trick is to make the salt interface generic so you can swap one in and swap another out. Bit like pkg almost does

@kojiromike kojiromike force-pushed the kojiromike:kms-renderer branch from 2a3d936 to 97c86f4 Feb 25, 2018

@kojiromike

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

Updates: I've rewritten some of the code to facilitate testing, tweaked the docblock a bit, renamed the renderer to aws_kms and the config key to aws_kms_data_key and written unit tests.

@kojiromike kojiromike force-pushed the kojiromike:kms-renderer branch from 97c86f4 to 6c5cfe3 Feb 25, 2018

@rallytime rallytime requested a review from gtmanfred Feb 28, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

@kojiromike Can you write some tests for these new files?

@kojiromike

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2018

@rallytime I did write tests for these new files. Did I miss something?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2018

@kojiromike Oh, you're right. I'm not sure how I missed that. My apologies. It does look like one of them is failing however: https://jenkins.saltstack.com/job/PR/job/salt-pr-rs-cent7-n/16770/.

There is also a small lint error: https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/19717/violations/file/salt/renderers/aws_kms.py/

@kojiromike

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2018

@rallytime Oh, thanks, I fixed the whitespace error. When I run the tests locally (./tests/runtests.py -n unit.renderers) I get zero errors. But I can't view the result in Jenkins -- it seems to just load forever.

@kojiromike

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2018

I was able to see the Jenkins error now. I will attempt to reproduce locally and fix.

@kojiromike

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2018

Whew, that was a bigger task than I thought. Not only was it a legitimate error, but I was clearly letting some library calls slide through the mocks.

I realized that I was testing locally with my AWS fully configured. Jenkins was showing a NoRegionError, but it could have been any number of aws configuration errors. Rather than re-invent aws configuration in salt configuration, I only implemented one additional and optional AWS configuration key: aws_kms:profile_name. Users may skip this setting and use the default profile, but whichever profile they use must have all the configuration that boto3.client('kms') needs. Still, this renderer will raise a SaltConfigurationError with a hopefully-helpful message for the most common AWS client configuration errors that affect it.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

@kojiromike kojiromike force-pushed the kojiromike:kms-renderer branch from d4d4bab to e8f2037 Mar 7, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2018

@kojiromike It looks like your new tests are failing on the PY3 builds. Mind taking another look?

kojiromike added some commits Feb 23, 2018

@kojiromike kojiromike force-pushed the kojiromike:kms-renderer branch from e8f2037 to 13cc08d Mar 10, 2018

@kojiromike

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

Yep, fixed up. Sorry, not used to working in both major versions of Python at the same time.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

re-run py3

@rallytime rallytime merged commit f5abf1b into saltstack:develop Mar 12, 2018

4 of 10 checks passed

codeclimate 9 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #3079 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #20714 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #7785 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #17110 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #23136 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #15430 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #20043 — SUCCESS
Details

@kojiromike kojiromike deleted the kojiromike:kms-renderer branch Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.