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

WIP: Add binary shims #553

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

HarryMichal
Copy link
Member

This PR adds support for creating binary shims in toolbox containers for binaries available on the host. This was heavily inspired by code snippets in #145. There's also very basic support for running the shim binaries with sudo.

Closes #145

@HarryMichal HarryMichal added 3. Enhancement Improvement to an existing feature 5. Help Wanted Extra attention is needed labels Sep 7, 2020
@HarryMichal HarryMichal added this to the Release 0.2.0 milestone Sep 7, 2020
@softwarefactory-project-zuul
Copy link

Build failed.

@cjao
Copy link
Contributor

cjao commented Oct 14, 2020

Using the technique of your sudo wrapper, I tried flatpak-spawn --host --watch-bus --forward-fd=1 --forward-fd=2 --env=TERM=xterm-256color sudo ls / but got

sudo: a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper

anthr76 added a commit to anthr76/boombox that referenced this pull request Mar 17, 2021
Loosely based off containers/toolbox#145

Doesn't work with sudo and hard coded shims in the docker container.
Pending better UIX with containers/toolbox#553

Signed-off-by: anthr76 <[email protected]>
Base automatically changed from master to main March 25, 2021 22:25
@1player
Copy link

1player commented Jul 1, 2022

Is there any update to this PR?

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,64a43547160cb0ea119cec2b62b4d9d820fafd5c

@debarshiray
Copy link
Member

Let's see if I can rebase this pull request.

Copy link
Member

@debarshiray debarshiray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should turn host-wrapper and sudo-wrapper into private toolbox(1) commands? The flatpak-spawn --host forwarding can benefit from shared code.


# Shims are located under /usr/libexec/toolbox; the list should not include
# "host-runner" and "sudo"
shims=$(ls /usr/libexec/toolbox | grep -v "host-runner" | grep -v "sudo")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using a separate sub-directory for the shims? It would remove the need for this filtering. Moreover, if we do want users to place their own shims, then we can't trust them not to overwrite things like host-runner and sudo.

executable="sudo $executable"
fi

exec flatpak-spawn --host --watch-bus --forward-fd=1 --forward-fd=2 --directory=$(pwd) --env=TERM=xterm-256color $executable "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to explicitly forward file descriptors 1 and 2? What about 0?

flatpak-spawn(1) has always forwarded file descriptors 0, 1 and 2. We don't do it explicitly elsewhere either.

executable="sudo $executable"
fi

exec flatpak-spawn --host --watch-bus --forward-fd=1 --forward-fd=2 --directory=$(pwd) --env=TERM=xterm-256color $executable "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't hard code TERM. Instead: --env=TERM=$TERM :)

Also, shouldn't we forward all the other environment variables that we usually do?

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,f534ed4737b8957f029d7df8466d45b5f465bcd8

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,e640e56f85711ec3133d2bd9709b68aaf4bba691

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,39d47748bd887ef1ace03c8f323cf81d7c1b0c85

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,546c23b99d09d4846b1f9fbf2888110ede7414b5

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,ed7f13751340bd7a91c88015fbe24c960a6cbcf9

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,267184ddac2feead15ff7b043618959e55483037

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,8df091444c505acad958cc52252f2b9548c39b75

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,c531e0ec5810b6dc294b6c94748071eaf5db9946

@@ -1,5 +1,9 @@
# shellcheck shell=sh

# Toolbox provides shim binaries for executing commands on the host from inside
# of a toolbox container
export PATH="/usr/libexec/toolbox:$PATH"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really do this unconditionally for both hosts and containers? I think this should ideally happen only for Toolbx containers, not even generic Podman containers. Or did I misunderstand something?

@softwarefactory-project-zuul
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/containers/toolbox for 553,49ac68e31d3ab07ed68c33e23daa9932b7275501

@debarshiray debarshiray force-pushed the add-binary-shims branch 11 times, most recently from e9be361 to 86f76e2 Compare March 31, 2023 16:21
'host-runner' is a shell script meant to be symlinked against to execute
commands on the host from inside of an OCI container. It makes use of
'flatpak-spawn' with it's option --host that takes care of the plumbing
from a container to the host. The command itself can not be run as root
because it relies on session bus, so instead the command, that is to be
executed on the host, needs to be prepended with "sudo <sudo-options>"
manually.

'sudo' is a shell script that is meant to be used before /bin/sudo to
make it possible to run commands on the host from inside of a container
with "sudo" prefixed. It checks first if the command, that is to be run
with sudo, is to be run on the host by checking if it is a symlink to
/usr/libexec/toolbox/host-runner. If it is not, it is executed normally
with /bin/sudo.

Both of these files should be put in /usr/libexec/toolbox which will
make them available in /run/host/usr/libexec/toolbox in toolbox
containers. Thanks to that there's no need to recreate already existing
containers because those should have the host /usr already available.
…zation

In previous commit the two files, 'host-runner' and 'sudo', were added.
This commit adds all the necessary plumbing to make them available in
already existing containers and in the new ones.

Preparation is done by the 'toolbox init-container' command (the
containers' entry-point) by creating symlinks in /usr/libexec/toolbox
pointing to the two files under /run/host/usr/libexec/toolbox. If the
/usr/libexec/toolbox directory does not exist, it gets created.

The final touch is done by /etc/profile.d/toolbox.sh that was previously
only used to printing welcome messages. It puts /usr/libexec/toolbox
at the beginning of the PATH variable and exports it.
This command is meant to be used in toolbox containers and will be
disabled on the host. The created binaries are put in
/usr/libexec/toolbox/. The command utilizes the redirectPath function in
cmd/initContainer.go.
@softwarefactory-project-zuul
Copy link

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/2f72edcc35f34582b1d4d85b5b9033b8

unit-test FAILURE in 9m 42s
unit-test-migration-path-for-coreos-toolbox FAILURE in 3m 28s
unit-test-restricted FAILURE in 8m 51s
✔️ system-test-fedora-rawhide SUCCESS in 21m 39s
✔️ system-test-fedora-38 SUCCESS in 21m 09s
✔️ system-test-fedora-37 SUCCESS in 20m 59s
✔️ system-test-fedora-36 SUCCESS in 19m 39s

executable="sudo $executable"
fi

exec flatpak-spawn --host --watch-bus --forward-fd=1 --forward-fd=2 --directory=$(pwd) --env=TERM=xterm-256color $executable "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the --directory=$(pwd) because flatpak-spawn(1) has been retaining the current working directory since before the --directory option became a thing.

executable="sudo $executable"
fi

exec flatpak-spawn --host --watch-bus --forward-fd=1 --forward-fd=2 --directory=$(pwd) --env=TERM=xterm-256color $executable "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious about the use of --watch-bus. It's not wrong, but given that flatpak-spawn(1) always forwards signals, I am curious if you came across a scenario where the container-side shim would cleanly exit without signals leaving the host-side running. I am trying to understand if we should use --watch-bus more widely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. Enhancement Improvement to an existing feature 5. Help Wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide shims for well-known binaries, which can only run on the host, inside the toolbox containers
4 participants