cmd/snap-confine,tests: bind-mount /etc/os-release #2947

Closed
wants to merge 6 commits into
from
@@ -404,6 +404,64 @@ static void sc_bootstrap_mount_namespace(const struct sc_mount_config *config)
dst);
}
}
+ // Since we mounted /etc from the host filesystem the /etc/os-release file
+ // (which may not be present) may be a symbolic link. It is expected that
+ // we see the real /etc/os-release file so let's do our best to show
+ // /etc/os-release as it looks like on the classic distribution.
+ //
+ // If the symbolic link in /etc is pointing to /usr/lib/os-release then
+ // bind mount /usr/lib/os-release over itself in the core snap. This way
+ // the symlink (which we cannot be the target of a bind mount as the kernel
+ // will always follow the link when performing the mount operation) will
+ // work as expected.
+ //
+ // https://bugs.launchpad.net/canonical-livepatch-client/+bug/1667470
@jdstrand

jdstrand Mar 1, 2017

Contributor

In thinking about this more, what should /etc/os-release actually be? Is it something for snaps to figure out what 'type: os' snaps (ie, runtimes) are? Ie, should /etc/os-release be different for a 'type: app' snap built for the Ubuntu 'core' snap and a 'type: app' snap built for Fedora's 'core' snap? If so, then the current implementation is correct and this PR is wrong. If not, how will a 'type: app' snap built for Fedora be expected to work if it was built with Fedora in mind and all of a sudden os-release says Ubuntu because someone is running a Fedora snap and core runtime on classic Ubuntu? On a classic 14.04 Ubuntu system, how meaningful is it for snaps built for 16.04 to see 14.04 in the os-release?

Reading https://www.freedesktop.org/software/systemd/man/os-release.html it feels to me like having os-release reflect the core snap's runtime (ie, the current implementation) is the most correct, because from the perspective of the snap, the runtime is the operating system. If we agree on that, then we need only decide on how livepatch should determine how it should run or not, which can be discussed elsewhere (though it seems that what it is most interested in is if a system is live patchable, therefore perhaps simply examining /proc/version_signature is enough. If this is not enough, it seems reasonable for the snapd rest api to be able to tell snaps the host's os-release; or alternatively, figure out a way to take advantage of the precedence of /etc/os-release over /usr/lib/os-release that is defined in https://www.freedesktop.org/software/systemd/man/os-release.html; or create a os-release.system (or similar) file somewhere that livepatch can examine).

@mvo5

mvo5 Mar 1, 2017

Collaborator

Its tricky, given that we mount /etc from the host. But I agree with the view of Jamie. IMO a snap inside core should see /etc/os-release from core because that is the runtime environment it is in. I would rather make it easy to get to the real os-release, e.g. by allowing to read /var/lib/snapd/hostfs/etc/os-release for everyone or something.

@zyga

zyga Mar 1, 2017

Contributor

This is against the previous design and nobody will read the hostfs os-relase. I think we need to get back to the drawing board but note that canonical-livepatch has been affected by this (maybe a patch in xenial, didn't check) that broke what they were seeing before.

@niemeyer

niemeyer Mar 6, 2017

Contributor

Agree with @mvo5 and @jdstrand.. the whole point of these files is to give an idea to the application what it can expect from the environment around it. I can only imagine pain out of pretending to be in the outer world when the local environment is completely unlike it.

@zyga

zyga Mar 6, 2017

Contributor

In that case we need a reverse PR where we do (as hard as we can) everything to make /etc/os-release show the one from the core snap. This has the same problem (symlinks cannot be mount targets or sources). I'll close this PR and open something else.

+ if (config->on_classic) {
+ const char *etc_os_release = "/etc/os-release";
+ const char *expected_symlink = "../usr/lib/os-release";
+ const char *usr_lib_os_release = "/usr/lib/os-release";
+ char link_target[PATH_MAX];
+ // Try to read link target from /etc/os-release and special case EINVAL
+ // instead of trying to lstat it first.
@jdstrand

jdstrand Feb 27, 2017

Contributor

This comment should indicate when readlink() will return EINVAL (eg, when not a symlink (or bufsize is negative)) and say why it is ok to special case (but before you do that, see below).

+ if (readlink(etc_os_release, link_target, sizeof link_target) <
+ 0) {
+ // readlink returns EINVAL when the buffer size is negative or when
+ // the specified file is not a symbolic link. Since the buffer has
+ // a constant size and we know it's not negative we can special
+ // case EINVAL as an indicator that /etc/os-release is not a
+ // symbolic link and that we can just skip this whole section.
+ if (errno != EINVAL) {
+ die("cannot read target of symbolic link from %s", etc_os_release);
+ }
+ goto skip_etc_os_release;
+ }
+ // Check if the link points to ../usr/lib/os-release (or just
+ // /usr/lib/os-release). If not then we're not ready for that case and
+ // we can just bail out.
+ if (strcmp(link_target, expected_symlink) != 0) {
+ goto skip_etc_os_release;
@jdstrand

jdstrand Feb 27, 2017

Contributor

I don't see why you're using the goto here. Why not just check if == 0 and put the next block of code in this conditional?

@zyga

zyga Feb 28, 2017

Contributor

Oh, just because the code gets uglier and uglier the deeper you nest. I don't like indent -linux that much.

@jdstrand

jdstrand Mar 1, 2017

Contributor

We aren't nested very far though (and 'ident -linux' is the coding standard for snap-confine so we should just deal with that).

+ }
@jdstrand

jdstrand Feb 27, 2017

Contributor

I think realpath() might simplify things here. Note that calling realpath(etc_os_release, NULL) will malloc() a string for you (see the 'man 3 realpath' for details).

@zyga

zyga Feb 28, 2017

Contributor

Indeed, I will use realpath, thanks for the suggestion.

@zyga

zyga Feb 28, 2017

Contributor

On second though I think realpath will make things harder because we only care about the first level of symbolic link. For example on Fedora the file /etc/os-release is a symbolic link to /usr/lib/os-release which itself is a symbolic link to something in /usr/lib/os-release.d/....

Our goal is to figure out if we should bind mount, inside the core snap, the immediate target of the link. We don't care where /etc/os-release really points to, just that we bind mount the immediate target so that once inside the core snap it will resolve correctly. Since mount follows symbolic links we can just bind mount /etc/os-release to $(readlink /etc/os-release) and have the desired effect.

I'll rework the code but I don't think realpath buys us anything so I won't use it.

+ // Bind mount whatever /etc/os-release is effectively pointing to
+ // (effectively because we only considered the first level of symbolic
+ // links) to /usr/lib/os-release in the core snap.
+ //
+ // This way once inside the core snap we will see /etc/os-release
+ // (which is a symlink to ../usr/lib/os-release as we've checked above)
+ // pointing to a bind mount to $(realpath /etc/os-release).
+ //
+ // This is because the mount operation below will follow all the symbolic links
+ // so /etc/os-release is followed to its final destination.
+ sc_must_snprintf(dst, sizeof dst, "%s%s", scratch_dir,
+ usr_lib_os_release);
+ debug("performing operation: mount --bind -o ro %s %s",
+ etc_os_release, dst);
+ if (mount
+ (etc_os_release, dst, NULL, MS_BIND | MS_RDONLY,
+ NULL) < 0) {
+ die("cannot perform operation: mount --bind -o ro %s %s", etc_os_release, dst);
+ }
+ }
+ skip_etc_os_release:
// Bind mount the directory where all snaps are mounted. The location of
// the this directory on the host filesystem may not match the location in
// the desired root filesystem. In the "core" and "ubuntu-core" snaps the
@@ -118,6 +118,10 @@
mount options=(rw rbind) /etc/ -> /tmp/snap.rootfs_*/etc/,
mount options=(rw rslave) -> /tmp/snap.rootfs_*/etc/,
+ # Allow bind mounting /usr/lib/os-release over itself in the core snap.
+ # See mount-support.c for a longer explanation.
+ mount options=(rw bind) /usr/lib/os-release -> /tmp/snap.rootfs_*/usr/lib/os-release,
+
mount options=(rw rbind) /home/ -> /tmp/snap.rootfs_*/home/,
mount options=(rw rslave) -> /tmp/snap.rootfs_*/home/,
@@ -0,0 +1,31 @@
+summary: check that /etc/os-release is genuine
+details: |
+ Software may rely on the /etc/os-release file to identify the system where
+ a snap is running. On a core system this is always ID=ubuntu-core but on
+ classic it is expected to be the /etc/os-release file from the host
+ distribution.
+ Since the /etc/os-release file may be a symlink (and it typically is) it
+ may point to other places, places that originate from the core snap, not
+ from the host system. In practice this happens, e.g. on xenial, where
+ /etc/os-release points to /usr/lib/os-release.
+ Since the symlink is resolved only after the chroot is set in place we
+ don't really know what it was previously pointing at. To counter this
+ snap-confine now bind mounts /etc/os-release over itself from the classic
+ distribution.
+prepare: |
+ . $TESTSLIB/snaps.sh
+ install_local test-snapd-tools
+execute: |
+ # The value on the host:
+ cat /etc/os-release > outside
+ test-snapd-tools.cmd cat /etc/os-release > inside
+ cmp outside inside
+restore: |
+ rm -f outside inside
+debug: |
+ echo "/etc/os-release as seen on the outside of snaps"
+ cat outside
+ echo "/etc/os-release as seen on the inside of snaps"
+ cat inside
+
+