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

implement clean_chroot, #267

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

adrelanos
Copy link
Contributor

avoid host environment variables such as TMP to leak into the chroot

fixes #232

avoid host environment variables such as TMP to leak into the chroot

fixes grml#232
@adrelanos
Copy link
Contributor Author

Some optional explanation why I made certain changes the way I made them.

If we use env -i then we can no longer export shell functions. So export -f "error_handler" had to be removed.

clean_chroot function has become more complex than we might have hoped for.

Even PATH needs to be set. Otherwise clean_chroot "$MNTPOINT" grub-install would fail because grub-install is in /usr/sbin/grub-install in the chroot.

A potential alternative that I personally would use is sudo as that already has sane environment scrubbing but I wasn't sure you would want a dependency on sudo. Also maybe in the future you would want to run most of grml-deboostrap as a unprivileged user and only use a privilege escalation tool (such as sudo) when needed and/or run commands unprivileged inside the chroot.

http_proxy has to be passed otherwise apt-cacher-ng would be broken by this commit. While at it, I completed it and added https_proxy, and ALL_PROXY there too for completeness sake.

Which environment variables are passed into the chroot is currently hardcoded. I didn't think it is important to be able to configure that. If somebody needs to configure that, I guess they can create a feature request and/or send a pull request.

The xtrace (bash -x) could be minimized if the code to set the environment variables was just run and by using global variables. That however would come at the cost at slightly more code complexity.

I was also wondering if it was better to use a similar mechanism to the one you're using for CHROOT_VARIABLES, but that would not work because only the chroot-script reads those. But you're not only using that but also other calls from grml-debootstrap to chroot (now clean_chroot) so the environment variables need to be set at the grml-debootstrap level.

@adrelanos
Copy link
Contributor Author

adrelanos commented Feb 3, 2024

Resolved merge conflict.

@adrelanos
Copy link
Contributor Author

Ping?

grml-debootstrap Outdated Show resolved Hide resolved
@adrelanos
Copy link
Contributor Author

Good now?

@adrelanos
Copy link
Contributor Author

Ping?

@@ -1766,7 +1766,7 @@ clean_chroot() {
local chroot_command=("${@:2}")

# Run chroot, then env -i with the specified environment variables inside the chroot
/usr/sbin/chroot "$chroot_dir" /usr/bin/env -i "${env_vars[@]}" "${chroot_command[@]}"
chroot "$chroot_dir" /usr/bin/env -i "${env_vars[@]}" "${chroot_command[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

I think we even don't need to hardcode the /usr/bin/env full path?

einfo "End of debootstrap.log"
fi
## Check if "bailout" function is available.
## This is not the case in chroot-script.
Copy link
Member

Choose a reason for hiding this comment

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

hm, I don't understand why this change ends up in this clean_chroot feature/commit, is the github web UI showing something unexpected? o_

@@ -26,7 +25,6 @@ set -e
set -E
set -o pipefail
trap "error_handler" ERR
export -f "error_handler"
Copy link
Member

Choose a reason for hiding this comment

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

is removing the export of the error_handler from grml-debootstrap and addition of it to chroot script related to the environment cleanup, right?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you mentioned this in a comment as I just noticed:

If we use env -i then we can no longer export shell functions. So export -f "error_handler" had to be removed.

We should squash all the relevant commits into one single commit and provide that useful information you collected into the commit message :)

Do you want me to give this a try or would you prefer to try on your own?

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.

libpam-tmpdir breaks grml-debootstrap
2 participants