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 "sctl read" command. #53

Merged
merged 3 commits into from
Nov 25, 2019
Merged

Add "sctl read" command. #53

merged 3 commits into from
Nov 25, 2019

Conversation

lazypower
Copy link
Contributor

sctl read will scan the envelope and attempt to find the NAMED secret.
If its found, it will decode, decrypt, and display the secret value.
This is to help ease the use-case for reading secrets serialized to
state without needing to intercept the values in an intermediary
process, similar to the hacky work-around of running sctl run env just
to view the secrets.

This feature needs test coverage before merge.

sctl read will scan the envelope and attempt to find the NAMED secret.
If its found, it will decode, decrypt, and display the secret value.
This is to help ease the use-case for reading secrets serialized to
state without needing to intercept the values in an intermediary
process, similar to the hacky work-around of running `sctl run env` just
to view the secrets.
commands/cli.go Outdated
var client cloud.KMS
if keyURI == "" {
log.Warn("No KeyURI found in envelope. Required usage of flag/env config.")
err := validateContext(c, "read")
Copy link
Contributor

Choose a reason for hiding this comment

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

validateContext is called both here and at the beginning of the read handler fn (L272) -- is this desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this gave me an idea for a better pattern that i used in the read function post review-feedback. I swap the context evaluation between default and named, which deviates just a slight bit from how it was being used before.

utils/secrets.go Outdated Show resolved Hide resolved
@@ -88,8 +92,23 @@ func BuildContextualMenu() []cli.Command {
plaintext = []byte(base64.StdEncoding.EncodeToString(plaintext))
}

// Init a KMS client
client := cloud.NewGCPKMS(c.String("key"))
// Work with the envelope's provided key or switch to CLI flags/env
Copy link
Contributor Author

@lazypower lazypower Nov 25, 2019

Choose a reason for hiding this comment

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

Gah, i should have probably refactored this chunk into another commit and a pr all to itself. The add method was a bit broken post key-uri-in-envelope. I didn't notice it would never attempt to open/read/use-the-key-in-envelope-format - always defaulting to ENV. Which can be problematic when the expectation is to only need to specify --key once in a great while once its in the env it should be favored.

LMK if you'd prefer I back these hunks out into a different PR. if not i'll just sqeak this in as a feature/bugfix release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be fine to include in this PR

@lazypower lazypower marked this pull request as ready for review November 25, 2019 19:27
commands/cli.go Outdated
Comment on lines 50 to 53
//:= validateContext(c, "add")
//if err != nil {
// return err
//}
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this if no longer using

@@ -88,8 +92,23 @@ func BuildContextualMenu() []cli.Command {
plaintext = []byte(base64.StdEncoding.EncodeToString(plaintext))
}

// Init a KMS client
client := cloud.NewGCPKMS(c.String("key"))
// Work with the envelope's provided key or switch to CLI flags/env
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be fine to include in this PR

@lazypower lazypower merged commit d385597 into master Nov 25, 2019
@lazypower lazypower deleted the feat/read branch November 25, 2019 21:52
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.

2 participants