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

Use dh_apparmor for installing profile #1856

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Use dh_apparmor for installing profile #1856

merged 1 commit into from
Mar 4, 2024

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Feb 21, 2024

Status

Ready for review

Description

dh_apparmor generates the appropriate postinst and postrm snippets for (re)loading apparmor profiles in a way that seems more robust than our current usage of aa-enforce.

If apparmor isn't enabled, then it gracefully skips instead of aborting the installation. This allows for installing the client package in contexts where apparmor isn't available, like containers. Notably we can now enable piuparts testing for the client.

Fixes #1853.

Test Plan

  • Build package, install in a qubes VM. Start client (just the login screen is fine), run sudo aa-status, see output like:
17 profiles are in enforce mode.
   ...
   /usr/bin/securedrop-client <-- this
...
4 processes are in enforce mode.
   /usr/bin/dash (10941) /usr/bin/securedrop-client <-- and
   /usr/bin/python3.9 (10944) /usr/bin/securedrop-client <-- this
  • Spin up a podman/docker container, install the package and it succeeds. Run sudo aa-status to verify apparmor is not enabled.

@legoktm legoktm requested a review from a team as a code owner February 21, 2024 19:39
@legoktm
Copy link
Member Author

legoktm commented Feb 21, 2024

The one major change that happens here is that there is no longer a hard failure is apparmor is disabled. I think that check was dubious because it only happened during install time in the template and not in the actual running VM. So maybe we do want some check that apparmor is enabled in the sd-app VM? I would rather do that as part of freedomofpress/securedrop-workstation#939.

I was wondering if the securedrop-client script could tell if it was unconfined, we could

  1. add enough things to the profile to allow running aa-status, and check the output
  2. run a banned command, i.e. sudo true, or try a banned file access; if it fails then we know we're confined. this will generate constant denials in the logs though
  3. have /usr/bin/securedrop-client be unconfined, check aa-status to ensure profile is loaded, then invoke /usr/bin/securedrop-client.real, which is confined.

No. 2 actually tests that we're confined, whereas 1 and 3 are just relying on aa-status to be true.

@legoktm legoktm force-pushed the dh_apparmor branch 2 times, most recently from 1262013 to 0460b2d Compare February 21, 2024 21:10
@legoktm
Copy link
Member Author

legoktm commented Feb 21, 2024

or try a banned file access; if it fails then we know we're confined

For example, adding this to /usr/bin/securedrop-client:

if ls /usr/bin/sudo 2>&1 >/dev/null; then
    echo "Unconfined"
    exit 1
fi

And then running sudo aa-complain /usr/bin/securedrop-client causes the client to not start.

dh_apparmor generates the appropriate postinst and postrm snippets for
(re)loading apparmor profiles in a way that seems more robust than our
current usage of `aa-enforce`.

If apparmor isn't enabled, then it gracefully skips instead of aborting
the installation. This allows for installing the client package in
contexts where apparmor isn't available, like containers. Notably
we can now enable piuparts testing for the client.

Fixes #1853.
@cfm cfm self-assigned this Mar 4, 2024
Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

This looks good, @legoktm, and I like the approach you've sketched here.

@cfm cfm merged commit b46c725 into main Mar 4, 2024
92 checks passed
@cfm cfm deleted the dh_apparmor branch March 4, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Use dh_apparmor for installing profile
2 participants