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

Fix compatibility with libpam-tmpdir. #234

Closed
wants to merge 2 commits into from
Closed

Fix compatibility with libpam-tmpdir. #234

wants to merge 2 commits into from

Conversation

adrelanos
Copy link
Contributor

By creating folder the required temporary folder.

fixes #232

@zeha
Copy link
Member

zeha commented Nov 11, 2023

I understand where you're coming from, but that seems to be the wrong fix. The host setup should (mostly) not affect the target.
Maybe we should reset TMPDIR when acting in the chroot instead.

@adrelanos
Copy link
Contributor Author

I understand where you're coming from, but that seems to be the wrong fix.

Why? Some tool at some stage needs to create the directory. I was also wondering which tool would be the most appropriate. Maybe libpam-tmpdir should do it (or provider a helper tool to do that) but then I thought it won't be running at that time.

Maybe we should reset TMPDIR when acting in the chroot instead.

Maybe. Then all invocations of chroot could be replaced with $chroot (or different variable name) which would prepend:

env --unset=TEMP --unset=TEMPDIR --unset=TMP --unset=TMPDIR

What however should not be done is on the host operating system (at the beginning of grml-debootstrap):

unset TEMP TEMPDIR TMP TMPDIR

Because that would disable libpam-tmpdir during all invocations of any tools running on the host operating system and not only inside the chroot.

On the other hand, wouldn't it be more secure to start using libpam-tmpdir as soon as possible even inside the chroot?

@zeha
Copy link
Member

zeha commented Nov 12, 2023

I understand where you're coming from, but that seems to be the wrong fix.

Why? Some tool at some stage needs to create the directory. I was also wondering which tool would be the most appropriate. Maybe libpam-tmpdir should do it (or provider a helper tool to do that) but then I thought it won't be running at that time.

libpam-tmpdir on the host is a host thing. Its existence on the host shall not affect the chroot, which can end up on a completely different system / VM / ...

I know there are some other things that today leak from the host, but I'd like to get rid of them too - at least for --vm mode.

Maybe we should reset TMPDIR when acting in the chroot instead.

Maybe. Then all invocations of chroot could be replaced with $chroot (or different variable name) which would prepend:

env --unset=TEMP --unset=TEMPDIR --unset=TMP --unset=TMPDIR

I was thinking more along the lines of env -i chroot ..., as a holistic approach to stop all ENV leakage.

On the other hand, wouldn't it be more secure to start using libpam-tmpdir as soon as possible even inside the chroot?

No, there is no concurrent access while the chroot is built. libpam-tmpdir does not add any value during this time.

Only after booting it might be useful, but this decision should be left to the user.

@adrelanos
Copy link
Contributor Author

Ok, very good. Lets define $chroot being env -i chroot and use that?

I would attempt a PR but wait for it until #231 is merged (otherwise merge conflict).

@mika
Copy link
Member

mika commented Nov 17, 2023

Ok, very good. Lets define $chroot being env -i chroot and use that?
I would attempt a PR but wait for it until #231 is merged (otherwise merge conflict).

Sounds like worth a try, yes :)

@zeha
Copy link
Member

zeha commented Nov 17, 2023

Instead of $chroot, we could have sth like clean_chroot() { env -i chroot "$@" } (?)

@adrelanos
Copy link
Contributor Author

Works for me either way. $chroot has the advantage that it could be user customized through an environment variable.

Instead of $chroot, we could have sth like clean_chroot() { env -i chroot "$@" } (?)

Since @mika liked the post, seems to agree with it (and I personally don't need the environment variable), I am happy to and will attempt to implement this at a later time when other PR is ready and merged to avoid merge conflicts.

@adrelanos adrelanos closed this by deleting the head repository Nov 22, 2023
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
3 participants