-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
ws: conditional start of ssh-agent #21202
base: main
Are you sure you want to change the base?
Conversation
In unprivileged mode the label-run script should respect an already existing set SSH_AUTH_SOCK env.
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.
Thanks! This also needs an integration test, but I'm happy to create that - unless you want to, of course!
@@ -119,6 +119,10 @@ You can also mount encrypted private keys inside the container. You can set an e | |||
|
|||
Private keys can be encrypted; then cockpit uses the provided password to decrypt the key. | |||
|
|||
### SSH_AUTH_SOCK |
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 this belongs into the previous "SSH auth" section.
@@ -119,6 +119,10 @@ You can also mount encrypted private keys inside the container. You can set an e | |||
|
|||
Private keys can be encrypted; then cockpit uses the provided password to decrypt the key. | |||
|
|||
### SSH_AUTH_SOCK | |||
|
|||
By default the container starts its own ssh-agent. Alternatively `SSH_AUTH_SOCK` environment variable can be set to surpress starting ssh-agent. |
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.
"surpress" typo, and I think it would be better to describe a "positive" use case:
By default the container starts its own ssh-agent. Alternatively `SSH_AUTH_SOCK` environment variable can be set to surpress starting ssh-agent. | |
By default the container starts its own ssh-agent. Alternatively you can pass an `SSH_AUTH_SOCK` environment variable and a corresponding socket volume to the container to re-use your host's socket. |
and perhaps give an example? Does this work?
-v $SSH_AUTH_SOCK:/ssh.sock -e SSH_AUTH_SOCK=/ssh.sock
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 might under a non SElinux system, for now I'm just running it as --privileged
.
Otherwise, yeah, that works.
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.
Ah, perhaps with -v $SSH_AUTH_SOCK:/ssh.sock:Z
then?
@@ -42,8 +42,10 @@ else | |||
|
|||
/usr/libexec/cockpit-certificate-ensure | |||
|
|||
if [ -z "$SSH_AUTH_SOCK" ]; then | |||
if [ -z ${SSH_AUTH_SOCK+x} ]; then |
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.
Please write that in standard shell syntax and with quotes: [ -z "${SSH_AUTH_SOCK:-}" ]
else | ||
echo Agent sock ${SSH_AUTH_SOCK} |
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.
Debugging leftover, please drop.
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.
Actually I put that in on purpose, as the previous output was Agent pid N
.
But sure, can remove.
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 meant, it's not really useful, as the user explicitly specified the path -- there's no surprise? And that path is only valid inside the container.
@engelant Do you still want to continue with this? No pressure, we all have other work to do, I was just curious if you lost interest or just don't have time. I.e. I can finish this at some point too, just don't want to step on your toes. |
@martinpitt terribly sorry for the long response time, but yeah stuff... Anyhow, as for the selinux part,, :z is propably insufficent, as this only affects the mount ns and not the ipc one afaik. Further more the integration tests, I'm not sure what's required there. |
Fixes #21170