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

selftest: detect if apparmor is unusable and error #5715

Merged
merged 1 commit into from Aug 27, 2018

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Aug 24, 2018

Under some configuration apparmor may look like its available but
we get permission denied errors when trying to use it. This can
happen on e.g. an lxd container that runs with:

raw.lxc: |
  lxc.apparmor.profile=unconfined

In this case lxd will not setup apparmor stacking so the container
looks unconfined however lxd will not grant CAP_MAC_ADMIN to the
container (which is quite sensible). But it means that snapd will
not be able to setup any apparmor profiles inside such containers.

When this is detected snapd will refuse to run because we cannot
support this configuration. The host apparmor confinement will
interfere with the container and inside the container we can
not do anything about this. See the unsuccessful PR
#5621 for an attempt to support this environment.

Under some configuration apparmor may look like its available but
we get permission denied errors when trying to use it. This can
happen on e.g. an lxd container that runs with:

```
raw.lxc: |
  lxc.apparmor.profile=unconfined
```

In this case lxd will not setup apparmor stacking so the container
looks unconfined however lxd will not grant CAP_MAC_ADMIN to the
container (which is quite sensible). But it means that snapd will
not be able to setup any apparmor profiles inside such containers.

When this is detected snapd will refuse to run because we cannot
support this configuration. The host apparmor confinement will
interfere with the container and inside the container we can
not do anything about this. See the unsuccessful PR
snapcore#5621 for an attempt
for an attempt to support this environment.
@codecov-io
Copy link

Codecov Report

Merging #5715 into master will increase coverage by <.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5715      +/-   ##
==========================================
+ Coverage   78.97%   78.98%   +<.01%     
==========================================
  Files         524      525       +1     
  Lines       40003    40010       +7     
==========================================
+ Hits        31594    31600       +6     
- Misses       5841     5842       +1     
  Partials     2568     2568
Impacted Files Coverage Δ
selftest/selftest.go 60% <ø> (ø) ⬆️
selftest/apparmor_lxd.go 57.14% <57.14%> (ø)
overlord/ifacestate/handlers.go 63.89% <0%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 368b957...b36a7b0. Read the comment docs.

Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the lower level bits nor root cause of the issue, but the code looks fine. See one minor comment about journallog.

lxd.lxc start my-ubuntu
# shellcheck disable=SC2016
lxd.lxc exec my-ubuntu -- sh -c 'set -x;for i in $(seq 120); do if journalctl -u snapd.service | grep -E "apparmor detected but insufficient permissions to use it"; then break; fi; sleep 1; done'
lxd.lxc exec my-ubuntu -- journalctl -u snapd | MATCH "apparmor detected but insufficient permissions to use it"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use get_journalctl_log helper here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I'm not sure, this runs inside the lxc container for which we have no cursor. I think its fine though because we use get_journal_log to avoid reading stuff that was in the journal from previous runs. This lxd container is short-lived so should be ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we doing the same test twice? Is it subtly different or just the same but implemented in a different way?

Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

LGTM. Have to say, I prefer the simplicity of this approach over the previous one.

@mvo5
Copy link
Contributor Author

mvo5 commented Aug 24, 2018

Yay, thank you @jdstrand

@mvo5
Copy link
Contributor Author

mvo5 commented Aug 24, 2018

I also added https://forum.snapcraft.io/t/running-snapd-inside-lxc-apparmor-profile-unconfined-containers/7032 so that there is a forum reference. I wonder if the error should somehow refer to topic? But maybe googling for it is enough.

@mvo5 mvo5 added this to the 2.35 milestone Aug 27, 2018
@mvo5 mvo5 merged commit 80b95d8 into snapcore:master Aug 27, 2018
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

LGTM with one question

lxd.lxc start my-ubuntu
# shellcheck disable=SC2016
lxd.lxc exec my-ubuntu -- sh -c 'set -x;for i in $(seq 120); do if journalctl -u snapd.service | grep -E "apparmor detected but insufficient permissions to use it"; then break; fi; sleep 1; done'
lxd.lxc exec my-ubuntu -- journalctl -u snapd | MATCH "apparmor detected but insufficient permissions to use it"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we doing the same test twice? Is it subtly different or just the same but implemented in a different way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants