[libvirt] [PATCH] Remove bogus virSecurityManagerSetProcessFDLabel method
The virSecurityManagerSetProcessFDLabel method was introduced after a mis-understanding from a conversation about SELinux socket labelling. The virSecurityManagerSetSocketLabel method should have been used for all such scenarios. * src/security/security_apparmor.c, src/security/security_apparmor.c, src/security/security_driver.h, src/security/security_manager.c, src/security/security_manager.h, src/security/security_selinux.c, src/security/security_stack.c: Remove SetProcessFDLabel driver --- src/security/security_apparmor.c | 29 ----------------------------- src/security/security_dac.c | 9 --------- src/security/security_driver.h | 4 ---- src/security/security_manager.c | 11 ----------- src/security/security_manager.h | 3 --- src/security/security_selinux.c | 14 -------------- src/security/security_stack.c | 18 ------------------ 7 files changed, 0 insertions(+), 88 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index dbd1290..299dcc6 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -799,34 +799,6 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, vm, fd_path, true); } -static int -AppArmorSetProcessFDLabel(virSecurityManagerPtr mgr, - virDomainObjPtr vm, - int fd) -{ - int rc = -1; - char *proc = NULL; - char *fd_path = NULL; - - const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - - if (secdef->imagelabel == NULL) - return 0; - - if (virAsprintf(&proc, "/proc/self/fd/%d", fd) == -1) { - virReportOOMError(); - return rc; - } - - if (virFileResolveLink(proc, &fd_path) < 0) { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("could not find path for descriptor")); - return rc; - } - - return reload_profile(mgr, vm, fd_path, true); -} - virSecurityDriver virAppArmorSecurityDriver = { 0, SECURITY_APPARMOR_NAME, @@ -863,5 +835,4 @@ virSecurityDriver virAppArmorSecurityDriver = { AppArmorRestoreSavedStateLabel, AppArmorSetImageFDLabel, - AppArmorSetProcessFDLabel, }; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e5465fc..af02236 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -697,14 +697,6 @@ virSecurityDACSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } -static int -virSecurityDACSetProcessFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainObjPtr vm ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED) -{ - return 0; -} - virSecurityDriver virSecurityDriverDAC = { sizeof(virSecurityDACData), @@ -743,5 +735,4 @@ virSecurityDriver virSecurityDriverDAC = { virSecurityDACRestoreSavedStateLabel, virSecurityDACSetImageFDLabel, - virSecurityDACSetProcessFDLabel, }; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 94f27f8..aea90b0 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -84,9 +84,6 @@ typedef int (*virSecurityDomainSecurityVerify) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetImageFDLabel) (virSecurityManagerPtr mgr, virDomainObjPtr vm, int fd); -typedef int (*virSecurityDomainSetProcessFDLabel) (virSecurityManagerPtr mgr, - virDomainObjPtr vm, - int fd); struct _virSecurityDriver { size_t privateDataLen; @@ -124,7 +121,6 @@ struct _virSecurityDriver { virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel; virSecurityDomainSetImageFDLabel domainSetSecurityImageFDLabel; - virSecurityDomainSetProcessFDLabel domainSetSecurityProcessFDLabel; }; virSecurityDriverPtr virSecurityDriverLookup(const char *name); diff --git a/src/security/security_manager.c b/src/security/security_manager.c index b2fd0d0..cae9b83 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -346,14 +346,3 @@ int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; } - -int virSecurityManagerSetProcessFDLabel(virSecurityManagerPtr mgr, - virDomainObjPtr vm, - int fd) -{ - if (mgr->drv->domainSetSecurityProcessFDLabel) - return mgr->drv->domainSetSecurityProcessFDLabel(mgr, vm, fd); - - virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); - return -1; -} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 38342c2..12cd498 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -96,8 +96,5 @@ int virSecurityManagerVerify(virSecurityManagerPtr mgr, int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm, int fd); -int virSecurityManagerSetProcessFDLabel(virSecurityManagerPtr mgr, - virDomainObjPtr vm, - int fd); #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index cddbed5..ca54f9b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1321,19 +1321,6 @@ SELinuxSetImageFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return SELinuxFSetFilecon(fd, secdef->imagelabel); } -static int -SELinuxSetProcessFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainObjPtr vm, - int fd) -{ - const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - - if (secdef->label == NULL) - return 0; - - return SELinuxFSetFilecon(fd, secdef->label); -} - virSecurityDriver virSecurityDriverSELinux = { 0, SECURITY_SELINUX_NAME, @@ -1370,5 +1357,4 @@ virSecurityDriver virSecurityDriverSELinux = { SELinuxRestoreSavedStateLabel, SELinuxSetImageFDLabel, - SELinuxSetProcessFDLabel, }; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index f263f5b..3f601c1 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -402,23 +402,6 @@ virSecurityStackSetImageFDLabel(virSecurityManagerPtr mgr, } -static int -virSecurityStackSetProcessFDLabel(virSecurityManagerPtr mgr, - virDomainObjPtr vm, - int fd) -{ - virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); - int rc = 0; - - if (virSecurityManagerSetProcessFDLabel(priv->secondary, vm, fd) < 0) - rc = -1; - if (virSecurityManagerSetProcessFDLabel(priv->primary, vm, fd) < 0) - rc = -1; - - return rc; -} - - virSecurityDriver virSecurityDriverStack = { sizeof(virSecurityStackData), "stack", @@ -455,5 +438,4 @@ virSecurityDriver virSecurityDriverStack = { virSecurityStackRestoreSavedStateLabel, virSecurityStackSetImageFDLabel, - virSecurityStackSetProcessFDLabel, }; -- 1.7.4.4
On 08/30/2011 10:37 AM, Daniel P. Berrange wrote:
The virSecurityManagerSetProcessFDLabel method was introduced after a mis-understanding from a conversation about SELinux socket labelling. The virSecurityManagerSetSocketLabel method should have been used for all such scenarios.
* src/security/security_apparmor.c, src/security/security_apparmor.c, src/security/security_driver.h, src/security/security_manager.c, src/security/security_manager.h, src/security/security_selinux.c, src/security/security_stack.c: Remove SetProcessFDLabel driver --- +++ b/src/security/security_apparmor.c @@ -799,34 +799,6 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, vm, fd_path, true); }
-static int -AppArmorSetProcessFDLabel(virSecurityManagerPtr mgr, - virDomainObjPtr vm, - int fd) -{ - int rc = -1; - char *proc = NULL; - char *fd_path = NULL; - - const virSecurityLabelDefPtr secdef =&vm->def->seclabel;
This is non-trivial code. While we've already determined that SELinux doesn't need SetProcessFDLabel, is there any chance that app-armor still needs this approach? If so, that would argue for keeping the function, but making it a no-op stub for SELinux, and still calling it in all the right places for the benefit of app-armor. I'm not familiar enough with app-armor theory of operation to answer this question, and without an answer, I can't give ack or nack. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Aug 30, 2011 at 10:51:33AM -0600, Eric Blake wrote:
On 08/30/2011 10:37 AM, Daniel P. Berrange wrote:
The virSecurityManagerSetProcessFDLabel method was introduced after a mis-understanding from a conversation about SELinux socket labelling. The virSecurityManagerSetSocketLabel method should have been used for all such scenarios.
* src/security/security_apparmor.c, src/security/security_apparmor.c, src/security/security_driver.h, src/security/security_manager.c, src/security/security_manager.h, src/security/security_selinux.c, src/security/security_stack.c: Remove SetProcessFDLabel driver --- +++ b/src/security/security_apparmor.c @@ -799,34 +799,6 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, vm, fd_path, true); }
-static int -AppArmorSetProcessFDLabel(virSecurityManagerPtr mgr, - virDomainObjPtr vm, - int fd) -{ - int rc = -1; - char *proc = NULL; - char *fd_path = NULL; - - const virSecurityLabelDefPtr secdef =&vm->def->seclabel;
This is non-trivial code. While we've already determined that SELinux doesn't need SetProcessFDLabel, is there any chance that app-armor still needs this approach? If so, that would argue for keeping the function, but making it a no-op stub for SELinux, and still calling it in all the right places for the benefit of app-armor.
I'm not familiar enough with app-armor theory of operation to answer this question, and without an answer, I can't give ack or nack.
The app armour code here was just copied from the similarly name SetImageFDLabel, which resolves the FD into a file path using /proc/self/fd/$FDNUM. This actually never worked for TCP sockets with apparmour, so I don't believe I'm making anything worse. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/30/2011 11:03 AM, Daniel P. Berrange wrote:
This is non-trivial code. While we've already determined that SELinux doesn't need SetProcessFDLabel, is there any chance that app-armor still needs this approach? If so, that would argue for keeping the function, but making it a no-op stub for SELinux, and still calling it in all the right places for the benefit of app-armor.
I'm not familiar enough with app-armor theory of operation to answer this question, and without an answer, I can't give ack or nack.
The app armour code here was just copied from the similarly name SetImageFDLabel, which resolves the FD into a file path using /proc/self/fd/$FDNUM. This actually never worked for TCP sockets with apparmour, so I don't believe I'm making anything worse.
Fair enough. If we actually need something for tcp socket labeling in apparmor, then we can add a working solution later; disabling the questionable code now is okay. You've given me an answer good enough that I feel comfortable for: ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, 2011-08-30 at 10:51 -0600, Eric Blake wrote:
On 08/30/2011 10:37 AM, Daniel P. Berrange wrote:
The virSecurityManagerSetProcessFDLabel method was introduced after a mis-understanding from a conversation about SELinux socket labelling. The virSecurityManagerSetSocketLabel method should have been used for all such scenarios.
* src/security/security_apparmor.c, src/security/security_apparmor.c, src/security/security_driver.h, src/security/security_manager.c, src/security/security_manager.h, src/security/security_selinux.c, src/security/security_stack.c: Remove SetProcessFDLabel driver --- +++ b/src/security/security_apparmor.c @@ -799,34 +799,6 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, vm, fd_path, true); }
-static int -AppArmorSetProcessFDLabel(virSecurityManagerPtr mgr, - virDomainObjPtr vm, - int fd) -{ - int rc = -1; - char *proc = NULL; - char *fd_path = NULL; - - const virSecurityLabelDefPtr secdef =&vm->def->seclabel;
This is non-trivial code. While we've already determined that SELinux doesn't need SetProcessFDLabel, is there any chance that app-armor still needs this approach? If so, that would argue for keeping the function, but making it a no-op stub for SELinux, and still calling it in all the right places for the benefit of app-armor.
I'm not familiar enough with app-armor theory of operation to answer this question, and without an answer, I can't give ack or nack.
ACK. Like Daniel said in another response, this was just copied over code and not something that is used by the AppArmor driver in any meaningful way. -- Jamie Strandboge | http://www.canonical.com
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jamie Strandboge