-
Notifications
You must be signed in to change notification settings - Fork 458
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 user to docker group if not superuser #1095
base: main
Are you sure you want to change the base?
Conversation
|
I thought any group changes requires we logout/back-in or maybe even restart the server? We had that issue with Omakub. |
86af751
to
61a92dd
Compare
Finished in 597.636880s, 0.8617 runs/s, 3.1909 assertions/s. |
Thanks for the comment @dhh I've updated the fix and it should now work with I've created a kamal container for anybody who wants to test this version with their configuration.
Please give it a go and flag up any issues. |
Update to reduce the ferocity of the session termination. Now only terminates the initiating kamal session. |
Hmm, starting to think that we're probably better off failing fast and telling the user to do this themselves with clear instructions. |
If you don’t like the kill then the original change is fine in the bootstrap since kamal disconnects. Then the next time kamal is run, all is well. What’s the issue you are seeing? |
Turns out a HUP signal does work with sshd when it's the session lead allowing it to clean up properly. |
The newgrp command can be used to load the new group during a session: https://linux.die.net/man/1/newgrp |
lib/kamal/commands/docker.rb
Outdated
# If we're not root and not already in the docker group | ||
# add us to the docker group and terminate all our current sessions | ||
def add_group | ||
[ '[ "${EUID:-$(id -u)}" -eq 0 ] || id -nG "${USER:-$(id -un)}" | grep -qw docker || { sudo usermod -aG docker "${USER:-$(id -un)}" && kill -HUP ${PPID:-ps -o ppid= -p $$}; }' ] |
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.
@NeilW - could you try the newgrp command that @kdiogenes suggested?
Also I think we should have sudo -n
to ensure that it fails rather than prompt for a password if one is needed.
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.
Also, /etc/ssh/sshd_config
should set PermitRootLogin no
and PasswordAuthentication no
. What you think?
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.
newgrp
won't work. The persistent session is on the parent user sshd
process, which will remain with the original group set, in a different terminal session. You have to terminate that to force a re-authentication to pick up the new group set.
Even exec newgrp
won't do the trick. There's no persistent shell process to replace
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.
Also, /etc/ssh/sshd_config should set PermitRootLogin no and PasswordAuthentication no. What you think?
I don't think we want to get too far into configuring the servers. The docker group change makes sense here because it is only used when we have just installed docker and we know it's going to be a problem.
newgrp won't work. The persistent session is on the parent user sshd process, which will remain with the original group set, in a different terminal session. You have to terminate that to force a re-authentication to pick up the new group set.
Ok I think signalling is probably ok then - it's a bit of a shame it has to done that way, but I think it's worth it for making this a smoother process.
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 I think we just need to add -n
to sudo, and then make sure there's a nice error message if the user doesn't have sudo access.
|
||
# If the groups change, the session is terminated to force a re-login. | ||
# Catch the resulting IOError and inform the user | ||
rescue IOError |
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.
Is there a more specific error we can rescue here?
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.
That is the specific error. We get back "IOError: closed stream" from the terminated ssh session. The only subclass of IOError is EOFError. It's not a part of the SystemCallError tree.
This allows docker commands to function with a non-root ssh user Fixes: basecamp#980
I've added the I've removed the If sudo cannot be called without a password or hasn't sufficient privileges to run the
|
The docker image above has been updated for anybody who wants to test out the new iteration |
This allows docker commands to function with a non-root ssh user
Fixes: #980