Skip to content

cmd/initContainer: Try to handle config files that're absolute symlinks#460

Merged
debarshiray merged 1 commit intocontainers:masterfrom
martymichal:follow-symlinks-when-linking
Aug 28, 2020
Merged

cmd/initContainer: Try to handle config files that're absolute symlinks#460
debarshiray merged 1 commit intocontainers:masterfrom
martymichal:follow-symlinks-when-linking

Conversation

@martymichal
Copy link
Copy Markdown
Member

@martymichal martymichal commented May 27, 2020

There has been a long-standing issue[0] when symlinking /etc/resolv.conf
to /run/host/etc/resolv.conf (host's resolv.conf). Many solutions were
proposed. Hardcode other paths, where the file could be, completely rely
on tools like flatpak-session-helper that will track such files in a
single directory, or update the symlinking function to follow symlinks
and update the target path.

The first solution is the easiest short-term but not very good
long-term. The second solution is possibly the best long-term but there
is a problem with using flatpak-session-helper. It cannot be used as
root. That leaves the third option, follow symlinks and update the
target path if the target is an invalid symlink.

This commit takes the third approach to solve the issue. Now the target
of the symlinking is first tested if it is a symlink. If it's not then
the symlinking is done right away. If it's a symlink then it is resolved.
When the target is valid, symlinking proceeds normally. If it it's not
then symlinking still proceeds but in two different ways depending on
target being an absolute or a relative symlink:

  • absolute - target is prepended with /run/host (the target may not be
    invalid)
  • relative - target is not changed (the target will be invalid)

This commit tries to rely on well-made relative symlinks. Those behave
correctly even when they are placed in a different prefix (in Toolbox's
case under /run/host).

Thanks Tudor Roman for raising concern about relative links.

Closes #187

@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

@martymichal martymichal added this to the Stable 1.0 milestone Jul 7, 2020
@martymichal martymichal added 3. Enhancement Improvement to an existing feature 6. Major Change May cause breakage labels Jul 8, 2020
@martymichal martymichal force-pushed the follow-symlinks-when-linking branch from a0882a1 to 0289674 Compare July 23, 2020 20:13
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

@martymichal martymichal added the 3. Bugfix Fixes a bug label Jul 23, 2020
@martymichal martymichal force-pushed the follow-symlinks-when-linking branch from 0289674 to 24c936a Compare July 28, 2020 08:36
@martymichal
Copy link
Copy Markdown
Member Author

I extended the comment in the function to clarify what it does, added a description of the redirectPath function and wrapped returned errors around the original errors.

@martymichal martymichal requested a review from debarshiray July 28, 2020 08:38
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

@martymichal martymichal force-pushed the follow-symlinks-when-linking branch 2 times, most recently from b8660cc to 88a7ae5 Compare August 13, 2020 10:36
@martymichal
Copy link
Copy Markdown
Member Author

@debarshiray when you have time, could you look at this?? 2c4405d and 88a7ae5 made this a bit larger change but I think it's necessary to include it to keep toolboxes work across system updates (or rebases on Silverblue/CoreOS).

@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

Copy link
Copy Markdown
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.

Thanks for working on this!

// If the target is a link, follow the chain. If needed, repeat the process.
for isSymlink {
logrus.Debugf("Path %s is a symlink. Resolving.", target)
newTarget, err := os.Readlink(target)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Am I wrong in thinking that a lot of this can be simplified by using filepath.EvalSymlinks ?

For example, I have:

$ ls -l /etc/os-release 
lrwxrwxrwx. 1 root root 21 Apr 28 14:50 /etc/os-release -> ../usr/lib/os-release

Invoking filepath.EvalSymlinks("/etc/os-release") gives me /usr/lib/os-release.

For the sake of sanity, I don't mind requiring that the redirectPath function accepts only absolute paths, because it's an internal function whose callers are under our control. If we go down this path, then I would check the inputs early on in the function and panic out (because it would be a programmer error) if the conditions aren't met.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignoring the exact Go APIs that are used, I think that the redirectPath function should roughly do the following:

  • If target is not a symbolic link, then don't bother checking if it exists or not. Just link containerPath to target.

  • If target is a symbolic link, then check if it's resolving. If it's resolving, then link containerPath to target.

  • If target is a symbolic link and if it's not resolving, then either the link is absolute or relative. If it's absolute, then prepend /run/host to the destination of target and link containerPath to it, which may or not lead to a valid file. If it's relative, then link containerPath to target even though we know that it doesn't resolve. In both these cases, we are not trying to guarantee that containerPath points to a valid file. We are merely doing the best we can to dig up the right destination, and giving up if we run out of guesses.

  • It should not fail the entry-point process even if target doesn't exist or resolve. We can log a debug message or such, but we shouldn't stop the container from starting up just because there's something wrong with a configuration file. In this case, a slightly broken container that starts is vastly better than one doesn't start at all. If nothing else, it makes it easier to debug.

At the end of the day, all sane operating systems should be using relative symbolic links on the host. Fedora does that, and this is exactly the reason why relative symlinks are nice. They work even if you change the root from / to /run/host.

What we are trying to fix, are those few cases where a human created an absolute symbolic link. We should do our best to handle the obvious and simple cases, but if a human has made a mistake, then it's their responsibility to fix that up.

// If the target is a link, follow the chain. If needed, repeat the process.
for isSymlink {
logrus.Debugf("Path %s is a symlink. Resolving.", target)
newTarget, err := os.Readlink(target)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ignoring the exact Go APIs that are used, I think that the redirectPath function should roughly do the following:

  • If target is not a symbolic link, then don't bother checking if it exists or not. Just link containerPath to target.

  • If target is a symbolic link, then check if it's resolving. If it's resolving, then link containerPath to target.

  • If target is a symbolic link and if it's not resolving, then either the link is absolute or relative. If it's absolute, then prepend /run/host to the destination of target and link containerPath to it, which may or not lead to a valid file. If it's relative, then link containerPath to target even though we know that it doesn't resolve. In both these cases, we are not trying to guarantee that containerPath points to a valid file. We are merely doing the best we can to dig up the right destination, and giving up if we run out of guesses.

  • It should not fail the entry-point process even if target doesn't exist or resolve. We can log a debug message or such, but we shouldn't stop the container from starting up just because there's something wrong with a configuration file. In this case, a slightly broken container that starts is vastly better than one doesn't start at all. If nothing else, it makes it easier to debug.

At the end of the day, all sane operating systems should be using relative symbolic links on the host. Fedora does that, and this is exactly the reason why relative symlinks are nice. They work even if you change the root from / to /run/host.

What we are trying to fix, are those few cases where a human created an absolute symbolic link. We should do our best to handle the obvious and simple cases, but if a human has made a mistake, then it's their responsibility to fix that up.

@debarshiray
Copy link
Copy Markdown
Member

Given that operating systems are expected to use relative symbolic links, it seems to me that updates aren't such a pressing problem.

When someone updates from Fedora 32 to 33, then their /etc/resolv.conf will turn into a relative symbolic link. Existing Toolbox containers had their /etc/resolv.conf pointing at /run/host/etc/resolv.conf, which will now be a relative symlink and therefore resolve.

Things can break if a user changes their /etc/resolv.conf into a symbolic link that either:

  • Points somewhere that's not bind mounted into the container. I won't worry about this unless someone actually comes up with a valid example.

  • Is absolute, and the change happened after there were already some Toolbox containers with their /etc/resolv.conf configured in a way that didn't expect that. In such a case, it's not that hard for the user to switch to a relative symlink. After all they are messing around with the OS' configuration, and this is a pretty minor tweak.

@martymichal martymichal force-pushed the follow-symlinks-when-linking branch from 88a7ae5 to 4e0ba3c Compare August 19, 2020 16:13
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

@martymichal martymichal force-pushed the follow-symlinks-when-linking branch from 4e0ba3c to 835fc3c Compare August 19, 2020 16:25
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

martymichal added a commit to martymichal/toolbox that referenced this pull request Aug 19, 2020
@martymichal martymichal force-pushed the follow-symlinks-when-linking branch from 835fc3c to dd2b3ec Compare August 19, 2020 16:33
martymichal added a commit to martymichal/toolbox that referenced this pull request Aug 19, 2020
There has been a long-standing issue[0] when symlinking /etc/resolv.conf
to /run/host/etc/resolv.conf (host's resolv.conf). Many solutions were
proposed. Hardcode other paths, where the file could be, completely rely
on tools like flatpak-session-helper that will track such files in a
single directory, or update the symlinking function to follow symlinks
and update the target path.

[0] containers#187

The first solution is the easiest short-term but not very good
long-term. The second solution is possibly the best long-term but there
is a problem with using flatpak-session-helper. It cannot be used as
root. That leaves the third option, follow symlinks and update the
target path if the target is an invalid symlink.

This commit takes the third approach to solve the issue. Now the target
of the symlinking is first tested if it is a symlink. If it's not then
the symlinking is done right away. If it's a symlink then it is resolved.
When the target is valid, symlinking proceeds normally. If it it's not
then symlinking still proceeds but in two different ways depending on
target being an absolute or a relative symlink:

  - absolute - target is prepended with /run/host (the target may not be
    invalid)
  - relative - target is not changed (the target will be invalid)

This commit tries to rely on well-made relative symlinks. Those behave
correctly even when they are placed in a different prefix (in Toolbox's
case under /run/host).

Thanks Tudor Roman for raising concern about relative links.

Based on: containers#380

containers#460
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

debarshiray pushed a commit to martymichal/toolbox that referenced this pull request Aug 27, 2020
There has been a long-standing issue[0] when symlinking /etc/resolv.conf
to /run/host/etc/resolv.conf (host's resolv.conf). Many solutions were
proposed. Hardcode other paths, where the file could be, completely rely
on tools like flatpak-session-helper that will track such files in a
single directory, or update the symlinking function to follow symlinks
and update the target path.

[0] containers#187

The first solution is the easiest short-term but not very good
long-term. The second solution is possibly the best long-term but there
is a problem with using flatpak-session-helper. It cannot be used as
root. That leaves the third option, follow symlinks and update the
target path if the target is an invalid symlink.

This commit takes the third approach to solve the issue. Now the target
of the symlinking is first tested if it is a symlink. If it's not then
the symlinking is done right away. If it's a symlink then it is resolved.
When the target is valid, symlinking proceeds normally. If it it's not
then symlinking still proceeds but in two different ways depending on
target being an absolute or a relative symlink:

  - absolute - target is prepended with /run/host (the target may not be
    invalid)
  - relative - target is not changed (the target will be invalid)

This commit tries to rely on well-made relative symlinks. Those behave
correctly even when they are placed in a different prefix (in Toolbox's
case under /run/host).

Thanks Tudor Roman for raising concern about relative links.

Based on: containers#380

containers#460
@debarshiray debarshiray force-pushed the follow-symlinks-when-linking branch from ae0cfed to d755b2a Compare August 27, 2020 17:47
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

debarshiray pushed a commit to martymichal/toolbox that referenced this pull request Aug 28, 2020
There has been a long-standing issue[0] when symlinking /etc/resolv.conf
to /run/host/etc/resolv.conf (host's resolv.conf). Many solutions were
proposed. Hardcode other paths, where the file could be, completely rely
on tools like flatpak-session-helper that will track such files in a
single directory, or update the symlinking function to follow symlinks
and update the target path.

[0] containers#187

The first solution is the easiest short-term but not very good
long-term. The second solution is possibly the best long-term but there
is a problem with using flatpak-session-helper. It cannot be used as
root. That leaves the third option, follow symlinks and update the
target path if the target is an invalid symlink.

This commit takes the third approach to solve the issue. Now the target
of the symlinking is first tested if it is a symlink. If it's not then
the symlinking is done right away. If it's a symlink then it is resolved.
When the target is valid, symlinking proceeds normally. If it it's not
then symlinking still proceeds but in two different ways depending on
target being an absolute or a relative symlink:

  - absolute - target is prepended with /run/host (the target may not be
    invalid)
  - relative - target is not changed (the target will be invalid)

This commit tries to rely on well-made relative symlinks. Those behave
correctly even when they are placed in a different prefix (in Toolbox's
case under /run/host).

Thanks Tudor Roman for raising concern about relative links.

Based on: containers#380

containers#460
@debarshiray debarshiray force-pushed the follow-symlinks-when-linking branch from d755b2a to 56e86e3 Compare August 28, 2020 10:34
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

debarshiray pushed a commit to martymichal/toolbox that referenced this pull request Aug 28, 2020
There has been a long-standing issue[0] when symlinking /etc/resolv.conf
to /run/host/etc/resolv.conf (host's resolv.conf). Many solutions were
proposed. Hardcode other paths, where the file could be, completely rely
on tools like flatpak-session-helper that will track such files in a
single directory, or update the symlinking function to follow symlinks
and update the target path.

[0] containers#187

The first solution is the easiest short-term but not very good
long-term. The second solution is possibly the best long-term but there
is a problem with using flatpak-session-helper. It cannot be used as
root. That leaves the third option, follow symlinks and update the
target path if the target is an invalid symlink.

This commit takes the third approach to solve the issue. Now the target
of the symlinking is first tested if it is a symlink. If it's not then
the symlinking is done right away. If it's a symlink then it is resolved.
When the target is valid, symlinking proceeds normally. If it it's not
then symlinking still proceeds but in two different ways depending on
target being an absolute or a relative symlink:

  - absolute - target is prepended with /run/host (the target may not be
    invalid)
  - relative - target is not changed (the target will be invalid)

This commit tries to rely on well-made relative symlinks. Those behave
correctly even when they are placed in a different prefix (in Toolbox's
case under /run/host).

Thanks Tudor Roman for raising concern about relative links.

Based on: containers#380

containers#460
@debarshiray debarshiray force-pushed the follow-symlinks-when-linking branch from 56e86e3 to 2e823c5 Compare August 28, 2020 11:02
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

debarshiray pushed a commit to martymichal/toolbox that referenced this pull request Aug 28, 2020
There has been a long-standing issue[0] when symlinking /etc/resolv.conf
to /run/host/etc/resolv.conf (host's resolv.conf). Many solutions were
proposed. Hardcode other paths, where the file could be, completely rely
on tools like flatpak-session-helper that will track such files in a
single directory, or update the symlinking function to follow symlinks
and update the target path.

[0] containers#187

The first solution is the easiest short-term but not very good
long-term. The second solution is possibly the best long-term but there
is a problem with using flatpak-session-helper. It cannot be used as
root. That leaves the third option, follow symlinks and update the
target path if the target is an invalid symlink.

This commit takes the third approach to solve the issue. Now the target
of the symlinking is first tested if it is a symlink. If it's not then
the symlinking is done right away. If it's a symlink then it is resolved.
When the target is valid, symlinking proceeds normally. If it it's not
then symlinking still proceeds but in two different ways depending on
target being an absolute or a relative symlink:

  - absolute - target is prepended with /run/host (the target may not be
    invalid)
  - relative - target is not changed (the target will be invalid)

This commit tries to rely on well-made relative symlinks. Those behave
correctly even when they are placed in a different prefix (in Toolbox's
case under /run/host).

Thanks Tudor Roman for raising concern about relative links.

Based on: containers#380

containers#460
@debarshiray debarshiray force-pushed the follow-symlinks-when-linking branch from 2e823c5 to e97fd51 Compare August 28, 2020 11:10
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

debarshiray pushed a commit to martymichal/toolbox that referenced this pull request Aug 28, 2020
There has been a long-standing issue[0] when symlinking /etc/resolv.conf
to /run/host/etc/resolv.conf (host's resolv.conf). Many solutions were
proposed. Hardcode other paths, where the file could be, completely rely
on tools like flatpak-session-helper that will track such files in a
single directory, or update the symlinking function to follow symlinks
and update the target path.

[0] containers#187

The first solution is the easiest short-term but not very good
long-term. The second solution is possibly the best long-term but there
is a problem with using flatpak-session-helper. It cannot be used as
root. That leaves the third option, follow symlinks and update the
target path if the target is an invalid symlink.

This commit takes the third approach to solve the issue. Now the target
of the symlinking is first tested if it is a symlink. If it's not then
the symlinking is done right away. If it's a symlink then it is resolved.
When the target is valid, symlinking proceeds normally. If it it's not
then symlinking still proceeds but in two different ways depending on
target being an absolute or a relative symlink:

  - absolute - target is prepended with /run/host (the target may not be
    invalid)
  - relative - target is not changed (the target will be invalid)

This commit tries to rely on well-made relative symlinks. Those behave
correctly even when they are placed in a different prefix (in Toolbox's
case under /run/host).

Thanks Tudor Roman for raising concern about relative links.

Based on: containers#380

containers#460
@debarshiray debarshiray force-pushed the follow-symlinks-when-linking branch from e97fd51 to 4c5906e Compare August 28, 2020 11:16
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

debarshiray pushed a commit to martymichal/toolbox that referenced this pull request Aug 28, 2020
There has been a long-standing issue[0] when symlinking /etc/resolv.conf
to /run/host/etc/resolv.conf (host's resolv.conf). Many solutions were
proposed. Hardcode other paths, where the file could be, completely rely
on tools like flatpak-session-helper that will track such files in a
single directory, or update the symlinking function to follow symlinks
and update the target path.

[0] containers#187

The first solution is the easiest short-term but not very good
long-term. The second solution is possibly the best long-term but there
is a problem with using flatpak-session-helper. It cannot be used as
root. That leaves the third option, follow symlinks and update the
target path if the target is an invalid symlink.

This commit takes the third approach to solve the issue. Now the target
of the symlinking is first tested if it is a symlink. If it's not then
the symlinking is done right away. If it's a symlink then it is resolved.
When the target is valid, symlinking proceeds normally. If it it's not
then symlinking still proceeds but in two different ways depending on
target being an absolute or a relative symlink:

  - absolute - target is prepended with /run/host (the target may not be
    invalid)
  - relative - target is not changed (the target will be invalid)

This commit tries to rely on well-made relative symlinks. Those behave
correctly even when they are placed in a different prefix (in Toolbox's
case under /run/host).

Thanks Tudor Roman for raising concern about relative links.

Based on: containers#380

containers#460
@debarshiray debarshiray force-pushed the follow-symlinks-when-linking branch from 4c5906e to 2b78d78 Compare August 28, 2020 11:49
Currently toolbox containers can get misconfigured if some
configuration files on the host are absolute symbolic links to some
other location.

For example, when systemd-resolved is used to manage /etc/resolv.conf
on the host, and if the file is an absolute link to
/run/systemd/resolve/stub-resolv.conf, then /etc/resolv.conf ends up
being invalid inside the container. This happens because the
container's /etc/resolv.conf points to /run/host/etc/resolv.conf, which
in turn points to /run/systemd/resolve/stub-resolv.conf, but that's
absent from the container.

This is, of course, not a problem with relative symbolic links. If the
host had a relative link to ../run/systemd/resolve/stub-resolv.conf,
then it would continue to work inside the container.

One solution would have been to use flatpak-session-helper to maintain
a copy of the host's /etc/resolv.conf in
$XDG_RUNTIME_DIR/.flatpak-helper/monitor. However, that doesn't work
when toolbox(1) is run as root.

The other option is to prepend the destination of the absolute symbolic
link with /run/host to make it resolve inside the container. It might
not work with funky links, but it's enough for the common case where a
an administrator changed the host's /etc/resolv.conf into a sane, but
absolute, symbolic link.

Properly configured hosts should anyway use relative symbolic links,
because they don't need to be adjusted in such scenarios. That's also
what Fedora and Ubuntu do, by default.

Thanks to Tudor Roman for raising a concern about relative symbolic
links.

Based on Martin Pitt's work on the POSIX shell implementation:
containers#380

containers#187
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

@debarshiray debarshiray force-pushed the follow-symlinks-when-linking branch from 2b78d78 to 88a95b0 Compare August 28, 2020 12:24
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.

@debarshiray debarshiray merged commit 88a95b0 into containers:master Aug 28, 2020
@debarshiray
Copy link
Copy Markdown
Member

I took the liberty to address the above issues.

Thanks for your work on this, @HarryMichal !

@debarshiray debarshiray changed the title cmd/initContainer: Reinforce symlink redirection cmd/initContainer: Try to handle config files that're absolute symlinks Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. Bugfix Fixes a bug 3. Enhancement Improvement to an existing feature 6. Major Change May cause breakage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/etc/resolv.conf is broken when it's an absolute symbolic link on the host

3 participants