On Wed, Jun 14, 2006 at 01:44:36PM +0100, Daniel P. Berrange wrote:
> On Mon, Jun 12, 2006 at 06:52:57PM -0400, Daniel Veillard wrote:
>
> I'm attaching an initial patch which implements the driver backend calls
> for these five methods.
Okay,
> I've tested that suspend & resume work, but my hardware died before I fully
> tested the other methods. I've got a couple questions before proceeding in
> any case:
>
> 1. In all of these methods I followed example from virDomainSetMemory and
> put in
>
> if (domain->conn->flags & VIR_CONNECT_RO)
> return (-1);
>
> This prevents these methods working with a 'read only' connection, however,
> this is a deviation from previous semantics. Even with a read only connection,
> XenD will let arbitrary unprivileged user shutdown/suspend/resume/etc a
> domain, so by putting this VIR_CONNECT_RO check in we'd be preventing an
> operation which used to work.
Hum, there is pros and cons. Pro is obviously cleaness and long term
maintainance/expectations (this will have to be fixed). Cons is the fact
it is allowed and putting the limitation in libvirt does not fix anything
and we don't know yet what the final security policy will be...
Also it blocks regression tests from running as an user and force to su
before running 'make tests' which is a bit inconvenient...
> However, IMHO this is a good thing, because it can be considered a *HUGE*
> security hole / denial of service attack, that unprivileged users can shutdown
> reboot, suspend, etc domains via XenD with no authentication. When this hole
> is eventually plugged, these methods would cease to work with a read only
> connection, so long term we'd end up in the same situation. Thus this patch
> could be considered a 'pre-emptive' solution.
pre-emptive but shooting a bit in the dark. I'm balanced, others may
have an opinion there, please express yourselve :-)
> 2. The current implementation of shutdown & reboot methods calls the same code
> twice in a succession:
>
> /*
> * try first with the xend daemon
> */
> ret = xenDaemonDomainShutdown(domain);
> if (ret == 0) {
> domain->flags |= DOMAIN_IS_SHUTDOWN;
> return (0);
> }
>
> /*
> * this is very hackish, the domU kernel probes for a special
> * node in the xenstore and launch the shutdown command if found.
> */
> ret = xenDaemonDomainShutdown(domain);
> if (ret == 0) {
> domain->flags |= DOMAIN_IS_SHUTDOWN;
> }
>
>
> What was the reason to call xenDaemonDomainShutdown twice ? With my
my guess is that's just an error due to a factoring remains from when the
xenDaemonDomainShutdown() code was directly inlined in that routine.
> change to use the driver backends, it will only be called once. So I want
> to make sure I'm not missing an edge case here.
I think that's fine :-)
> Index: src/libvirt.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/libvirt.c,v
> retrieving revision 1.29
> diff -u -r1.29 libvirt.c
I need to remember to not modify those 5 functions.
thanks !
Daniel
--
Daniel Veillard | Red Hat http://redhat.com/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/