Remove check for apparmor_parser in apparmor.IsEnabled() #2554

saschagrunert · 2020-08-16T12:30:57Z

Not all distributions ship the apparmor_parser in /sbin/ (like NixOS). To be able to use the AppArmor feature on such distributions as well we have to check for the apparmor_parser in the $PATH environment variable.

New approach: The apparmor_parser binary is not really required for a system to run AppArmor from a runc perspective. How to apply the profile is more in the responsibility of higher level runtimes like Podman and Docker, which may do the binary check on their own

Possible release note:

- Removed the check for the `/sbin/apparmor_parser` binary when checking for `apparmor.IsEnabled()`.
  Every runtime which relies on the existence on the binary should now take action and verify that on
  its own. The binary can be present in `/sbin` as well as in `$PATH` on some distributions.

OTOH Not all distro have /sbin in the $PATH, so we should test both /sbin and $PATH

saschagrunert · 2020-08-16T19:50:56Z

@AkihiroSuda you dismissed the review comment, I still added the check for /sbin. Do you have any concerns about this?

saschagrunert · 2020-08-16T19:57:12Z

@mrunalp @cyphar please take a look, too :)

dims · 2020-08-16T21:46:32Z

LGTM 👍

libcontainer/apparmor/apparmor.go

saschagrunert · 2020-08-17T07:07:22Z

I proposed to remove the check with respect to the discussions on #2554 (comment) and #2554 (comment).

The original commit where this check has been added is 44c7afa (cc @tifayuki)

libcontainer/apparmor/apparmor.go

kolyshkin · 2020-08-17T22:42:08Z

The original commit where this check has been added is 44c7afa (cc @tifayuki)

The code was actually in a different repository back then. Tracking it back to the original repo, this was done in docker-archive/libcontainer#532 which describes the reason for this (docker tries to exec this binary and fails).

More interestingly, this check was later removed (see docker-archive/libcontainer#645) but somehow the change was not propagated to runc.

So, yes, I agree, there's no need to check for the binary existence

cyphar

We should also drop the container environment variable check.

saschagrunert · 2020-08-18T06:59:07Z

We should also drop the container environment variable check.

Dropped that check as well 👍

cyphar · 2020-08-18T09:34:14Z

Now shellcheck is failing. You need to rebase on top of #2549 (or rather, master).

saschagrunert · 2020-08-18T09:43:53Z

Now shellcheck is failing. You need to rebase on top of #2549 (or rather, master).

Rebased on top of the latest master branch ✔️

saschagrunert · 2020-08-18T10:10:59Z

Looks like the master is broken, too. :)

libcontainer/apparmor/apparmor.go

kolyshkin · 2020-08-19T23:14:34Z

Looks like the master is broken, too. :)

Once #2561 is fixed you'll need to rebase this one; sorry for the trouble

kolyshkin · 2020-08-20T15:00:02Z

Once #2561 is fixed you'll need to rebase this one; sorry for the trouble

@saschagrunert the fix is merged; please rebase

The `apparmor_parser` binary is not really required for a system to run AppArmor from a runc perspective. How to apply the profile is more in the responsibility of higher level runtimes like Podman and Docker, which may do the binary check on their own. Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert · 2020-08-20T17:25:01Z

Once #2561 is fixed you'll need to rebase this one; sorry for the trouble

@saschagrunert the fix is merged; please rebase

Thanks for the hint, rebased on top of the latest master branch.

kolyshkin

LGTM

dims · 2020-08-20T20:19:18Z

LGTM

thaJeztah · 2020-11-10T10:25:03Z

@kolyshkin @AkihiroSuda regarding #2554 (comment)

Dusting off git history, the check for container env var comes from moby/moby#5534. And yes, container= was set at least by Docker's LXC driver.

I think it's obsoleted by now and should be removed.

Wondering if the env-var is still relevant when running containerised (docker-in-docker); The dind script still sets it;
https://github.com/docker/docker/blob/ed89041433a031cafc0a0f19cfe573c31688d377/hack/dind#L13-L14

(originally was added to address moby/moby#5529)

/cc @tianon PTAL

AkihiroSuda previously approved these changes Aug 16, 2020

View reviewed changes

AkihiroSuda added the kind/bug label Aug 16, 2020

AkihiroSuda added this to the 1.0.0 milestone Aug 16, 2020

saschagrunert force-pushed the apparmor-parser branch 2 times, most recently from 7d0de63 to ae891b5 Compare August 16, 2020 19:49

AkihiroSuda previously approved these changes Aug 16, 2020

View reviewed changes

cyphar reviewed Aug 17, 2020

View reviewed changes

libcontainer/apparmor/apparmor.go Outdated Show resolved Hide resolved

kolyshkin reviewed Aug 17, 2020

View reviewed changes

libcontainer/apparmor/apparmor.go Outdated Show resolved Hide resolved

cyphar reviewed Aug 17, 2020

View reviewed changes

libcontainer/apparmor/apparmor.go Outdated Show resolved Hide resolved

saschagrunert dismissed AkihiroSuda’s stale review via a5d5280 August 17, 2020 07:04

saschagrunert force-pushed the apparmor-parser branch from ae891b5 to a5d5280 Compare August 17, 2020 07:04

AkihiroSuda changed the title ~~Allow searching for apparmor_parser in $PATH~~ Remove check for apparmor_parser in apparmor.IsEnabled() Aug 17, 2020

AkihiroSuda previously approved these changes Aug 17, 2020

View reviewed changes

kolyshkin reviewed Aug 17, 2020

View reviewed changes

libcontainer/apparmor/apparmor.go Outdated Show resolved Hide resolved

mrunalp previously approved these changes Aug 17, 2020

View reviewed changes

cyphar requested changes Aug 17, 2020

View reviewed changes

saschagrunert dismissed stale reviews from mrunalp and AkihiroSuda via 54d3986 August 18, 2020 06:59

saschagrunert force-pushed the apparmor-parser branch from a5d5280 to 54d3986 Compare August 18, 2020 06:59

cyphar previously approved these changes Aug 18, 2020

View reviewed changes

saschagrunert dismissed cyphar’s stale review via 707b06d August 18, 2020 09:43

saschagrunert force-pushed the apparmor-parser branch from 54d3986 to 707b06d Compare August 18, 2020 09:43

kolyshkin reviewed Aug 19, 2020

View reviewed changes

libcontainer/apparmor/apparmor.go Show resolved Hide resolved

saschagrunert force-pushed the apparmor-parser branch from 707b06d to bfb4ea1 Compare August 20, 2020 17:23

kolyshkin approved these changes Aug 20, 2020

View reviewed changes

AkihiroSuda approved these changes Aug 21, 2020

View reviewed changes

AkihiroSuda merged commit 19b63de into opencontainers:master Aug 21, 2020

saschagrunert deleted the apparmor-parser branch August 21, 2020 21:08

AkihiroSuda modified the milestones: 1.0.0, 1.0.0-rc93 Sep 29, 2020

thaJeztah mentioned this pull request Nov 10, 2020

pkg/cri/server: remove dependency on libcontainer/apparmor, libcontainer/utils containerd/containerd#4715

Merged

saschagrunert mentioned this pull request Jan 12, 2021

Remove check for apparmor_parser in AppArmor host validation kubernetes/kubernetes#97968

Merged

Remove check for apparmor_parser in apparmor.IsEnabled() #2554

Remove check for apparmor_parser in apparmor.IsEnabled() #2554

Uh oh!

Conversation

saschagrunert commented Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschagrunert commented Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschagrunert commented Aug 16, 2020

Uh oh!

dims commented Aug 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

saschagrunert commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Aug 17, 2020

Uh oh!

cyphar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschagrunert commented Aug 18, 2020

Uh oh!

cyphar commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschagrunert commented Aug 18, 2020

Uh oh!

saschagrunert commented Aug 18, 2020

Uh oh!

Uh oh!

kolyshkin commented Aug 19, 2020

Uh oh!

kolyshkin commented Aug 20, 2020

Uh oh!

saschagrunert commented Aug 20, 2020

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

dims commented Aug 20, 2020

Uh oh!

thaJeztah commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

saschagrunert commented Aug 16, 2020 •

edited

Loading

saschagrunert commented Aug 16, 2020 •

edited

Loading

saschagrunert commented Aug 17, 2020 •

edited

Loading

cyphar left a comment •

edited

Loading

cyphar commented Aug 18, 2020 •

edited

Loading

thaJeztah commented Nov 10, 2020 •

edited

Loading