/snapd

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

Closed
wants to merge 6 commits into
from

Conversation

Reviewers

@niemeyer niemeyer

@mvo5 mvo5

@jdstrand jdstrand

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@zyga @niemeyer @mvo5 @jdstrand
@zyga
Contributor

zyga commented Feb 27, 2017

This patch changes snap-confine to allow to see the /etc/os-release file
from the host distribution even if it is a symlink to a well-known
location (/usr/lib/os-release).

Fixes: https://bugs.launchpad.net/canonical-livepatch-client/+bug/1667470
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com
@zyga
cmd/snap-confine,tests: bind-mount /etc/os-release 
This patch changes snap-confine to allow to see the /etc/os-release file
from the host distribution even if it is a symlink to a well-known
location (/usr/lib/os-release).

Fixes: https://bugs.launchpad.net/canonical-livepatch-client/+bug/1667470
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 errored, 2 failing, and 3 successful checks
continuous-integration/travis-ci/pr — The Travis CI build could not complete due to an error
Details
@mvo5
xenial-i386 — autopkgtest finished (failure)
Details
@mvo5
zesty-amd64 — autopkgtest finished (failure)
Details
@mvo5
xenial-amd64 — autopkgtest finished (success)
Details
@mvo5
xenial-ppc64el — autopkgtest finished (success)
Details
@mvo5
yakkety-amd64 — autopkgtest finished (success)
Details
9fd095b
cmd/snap-confine/mount-support.c
@@ -404,6 +404,49 @@ static void sc_bootstrap_mount_namespace(const struct sc_mount_config *config)
dst);
}
}
+ // Since we mountec /etc from the host filesystem the /etc/os-release file
@jdstrand

jdstrand Feb 27, 2017

Contributor

typo: 'mountec'

cmd/snap-confine/mount-support.c
+ //
+ // 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 over-bind mount anything) will work as
@jdstrand

jdstrand Feb 27, 2017

Contributor

'over-bind mount anything' is a confusing phrase

cmd/snap-confine/mount-support.c
+ // expected.
+ //
+ // https://bugs.launchpad.net/canonical-livepatch-client/+bug/1667470
+ if (config->on_classic) {
@jdstrand

jdstrand Feb 27, 2017

Contributor

if (config->on_classic && access(etc_os_release, F_OK) == 0) ? (you'd have to move etc_os_release up of course).

Note that os-release is a systemd-only thing. Does it exist everywhere snapd plans to go? The access() call may help future proof.

(Note that /etc/os-release is provided by the base-files deb on Ubuntu and it exists in 14.04).

@zyga

zyga Feb 28, 2017

Contributor

/etc/os-release it is standard enough now (not just a systemd thing, it's a pretty good thing in general) and it is the preferred way to figure out what people are running on.

cmd/snap-confine/mount-support.c
+ 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).

cmd/snap-confine/mount-support.c
+ // /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).

cmd/snap-confine/mount-support.c
+ // we can just bail out.
+ if (strcmp(link_target, expected_symlink) != 0) {
+ goto skip_etc_os_release;
+ }
@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.

cmd/snap-confine/mount-support.c
+ usr_lib_os_release, dst);
+ if (mount
+ (usr_lib_os_release, dst, NULL, MS_BIND | MS_RDONLY,
+ NULL) != 0) {
@jdstrand

jdstrand Feb 27, 2017

Contributor

Elsewhere we use < 0.

zyga added some commits Feb 28, 2017

@zyga
cmd/snap-confine: fix typos 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
8b4eea7
@zyga
cmd/snap-confine: reword confusing phrase 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
a9294d7
@zyga
cmd/snap-confine: use < 0 vs != 0 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 failing and 5 successful checks
@mvo5
zesty-amd64 — autopkgtest finished (failure)
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
@mvo5
xenial-amd64 — autopkgtest finished (success)
Details
@mvo5
xenial-i386 — autopkgtest finished (success)
Details
@mvo5
xenial-ppc64el — autopkgtest finished (success)
Details
@mvo5
yakkety-amd64 — autopkgtest finished (success)
Details
3531173
@zyga
cmd/snap-confine: add comment justifying EINVAL + readlink 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
7fcd809
@zyga
cmd/snap-confine: describe and tweak the /etc/os-release mount logic 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

2 failing and 4 successful checks
@mvo5
xenial-amd64 — autopkgtest finished (failure)
Details
@mvo5
zesty-amd64 — autopkgtest finished (failure)
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
@mvo5
xenial-i386 — autopkgtest finished (success)
Details
@mvo5
xenial-ppc64el — autopkgtest finished (success)
Details
@mvo5
yakkety-amd64 — autopkgtest finished (success)
Details
a25fb2b
@zyga
Contributor

zyga commented Feb 28, 2017

I changed the logic a little bit and described more what will happen and why I think this makes sense. The only thing I'm not checking if /etc/os-release is itself a non-broken symbolic link. I think this could be done by checking the return code from mount but perhaps it is better to let if fail this way.

Let me know what you think please.

@zyga zyga requested a review from jdstrand Mar 1, 2017

cmd/snap-confine/mount-support.c
+ // 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 •
Edited 1 time

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.

cmd/snap-confine/mount-support.c
+ // /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).

@zyga
Contributor

zyga commented Mar 9, 2017

I'm closing this PR based on feedback. Thank you for the reviews!

@zyga zyga closed this Mar 9, 2017

@zyga zyga deleted the zyga:bindmount-os-release branch Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment