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 doc warning about shell history logging for commands that accept credentials #3371

Closed
2 tasks done
cglacet opened this issue Nov 17, 2020 · 8 comments · Fixed by #5726
Closed
2 tasks done

Add doc warning about shell history logging for commands that accept credentials #3371

cglacet opened this issue Nov 17, 2020 · 8 comments · Fixed by #5726
Labels
area/docs Documentation issues/improvements

Comments

@cglacet
Copy link

cglacet commented Nov 17, 2020

  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have searched the documentation and believe that my question is not covered.

Feature Request

I think to make sure private keys/password are secured, we should remove poetry config commands that have any sensitive information from the terminal history (or replace sensitive information with stars?):

$ poetry config http-basic.mypi __token__ azpAEkFPOK5pokErkPOFd
$ history | grep "poetry config http-basic"
poetry config http-basic.mypi __token__ azpAEkFPOK5pokErkPOFd

Expected behaviour would be to have:

$ poetry config http-basic.mypi __token__ azpAEkFPOK5pokErkPOFd
$ history | grep "poetry config http-basic"
poetry config http-basic.mypi __token__ azp***********Fd

It seems like a bit of work to make that work in every situation, but on the other hand its probably relatively easy for the most common use cases (bash/zsh/fish). Did anyone worked on that yet?

@cglacet cglacet added kind/feature Feature requests/implementations status/triage This issue needs to be triaged labels Nov 17, 2020
@sinoroc
Copy link

sinoroc commented Nov 17, 2020

In theory it is a good idea. In practice that seems quite complicated, if not impossible. But...

1. If I remember right, a good trick to know for such cases (at least in bash) is to prefix the command with an empty space so that it is not added to the history.

2. If you omit the value, then it is prompted interactively, and thus the actual token is not added to the history.

$ poetry config http-basic.mypi __token__
Password: 
$ history | grep "poetry config http-basic"
poetry config http-basic.mypi __token__

@cglacet
Copy link
Author

cglacet commented Nov 18, 2020

@sinoroc these two solutions look quite good to me. That would just be a matter of documenting this so people are at least aware of that (small) potential risk.

You think it's impossible to patch the current command because it's too dependent on the system you are on? (Eg. deleting the last history line)

@sinoroc
Copy link

sinoroc commented Nov 18, 2020

@cglacet

You think it's impossible to patch the current command because it's too dependent on the system you are on? (Eg. deleting the last history line)

Because as far as I know, no command/binary/program/executable can instruct the calling shell to alter (or not save) the command into its history. I would not know how to do that. And it would need to work with all shells, even ones that might not exist yet. Maybe there are solutions, and I just do not know about them...

Even the trick with the leading empty space, might not always work. It seems to work with bash by default, but it actually depends on how the HISTCONTROL environment variable is set. This StackOverflow discussion has some details.

You best bet is to simply not write the secret on the command line directly.

@cglacet
Copy link
Author

cglacet commented Nov 18, 2020

@sinoroc For me, indeed, the simplest solution is to omit the password.

I feel like that should be the standard (documented) way because having a password in the history isn't something I find very safe. On the other hand when someone has access to your local history its very likely that this will not be the only issue. I just noticed that and came here to let you know about that minor issue.

I'm still curious about how this could be solved tho. I don't know how commonly the HISTFILE env variable is used. Using this could allow rewriting the history (rewriting $HISTFILE's file content).

@sinoroc
Copy link

sinoroc commented Nov 18, 2020

I feel like that should be the standard (documented) way because having a password in the history isn't something I find very safe.

Thing is that poetry config is not used for secrets only, as far as I remember. So for other configuration settings, having the value in the command line is probably harmless and more comfortable. So, there would have to be a UX compromise there.

I just noticed that and came here to let you know about that minor issue.

Agreed, I believe it should be considered as well.

I'm still curious about how this could be solved tho. I don't know how commonly the HISTFILE env variable is used. Using this could allow rewriting the history (rewriting $HISTFILE's file content).

I would also be interested to see how this could be solved. HISTFILE is only bash though, right? There are many other shells to cover.

@cglacet
Copy link
Author

cglacet commented Nov 18, 2020

HISTFILE is only bash though, right?

It's also used by zsh. But apparently not by fish.

@abn abn added area/docs Documentation issues/improvements and removed kind/feature Feature requests/implementations status/triage This issue needs to be triaged labels Nov 19, 2020
@abn
Copy link
Member

abn commented Nov 19, 2020

We should just add a warning about this risk in the documentation alongside examples talking about configuring secrets. Maybe update the title and description as such.

@abn abn changed the title Remove config commands from history (CLI) Add docuemntation warning about shell history logging for commands that accept credentials May 12, 2022
@abn abn changed the title Add docuemntation warning about shell history logging for commands that accept credentials Add doc warning about shell history logging for commands that accept credentials May 12, 2022
Copy link

github-actions bot commented Mar 2, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/docs Documentation issues/improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants