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

added encrypted-regex option #509

Merged
merged 5 commits into from
Aug 28, 2019
Merged

added encrypted-regex option #509

merged 5 commits into from
Aug 28, 2019

Conversation

jpriebe
Copy link
Contributor

@jpriebe jpriebe commented Aug 14, 2019

Our team would like to use sops to protect kubernetes secrets in YAML files.

Here's what we want to do:

  • encrypt all secret literals, regardless of whether they are under the "data" or "stringData" keys
  • not encrypt any other values in the file, so we can still easily visually scan the encrypted files (so for instance, we could see the unencrypted secret names in a git diff)
  • not require developers to put any kind of suffix onto key names
  • not require any additional pre- or post-processors; we want to use sops decrypt and pipe straight to kubectl apply.

Here's what we've tried:

  • without using encrypted suffix, all values will be encrypted, which makes it impossible to visually scan an encrypted YAML file for fields like metadata.name
  • you can add an encrypted suffix like "-encrypted" to the "data" and "stringData" keys, but then the decoded YAML will not be legal, and you'll need a post-processor to apply the files to a k8s cluster (and you'd have to rely on your developers always adding the "-encrypted", or they risk putting unencrypted secrets into the git repo)
  • you could use an encrypted suffix like "ata" to automatically encrypting up the "data" and "stringData" keys, but that has the side effect of encrypting the "metadata" keys, which means you can't visually inspect the metadata.name while the file is encrypted
  • you could use an encrypted suffix like "stringData" and try to enforce a policy that your developers always put their secrets into "stringData" not "data", but all it takes is one mistake to commit a secret to the git repo

None of these were satisfactory. So I added a new option: encrypted-regex, which lets you encrypt only those values whose keys match the given regular expression. With this option, you can do this:

sops --encrypt --encrypted-regex '^(data|stringData)$'

This will encrypt our data and stringData values, but nothing else.

I know there was an objection to PR 385 (#385) last year on the grounds that the behavior should be moved into the library. I am hoping that you won't view this PR the same way. I'm not sure I have the resources for a deep dive into the sops codebase. But this small change I'm making brings (IMHO) tremendous value to the product wrt kubernetes secrets files.

cmd/sops/main.go Outdated Show resolved Hide resolved
@autrilla
Copy link
Contributor

autrilla commented Aug 14, 2019

Thanks for the detailed comment explaining your use case and what you've tried. My only concern with this is that it complicates the CLI and code a bit.

I'd like to see the feedback of @ajvb as well

Julien also had this concern but I don't think it applies here as it's entirely a new flag.

@jpriebe
Copy link
Contributor Author

jpriebe commented Aug 15, 2019

Thanks for your consideration. I can see the case for multiple instances of encrypted-suffix (or even something more explicit like encrypted-key).

I implemented encrypted-regex because it was the shortest path to getting the functionality I need. Supporting multiple flag instances and []string would require much more significant refactoring of the existing code.

@jpriebe
Copy link
Contributor Author

jpriebe commented Aug 15, 2019

Caught an issue with config_test.go. I didn't realize that "go test" didn't run all the tests -- had to run ./test.sh to get all tests running, which exposed my mistake.

README.rst Outdated Show resolved Hide resolved
@autrilla
Copy link
Contributor

@ajvb: based on the number of people who've been active on related issues, I'd say this feature is useful enough to be added to sops. Let me know if you have any concerns, otherwise I'll merge it.

Co-Authored-By: Adrian Utrilla <adrianutrilla@gmail.com>
Copy link
Contributor

@ajvb ajvb left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thank you @jpriebe for the PR, as well as the detailed use-case and why the current config options do not work.

@autrilla 👍 I agree.

@autrilla autrilla merged commit d505c3e into getsops:develop Aug 28, 2019
@autrilla
Copy link
Contributor

Looks like merging this broke some tests

@autrilla
Copy link
Contributor

Looks like merging this broke some tests

It actually didn't, that seems unrelated because when reverting this, they still fail.

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