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

apparmor: handle signal mediation #4467

Merged
merged 1 commit into from Mar 9, 2021

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Aug 11, 2020

On newer kernels and systems, AppArmor will block sending signals in
many scenarios by default resulting in strange behaviours (container
programs cannot signal each other, or host processes like containerd
cannot signal containers). This issue was fixed in Docker in 2018 but
this code was copied before then and thus the patches weren't carried.
It also contains a new fix for a more esoteric case.

Ideally this code should live in a project like "containerd/apparmor" so
that Docker, libpod, and containerd can share it, but that's probably
something to do separately.

In addition, the copyright header is updated to reference that the code
is copied from Docker (and thus was not written entirely by the
containerd authors).

Signed-off-by: Aleksa Sarai cyphar@cyphar.com

@cyphar cyphar force-pushed the apparmor-update-profile branch from 4b36181 to e35e5f1 Compare Aug 11, 2020
@theopenlab-ci
Copy link

theopenlab-ci bot commented Aug 11, 2020

Build succeeded.

@cyphar cyphar force-pushed the apparmor-update-profile branch from e35e5f1 to 4084343 Compare Aug 11, 2020
@cyphar
Copy link
Contributor Author

cyphar commented Aug 11, 2020

/cc @vrothberg

You might care about discussions of having a single package that does this, since libpod also has a copy of this code too.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Aug 11, 2020

Build succeeded.

@vrothberg
Copy link

vrothberg commented Aug 11, 2020

/cc @vrothberg

You might care about discussions of having a single package that does this, since libpod also has a copy of this code too.

Thanks for the ping! CRI-O, Podman, Buildah and Skopeo (and possibly Sarus) share github.com/containers/common but maybe we can create a new github.com/opencontainers/apparmor that all container engines can share?

@rhatdan @mrunalp WDYT?

contrib/apparmor/template.go Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda added this to the 1.4.1 milestone Aug 11, 2020
contrib/apparmor/template.go Outdated Show resolved Hide resolved
@rhatdan
Copy link

rhatdan commented Aug 11, 2020

I am fine either way.I don't see why Moby and Containerd cannot use containers/common. But if we want to create a new repo, I am fine with that also.

@cyphar
Copy link
Contributor Author

cyphar commented Aug 12, 2020

Well, the containers/common implementation removed the DaemonProfile feature -- which is needed for Docker at least (you can run Docker under an apparmor profile). If it was re-added we could probably just use containers/common. I don't really care where it lives.

@vrothberg
Copy link

vrothberg commented Aug 12, 2020

Well, the containers/common implementation removed the DaemonProfile feature -- which is needed for Docker at least (you can run Docker under an apparmor profile). If it was re-added we could probably just use containers/common.

Sure! Contributions, maintainers, etc. more than welcome :)

@AkihiroSuda AkihiroSuda added the cherry-pick/1.4.x Change to be cherry picked to release/1.4 branch label Aug 25, 2020
@thaJeztah
Copy link
Member

thaJeztah commented Sep 10, 2020

@cyphar I see there's some pending review comments, could you have a look at those?

@dmcgowan dmcgowan modified the milestones: 1.4.1, 1.4.2 Sep 14, 2020
@cyphar cyphar force-pushed the apparmor-update-profile branch from 4084343 to 58d67c0 Compare Nov 3, 2020
@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 3, 2020

Build succeeded.

@cpuguy83 cpuguy83 requested a review from AkihiroSuda Nov 19, 2020
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Jan 25, 2021

How can we test this?

Adding automated tests would be preferable, but just explaining manual testing steps in the git commit message would be also ok for now.

@AkihiroSuda AkihiroSuda modified the milestones: 1.4.2, 1.4.4 Jan 25, 2021
contrib/apparmor/template.go Outdated Show resolved Hide resolved
@brandond
Copy link

brandond commented Jan 29, 2021

We've found that we need this pulled in for successful operation on SLES. Is there anything we can do to help move this along?

@cyphar
Copy link
Contributor Author

cyphar commented Jan 29, 2021

/ping @AkihiroSuda

@cyphar
Copy link
Contributor Author

cyphar commented Jan 29, 2021

Adding automated tests would be preferable, but just explaining manual testing steps in the git commit message would be also ok for now.

I can add a comment to the commit message but the test is "without this change, containers cannot send signals at all and the container runtime cannot kill them".

The reason this doesn't happen on all distributions is that it depends on whether /etc/apparmor.d/abstractions/base contains a signal directive -- which is a distribution policy. If it doesn't contain a signal directive then the kernel disables signal mediation, but if it does then it enforces on all signal mediation. So while this isn't a kernel regression, distributions caused this to be a regression. The original bug report we had at SUSE (ages ago when I fixed this in Docker originally) was that you couldn't even do docker kill of a container.

@cyphar cyphar force-pushed the apparmor-update-profile branch from 3d0288a to cdd2bb3 Compare Jan 29, 2021
On newer kernels and systems, AppArmor will block sending signals in
many scenarios by default resulting in strange behaviours (container
programs cannot signal each other, or host processes like containerd
cannot signal containers).

The reason this happens only on some distributions (and is not a kernel
regression) is that the kernel doesn't enforce signal mediation unless
the profile contains signal rules. However because our profies #include
the distribution-managed <abstractions/base>, some distributions added
signal rules -- which results in AppArmor enforcing signal mediation and
thus a regression. On these systems, containers cannot send and receive
signals at all -- meaning they cannot signal each other and the
container runtime cannot kill them either.

This issue was fixed in Docker in 2018[1] but this code was copied
before then and thus the patches weren't carried. It also contains a new
fix for a more esoteric case[2]. Ideally this code should live in a
project like "containerd/apparmor" so that Docker, libpod, and
containerd can share it, but that's probably something to do separately.

In addition, the copyright header is updated to reference that the code
is copied from Docker (and thus was not written entirely by the
containerd authors).

[1]: moby/moby#37831
[2]: moby/moby#41337

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@cyphar cyphar force-pushed the apparmor-update-profile branch from cdd2bb3 to d8572b6 Compare Jan 29, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 29, 2021

Build succeeded.

@AkihiroSuda AkihiroSuda requested a review from thaJeztah Mar 4, 2021
estesp
estesp approved these changes Mar 4, 2021
Copy link
Member

@estesp estesp left a comment

LGTM

Copy link
Member

@mikebrow mikebrow left a comment

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/1.4.x Change to be cherry picked to release/1.4 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants