Skip to content

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

Merged
merged 5 commits into from
Jun 14, 2021
Merged

Conversation

deansheather
Copy link
Member

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.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 10, 2021

✨ Coder.com for PR #401 deployed! It will be updated on every commit.

@@ -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`
Copy link
Contributor

@jawnsy jawnsy Jun 10, 2021

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

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Contributor

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? 🤷‍♂️

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@deansheather deansheather requested a review from khorne3 June 14, 2021 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants