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

kamal app exec includes secrets #1223

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrewjanssen
Copy link

@andrewjanssen andrewjanssen commented Nov 18, 2024

Currently, containers created by kamal app exec don't have secrets. It would be helpful if they did have secrets, as env vars, like Kamal's other containers. This PR does that.

Resolves #1180.

@djmb
Copy link
Collaborator

djmb commented Nov 21, 2024

Not requiring the secrets allows someone to run kamal app exec without having access to the secrets manager set up.

This means you can do things like giving someone access to a Rails console without having to set up authentication for wherever the secrets come from.

It is a little bit janky in that you need a deployment first to ensure the right secrets are in place, but I think that's worth it.

@andrewjanssen
Copy link
Author

andrewjanssen commented Nov 30, 2024

Not requiring the secrets allows someone to run kamal app exec without having access to the secrets manager set up.

Not requiring secrets is certainly beneficial, that's how I want Kamal to be too. This PR doesn't change that; if you haven't set any secrets, the behavior remains the same as it was without this PR.

This means you can do things like giving someone access to a Rails console without having to set up authentication for wherever the secrets come from.

Yes— that specific low-friction onboarding experience is important to me too and should be kept. This PR doesn't affect it. (In my case, the Rails console is not usable by default without the secrets, because RAILS_MASTER_KEY is a secret, which is what prompted me to write this PR. Certainly some meaningful percentage other people have a similar set-up as me.)

It is a little bit janky in that you need a deployment first to ensure the right secrets are in place, but I think that's worth it.

I don't intend for this PR to make secrets required. :) I only wanted this PR to inject any secrets which have been set. Lmk if I'm on the same page as you! Thanks for the review.

@djmb
Copy link
Collaborator

djmb commented Dec 2, 2024

Not requiring secrets is certainly beneficial, that's how I want Kamal to be too. This PR doesn't change that; if you haven't set any secrets, the behavior remains the same as it was without this PR.

Oh sorry, I wasn't very clear there! What I mean is a situation where you have multiple users and some have the secret access set up and some don't. So you do have secrets, but you want the commands that don't need them to work for everyone.

This is for simplicity and not security though - anyone who can run commands can SSH to the servers and read the secrets anyway.

Where you have integrated a secret manager, your PR will make setting up secret access required for the app exec command because the secrets are lazy loaded by KAMAL.primary_role.env(KAMAL.primary_host).secrets.to_h.

If you have deployed to a server already, the secrets should be there, so I think this should only be an issue if that's not the case.

@cromega
Copy link

cromega commented Dec 2, 2024

I would be more than happy with a secrets push command or even an optional argument to exec that would do this (and would fail if some secrets are missing)

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.

kamal app exec doesn't push secrets to servers
3 participants