On 1/8/25 06:50, Georgia Garcia wrote:
On Tue, 2025-01-07 at 17:29 -0700, Jim Fehlig wrote:
On 1/7/25 08:23, Georgia Garcia wrote:
Some rules are generated dynamically during boot and added to the
AppArmor policy. An example of that is macvtap devices that call the
AppArmorSetFDLabel hook to add a rule for the tap device path.
Since this information is dynamic, it is not available in the xml
config, therefore whenever a "Restore" hook is called, the entire
profile is regenerated by virt-aa-helper based only the information
from the VM definition, so the dynamic/runtime information is lost.
Have you considered, or experimented with, adding a "remove file" option to the
"replace" mode of virt-aa-helper? Figuring out the short name of the option
might be the most difficult part :-P.
I didn't experiment with it because I thought it was a change too
drastic to change the behavior of "Restore" to instead of regenerate
the policy based on the xml, to read the policy, string match the drive
(for example) being removed, remove that entry and rewrite the policy
file. By maintaining current behavior for the most part I would also
lower the risk of regressions. It might be possible but I'd have to
look into more detail into all the "Restore" hooks to say for certain.
Given the current behavior of Restore, I share your fear of regressions.
This patch stores the dynamically generated rules in a new file called
libvirt-uuid.runtime_files which is included by the AppArmor
policy. This file should exist while the domain is running and should
be reloaded automatically whenever there's a restore operation. These
rules only make sense when the VM is running, so the file is removed
when the VM is shutdown.
I'm not super excited about this approach, but I'm also no apparmor expert.
Perhaps my preference for a '--remove-file' option to supplement '--add-file' is
not really possible or realistic with the current apparmor integration.
Andrea also has some experience with apparmor and its libvirt support. He may
have better advice on fixing this issue.
Since there aren't hooks for removing permissions for files that were
created by FD (domainSetSecurityImageFDLabel /
domainSetSecurityTapFDLabel) I figured that separating them in a
different file was the best approach but I'm open to changing it if
it's more appropriate. Any feedback is welcome!
I'm not against the approach, and indeed it may be the safest way to go. Before
I invest time reviewing and testing this patch, let's see if others have
comments/suggestions.
Regards,
Jim
PS: Add Andrea to cc this time :-)
