feat: added credential_process option to config #17
Conversation
|
Thanks @elyzov! I´ll try to take a look over the week-end |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #17 +/- ##
==========================================
- Coverage 89.61% 88.88% -0.73%
==========================================
Files 6 7 +1
Lines 260 279 +19
==========================================
+ Hits 233 248 +15
- Misses 13 15 +2
- Partials 14 16 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for the contribution, love the way AWS CLI allows this, makes it pretty flexible.
To address:
- Some minor comments
- Couple of tests missing
- Adding the new variable to the man config in
README.md
Maybe something like
# External process for sourcing Vault token
# If specified, `token` will be ignored
# credential_process = "get-vault-token -f credential-process"You already documented in its own section, but I like to have a complete configuration
someone can copy paste if needed.
- Also one question: I see this new behaviour is added as configuration variable but not as a runtime argument; is this on purpose?
CHANGELOG.md
Outdated
|
|
||
| ### Added | ||
|
|
||
| - Ability to specify `credential_process` option in the config to sourcing Vault token with an external process |
There was a problem hiding this comment.
| - Ability to specify `credential_process` option in the config to sourcing Vault token with an external process | |
| - Ability to specify `credential_process` option in the config to source Vault token from an external process |
README.md
Outdated
| [global] | ||
| # Vault address | ||
| address = "http://127.0.0.1:8200" | ||
| # External process for sourcing Valet token |
There was a problem hiding this comment.
| # External process for sourcing Valet token | |
| # External process for sourcing Vault token |
README.md
Outdated
| ```json | ||
| { | ||
| "Version": "1", | ||
| "Token": "an Vault access token" |
There was a problem hiding this comment.
| "Token": "an Vault access token" | |
| "Token": "a Vault access token" |
README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| - **RVault** should run the process, parse the JSON from stdout, and add the token to the global configuration. |
There was a problem hiding this comment.
| - **RVault** should run the process, parse the JSON from stdout, and add the token to the global configuration. | |
| - **RVault** will run the process, parse the JSON from stdout, and add the token to the global configuration. |
cmd/root.go
Outdated
| if err != nil { | ||
| klog.Exitf("Can't execute credential process '%s': %v", process, err) | ||
| } | ||
| viper.SetDefault("global.token", token) |
There was a problem hiding this comment.
To get the behaviour stated in the documentation, viper.Set should be used instead. Otherwise an existing global.tokenin the configuration file will take precedence over the default value.
| ) | ||
|
|
||
| const ( | ||
| DefaultVersion = "1" |
There was a problem hiding this comment.
I would rather unexport this one as it's not consumed outside of the package
There was a problem hiding this comment.
@kir4h I see your concerns, but, as far as I can see, it's not violates any security aspects, but allow to refer this values from the other packages if it's required. On the other hand your proposal restricts the usage of this variables within the single package and will require additional efforts to to provides them for extarnal usage if it's required. Maybe I can't see some other rational in your proposal, so could you please elaborate a bit with your point of view.
There was a problem hiding this comment.
Not security related, more in line of YAGNI principle (the reason I noticed was the code inspector showing a warning for it)
Also In my view exporting a constant makes think that the constant is used elsewhere, and thus a refactor could be a breaking change where in reality that variable was only consumed internally and thus there is no breaking change.
And if at some point we want to export it it´s just a matter of a small refactor to do so, no big effort there at all.
internal/pkg/credential/process.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| NoCommandPassed = fmt.Errorf("expected at least a single command to execute") |
| cmd.Env = []string{ | ||
| fmt.Sprintf("VAULT_ADDR=%s", viper.GetString("global.address")), | ||
| } |
There was a problem hiding this comment.
Worth documenting that this logic will inject VAULT_ADDR environment variable
resolves #16