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

machined: make shell that is spawned by "machinectl shell" configurable #7682

Closed
wants to merge 1 commit into from

Conversation

mbiebl
Copy link
Contributor

@mbiebl mbiebl commented Dec 18, 2017

On distros like Debian or Ubuntu, /bin/sh points at dash which is not a
suitable user shell. Thus make the default shell that is spawned by
"machinectl shell" configurable.

See: #1395

On distros like Debian or Ubuntu, /bin/sh points at dash which is not a
suitable user shell. Thus make the default shell that is spawned by
"machinectl shell" configurable.

See: systemd#1395
@shawnl
Copy link
Contributor

shawnl commented Dec 18, 2017

Why not check /etc/passwd, and use the configured shell? This is a little more work, but IMHO is the proper solution.

@keszybz
Copy link
Member

keszybz commented Dec 18, 2017

The user might not be configured in /etc/passwd, but elsewhere. But let's continue the discussion in #1395.

@poettering
Copy link
Member

poettering commented Dec 18, 2017

This doesn't appear right to me. The only shell we know to exist in containers is "/bin/sh" really. Containers might contain other shells, but that's pretty much a container-specific property, and hence having a host-side compile-time option to change the default shell for all containers at once doesn't appear right to me, as it will not be right for some containers, and accessing those will then be broken for good, with no chance of recovery (in particular as you made it a compile time option).

I figure what could work is to make "ExecStart=$SHELL" work. Currently we allow env var expansion only for the $1… parameters, out of selinux considerations (because if socket activation is used we need to be able to derive the label to use for the listening socket from the binary we'll eventually invoke on it, but if that's an env var we can't know that yet as the env var block we put together at the moment of activation only), but we should really relax that, and just support specifier and env var expansion for $0 too, except for the case where socket activation is actually used. I mean, it's kinda sick that SELinux sours the soup for everyone and all cases here...

If /bin/sh is unusable on Debian then maybe Debian should fix that though? At least forward execution to /bin/bash or so if invoked interactively and bash exists?

@mbiebl
Copy link
Contributor Author

mbiebl commented Dec 18, 2017

I don't think there is anything to fix on the Debian side. The assumption that /bin/sh points to a user shell is simply wrong. By being able to pick a specific implementation like /bin/bash this uncertainty can be avoided. Which containers / distributions do not install /bin/bash ?

@poettering
Copy link
Member

Which containers / distributions do not install /bin/bash

Debian ones? I part of the reason to make /bin/sh that werid shell was the interest in being able to make bash optional?

If it's not Debian, then busybox kind of stuff at least, for example the test container that the nspawn smoke test puts together in test/TEST-13-NSPAWN-SMOKE/

@poettering
Copy link
Member

Also, anything that "dracut_install" puts together really, i.e. minimal initrd-style environments that just include the bits explicitly referenced, might lack /bin/bash

@mbiebl
Copy link
Contributor Author

mbiebl commented Dec 18, 2017

Debian ones? I part of the reason to make /bin/sh that werid shell was the interest in being able to make bash optional?

bash is an Essential package in Debian, which means it's always available.
Afair the switch of /bin/sh to dash was primarily done for performance reasons, as bash is just horribly slow (or was horribly slow back then) compared to dash

@mbiebl
Copy link
Contributor Author

mbiebl commented Dec 18, 2017

Let's close this PR. It's apparent that hard-coding the shell (either directly in the code or during compile time) is not a proper solution as we can't know what's inside the container.

@mbiebl mbiebl closed this Dec 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants