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.

Sign up for GitHub

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

Jump to bottom

Update apparmor to allow confined runc to kill containers #10123

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Update apparmor to allow confined runc to kill containers #10123

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions contrib/apparmor/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ profile {{.Name}} flags=(attach_disconnected,mediate_deleted) {
umount,
# Host (privileged) processes may send signals to container processes.
signal (receive) peer=unconfined,
# runc may send signals to container processes.
signal (receive) peer=runc,
# crun may send signals to container processes.
signal (receive) peer=crun,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be set only when the corresponding profile exist?

Copy link
Member

Choose a reason for hiding this comment

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

Also, what about youki, gvisor, kata, etc ?

Copy link
Contributor Author

@woky woky Apr 23, 2024 •
edited

Choose a reason for hiding this comment

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

Should this be set only when the corresponding profile exist?

I will update the template to check for known profiles.

Also, what about youki, gvisor, kata, etc ?

These currently don't have any default profiles, see https://gitlab.com/apparmor/apparmor/-/tree/master/profiles/apparmor.d.

I don't know much about gVisor and kata runtimes, but I think those run containers in virtual machines, so their kill shouldn't involve sending signals (to container processes).

Copy link
Contributor Author

@woky woky Apr 24, 2024 •
edited

Choose a reason for hiding this comment

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

@AkihiroSuda What is the deadline for this change to be approved and merged to make it into v1.7.16?

I'm not sure what method would you recommend to conditionally add the signal receive rules. I can think of 4 ways:

  1. Abuse macroExists(profile) to check if /etc/apparmor.d/$profile exists for each profile in a hardcoded list of OCI runtime profiles known to exist in latest AppArmor
  2. Use isLoaded(profile) to check if the profile is loaded for each profile as above.
  3. Bring back parseVersion that was removed here contrib/apparmor: remove code related to apparmor_parser version #8069 and conditionally add profiles based on known OCI runtime profiles in the parsed AppArmor version.
  4. Keep it as it is, rely on the fact that it's regular expression and it won't match profiles that are not loaded. This is almost like option 2.

Which option would you go with or do you know of a better way?

Copy link
Member

Choose a reason for hiding this comment

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

If you can confirm that signal (receive) peer=crun, does not cause an error when the crun profile is missing, I think we can just leave it as is and call it for a day.

If it causes an error, checking /etc/apparmor.d/$profile seems good

Copy link

Choose a reason for hiding this comment

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

If you can confirm that signal (receive) peer=crun, does not cause an error when the crun profile is missing, I think we can just leave it as is and call it for a day.

You can call it a day ;-) - the rule will just sit there and do nothing if the crun profile doesn't exist.

# Manager may send signals to container processes.
signal (receive) peer={{.DaemonProfile}},
# Container processes may send signals amongst themselves.
Expand Down