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

Update keyvalue.py to be able to add new values without leaking them in the shell history. #5164

Merged

Conversation

wingiti
Copy link
Contributor

@wingiti wingiti commented Feb 24, 2021

Add possibility to add a value by using a prompt instead of a parameter. This prevents secrets from being documented/leaked in the shell history.

Add possibility to add a value by using a prompt instead of a parameter. This prevents secrets from being documented/leaked in the shell history.
@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Feb 24, 2021
@CLAassistant
Copy link

CLAassistant commented Feb 24, 2021

CLA assistant check
All committers have signed the CLA.

@Kami Kami added this to the 3.5.0 milestone Feb 24, 2021
instance.scope = args.scope
instance.user = args.user

if not args.value:
instance.value = input("Please insert value for key: ")
Copy link
Member

Choose a reason for hiding this comment

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

There is also getpass.getpass() which displays entered value masked on the screen.

But if we go with that approach, we will likely need to ask for confirmation (aka input the secret value twice).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there anything you want to change here or will it be implemented in version 3.5? Nevertheless it is possible to check the value after it was inserted in the key value store.

Copy link
Member

@cognifloyd cognifloyd Jun 2, 2021

Choose a reason for hiding this comment

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

In a future PR, I think we could add getpass.getpass() support, but only turn it on if encrypt is True. I think this PR is good enough to merge as is.

@cognifloyd
Copy link
Member

We just reformatted the code with black. (Hooray!) And this PR got caught in the cross fire too. (Arrgh!)
Please merge master into your branch, resolve the conflicts (Ouch! Sorry!), and reformat with black (I recommend running pre-commit install after you've merged master so that black reformatting happens automatically on commit).

@wingiti wingiti force-pushed the add-value-without-leak-in-history branch from 4dd01ed to cd76da9 Compare March 11, 2021 17:58
@arm4b
Copy link
Member

arm4b commented Jun 2, 2021

The tests look 💚 again.

I expect @Kami to review it again, but @wingiti can you please also add the Changelog.rst record for this PR? We'll need it anyways.

Thanks!

@arm4b arm4b requested a review from Kami June 2, 2021 11:38
instance.scope = args.scope
instance.user = args.user

if not args.value:
instance.value = input("Please insert value for key: ")
Copy link
Member

@cognifloyd cognifloyd Jun 2, 2021

Choose a reason for hiding this comment

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

In a future PR, I think we could add getpass.getpass() support, but only turn it on if encrypt is True. I think this PR is good enough to merge as is.

@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/XS PR that changes 0-9 lines. Quick fix/merge. labels Jun 2, 2021
@amanda11 amanda11 modified the milestones: 3.5.0, 3.6.0 Jun 29, 2021
@wingiti
Copy link
Contributor Author

wingiti commented Jul 14, 2021

@Kami do you want to approve this? Would be nice, otherwise I have to overwrite this all the time after a Stackstorm upgrade in my local installations.

Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

@wingiti The lint checks are failing as there is trailing space on the CHANGELOG.rst.

Could you amend, and check if you have signed the CLA (there was a new version issued, so you may have to re-sign).

Many thanks.

@cognifloyd cognifloyd merged commit 6778fd6 into StackStorm:master Oct 3, 2021
@wingiti wingiti deleted the add-value-without-leak-in-history branch December 17, 2021 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI component:st2client size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants