contrib/apparmor: fix whitespace handling in profile names#13278
contrib/apparmor: fix whitespace handling in profile names#13278thaJeztah wants to merge 2 commits intocontainerd:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes AppArmor profile generation by ensuring the daemon’s current profile name (read from /proc/self/attr/current) is sanitized to avoid trailing newlines/whitespace that can corrupt the rendered policy template.
Changes:
- Trim leading/trailing whitespace (including newlines) from the cleaned AppArmor profile name after removing the
" (enforce)"suffix.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
With testify, expected values go to the left. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
apparmor_fix_whitespace
branch
from
c8bb2f8 to
a419671
Compare
thaJeztah
force-pushed
the
apparmor_fix_whitespace
branch
from
a419671 to
f456e92
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thaJeztah
force-pushed
the
apparmor_fix_whitespace
branch
from
f456e92 to
1c89c8c
Compare
thaJeztah
added
cherry-pick/1.7.x
thaJeztah
force-pushed
the
apparmor_fix_whitespace
branch
from
1c89c8c to
a1fc501
Compare
thaJeztah
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thaJeztah
force-pushed
the
apparmor_fix_whitespace
branch
from
a1fc501 to
6ca7f8a
Compare
thaJeztah
force-pushed
the
apparmor_fix_whitespace
branch
from
6ca7f8a to
d0325a7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for scanner.Scan() { | ||
| // Entries are of the form "<profile> (<mode>)", e.g. "foo (enforce)". | ||
| // Profile names may contain spaces (quoted names are supported in AppArmor), | ||
| // so split on " (" rather than the first space. | ||
| if prefix, _, ok := strings.Cut(scanner.Text(), " ("); ok && prefix == name { | ||
| return true, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The updated parsing logic in isLoaded (handling profile names with spaces by splitting on " (") is not currently unit-tested. To prevent regressions, consider extracting the line-matching logic into a helper that accepts an io.Reader (or a []string of lines) and add tests that cover names with spaces and the no-trailing-newline case.
There was a problem hiding this comment.
Yeah, we should just use the moby implementation of this whole package (once I reconciled the remaining bits); it has coverage for this.
The profile we read from /proc/self/attr/current will contain a newline,
resulting in a stray newline to be added to the profile;
Diff:
--- Expected
+++ Actual
@@ -16,3 +16,4 @@
# Manager may send signals to container processes.
- signal (receive) peer=unconfined,
+ signal (receive) peer=unconfined
+,
# Container processes may send signals amongst themselves.
Trim whitespace to account for newlines and fix handling of whitespace as
AppArmor profile names are allowed to contain spaces when quoted; from the
[apparmor.d(5)] man-page:
PROFILE NAME ( UNQUOTED PROFILE NAME | QUOTED PROFILE NAME )
QUOTED PROFILE NAME = '"' UNQUOTED PROFILE NAME '"'
UNQUOTED PROFILE NAME = (must start with alphanumeric character (after
variable expansion), or '/' AARE have special meanings; see below. May
include VARIABLE. Rules with embedded spaces or tabs must be quoted.)
While we don't use those names in our code, let's make the code correct.
Also update the template to use quotes.
[apparmor.d(5)]: https://manpages.ubuntu.com/manpages/xenial/man5/apparmor.d.5.html
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
apparmor_fix_whitespace
branch
from
d0325a7 to
4ab47b8
Compare
| // Profile names may contain spaces, so split on " (" rather than the | ||
| // first space. Trim whitespace first because the value includes a | ||
| // trailing newline. | ||
| profile, _, _ = strings.Cut(strings.TrimSpace(profile), " (") |
There was a problem hiding this comment.
Hmm I think it would be safer to go from the end - find the LAST ) and then find a matching ( before it.
Technically the profile name could also include parents.
That's also what the libapparmor does:
https://gitlab.com/apparmor/apparmor/-/blob/master/libraries/libapparmor/src/kernel.c?ref_type=heads#L578-615
contrib/apparmor: fix whitespace handling in profile names
The profile we read from /proc/self/attr/current will contain a newline,
resulting in a stray newline to be added to the profile;
Trim whitespace to account for newlines and fix handling of whitespace as
AppArmor profile names are allowed to contain spaces when quoted; from the
apparmor.d(5) man-page:
While we don't use those names in our code, let's make the code correct.