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

Container: fix all apparmor ro+remount rules #13826

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

mihalicyn
Copy link
Member

@mihalicyn mihalicyn commented Jul 26, 2024

While investigating #13810 I found that all ro+remount rules in the form:

mount options=(ro,remount,bind,A,B,C) /some_pattern{,/**},

just does not work at all. This remount+bind case is a very special one, and we should rewrite all rules in this form:

mount options=(ro,remount,bind,A,B,C) -> /some_pattern{,/**},

This syntax is not new. This change should be compatible with very old AppArmor versions including 2.11.

Explanation why it was not noticed for years is that for unprivileged container case we have analogical rule but in a wider form:

mount options=(ro,remount,bind,nodev,A,B,C),

which masks the issue. But for privileged containers it's not.

So, let's fix this for correctness.

While investigating canonical#13810 I found that all ro+remount rules in the form:

mount options=(ro,remount,bind,A,B,C) /some_pattern{,/**},

just does not work at all. This remount+bind case is a very special one,
and we should rewrite all rules in this form:

mount options=(ro,remount,bind,A,B,C) -> /some_pattern{,/**},

This syntax is not new. This change should be compatible with very old
AppArmor versions including 2.11.

Explanation why it was not noticed for years is that for unprivileged
container case we have analogical rule but in a wider form:

mount options=(ro,remount,bind,nodev,A,B,C),

which masks the issue. But for privileged containers it's not.

So, let's fix this for correctness.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn mihalicyn requested a review from tomponline July 26, 2024 09:33
@mihalicyn
Copy link
Member Author

cc @jrjohansen

@tomponline tomponline changed the title lxd/apparmor/instance_lxc: fix all ro+remount rules Container: fix all apparmor ro+remount rules Jul 26, 2024
@tomponline
Copy link
Member

tomponline commented Jul 26, 2024

@mihalicyn nice! Does this fix any specific GH issue (like the Oracular issue for privileged containers)?

@tomponline tomponline merged commit ea4610f into canonical:main Jul 26, 2024
27 of 28 checks passed
@mihalicyn
Copy link
Member Author

@mihalicyn nice! Does this fix any specific GH issue (like the Oracular issue for privileged containers)?

this PR definitely makes things better for privileged containers, but does not fix the issue. So we need this change, but it's not a final point.

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

Successfully merging this pull request may close these issues.

2 participants