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 --encrypted-selector option #385

Closed
wants to merge 3 commits into from

Conversation

dictcp
Copy link
Contributor

@dictcp dictcp commented Oct 20, 2018

per #368, sometimes we need to pick only a subtree to encrypt (eg. Kubernetes secret). then it should be able to encrypt Kubernetes secret just like https://github.com/shyiko/kubesec

@codecov-io
Copy link

codecov-io commented Oct 20, 2018

Codecov Report

Merging #385 into master will increase coverage by 0.37%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
+ Coverage   45.32%   45.69%   +0.37%     
==========================================
  Files          12       12              
  Lines        1593     1604      +11     
==========================================
+ Hits          722      733      +11     
  Misses        797      797              
  Partials       74       74
Impacted Files Coverage Δ
config/config.go 77.5% <100%> (+0.28%) ⬆️
sops.go 54.69% <100%> (+1.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c72237...457297f. Read the comment docs.

Copy link
Contributor

@autrilla autrilla left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot for the patch!

Other than my comments, the only other thing blocking merge is the lack of documentation. It would be nice to include an example of how one might use this for Kubernetes files.

@@ -133,6 +133,66 @@ func TestEncryptedSuffix(t *testing.T) {
}
}


func TestEncryptedSelector(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add test cases for YAML lists?

@@ -45,6 +45,7 @@ type Metadata struct {
PGPKeys []pgpkey `yaml:"pgp" json:"pgp"`
UnencryptedSuffix string `yaml:"unencrypted_suffix,omitempty" json:"unencrypted_suffix,omitempty"`
EncryptedSuffix string `yaml:"encrypted_suffix,omitempty" json:"encrypted_suffix,omitempty"`
EncryptedSelector string `yaml:"encrypted_selector,omitempty" json:"encrypted_selector,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: gofmt this file

@@ -318,6 +318,13 @@ func (tree Tree) Encrypt(key []byte, cipher Cipher) (string, error) {
}
}
}
if tree.Metadata.EncryptedSelector != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is good, but I think a better approach would be to convert the selector to a []string (in main.go, so the SOPS API itself would take the []string, and the command line will handle conversion) and then compare the slices. It saves us some computations we currently do for every decryption, and more importantly would provide a cleaner API to those using SOPS as a Go library.

What do you think?

@@ -429,6 +429,7 @@ func main() {

unencryptedSuffix := c.String("unencrypted-suffix")
encryptedSuffix := c.String("encrypted-suffix")
encryptedSelector := c.String("encrypted-selector")
Copy link
Contributor

Choose a reason for hiding this comment

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

@jvehent
Copy link
Contributor

jvehent commented Oct 11, 2019

This PR is stale and conflicts with a lot of other files. Should we close it?

@autrilla
Copy link
Contributor

It can be reopened if needed, so I guess so.

@autrilla autrilla closed this Oct 11, 2019
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.

4 participants