Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-03-03 Thread Daniel P. Berrange 
On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 Another patch off latest repository.
 
 This patch does not require the XML to include a label, although this is
 still supported.
 
 Implemented most of the comments from Jim.  make check and make
 syntax-check passes, Added seclabeltest.c to run in tests, Updated
 capability.rng, although not really sure I did it right.
 
 This patch will generate random MCS Labels and relabels the image files
 to match.  Seems to work well on F11.
 
 I will back port some policy to allow it to work on F10.
 
 I think we need a mechanism in libvirtd.conf to turn this off.   And
 allow perhaps three modes.
 
 svirt=Disabled.  No Security Driver.
 svirt=MLS (Requires context in xml, no relabel of disks)
 svirt=Standard, (If no XML label, then random generate one and reset
 file context).
 
 How should I read config from libvirt.conf and and not enable he
 SecurityModel?
 
 http://people.fedoraproject.org/~dwalsh/SELinux/svirt.patch

I have finally applied this patch. I broke it up into  a series of
7 patches across the different functional areas, to make it easier
to bisect individual changes, so I applied it in the following pieces

 - Public API definitions
 - Internal driver API glue
 - Remote protocol API  glue
 - Core security driver infrastructure
 - Virsh additions for dominfo
 - SElinux security driver
 - QEMU integration with security driver


I made a couple of small changes along the way...

 - virSecurityDriverStartup() takes a driver name, so HV drivers
   can explicitly configure which sec driver they want, overriding
   the default probed order. 'none' disables it completely

 - /etc/libvirt/qemu.conf gains a security_driver='XXX' config param
   accepting 'none' or 'selinux' to choose drivers. If not set it will
   probe for a driver, thus defaulting to SELinux if availab.e

 - Fixed the RNG schema for capabilities  domain XML format additions

 - Added a configure.in check for selinux_virtual_domain_context_path()
   and selinux_virtual_image_context_path() and make it disable the
   SELinux driver if these aren't found. These functions are new on
   F11, so we don't want to break build on RHEL-5  earlier Fedora.


I still think we need one further tweak to the XML. We have the ability
to turn on / off of the security driver in QEMU, but I think we need
better support for the automatic label generation. The current logic
is doing

 - If seclabel is element in the XML, use that
 - Else generate a seclabel when starting a VM

The trouble is when you then query the XML for a guest, you have no way
of telling whether the seclabel is showing a generated one, or a predefined
one. And if you dump and then reload the XML, your VM that used to be using
a generated label, now gets fixed to that current label forever. This has
caused us a great deal of pain in the past with generated VNC ports, and
generated TAP device names. So I think we need to add an XML attribute to
explicitly note that the label is generated

eg, add  type=static|dynamic to seclabel ...

  seclabel model='selinux' type=static
labelsystem_u:system_r:qemu_t:s0:c210.c502/label
  /seclabel


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

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-03-03 Thread Daniel P. Berrange 
On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 Another patch off latest repository.
 
 This patch does not require the XML to include a label, although this is
 still supported.
 
 Implemented most of the comments from Jim.  make check and make
 syntax-check passes, Added seclabeltest.c to run in tests, Updated
 capability.rng, although not really sure I did it right.
 
 This patch will generate random MCS Labels and relabels the image files
 to match.  Seems to work well on F11.

I had a few problems with label generation on my F11 machine - perhaps
you have a newer version of the patch than the one I applied.

I found I need the following additional patch..

 - Make domainGenSecurityLabel() give diagnostics for each type of error
   instead of using generic error message in caller
 - Change  logic bug 'c1 == c2' to 'c1  c2'
 - Change 'c%d,c%d' to 'c%d.c%d' - it doesn't like labels with
   the form c210,c502  only wanting c210.c502
 - Fix use of STREQ - no need for == 0 in there
 - Use VIR_FREE/VIR_ALLOC for memory mgmt

With this I can successfully start several VMs, and see them all
using different contexts, and see the files labelled

# ps -xZ | grep qemu | awk '{print $1}'
system_u:system_r:qemu_t:s0:c35.c537
system_u:system_r:qemu_t:s0:c210.c502

# ls -Zl /var/lib/libvirt/images/
total 504
-rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c210.c502 root root 1073741824 
2009-03-03 12:15 demo2.img
-rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c35.c537 root root 1073741824 
2009-03-03 11:49 demo.img


Daniel

Index: src/qemu_driver.c
===
RCS file: /data/cvs/libvirt/src/qemu_driver.c,v
retrieving revision 1.212
diff -u -p -r1.212 qemu_driver.c
--- src/qemu_driver.c   3 Mar 2009 12:03:44 -   1.212
+++ src/qemu_driver.c   3 Mar 2009 12:25:47 -
@@ -1316,13 +1316,11 @@ static int qemudStartVMDaemon(virConnect
 
/* If you are using a SecurityDriver and there was no security label in
   database, then generate a security label for isolation */
-if (vm-def-seclabel.label == NULL  driver-securityDriver) {
-if (driver-securityDriver-domainGenSecurityLabel(vm)  0) {
-qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
- %s, _(Unable to generate Security Label));
-return -1;
-}
-}
+if (vm-def-seclabel.label == NULL 
+   driver-securityDriver 
+   driver-securityDriver-domainGenSecurityLabel 
+   driver-securityDriver-domainGenSecurityLabel(conn, vm)  0)
+return -1;
 
 FD_ZERO(keepfd);
 
Index: src/security.h
===
RCS file: /data/cvs/libvirt/src/security.h,v
retrieving revision 1.1
diff -u -p -r1.1 security.h
--- src/security.h  3 Mar 2009 09:44:42 -   1.1
+++ src/security.h  3 Mar 2009 12:25:47 -
@@ -37,7 +37,8 @@ typedef int (*virSecurityDomainRestoreIm
 typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
virDomainObjPtr vm,
virDomainDeviceDefPtr dev);
-typedef int (*virSecurityDomainGenLabel) (virDomainObjPtr sec);
+typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
+ virDomainObjPtr sec);
 typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn,
   virDomainObjPtr vm,
   virSecurityLabelPtr sec);
Index: src/security_selinux.c
===
RCS file: /data/cvs/libvirt/src/security_selinux.c,v
retrieving revision 1.1
diff -u -p -r1.1 security_selinux.c
--- src/security_selinux.c  3 Mar 2009 10:06:49 -   1.1
+++ src/security_selinux.c  3 Mar 2009 12:25:47 -
@@ -24,6 +24,9 @@
 #include util.h
 #include memory.h
 
+
+#define VIR_FROM_THIS VIR_FROM_SECURITY
+
 static char default_domain_context[1024];
 static char default_image_context[1024];
 #define SECURITY_SELINUX_VOID_DOI   0
@@ -45,10 +48,11 @@ mcsAdd(const char *mcs)
 struct MCS *ptr;
 
 for (ptr = mcsList; ptr; ptr = ptr-next) {
-if (STREQ(ptr-mcs, mcs) == 0)
+if (STREQ(ptr-mcs, mcs))
 return -1;
 }
-ptr = malloc(sizeof(struct MCS));
+if (VIR_ALLOC(ptr)  0)
+return -1;
 ptr-mcs = strdup(mcs);
 ptr-next = mcsList;
 mcsList = ptr;
@@ -62,7 +66,7 @@ mcsRemove(const char *mcs)
 struct MCS *ptr = NULL;
 
 for (ptr = mcsList; ptr; ptr = ptr-next) {
-if (STREQ(ptr-mcs, mcs) == 0) {
+if (STREQ(ptr-mcs, mcs)) {
 if (prevptr)
 prevptr-next = ptr-next;
 else {
@@ -149,7 +153,8 @@ SELinuxInitialize(virConnectPtr conn)
 }
 
 static int
-SELinuxGenSecurityLabel(virDomainObjPtr vm)

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-03-03 Thread Daniel J Walsh 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Daniel P. Berrange wrote:
 On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1

 Another patch off latest repository.

 This patch does not require the XML to include a label, although this is
 still supported.

 Implemented most of the comments from Jim.  make check and make
 syntax-check passes, Added seclabeltest.c to run in tests, Updated
 capability.rng, although not really sure I did it right.

 This patch will generate random MCS Labels and relabels the image files
 to match.  Seems to work well on F11.
 
 I had a few problems with label generation on my F11 machine - perhaps
 you have a newer version of the patch than the one I applied.
 
 I found I need the following additional patch..
 
  - Make domainGenSecurityLabel() give diagnostics for each type of error
instead of using generic error message in caller
  - Change  logic bug 'c1 == c2' to 'c1  c2'
  - Change 'c%d,c%d' to 'c%d.c%d' - it doesn't like labels with
the form c210,c502  only wanting c210.c502
This does not make sense.  c210,c502 is valid.  c210.c502 means include
the range.  c210, c211, c212...c502.
  - Fix use of STREQ - no need for == 0 in there
I am reworking this code to use INT instead of strings.
  - Use VIR_FREE/VIR_ALLOC for memory mgmt
 
 With this I can successfully start several VMs, and see them all
 using different contexts, and see the files labelled
 
 # ps -xZ | grep qemu | awk '{print $1}'
 system_u:system_r:qemu_t:s0:c35.c537
 system_u:system_r:qemu_t:s0:c210.c502
 
 # ls -Zl /var/lib/libvirt/images/
 total 504
 -rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c210.c502 root root 
 1073741824 2009-03-03 12:15 demo2.img
 -rwxr-xr-x. 1 system_u:object_r:virt_image_t:s0:c35.c537 root root 1073741824 
 2009-03-03 11:49 demo.img
 
 
 Daniel
 
 Index: src/qemu_driver.c
 ===
 RCS file: /data/cvs/libvirt/src/qemu_driver.c,v
 retrieving revision 1.212
 diff -u -p -r1.212 qemu_driver.c
 --- src/qemu_driver.c 3 Mar 2009 12:03:44 -   1.212
 +++ src/qemu_driver.c 3 Mar 2009 12:25:47 -
 @@ -1316,13 +1316,11 @@ static int qemudStartVMDaemon(virConnect
  
 /* If you are using a SecurityDriver and there was no security label in
database, then generate a security label for isolation */
 -if (vm-def-seclabel.label == NULL  driver-securityDriver) {
 -if (driver-securityDriver-domainGenSecurityLabel(vm)  0) {
 -qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
 - %s, _(Unable to generate Security Label));
 -return -1;
 -}
 -}
 +if (vm-def-seclabel.label == NULL 
 + driver-securityDriver 
 + driver-securityDriver-domainGenSecurityLabel 
 + driver-securityDriver-domainGenSecurityLabel(conn, vm)  0)
 +return -1;
  
  FD_ZERO(keepfd);
  
 Index: src/security.h
 ===
 RCS file: /data/cvs/libvirt/src/security.h,v
 retrieving revision 1.1
 diff -u -p -r1.1 security.h
 --- src/security.h3 Mar 2009 09:44:42 -   1.1
 +++ src/security.h3 Mar 2009 12:25:47 -
 @@ -37,7 +37,8 @@ typedef int (*virSecurityDomainRestoreIm
  typedef int (*virSecurityDomainSetImageLabel) (virConnectPtr conn,
 virDomainObjPtr vm,
 virDomainDeviceDefPtr dev);
 -typedef int (*virSecurityDomainGenLabel) (virDomainObjPtr sec);
 +typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn,
 +   virDomainObjPtr sec);
  typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn,
virDomainObjPtr vm,
virSecurityLabelPtr sec);
 Index: src/security_selinux.c
 ===
 RCS file: /data/cvs/libvirt/src/security_selinux.c,v
 retrieving revision 1.1
 diff -u -p -r1.1 security_selinux.c
 --- src/security_selinux.c3 Mar 2009 10:06:49 -   1.1
 +++ src/security_selinux.c3 Mar 2009 12:25:47 -
 @@ -24,6 +24,9 @@
  #include util.h
  #include memory.h
  
 +
 +#define VIR_FROM_THIS VIR_FROM_SECURITY
 +
  static char default_domain_context[1024];
  static char default_image_context[1024];
  #define SECURITY_SELINUX_VOID_DOI   0
 @@ -45,10 +48,11 @@ mcsAdd(const char *mcs)
  struct MCS *ptr;
  
  for (ptr = mcsList; ptr; ptr = ptr-next) {
 -if (STREQ(ptr-mcs, mcs) == 0)
 +if (STREQ(ptr-mcs, mcs))
  return -1;
  }
 -ptr = malloc(sizeof(struct MCS));
 +if (VIR_ALLOC(ptr)  0)
 +return -1;
  ptr-mcs = strdup(mcs);
  ptr-next = mcsList;
  mcsList = ptr;
 @@ -62,7 +66,7 @@ mcsRemove(const char *mcs)
  struct MCS *ptr = NULL;
  
  for

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-03-03 Thread Daniel P. Berrange 
On Tue, Mar 03, 2009 at 09:08:20AM -0500, Daniel J Walsh wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 Daniel P. Berrange wrote:
  On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote:
  -BEGIN PGP SIGNED MESSAGE-
  Hash: SHA1
 
  Another patch off latest repository.
 
  This patch does not require the XML to include a label, although this is
  still supported.
 
  Implemented most of the comments from Jim.  make check and make
  syntax-check passes, Added seclabeltest.c to run in tests, Updated
  capability.rng, although not really sure I did it right.
 
  This patch will generate random MCS Labels and relabels the image files
  to match.  Seems to work well on F11.
  
  I had a few problems with label generation on my F11 machine - perhaps
  you have a newer version of the patch than the one I applied.
  
  I found I need the following additional patch..
  
   - Make domainGenSecurityLabel() give diagnostics for each type of error
 instead of using generic error message in caller
   - Change  logic bug 'c1 == c2' to 'c1  c2'
   - Change 'c%d,c%d' to 'c%d.c%d' - it doesn't like labels with
 the form c210,c502  only wanting c210.c502
 This does not make sense.  c210,c502 is valid.  c210.c502 means include
 the range.  c210, c211, c212...c502.

Hmm, I reverted this bit of the change  it works fine now - it was probably
one of the other bugs (or outdated RPMs) that caused the original failure

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

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-03-02 Thread Daniel P. Berrange 
On Mon, Mar 02, 2009 at 09:18:05AM +1100, James Morris wrote:
 On Fri, 27 Feb 2009, Daniel J Walsh wrote:
 
  I think we need a mechanism in libvirtd.conf to turn this off.   And
  allow perhaps three modes.
  
  svirt=Disabled.  No Security Driver.
  svirt=MLS (Requires context in xml, no relabel of disks)
  svirt=Standard, (If no XML label, then random generate one and reset
  file context).
 
 I wouldn't call these MLS and Standard.  The simple isolation scheme with 
 automatic labeling is just one way to do things.  Down the track, we'll 
 want to be able to specify arbitrary types for guests, not just for MLS.

I think perhaps we should make this a QEMU driver config option (ie be
in /etc/libvirt/qemu.conf) and have 2 flags 

  security_driver=selinux|none
  security_autolabel=yes|no

If security_autolabel is set to 'no', then the app must pass an explicit
security context in the domain XML, otherwise the domain is unconfined.

If security_autolabel is set to 'yes', then if the app passes an explicit
security context this is used, otherwise it will auto-generate one at
startup of the VM.

Regards,
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 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-03-02 Thread Cole Robinson 
Daniel P. Berrange wrote:
 On Mon, Mar 02, 2009 at 09:18:05AM +1100, James Morris wrote:
 On Fri, 27 Feb 2009, Daniel J Walsh wrote:

 I think we need a mechanism in libvirtd.conf to turn this off.   And
 allow perhaps three modes.

 svirt=Disabled.  No Security Driver.
 svirt=MLS (Requires context in xml, no relabel of disks)
 svirt=Standard, (If no XML label, then random generate one and reset
 file context).
 I wouldn't call these MLS and Standard.  The simple isolation scheme with 
 automatic labeling is just one way to do things.  Down the track, we'll 
 want to be able to specify arbitrary types for guests, not just for MLS.
 
 I think perhaps we should make this a QEMU driver config option (ie be
 in /etc/libvirt/qemu.conf) and have 2 flags 
 
   security_driver=selinux|none
   security_autolabel=yes|no
 
 If security_autolabel is set to 'no', then the app must pass an explicit
 security context in the domain XML, otherwise the domain is unconfined.
 
 If security_autolabel is set to 'yes', then if the app passes an explicit
 security context this is used, otherwise it will auto-generate one at
 startup of the VM.
 

Would we just use capabilities to communicate this choice? If so, would
it be in the host section, or driver specific?

Thanks,
Cole

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-03-01 Thread James Morris 
On Fri, 27 Feb 2009, Daniel J Walsh wrote:

 I think we need a mechanism in libvirtd.conf to turn this off.   And
 allow perhaps three modes.
 
 svirt=Disabled.  No Security Driver.
 svirt=MLS (Requires context in xml, no relabel of disks)
 svirt=Standard, (If no XML label, then random generate one and reset
 file context).

I wouldn't call these MLS and Standard.  The simple isolation scheme with 
automatic labeling is just one way to do things.  Down the track, we'll 
want to be able to specify arbitrary types for guests, not just for MLS.


-- 
James Morris
jmor...@namei.org

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-28 Thread Daniel P. Berrange 
On Fri, Feb 27, 2009 at 03:37:55PM -0500, Daniel J Walsh wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 Another patch off latest repository.
 
 This patch does not require the XML to include a label, although this is
 still supported.
 
 Implemented most of the comments from Jim.  make check and make
 syntax-check passes, Added seclabeltest.c to run in tests, Updated
 capability.rng, although not really sure I did it right.
 
 This patch will generate random MCS Labels and relabels the image files
 to match.  Seems to work well on F11.
 
 I will back port some policy to allow it to work on F10.
 
 I think we need a mechanism in libvirtd.conf to turn this off.   And
 allow perhaps three modes.
 
 svirt=Disabled.  No Security Driver.
 svirt=MLS (Requires context in xml, no relabel of disks)
 svirt=Standard, (If no XML label, then random generate one and reset
 file context).
 
 How should I read config from libvirt.conf and and not enable he
 SecurityModel?

libvirtd.conf is for the general daemon configuration.

The QEMU driver has a separate /etc/libvirt/qemu.conf which is
read by qemudLoadDriverConfig() into struct qemud_driver. The
code for this is in src/qemu_conf.c 

The security driver is really more of a generic resource though,
that we could use in several drivers, so another alternative
is an /etc/libvirt/svirt.conf loaded the svirt driver. In any
case look at the qemudLoadDriverConfig() for example of the API
usage for config files

Regards,
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 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-27 Thread Daniel J Walsh 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Another patch off latest repository.

This patch does not require the XML to include a label, although this is
still supported.

Implemented most of the comments from Jim.  make check and make
syntax-check passes, Added seclabeltest.c to run in tests, Updated
capability.rng, although not really sure I did it right.

This patch will generate random MCS Labels and relabels the image files
to match.  Seems to work well on F11.

I will back port some policy to allow it to work on F10.

I think we need a mechanism in libvirtd.conf to turn this off.   And
allow perhaps three modes.

svirt=Disabled.  No Security Driver.
svirt=MLS (Requires context in xml, no relabel of disks)
svirt=Standard, (If no XML label, then random generate one and reset
file context).

How should I read config from libvirt.conf and and not enable he
SecurityModel?



http://people.fedoraproject.org/~dwalsh/SELinux/svirt.patch



-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkmoTyMACgkQrlYvE4MpobPuHwCgkJqZenEwCWov96tTv+h3x8ec
wmEAoMecJotrN009adtO3JOmkNLR3uXN
=waHN
-END PGP SIGNATURE-

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-26 Thread Daniel P. Berrange 
On Fri, Feb 20, 2009 at 02:38:05PM +, Daniel P. Berrange wrote:

 I think we need to move place where we set the exec context to after
 the fork() call, ideally to be the very last call made before the
 actual execve().
 
 We do not currently have an easy way todo this, but I have the exact
 same problem in my patches to integrate with cgroups - I need to add
 the new PID to the appropriate cgroup immediately before exec'ing.
 So i suggest the following patch whichs a generic callback to the
 virExec() call, so we can implant the neccessary logic after the fork()
 and just before the real execve(), and safely in the child process.
 
 To use this, we'd make qemudStartVM() pass in a virExecHook callback
 which does the call to  qemudDomainSetSecurityLabel()


I've committed this patch to CVS now.


 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -291,6 +291,7 @@ virEnumToString;
  virEventAddHandle;
  virEventRemoveHandle;
  virExec;
 +virExecWithHook;
  virSetNonBlock;
  virFormatMacAddr;
  virGetHostname;
 diff --git a/src/util.c b/src/util.c
 --- a/src/util.c
 +++ b/src/util.c
 @@ -199,7 +199,10 @@ __virExec(virConnectPtr conn,
const fd_set *keepfd,
pid_t *retpid,
int infd, int *outfd, int *errfd,
 -  int flags) {
 +  int flags,
 +  virExecHook hook,
 +  void *data)
 +{
  pid_t pid;
  int null, i, openmax;
  int pipeout[2] = {-1,-1};
 @@ -411,6 +414,9 @@ __virExec(virConnectPtr conn,
  childerr != childout)
  close(childerr);
  
 +if (hook)
 +(hook)(data);
 +
  if (envp)
  execve(argv[0], (char **) argv, (char**)envp);
  else
 @@ -445,13 +451,16 @@ __virExec(virConnectPtr conn,
  }
  
  int
 -virExec(virConnectPtr conn,
 -const char *const*argv,
 -const char *const*envp,
 -const fd_set *keepfd,
 -pid_t *retpid,
 -int infd, int *outfd, int *errfd,
 -int flags) {
 +virExecWithHook(virConnectPtr conn,
 +const char *const*argv,
 +const char *const*envp,
 +const fd_set *keepfd,
 +pid_t *retpid,
 +int infd, int *outfd, int *errfd,
 +int flags,
 +virExecHook hook,
 +void *data)
 +{
  char *argv_str;
  
  if ((argv_str = virArgvToString(argv)) == NULL) {
 @@ -462,7 +471,21 @@ virExec(virConnectPtr conn,
  VIR_FREE(argv_str);
  
  return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd,
 - flags);
 + flags, hook, data);
 +}
 +
 +int
 +virExec(virConnectPtr conn,
 +const char *const*argv,
 +const char *const*envp,
 +const fd_set *keepfd,
 +pid_t *retpid,
 +int infd, int *outfd, int *errfd,
 +int flags)
 +{
 +return virExecWithHook(conn, argv, envp, keepfd, retpid,
 +   infd, outfd, errfd,
 +   flags, NULL, NULL);
  }
  
  static int
 @@ -580,7 +603,7 @@ virRun(virConnectPtr conn,
  
  if ((execret = __virExec(conn, argv, NULL, NULL,
   childpid, -1, outfd, errfd,
 - VIR_EXEC_NONE))  0) {
 + VIR_EXEC_NONE, NULL, NULL))  0) {
  ret = execret;
  goto error;
  }
 diff --git a/src/util.h b/src/util.h
 --- a/src/util.h
 +++ b/src/util.h
 @@ -40,6 +40,21 @@ enum {
  
  int virSetNonBlock(int fd);
  
 +/* This will execute in the context of the first child
 + * immediately after fork() */
 +typedef int (*virExecHook)(void *data);
 +
 +int virExecWithHook(virConnectPtr conn,
 +const char *const*argv,
 +const char *const*envp,
 +const fd_set *keepfd,
 +int *retpid,
 +int infd,
 +int *outfd,
 +int *errfd,
 +int flags,
 +virExecHook hook,
 +void *data);
  int virExec(virConnectPtr conn,
  const char *const*argv,
  const char *const*envp,
 
 


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

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-23 Thread Daniel J Walsh 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Jim Meyering wrote:
 Daniel P. Berrange wrote:
 Just spotted one serious problem we need to address. The
 method 'qemudStartVMDaemon' quoted here is where we set the
 security label:
 ...
 
 Good catch.
 
 To use this, we'd make qemudStartVM() pass in a virExecHook callback
 which does the call to  qemudDomainSetSecurityLabel()

 Daniel

 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -291,6 +291,7 @@ virEnumToString;
  virEventAddHandle;
  virEventRemoveHandle;
  virExec;
 +virExecWithHook;
 
 Looks right to me.
 ACK.
This patch looks good to me and I have already rebased the svirt patch
to match, waiting for this to get applied, then I will resubmit my patch.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkmiqhYACgkQrlYvE4MpobMkFACeMWEmQfwKMe4Cn7NNikPk9f3+
zlMAoK7DeqbpTKho4Kw/bZtuA86vtMJK
=eVSb
-END PGP SIGNATURE-

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-21 Thread Jim Meyering 
Daniel P. Berrange wrote:
 Just spotted one serious problem we need to address. The
 method 'qemudStartVMDaemon' quoted here is where we set the
 security label:
...

Good catch.

 To use this, we'd make qemudStartVM() pass in a virExecHook callback
 which does the call to  qemudDomainSetSecurityLabel()

 Daniel

 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -291,6 +291,7 @@ virEnumToString;
  virEventAddHandle;
  virEventRemoveHandle;
  virExec;
 +virExecWithHook;

Looks right to me.
ACK.

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-20 Thread Daniel P. Berrange 
Just spotted one serious problem we need to address. The
method 'qemudStartVMDaemon' quoted here is where we set the
security label:

On Tue, Feb 17, 2009 at 11:20:17AM -0500, Daniel J Walsh wrote:
 @@ -1178,6 +1237,16 @@ static int qemudStartVMDaemon(virConnect
  return -1;
  }
  
 +/*
 + * Set up the security label for the domain here, before doing
 + * too much else.
 + */
 +if (qemudDomainSetSecurityLabel(conn, driver, vm)  0) {
 +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
 + _(Failed to set security label));
 +return -1;
 +}
 +
  if (qemudExtractVersionInfo(emulator,
  NULL,
  qemuCmdFlags)  0) {



Which ultimately calls the following method, which sets the context
to use for the next exec call.

 +static int
 +SELinuxSecurityDomainSetSecurityLabel(virConnectPtr conn,
 +  virSecurityDriverPtr drv,
 +  const virSecurityLabelDefPtr secdef)
 +{
 +/* TODO: verify DOI */
 +
 +if (!STREQ(drv-name, secdef-model)) {
 +virSecurityReportError(conn, VIR_ERR_ERROR,
 +   _(%s: security label driver mismatch: 
 + \'%s\' model configured for domain, but 
 + hypervisor driver is \'%s\'.),
 + __func__, secdef-model, drv-name);
 +return -1;
 +}
 +
 +if (setexeccon(secdef-label) == -1) {
 +virSecurityReportError(conn, VIR_ERR_ERROR,
 +   _(%s: unable to set security context 
 +   '\%s\': %s.), __func__, secdef-label,
 +   strerror(errno));
 +return -1;
 +}
 +return 0;
 +}

The problem is that between the point where we set the exec context,
and the place where QEMU is finally exec'd, there is scope for several
other programs to be exec'd.  Also there are other threads running
concurrently, and I'm under whether 'setexeccon' scope is per thread
or per process.

I think we need to move place where we set the exec context to after
the fork() call, ideally to be the very last call made before the
actual execve().

We do not currently have an easy way todo this, but I have the exact
same problem in my patches to integrate with cgroups - I need to add
the new PID to the appropriate cgroup immediately before exec'ing.
So i suggest the following patch whichs a generic callback to the
virExec() call, so we can implant the neccessary logic after the fork()
and just before the real execve(), and safely in the child process.

To use this, we'd make qemudStartVM() pass in a virExecHook callback
which does the call to  qemudDomainSetSecurityLabel()

Daniel

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -291,6 +291,7 @@ virEnumToString;
 virEventAddHandle;
 virEventRemoveHandle;
 virExec;
+virExecWithHook;
 virSetNonBlock;
 virFormatMacAddr;
 virGetHostname;
diff --git a/src/util.c b/src/util.c
--- a/src/util.c
+++ b/src/util.c
@@ -199,7 +199,10 @@ __virExec(virConnectPtr conn,
   const fd_set *keepfd,
   pid_t *retpid,
   int infd, int *outfd, int *errfd,
-  int flags) {
+  int flags,
+  virExecHook hook,
+  void *data)
+{
 pid_t pid;
 int null, i, openmax;
 int pipeout[2] = {-1,-1};
@@ -411,6 +414,9 @@ __virExec(virConnectPtr conn,
 childerr != childout)
 close(childerr);
 
+if (hook)
+(hook)(data);
+
 if (envp)
 execve(argv[0], (char **) argv, (char**)envp);
 else
@@ -445,13 +451,16 @@ __virExec(virConnectPtr conn,
 }
 
 int
-virExec(virConnectPtr conn,
-const char *const*argv,
-const char *const*envp,
-const fd_set *keepfd,
-pid_t *retpid,
-int infd, int *outfd, int *errfd,
-int flags) {
+virExecWithHook(virConnectPtr conn,
+const char *const*argv,
+const char *const*envp,
+const fd_set *keepfd,
+pid_t *retpid,
+int infd, int *outfd, int *errfd,
+int flags,
+virExecHook hook,
+void *data)
+{
 char *argv_str;
 
 if ((argv_str = virArgvToString(argv)) == NULL) {
@@ -462,7 +471,21 @@ virExec(virConnectPtr conn,
 VIR_FREE(argv_str);
 
 return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd,
- flags);
+ flags, hook, data);
+}
+
+int
+virExec(virConnectPtr conn,
+const char *const*argv,
+const char *const*envp,
+const fd_set *keepfd,
+pid_t *retpid,
+int infd, int *outfd, int *errfd,
+int flags)
+{
+return virExecWithHook(conn, argv, envp, keepfd, retpid,
+

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-20 Thread Daniel J Walsh 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Daniel P. Berrange wrote:
 Just spotted one serious problem we need to address. The
 method 'qemudStartVMDaemon' quoted here is where we set the
 security label:
 
 On Tue, Feb 17, 2009 at 11:20:17AM -0500, Daniel J Walsh wrote:
 @@ -1178,6 +1237,16 @@ static int qemudStartVMDaemon(virConnect
  return -1;
  }
  
 +/*
 + * Set up the security label for the domain here, before doing
 + * too much else.
 + */
 +if (qemudDomainSetSecurityLabel(conn, driver, vm)  0) {
 +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
 + _(Failed to set security label));
 +return -1;
 +}
 +
  if (qemudExtractVersionInfo(emulator,
  NULL,
  qemuCmdFlags)  0) {
 
 
 
 Which ultimately calls the following method, which sets the context
 to use for the next exec call.
 
 +static int
 +SELinuxSecurityDomainSetSecurityLabel(virConnectPtr conn,
 +  virSecurityDriverPtr drv,
 +  const virSecurityLabelDefPtr secdef)
 +{
 +/* TODO: verify DOI */
 +
 +if (!STREQ(drv-name, secdef-model)) {
 +virSecurityReportError(conn, VIR_ERR_ERROR,
 +   _(%s: security label driver mismatch: 
 + \'%s\' model configured for domain, but 
 + hypervisor driver is \'%s\'.),
 + __func__, secdef-model, drv-name);
 +return -1;
 +}
 +
 +if (setexeccon(secdef-label) == -1) {
 +virSecurityReportError(conn, VIR_ERR_ERROR,
 +   _(%s: unable to set security context 
 +   '\%s\': %s.), __func__, secdef-label,
 +   strerror(errno));
 +return -1;
 +}
 +return 0;
 +}
 
 The problem is that between the point where we set the exec context,
 and the place where QEMU is finally exec'd, there is scope for several
 other programs to be exec'd.  Also there are other threads running
 concurrently, and I'm under whether 'setexeccon' scope is per thread
 or per process.
 
 I think we need to move place where we set the exec context to after
 the fork() call, ideally to be the very last call made before the
 actual execve().
 
 We do not currently have an easy way todo this, but I have the exact
 same problem in my patches to integrate with cgroups - I need to add
 the new PID to the appropriate cgroup immediately before exec'ing.
 So i suggest the following patch whichs a generic callback to the
 virExec() call, so we can implant the neccessary logic after the fork()
 and just before the real execve(), and safely in the child process.
 
 To use this, we'd make qemudStartVM() pass in a virExecHook callback
 which does the call to  qemudDomainSetSecurityLabel()
 
 Daniel
 
 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -291,6 +291,7 @@ virEnumToString;
  virEventAddHandle;
  virEventRemoveHandle;
  virExec;
 +virExecWithHook;
  virSetNonBlock;
  virFormatMacAddr;
  virGetHostname;
 diff --git a/src/util.c b/src/util.c
 --- a/src/util.c
 +++ b/src/util.c
 @@ -199,7 +199,10 @@ __virExec(virConnectPtr conn,
const fd_set *keepfd,
pid_t *retpid,
int infd, int *outfd, int *errfd,
 -  int flags) {
 +  int flags,
 +  virExecHook hook,
 +  void *data)
 +{
  pid_t pid;
  int null, i, openmax;
  int pipeout[2] = {-1,-1};
 @@ -411,6 +414,9 @@ __virExec(virConnectPtr conn,
  childerr != childout)
  close(childerr);
  
 +if (hook)
 +(hook)(data);
 +
  if (envp)
  execve(argv[0], (char **) argv, (char**)envp);
  else
 @@ -445,13 +451,16 @@ __virExec(virConnectPtr conn,
  }
  
  int
 -virExec(virConnectPtr conn,
 -const char *const*argv,
 -const char *const*envp,
 -const fd_set *keepfd,
 -pid_t *retpid,
 -int infd, int *outfd, int *errfd,
 -int flags) {
 +virExecWithHook(virConnectPtr conn,
 +const char *const*argv,
 +const char *const*envp,
 +const fd_set *keepfd,
 +pid_t *retpid,
 +int infd, int *outfd, int *errfd,
 +int flags,
 +virExecHook hook,
 +void *data)
 +{
  char *argv_str;
  
  if ((argv_str = virArgvToString(argv)) == NULL) {
 @@ -462,7 +471,21 @@ virExec(virConnectPtr conn,
  VIR_FREE(argv_str);
  
  return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd,
 - flags);
 + flags, hook, data);
 +}
 +
 +int
 +virExec(virConnectPtr conn,
 +const char *const*argv,
 +const char *const*envp,
 +

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-17 Thread Jim Meyering 
Daniel J Walsh dwa...@redhat.com wrote:


[I removed the 1900+ lines of useless context]

 Ok, I have added your patches and make syntax-check succeeds except it
 does not like

 po_check
 --- po/POTFILES.in
 +++ po/POTFILES.in
 @@ -22,8 +22,6 @@
  src/qemu_conf.c
  src/qemu_driver.c
  src/remote_internal.c
 -src/security.c
 -src/security_selinux.c
  src/storage_backend.c
  src/storage_backend_disk.c
  src/storage_backend_fs.c
 Makefile.maint: you have changed the set of files with translatable
 diagnostics;
  apply the above patch

 Since these files add translations, what do I need to do to get this to
 pass?

That check passes for me, but I'm using git.
Are you using CVS?

If so, have you cvs-added those two files?
If you have not, that would explain it.

Otherwise, the check (which runs a script to search
all version-controlled files) may need to be adjusted.

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Updated James Morris patch to apply to libvirt-0.6.0 version

2009-02-17 Thread Daniel J Walsh 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Jim Meyering wrote:
 Daniel J Walsh dwa...@redhat.com wrote:
 
 [I removed the 1900+ lines of useless context]
 
 Ok, I have added your patches and make syntax-check succeeds except it
 does not like

 po_check
 --- po/POTFILES.in
 +++ po/POTFILES.in
 @@ -22,8 +22,6 @@
  src/qemu_conf.c
  src/qemu_driver.c
  src/remote_internal.c
 -src/security.c
 -src/security_selinux.c
  src/storage_backend.c
  src/storage_backend_disk.c
  src/storage_backend_fs.c
 Makefile.maint: you have changed the set of files with translatable
 diagnostics;
  apply the above patch

 Since these files add translations, what do I need to do to get this to
 pass?
 
 That check passes for me, but I'm using git.
 Are you using CVS?
 
 If so, have you cvs-added those two files?
 If you have not, that would explain it.
 
 Otherwise, the check (which runs a script to search
 all version-controlled files) may need to be adjusted.
Ok that is what I missed, I was not checking them in before running the
test.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkmbINsACgkQrlYvE4MpobP4FgCffugZzfPrm/it38q0wVLsJRiN
T6cAn1/Vdfmhoo2UF8hbaq2c9hH9/3qP
=1iaw
-END PGP SIGNATURE-

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list