Re: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules

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 :-)