Repository navigation
fix(apparmor): install no profile where the parser has no abi/4.0 - #3285
bryan-minimal wants to merge 3 commits into
Conversation
The minimald profile declares abi/4.0 and carries one rule, `userns`, both of which exist from AppArmor 4.0. On Debian 12 (AppArmor 3.0.8) the parser cannot open that abi and refuses the file, so a package's postinst or the hand loader reported an error on every install there (gominimal/inbox#1197). That kernel does not restrict unprivileged user namespaces either, so the profile has nothing to allow on it, which is why `min finalize-install` already installs it only where the sysctl restricts them. The loader now does the same: without abi/4.0 under the AppArmor directory it says the host mediates no user namespaces and installs nothing, and --uninstall still removes a file an earlier run left. The profile's header says why there is no second rendering: a 3.x profile could not say `userns`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… PATH `--check` runs as the user, as the docs say, and on Debian and Ubuntu apparmor_parser lives in /usr/sbin, which a user's PATH does not hold, so the check died with "apparmor_parser not found" on a host that has AppArmor (a lab run of the installed loader on Ubuntu 24.04). The loader adds /usr/sbin and /sbin to its PATH before looking. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 2 billable files and costs up to $0.50.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 57 minutes for your next included review. View limit details
|
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none seen. |
…bi/4.0 A lane that runs `--check` on an older host read a 0 and two lines about installing nothing. The check mode now says the profile was not parsed here and why, so a lane that depends on the parse can read it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
| # allow there: say so and install nothing (gominimal/inbox#1197). The | ||
| # --uninstall path still runs, for a file an earlier run left. | ||
| if [ "$mode" != uninstall ] && [ ! -e "$APPARMOR_D/abi/4.0" ]; then | ||
| note "this host's AppArmor has no abi/4.0 (parser: $(apparmor_parser --version 2>/dev/null | head -1)):" | ||
| if [ "$mode" = check ]; then | ||
| # a lane that depends on the parse reads this line, not a silent 0 | ||
| note "check skipped: the profile declares abi/4.0, which this parser cannot read; it was not parsed here" | ||
| else | ||
| note "it does not mediate user namespaces, so minimald needs no profile here; nothing installed" | ||
| fi | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
MEDIUM · correctness · confidence 0.55 · in install-apparmor-profile.sh
The guard tests for the file abi/4.0, not for the sysctl. A host with kernel.apparmor_restrict_unprivileged_userns=1 and a parser older than 4.0 gets exit 0 and the text 'does not mediate user namespaces'. That text is false there, and no profile is installed, so every box fails with EPERM. finalize-install (crates/minimal/src/cmd/finalize_install.rs, userns_item_over) decides on the sysctl, so the PR text 'does what finalize-install does' is not accurate. Read /proc/sys/kernel/apparmor_restrict_unprivileged_userns first. Skip only when it is absent or 0. When it is 1 and abi/4.0 is missing, fail with a clear message.
What the reviewer checked
if [ "$mode" != uninstall ] && [ ! -e "$APPARMOR_D/abi/4.0" ]; then ... exit 0
Hand this to an agent
Act on a code-review finding.
File: scripts/install-apparmor-profile.sh
Lines: 71-82
Category: correctness
Severity: medium (reviewer confidence 0.55)
Finding:
The guard tests for the file abi/4.0, not for the sysctl. A host with kernel.apparmor_restrict_unprivileged_userns=1 and a parser older than 4.0 gets exit 0 and the text 'does not mediate user namespaces'. That text is false there, and no profile is installed, so every box fails with EPERM. `finalize-install` (crates/minimal/src/cmd/finalize_install.rs, userns_item_over) decides on the sysctl, so the PR text 'does what finalize-install does' is not accurate. Read /proc/sys/kernel/apparmor_restrict_unprivileged_userns first. Skip only when it is absent or 0. When it is 1 and abi/4.0 is missing, fail with a clear message.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
Scope matches the title. The diff adds an abi/4.0 guard and a PATH widening to the loader, and it adds a header comment to the profile. Clobber none. The diff touches only the guard region of scripts/install-apparmor-profile.sh and one comment block in packaging/apparmor/minimald. Correctness The guard skips the install and --check paths and keeps --uninstall. The apparmor_parser --version substitution inside note does not trip set -e. The CI apparmor job runs on ubuntu-latest (24.04, AppArmor 4.x), so --check still parses the profile there. An open thread already covers the gap between the abi test and the kernel.apparmor_restrict_unprivileged_userns sysctl that userns_item_over in crates/minimal/src/cmd/finalize_install.rs reads. This review does not raise it again. That gap also makes the new profile comment's claim about min finalize-install depend on the same assumption. The two findings below are about the printed wording of the new notes and about the absence of an in-repo test for the new branch. Not checked: the parser behaviour on a real AppArmor 3.0 host, because it needs a Debian 12 machine.
| # a lane that depends on the parse reads this line, not a silent 0 | ||
| note "check skipped: the profile declares abi/4.0, which this parser cannot read; it was not parsed here" | ||
| else | ||
| note "it does not mediate user namespaces, so minimald needs no profile here; nothing installed" |
There was a problem hiding this comment.
low · The new notes break the printed-text rules. Line 74 ends in a colon and continues on the next printed line. Line 79 starts with it, which does not say what it is, and joins two facts with a semicolon. Line 77 also joins two facts with a semicolon. A user on Debian 12 reads three fragments in place of plain sentences. Write each fact as its own sentence, for example: This host's AppArmor has no abi/4.0 (parser: ...). then minimald needs no AppArmor profile on this host. Nothing was installed. This is local to these lines: the older note calls at lines 105 and 122-125 do not use semicolon chains.
| note "it does not mediate user namespaces, so minimald needs no profile here; nothing installed" | ||
| fi | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
low · The PR fixes two defects: the Debian 12 abi refusal and the apparmor_parser not found error on --check. No test in this repo drives either fix. scripts/install_test.sh:308 replaces the loader with a mock. scripts/session-e2e.sh runs the real loader only with sudo, on hosts that have abi/4.0. The PR body says the lab scenario will cover the fix only after a release ships. A later edit can bring either defect back and CI will not see it. APPARMOR_D is a hard-coded readonly variable, so a test needs an override, for example an env var that defaults to /etc/apparmor.d, and a stub apparmor_parser on PATH.
Fixes gominimal/inbox#1197.
What happened. The minimald AppArmor profile declares
abi <abi/4.0>and carries one rule,userns. Both exist from AppArmor 4.0. On Debian 12 (AppArmor 3.0.8) the parser cannot open that abi and refuses the file, so a package postinst that loads it, or the hand loaderscripts/install-apparmor-profile.sh, reportedCould not open 'abi/4.0'on every install there, even--check.Why nothing is lost by not loading it there. The profile exists to carry the
usernspermission on hosts whose kernel restricts unprivileged user namespaces (Ubuntu 24.04+). Debian 12's kernel does not, which is whymin finalize-installalready installs the profile only where the sysctl restricts them and leaves such a host alone. A second rendering for AppArmor 3.x could not sayuserns, so it would load and do nothing.Change. The loader does what
finalize-installdoes: withoutabi/4.0under the AppArmor directory it says the host mediates no user namespaces and that minimald needs no profile, installs nothing and exits 0, for the install and--checkalike;--uninstallstill removes a file an earlier run left. The profile's header says why there is no second rendering.shellcheckis clean.Evidence. The loader is a file the installer ships beside the profile, not part of the daemon or CLI, so a lab candidate build cannot run this branch's loader: a new scenario that runs the installed loader as the docs say (
--checkas the user, the install and--uninstallas root) reads the release's loader on both sides. It reproduces the Debian 12 refusal (--checkand the install both fail with the parser's abi error), and it found a second bug on Ubuntu 24.04:--checkdied withapparmor_parser not found, because the parser lives in /usr/sbin and a user's PATH does not hold it; the second commit here adds /usr/sbin and /sbin before looking. The baseline scenario passes on both sides, and the installer rehearsal covers the script. The fixed loader proves itself the first time that scenario runs against a release that ships it.🤖 Generated with Claude Code