Skip to content

Conversation

@georgeliao
Copy link
Contributor

@georgeliao georgeliao commented Jan 10, 2025

Fix #3834
Possible explanation on the behavior we saw.

  1. The original code just added %4/bin/bridge_helper without any permissions. The behavior for this case is the bridging feature can run with the permissions it needs and the mount folder lost its write permission somehow. The reason behind this is that bridge_helper operates in the unrestricted mode when no permissions are specified, meaning that it inherits all permissions and capabilities from the parent process. That is why the bridging feature is able to run. When AppArmor is in unrestricted mode (%4/bin/bridge_helper without explicit permissions), the mode may interfere with the rule order in the profile. More specifically, it can result in mounted folder permissions which is defined later in this profile being overridden or deprioritized. That is why the write permission issue occurs.
  2. If %4/bin/bridge_helper ix is used, AppArmor applies strict enforcement on the executable. In this case, it does not seem to disrupt the mount folder permissions setting, so the mount folder feature works properly. On the other side, since it is strict mode, we need to specify everything it requires to make the bridging works. That includes
capability net_admin,
capability net_raw,

The key take away from this is always use explicit permissions (strict mode) on newly added binaries and find the minimal setup (permissions and capabilities) to make it work.

The functional testing should cover the bridging feature (multipass launch -n vm2 --network=mpqemubr0) and the linux qemu native mount.

Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

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

I confirm this makes both native mounts and bridging work properly. Just curious about something inline.

Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

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

Alright 👍

@ricab ricab enabled auto-merge January 13, 2025 19:27
@georgeliao georgeliao force-pushed the fix_qemu_native_mount_regression branch from dc71170 to f87ce6b Compare January 14, 2025 14:21
@codecov
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.02%. Comparing base (966382a) to head (f87ce6b).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3871   +/-   ##
=======================================
  Coverage   89.02%   89.02%           
=======================================
  Files         255      255           
  Lines       14577    14577           
=======================================
  Hits        12977    12977           
  Misses       1600     1600           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ricab ricab added this pull request to the merge queue Jan 14, 2025
Merged via the queue into main with commit eb18d70 Jan 14, 2025
14 checks passed
@ricab ricab deleted the fix_qemu_native_mount_regression branch January 14, 2025 16:50
ricab added a commit that referenced this pull request Feb 7, 2025
ricab added a commit that referenced this pull request Feb 7, 2025
ricab added a commit that referenced this pull request Feb 7, 2025
ricab added a commit that referenced this pull request Feb 8, 2025
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.

permission denied in writing / removing files

4 participants