grml-debootstrap icon indicating copy to clipboard operation
grml-debootstrap copied to clipboard

Implement clean_chroot to avoid host env variables like TMP to leak into the chroot

Open mika opened this issue 1 year ago • 3 comments

Some implementation notes:

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

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.

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.

FTR, 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 we'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.

Closes: grml/grml-debootstrap#232

mika avatar Aug 14 '24 14:08 mika

This is the result of squashing the commits from https://github.com/grml/grml-debootstrap/pull/267 and extending the commit message accordingly.

What I'm not entirely sure about yet is the usage of calls like:

clean_chroot "$MNTPOINT" DEBIAN_FRONTEND=$DEBIAN_FRONTEND apt-get [...]

@adrelanos are you sure this behaves as expected? :thinking: Would it make sense to support DEBIAN_FRONTEND via the additional_vars you implemented in clean_chroot()? :thinking:

mika avatar Aug 14 '24 14:08 mika

This is the result of squashing the commits from #267 and extending the commit message accordingly.

Thank you!

What I'm not entirely sure about yet is the usage of calls like:

clean_chroot "$MNTPOINT" DEBIAN_FRONTEND=$DEBIAN_FRONTEND apt-get [...]

@adrelanos are you sure this behaves as expected? 🤔

I've built several releases using that line. It is functional because we are using /usr/bin/env -i and are therefore allowed to extend that.

chroot bookworm /usr/bin/env -i testvar=test env | grep testvar

testvar=test

Would it make sense to support DEBIAN_FRONTEND via the additional_vars you implemented in clean_chroot()? 🤔

It would make sense but I did not include it to keep this PR smaller. It wasn't strictly required, I thought. And it made the diff look simpler. I could be done in a follow-up.

adrelanos avatar Sep 24 '24 04:09 adrelanos

What I'm not entirely sure about yet is the usage of calls like:

clean_chroot "$MNTPOINT" DEBIAN_FRONTEND=$DEBIAN_FRONTEND apt-get [...]

@adrelanos are you sure this behaves as expected? 🤔

I've built several releases using that line. It is functional because we are using /usr/bin/env -i and are therefore allowed to extend that.

Ahhhh right, now I see the magic in Capture additional environment variables passed as arguments that makes that possible, thanks :)

chroot bookworm /usr/bin/env -i testvar=test env | grep testvar

testvar=test

Would it make sense to support DEBIAN_FRONTEND via the additional_vars you implemented in clean_chroot()? 🤔

It would make sense but I did not include it to keep this PR smaller. It wasn't strictly required, I thought. And it made the diff look simpler. I could be done in a follow-up.

Ok :)

mika avatar Sep 24 '24 05:09 mika

Mergable?

adrelanos avatar Nov 13 '24 16:11 adrelanos

Mergable?

Had to rebase and fix conflicts, taking care via https://github.com/grml/grml-debootstrap/pull/295

mika avatar Nov 22 '24 11:11 mika