prashanthr2 commented on PR #13281:
URL: https://github.com/apache/cloudstack/pull/13281#issuecomment-6014539532
LGTM overall, this is the right direction.
+1 to @andrijapanicsb's points, all four are correct.
On the first one, it's worth being specific about what a host set up by the
old code is left holding, because that code wrote persistent on-disk state, not
just runtime state:
- securityPolicyConfigUbuntu.config() symlinked both libvirt profiles into
/etc/apparmor.d/disable/ and ran apparmor_parser -R. restore() only re-enables
the apparmor service. It never removes those symlinks, so the profiles stay
disabled across reboots.
- securityPolicyConfigRedhat.config() ran setenforce 0 and rewrote
/etc/selinux/config to SELINUX=permissive. restore() only runs setenforce 1,
which is lost at the next boot.
This PR is right not to undo any of that automatically , re-enforcing a
host's security policy behind the operator's back would be as bad as disabling
it was. The consequence is just that an existing host carries on with SELinux
permissive / AppArmor disabled, and nothing surfaces that. Anyone who wants it
back has to do it by hand: remove the two _/etc/apparmor.d/disable/ symlinks_
and re-parse the profiles, or set _SELINUX=enforcing_ and relabel, plus unset
_security_driver_ in _/etc/libvirt/qemu.conf._ That procedure should be in the
**release notes**, not only the docs , release notes reach the operator before
they upgrade; docs only reach whoever goes looking afterwards. Worth
remembering the agent packages are upgraded per host, manually, well after the
management server, so a zone will sit with both behaviours for a while.
The other three: agreed that on fresh installs libvirt may apply
AppArmor/SELinux to QEMU automatically so post-install behaviour genuinely
differs from before and should be stated plainly; AppArmor section titles
should mention Debian, not just Ubuntu; and a blanket aa-enforce
/etc/apparmor.d/* should be avoided since it enforces profiles unrelated to
libvirt/QEMU.
**Suggestion (nice-to-have, not a blocker):** have agent setup detect and
report the current state rather than change it. The detection calls already
exist in the code being deleted _bash("selinuxenabled")_ in
_securityPolicyConfigRedhat.config()_, _bash("service apparmor status") /
bash("apparmor_status | grep libvirt")_ in
_securityPolicyConfigUbuntu.config()_ so keeping just the read half is close to
zero risk. The value is diagnostic: when a policy blocks QEMU, the symptom in
agent.log is a generic libvirt permission error while the real cause is an AVC
denial in audit.log, a different layer entirely. One line at setup ("SELinux:
enforcing", "AppArmor: libvirtd profile enforcing") shortens that path a lot,
and it makes the leftover state on older hosts visible.
Relatedly, Requires: (selinux-tools if selinux-tools) in the agent
subpackage of both packaging/el8/cloud.spec and packaging/suse15/cloud.spec ,
still wanted once setup no longer touches SELinux?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
