-
Notifications
You must be signed in to change notification settings - Fork 138
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 Decoding to Secrets #408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good! Left a few small comments.
src/action.js
Outdated
@@ -17,6 +17,8 @@ async function exportSecrets() { | |||
const secretsInput = core.getInput('secrets', { required: false }); | |||
const secretRequests = parseSecretsInput(secretsInput); | |||
|
|||
const secretEncoding = core.getInput('secretEncoding', { required: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anything validate that the given encoding is one of "base64", "hex", "utf8"? Are their other encodings that could work via the buffer.From API that we'd want to prevent from using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good note. I will add a check for supported encoding type.
action.yml
Outdated
@@ -76,6 +76,9 @@ inputs: | |||
description: 'Time in seconds, after which token expires' | |||
required: false | |||
default: 3600 | |||
secretEncoding: | |||
description: 'Encoding of the secret value. Can be "base64", "hex", "utf8".' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first read this, I thought it would encode the secret value. I see that at actually decodes based on the encoding set here. Perhaps we can improve the description to make that more obvious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true. Reading it again now I can see how it is fairly confusing. Will work on making that clearer
@@ -17,6 +17,8 @@ async function exportSecrets() { | |||
const secretsInput = core.getInput('secrets', { required: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth dropping a test case in here to cover the behavior. Not sure how challenging that is (still learning about vault-action myself 🙂).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* Add decoding to secrets * remove index.js * Add test case, and other updates
Adds support for encoded Vault secrets.
Example usage:
Resolves #105