Re: [libvirt] [PATCH] Solaris least privilege support

On Wed, Jan 14, 2009 at 07:32:28PM -0800, john.levon(a)sun.com wrote: This chunk is potentially usefull on Linux too, so can just remove the #ifdef here. Also make sure to use VIR_FREE rather than free() Finally, the exit(-1) should be a return -1; Rather than have this magic value in the code, lets put a #define at the top of the file #ifdef __sun #define SYSTEM_ACCOUNT 60 #else #define SYSTEM_ACCOUNT 0 #endif And then just change all use of !uid to 'uid == SYSTEM_ACCOUNT' Can move the ucred_free up before priv_ismember() call and thus avoid the need for the call in the cleanup path. Can we move this up to the declaration point - this function should only be reacting to config file settings. FOr the global default we can just declare it #ifdef __sun static int unix_sock_rw_mask = 0666; /* Allow world */ static int unix_sock_ro_mask = 0777; /* Allow world */ #else static int unix_sock_rw_mask = 0700; /* Allow user only */ static int unix_sock_ro_mask = 0777; /* Allow world */ #endif Again prefer to see the #define SYTEM_ACCOUNT instead of the magic numbers. Is the chmod of the socket really required for solaris ? We already set the umask before creating the socket, so it ought to get desired permissions from moment it appears. We avoided an explicit chmod because that leaves a gap between socket creation, and correct ownership and permissions being configured. Also, if this libvirtd is running as UID 60, so the chown really needed ? We also setgid before creating the socket so that it gets desired group ownership at time of creation, rather than having to change it post-create. This would appear to make it try to change the socket ownership and permissions, before we've actually created the socket, which is much later in the main() method where we call NetworkInit This should not be neccessary if the client end + drivers are correctly written. The RPC calls handlers should not be trying to re-interpret args, they should just pass everything straight through to the libvirt APIs. This should also not be neccessary. If you want Xen to always go via the demon, the only change that should be required, is to make xenUnifiedOpen() return VIR_DRV_OPEN_DECLINED. The main virConnectOpen() logic will then fallthrough to the remote driver which will always accept any URI not handled by an earlier driver. So basically just need to edit xenUnifiedOpen() and make it rejects all open requests when not in the daemon context - I guess a privilege check would suffic for that logic ? If not, then we can register a trivial virStateDrv impl that just sets a flag when run as part of the daemon - see the 'remoteStartup' method in remote_internal.c for example of how to set a flag when running inside the daemon. Ok, not a big deal - it'll still work if explicitly requested with a qemu:///session URI - if we make QEMU driver work on Solaris later. These two I/O methods here have been completely re-written in my thread patches. Why is removing the error messages required ? It will also be seen if the daemon drops the connection due to an OOM condition, or the max-clients limit being exceeded, so perhaps a little more detailed message. ACK, we've reviewed this bit several times before. ACK to this bit too. ACK ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|