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