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

feat: Optionally allow scoped access to vault token through outputs #353

Closed
wants to merge 0 commits into from

Conversation

TomNorth
Copy link
Contributor

@TomNorth TomNorth commented Aug 3, 2022

This PR is to address #352

  • Optionally sets vault_token output to the action, so that we can use it in our workflows after authenticating in a scoped fashion.

@hashicorp-cla
Copy link

hashicorp-cla commented Aug 3, 2022

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


11 out of 12 committers have signed the CLA.

  • dependabot[bot]
  • TomNorth
  • tvoran
  • benashz
  • prizov
  • vinay-gopalan
  • kevinschoonover
  • mldahl
  • austingebauer
  • maxcoulombe
  • ltcarbonell
  • Tom North

Tom North seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA. If you already have a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

@TomNorth TomNorth changed the title feat: Always allow scoped access to vault token through outputs feat: Optionally allow scoped access to vault token through outputs Aug 3, 2022
src/action.js Outdated
@@ -69,8 +70,11 @@ async function exportSecrets() {
defaultOptions.headers['X-Vault-Token'] = vaultToken;
const client = got.extend(defaultOptions);

command.issue('add-mask', vaultToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you used the line that already existed below but I believe that it is recommended to use the specific functions from the toolkit to mark a variable or value as sensitive if implementing a reusable action in javascript.

core.setSecret(vaultToken)

Also, dynamic sensitive values created by the action should be marked as secret as close to their instantiation as possible so they are not logged by mistake in between their creation and masking. I think it'd be preferable to move this line right after the moment the token is generated on l.69.

Copy link
Contributor

@RichiCoder1 RichiCoder1 Feb 28, 2023

Choose a reason for hiding this comment

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

Yup. If this is emulating this line I believe it's using the manual commmand just because the setSecret function wasn't available at the time.

@maxcoulombe
Copy link
Contributor

One small suggestion and fixing the merge conflicts and I think this would be good to ship. Thanks for taking care of this.

@maxcoulombe maxcoulombe added the waiting-for-response Indicates a maintainer asked a question that must be answered by contributors before proceeding label Feb 28, 2023
@TomNorth TomNorth closed this Mar 25, 2023
@TomNorth TomNorth mentioned this pull request Mar 25, 2023
@TomNorth
Copy link
Contributor Author

Sorry @maxcoulombe I borked my fork trying to sort this out and accidentally closed this PR. I have reopened another one. Unfortunately the CLA is failing because some commits were made with an email address associated with my old workplace, but the new one is added, you can see signed as TomNorth.

@maxcoulombe
Copy link
Contributor

maxcoulombe commented May 19, 2023

@TomNorth Thanks for the fixes and reopening the PR. I'm confirming internally if I can merge the PR #441 even with one signature missing on the CLA due to the circumstances you described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-response Indicates a maintainer asked a question that must be answered by contributors before proceeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants