-
Notifications
You must be signed in to change notification settings - Fork 81
Avoid pulling full values file during install #401
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
Conversation
✨ Coder.com for PR #401 deployed! It will be updated on every commit.
|
setup/installation.md
Outdated
@@ -96,14 +94,14 @@ kubectl config set-context --current --namespace=coder | |||
``` | |||
|
|||
To create the `passwordSecret`, run | |||
`kubectl create secret generic <NAME> --from-file=test=/dev/stdin` | |||
`echo "UserDefinedPassword" | kubectl create secret generic <NAME> --from-file=password=/dev/stdin` |
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 defeats the purpose... the reason this was suggested before was to avoid putting secrets in history files (the same can be achieved by prefixing the command with a space, of course). Doing this echo puts the password back in the history
If you want to put the password in the command line, we can use --from-literal instead of --from-file, then avoid the echo
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.
If we're saying to use stdin in a terminal then they have to type the password and hit Ctrl+D
, which we don't tell people to do right now
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.
So really it's a balance between making the installation experience easy and risk logging password, or making the installation experience more complicated: "Oh just run this command, type your password then if you're on windows click ctrl+d or if you're on mac click cmd+d..."
I think we should use from-literal and put a warning saying it may be logged in shell history, and if they don't want that then put a space at the front
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.
I think @sreya suggested this here: #302 (comment), you might want to check with him. I do not have strong feelings about it. It's too bad kubectl doesn't have something built-in to do this.
This way of doing it probably doesn't work on Windows, right? I also dunno about mac, presumably it's Unix-y enough that it works there? 🤷♂️
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.
One other small thing: I think command lines can be seen by anyone in /proc for multi-user systems, so putting the password in the command line means that it's potentially visible to everyone logged into the system. We can just document it though, too.
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.
I think doing it on the command line is fine. Most people aren't working on a multi-tenant box.
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.
if it's easy or clean to show both ways so that we can avoid some rando opening a PR saying our docs leak secrets in a multi tenant system that would be ideal but either way this is pretty minor.
Pulling full values file from the helm chart has stuff like
cemanager.image
etc. which we don't want the user to have in their values file unless they are specifically pulling images from another registry.