4:57 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=912499
(especially comments 7 & 10)
This patch set is the final fix so that we can access disks in use by
other guests when SELinux and sVirt are enabled.
Previously such disks were inaccessible because sVirt labels the disks
with a random SELinux label to prevent other instances of qemu from
being able to read them. So naturally the libguestfs appliance
(ie. qemu) cannot read these disks, not even if it is running as root.
The fix is to read that SELinux label from libvirt, then label the
libguestfs appliance the same way, so it can access the disks.
This is done by modifying the guestfs_add_domain call so that it reads
the label from libvirt, and passes the label through to the libvirt
back end which uses it when creating the appliance. It's actually a
bit more complex than this because we have to label the overlay disks
and the appliance disk correctly.
These patches are only the final part of the whole fix. I have
already pushed fixes to virt-df and virt-alignment-scan (34e77af1bf +
6e3aab2f0c + several dependent commits) so that these tools now use
just one appliance per guest, allowing us to label that appliance
correctly, because if an appliance is attached to multiple guests then
there is no single label that could be used.
As a result of the total complexity of this fix, I very much doubt
that it can be backported to 1.20.
Rich.
4:57 a.m.
New subject: [PATCH 1/7] launch: libvirt: Refactor SELinux warning code.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
This is just code motion.
---
src/launch-libvirt.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 6ad19de..7db2ce5 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -138,6 +138,7 @@ static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive
*drv);
static void drive_free_priv (void *);
static void set_socket_create_context (guestfs_h *g);
static void clear_socket_create_context (guestfs_h *g);
+static void selinux_warning (guestfs_h *g, const char *func, const char *selinux_op,
const char *data);
static int
launch_libvirt (guestfs_h *g, const char *libvirt_uri)
@@ -588,30 +589,27 @@ set_socket_create_context (guestfs_h *g)
context_t con;
if (getcon (&scon) == -1) {
- debug (g, "%s: getcon failed: %m", __func__);
+ selinux_warning (g, __func__, "getcon", NULL);
return;
}
con = context_new (scon);
if (!con) {
- debug (g, "%s: context_new failed: %m", __func__);
+ selinux_warning (g, __func__, "context_new", scon);
goto out1;
}
if (context_type_set (con, SOCKET_CONTEXT) == -1) {
- debug (g, "%s: context_type_set failed: %m", __func__);
+ selinux_warning (g, __func__, "context_type_set", scon);
goto out2;
}
-#define SETSOCKCREATECON_WARNING_NOTICE "[you can ignore this UNLESS using SELinux +
sVirt]"
-
/* Note that setsockcreatecon sets the per-thread socket creation
* context (/proc/self/task/<tid>/attr/sockcreate) so this is
* thread-safe.
*/
if (setsockcreatecon (context_str (con)) == -1) {
- debug (g, "%s: setsockcreatecon (%s) failed: %m %s",
- __func__, context_str (con), SETSOCKCREATECON_WARNING_NOTICE);
+ selinux_warning (g, __func__, "setsockcreatecon", context_str (con));
goto out2;
}
@@ -625,8 +623,7 @@ static void
clear_socket_create_context (guestfs_h *g)
{
if (setsockcreatecon (NULL) == -1)
- debug (g, "%s: setsockcreatecon (NULL) failed: %m %s", __func__,
- SETSOCKCREATECON_WARNING_NOTICE);
+ selinux_warning (g, __func__, "setsockcreatecon", "NULL");
}
#else /* !HAVE_LIBSELINUX */
@@ -1454,6 +1451,15 @@ libvirt_error (guestfs_h *g, const char *fs, ...)
free (msg);
}
+static void
+selinux_warning (guestfs_h *g, const char *func,
+ const char *selinux_op, const char *data)
+{
+ debug (g, "%s: %s failed: %s: %m"
+ " [you can ignore this UNLESS using SELinux + sVirt]",
+ func, selinux_op, data ? data : "(none)");
+}
+
/* This backend assumes virtio-scsi is available. */
static int
max_disks_libvirt (guestfs_h *g)
--
1.8.1.2
7:23 a.m.
New subject: [PATCH 1/7] launch: libvirt: Refactor SELinux warning code.
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)redhat.com>
This is just code motion.
---
src/launch-libvirt.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
ACK. Might as well push this and get it out of the way.
Matt
4:57 a.m.
New subject: [PATCH 2/7] New internal API: internal_set_libvirt_selinux_label
From: "Richard W.M. Jones" <rjones(a)redhat.com>
This internal API sets two SELinux labels in the handle (the process
label and the image label -- they are closely related).
If using the libvirt attach-method with SELinux and sVirt, then this
will cause the following XML to be added to the appliance definition:
<seclabel type=static model=selinux relabel=yes>
<label>[LABEL HERE]</label>
<imagelabel>[IMAGELABEL HERE]</imagelabel>
</seclabel>
It is ignored by other attach-methods.
---
generator/actions.ml | 12 ++++++++++++
src/guestfs-internal.h | 2 ++
src/handle.c | 2 ++
src/launch-libvirt.c | 36 ++++++++++++++++++++++++++++++++++++
4 files changed, 52 insertions(+)
diff --git a/generator/actions.ml b/generator/actions.ml
index 8a8e3ff..59e667d 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -2694,6 +2694,18 @@ the default. Else C</var/tmp> is the default." };
longdesc = "\
Get the directory used by the handle to store the appliance cache." };
+ { defaults with
+ name = "internal_set_libvirt_selinux_label";
+ style = RErr, [String "label"; String "imagelabel"], [];
+ blocking = false;
+ visibility = VInternal;
+ shortdesc = "set SELinux label used by the libvirt attach method";
+ longdesc = "\
+This internal function sets the SELinux security label (in
+reality, two labels: the process label and the image label)
+used by the appliance when the libvirt attach method is selected
+(it is ignored by other attach methods)." };
+
]
(* daemon_functions are any functions which cause some action
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index e1a7d31..78e2bf5 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -322,6 +322,8 @@ struct guestfs_h
virDomainPtr dom; /* libvirt domain */
} virt;
#endif
+ char *virt_selinux_label;
+ char *virt_selinux_imagelabel;
};
/* Per-filesystem data stored for inspect_os. */
diff --git a/src/handle.c b/src/handle.c
index c630daf..2f44632 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -326,6 +326,8 @@ guestfs_close (guestfs_h *g)
if (g->pda)
hash_free (g->pda);
+ free (g->virt_selinux_label);
+ free (g->virt_selinux_imagelabel);
free (g->tmpdir);
free (g->env_tmpdir);
free (g->int_tmpdir);
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 7db2ce5..0a59cb6 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -855,6 +855,31 @@ construct_libvirt_xml_seclabel (guestfs_h *g,
BAD_CAST "none"));
XMLERROR (-1, xmlTextWriterEndElement (xo));
}
+ else if (g->virt_selinux_label) {
+ /* Enable sVirt and pass a custom <seclabel/> inherited from the
+ * original libvirt domain (when guestfs_add_domain was called).
+ * https://bugzilla.redhat.com/show_bug.cgi?id=912499#c7
+ */
+ XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "seclabel"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
+ BAD_CAST "static"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "model",
+ BAD_CAST "selinux"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "relabel",
+ BAD_CAST "yes"));
+ XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "label"));
+ XMLERROR (-1, xmlTextWriterWriteString (xo,
+ BAD_CAST g->virt_selinux_label));
+ XMLERROR (-1, xmlTextWriterEndElement (xo));
+ XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "imagelabel"));
+ XMLERROR (-1, xmlTextWriterWriteString (xo,
+ BAD_CAST g->virt_selinux_imagelabel));
+ XMLERROR (-1, xmlTextWriterEndElement (xo));
+ XMLERROR (-1, xmlTextWriterEndElement (xo));
+ }
return 0;
}
@@ -1603,3 +1628,14 @@ struct attach_ops attach_ops_libvirt = {
};
#endif /* no libvirt or libxml2 at compile time */
+
+int
+guestfs__internal_set_libvirt_selinux_label (guestfs_h *g, const char *label,
+ const char *imagelabel)
+{
+ free (g->virt_selinux_label);
+ g->virt_selinux_label = safe_strdup (g, label);
+ free (g->virt_selinux_imagelabel);
+ g->virt_selinux_imagelabel = safe_strdup (g, imagelabel);
+ return 0;
+}
--
1.8.1.2
7:46 a.m.
New subject: [PATCH 2/7] New internal API: internal_set_libvirt_selinux_label
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)redhat.com>
This internal API sets two SELinux labels in the handle (the process
label and the image label -- they are closely related).
If using the libvirt attach-method with SELinux and sVirt, then this
will cause the following XML to be added to the appliance definition:
<seclabel type=static model=selinux relabel=yes>
<label>[LABEL HERE]</label>
<imagelabel>[IMAGELABEL HERE]</imagelabel>
</seclabel>
We're hard-coding type=static, model=selinux, relabel=yes here. I have
no idea what the implications of this are. Are we sure this is ok? I
guess Dan would be the person to ask.
It is ignored by other attach-methods.
---
generator/actions.ml | 12 ++++++++++++
src/guestfs-internal.h | 2 ++
src/handle.c | 2 ++
src/launch-libvirt.c | 36 ++++++++++++++++++++++++++++++++++++
4 files changed, 52 insertions(+)
diff --git a/generator/actions.ml b/generator/actions.ml
index 8a8e3ff..59e667d 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -2694,6 +2694,18 @@ the default. Else C</var/tmp> is the default." };
longdesc = "\
Get the directory used by the handle to store the appliance cache." };
+ { defaults with
+ name = "internal_set_libvirt_selinux_label";
+ style = RErr, [String "label"; String "imagelabel"], [];
+ blocking = false;
+ visibility = VInternal;
+ shortdesc = "set SELinux label used by the libvirt attach method";
+ longdesc = "\
+This internal function sets the SELinux security label (in
+reality, two labels: the process label and the image label)
+used by the appliance when the libvirt attach method is selected
+(it is ignored by other attach methods)." };
+
]
(* daemon_functions are any functions which cause some action
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index e1a7d31..78e2bf5 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -322,6 +322,8 @@ struct guestfs_h
virDomainPtr dom; /* libvirt domain */
} virt;
#endif
+ char *virt_selinux_label;
+ char *virt_selinux_imagelabel;
};
/* Per-filesystem data stored for inspect_os. */
diff --git a/src/handle.c b/src/handle.c
index c630daf..2f44632 100644
--- a/src/handle.c
+++ b/src/handle.c
@@ -326,6 +326,8 @@ guestfs_close (guestfs_h *g)
if (g->pda)
hash_free (g->pda);
+ free (g->virt_selinux_label);
+ free (g->virt_selinux_imagelabel);
free (g->tmpdir);
free (g->env_tmpdir);
free (g->int_tmpdir);
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 7db2ce5..0a59cb6 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -855,6 +855,31 @@ construct_libvirt_xml_seclabel (guestfs_h *g,
BAD_CAST "none"));
XMLERROR (-1, xmlTextWriterEndElement (xo));
}
+ else if (g->virt_selinux_label) {
+ /* Enable sVirt and pass a custom <seclabel/> inherited from the
+ * original libvirt domain (when guestfs_add_domain was called).
+ * https://bugzilla.redhat.com/show_bug.cgi?id=912499#c7
+ */
+ XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "seclabel"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
+ BAD_CAST "static"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "model",
+ BAD_CAST "selinux"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "relabel",
+ BAD_CAST "yes"));
+ XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "label"));
+ XMLERROR (-1, xmlTextWriterWriteString (xo,
+ BAD_CAST g->virt_selinux_label));
+ XMLERROR (-1, xmlTextWriterEndElement (xo));
+ XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "imagelabel"));
+ XMLERROR (-1, xmlTextWriterWriteString (xo,
+ BAD_CAST g->virt_selinux_imagelabel));
+ XMLERROR (-1, xmlTextWriterEndElement (xo));
+ XMLERROR (-1, xmlTextWriterEndElement (xo));
+ }
return 0;
}
@@ -1603,3 +1628,14 @@ struct attach_ops attach_ops_libvirt = {
};
#endif /* no libvirt or libxml2 at compile time */
+
+int
+guestfs__internal_set_libvirt_selinux_label (guestfs_h *g, const char *label,
+ const char *imagelabel)
+{
+ free (g->virt_selinux_label);
+ g->virt_selinux_label = safe_strdup (g, label);
+ free (g->virt_selinux_imagelabel);
+ g->virt_selinux_imagelabel = safe_strdup (g, imagelabel);
+ return 0;
+}
Assuming the hard-coding is acceptable, ACK.
Matt
7:58 a.m.
New subject: [PATCH 2/7] New internal API: internal_set_libvirt_selinux_label
On Thu, Feb 28, 2013 at 01:46:24PM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones(a)redhat.com>
>
> This internal API sets two SELinux labels in the handle (the process
> label and the image label -- they are closely related).
>
> If using the libvirt attach-method with SELinux and sVirt, then this
> will cause the following XML to be added to the appliance definition:
>
> <seclabel type=static model=selinux relabel=yes>
> <label>[LABEL HERE]</label>
> <imagelabel>[IMAGELABEL HERE]</imagelabel>
> </seclabel>
We're hard-coding type=static, model=selinux, relabel=yes here. I have
no idea what the implications of this are. Are we sure this is ok? I
guess Dan would be the person to ask.
That's definitely one reason why this is an internal API, not a
published one :-)
At the moment my plan is to do what works (which is this), not what is
elegant or even long-term supportable. I hope in the long term we
could have some XML we could pass to libvirt to say "I want to peek
into the disks of this domain, just do it".
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
4:57 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
If set, this causes <seclabel model=selinux relabel=no> to be added to
the disk element in the libvirt XML.
It has no effect *except* on the libvirt attach method when SELinux
and sVirt is being used.
---
generator/actions.ml | 8 +++++++-
src/guestfs-internal.h | 1 +
src/launch-libvirt.c | 20 ++++++++++++++++++++
src/launch.c | 21 ++++++++++++++-------
4 files changed, 42 insertions(+), 8 deletions(-)
diff --git a/generator/actions.ml b/generator/actions.ml
index 59e667d..ececa7b 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -1247,7 +1247,7 @@ not all belong to a single logical operating system
{ defaults with
name = "add_drive";
- style = RErr, [String "filename"], [OBool "readonly"; OString
"format"; OString "iface"; OString "name"; OString
"label"];
+ style = RErr, [String "filename"], [OBool "readonly"; OString
"format"; OString "iface"; OString "name"; OString
"label"; OBool "selinuxnorelabel"];
once_had_no_optargs = true;
blocking = false;
fish_alias = ["add"];
@@ -1319,6 +1319,12 @@ the drive will also be named
C</dev/disk/guestfs/I<label>>.
See L<guestfs(3)/DISK LABELS>.
+=item C<selinuxnorelabel>
+
+If set, this option (only applicable when using the libvirt
+attach-method with SELinux and sVirt) causes libvirt to I<not>
+relabel the disk.
+
=back" };
{ defaults with
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 78e2bf5..9a468dc 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -119,6 +119,7 @@ struct drive {
char *name;
char *disk_label;
bool use_cache_none;
+ int selinuxnorelabel;
void *priv; /* Data used by attach method. */
void (*free_priv) (void *);
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 0a59cb6..45950e2 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -1035,6 +1035,16 @@ construct_libvirt_xml_disk (guestfs_h *g,
XMLERROR (-1,
xmlTextWriterWriteAttribute (xo, BAD_CAST "file",
BAD_CAST drv_priv->path));
+ if (drv->selinuxnorelabel) {
+ XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "seclabel"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "model",
+ BAD_CAST "selinux"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "relabel",
+ BAD_CAST "no"));
+ XMLERROR (-1, xmlTextWriterEndElement (xo));
+ }
XMLERROR (-1, xmlTextWriterEndElement (xo));
}
else {
@@ -1046,6 +1056,16 @@ construct_libvirt_xml_disk (guestfs_h *g,
XMLERROR (-1,
xmlTextWriterWriteAttribute (xo, BAD_CAST "dev",
BAD_CAST drv_priv->path));
+ if (drv->selinuxnorelabel) {
+ XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "seclabel"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "model",
+ BAD_CAST "selinux"));
+ XMLERROR (-1,
+ xmlTextWriterWriteAttribute (xo, BAD_CAST "relabel",
+ BAD_CAST "no"));
+ XMLERROR (-1, xmlTextWriterEndElement (xo));
+ }
XMLERROR (-1, xmlTextWriterEndElement (xo));
}
diff --git a/src/launch.c b/src/launch.c
index 7c37667..518ecd7 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -88,7 +88,7 @@ static struct drive *
create_drive_struct (guestfs_h *g, const char *path,
int readonly, const char *format,
const char *iface, const char *name,
- const char *disk_label,
+ const char *disk_label, int selinuxnorelabel,
int use_cache_none)
{
struct drive *drv = safe_malloc (g, sizeof (struct drive));
@@ -100,6 +100,7 @@ create_drive_struct (guestfs_h *g, const char *path,
drv->name = name ? safe_strdup (g, name) : NULL;
drv->disk_label = disk_label ? safe_strdup (g, disk_label) : NULL;
drv->use_cache_none = use_cache_none;
+ drv->selinuxnorelabel = selinuxnorelabel;
drv->priv = drv->free_priv = NULL;
return drv;
@@ -111,7 +112,7 @@ guestfs___add_dummy_appliance_drive (guestfs_h *g)
{
struct drive *drv;
- drv = create_drive_struct (g, "", 0, NULL, NULL, NULL, NULL, 0);
+ drv = create_drive_struct (g, "", 0, NULL, NULL, NULL, NULL, 0, 0);
add_drive_to_handle (g, drv);
}
@@ -276,7 +277,8 @@ add_drive (guestfs_h *g, struct drive *drv)
*/
static int
add_null_drive (guestfs_h *g, int readonly, const char *format,
- const char *iface, const char *name, const char *disk_label)
+ const char *iface, const char *name, const char *disk_label,
+ int selinuxnorelabel)
{
char *tmpfile = NULL;
int fd = -1, r;
@@ -310,7 +312,8 @@ add_null_drive (guestfs_h *g, int readonly, const char *format,
goto err;
}
- drv = create_drive_struct (g, tmpfile, readonly, format, iface, name, disk_label, 0);
+ drv = create_drive_struct (g, tmpfile, readonly, format, iface, name,
+ disk_label, selinuxnorelabel, 0);
r = add_drive (g, drv);
free (tmpfile);
if (r == -1) {
@@ -337,6 +340,7 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
const char *name;
const char *disk_label;
int use_cache_none;
+ int selinuxnorelabel;
struct drive *drv;
if (strchr (filename, ':') != NULL) {
@@ -355,6 +359,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
? optargs->name : NULL;
disk_label = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_LABEL_BITMASK
? optargs->label : NULL;
+ selinuxnorelabel = optargs->bitmask &
GUESTFS_ADD_DRIVE_OPTS_SELINUXNORELABEL_BITMASK
+ ? optargs->selinuxnorelabel : 0;
if (format && !valid_format_iface (format)) {
error (g, _("%s parameter is empty or contains disallowed characters"),
@@ -372,7 +378,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
}
if (STREQ (filename, "/dev/null"))
- return add_null_drive (g, readonly, format, iface, name, disk_label);
+ return add_null_drive (g, readonly, format, iface, name, disk_label,
+ selinuxnorelabel);
/* For writable files, see if we can use cache=none. This also
* checks for the existence of the file. For readonly we have
@@ -389,8 +396,8 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
}
}
- drv = create_drive_struct (g, filename, readonly, format, iface, name, disk_label,
- use_cache_none);
+ drv = create_drive_struct (g, filename, readonly, format, iface, name,
+ disk_label, selinuxnorelabel, use_cache_none);
if (add_drive (g, drv) == -1) {
free_drive_struct (drv);
return -1;
--
1.8.1.2
8 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)redhat.com>
If set, this causes <seclabel model=selinux relabel=no> to be added to
the disk element in the libvirt XML.
It has no effect *except* on the libvirt attach method when SELinux
and sVirt is being used.
---
generator/actions.ml | 8 +++++++-
src/guestfs-internal.h | 1 +
src/launch-libvirt.c | 20 ++++++++++++++++++++
src/launch.c | 21 ++++++++++++++-------
4 files changed, 42 insertions(+), 8 deletions(-)
Is there any instance where we would *not* want to do this? Off the top
of my head, I can't think of a good reason for libguestfs to be in the
business of relabelling drives. Can we not just hard-code this off?
If, on the other hand, there is a good reason to make this option, I
think it should be explained in the docs below.
Matt
8:20 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, Feb 28, 2013 at 02:00:50PM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones(a)redhat.com>
>
> If set, this causes <seclabel model=selinux relabel=no> to be added to
> the disk element in the libvirt XML.
>
> It has no effect *except* on the libvirt attach method when SELinux
> and sVirt is being used.
> ---
> generator/actions.ml | 8 +++++++-
> src/guestfs-internal.h | 1 +
> src/launch-libvirt.c | 20 ++++++++++++++++++++
> src/launch.c | 21 ++++++++++++++-------
> 4 files changed, 42 insertions(+), 8 deletions(-)
Is there any instance where we would *not* want to do this? Off the top
of my head, I can't think of a good reason for libguestfs to be in the
business of relabelling drives. Can we not just hard-code this off?
If, on the other hand, there is a good reason to make this option, I
think it should be explained in the docs below.
It's a tricky situation, but if libguestfs's appliance is confined by
sVirt, and if we're accessing an ordinary, non-shared disk, then you
have to have libvirt label the disk otherwise qemu won't be able to open it.
The situation here is different: we want to confine libguestfs's
appliance with sVirt, and access a shared disk which already has some
other label, so we label the appliance to conform to the disk. The
reason we don't want libvirt to label the disk is really a bug in
libvirt: when the libvirt connection is closed, it resets the label
*not* to what it was before, but to some other label. We want to tell
libvirt to leave it alone and not do this. So you can argue that the
need for this flag is to work around a bug in libvirt.
Another way to do this would be not to use svirt_t at all, but to have
some other process label (sguestfs_t) which has fewer restrictions
than svirt_t. The trick would be to make the confinement meaningful,
since the whole point of using sVirt with libguestfs is to confine the
appliance against the threat of rogue disk images. Also I guess this
would need deep changes to libvirt.
[Adding Dan & Dan]
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
8:37 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, Feb 28, 2013 at 02:20:54PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 28, 2013 at 02:00:50PM +0000, Matthew Booth wrote:
> On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
> > From: "Richard W.M. Jones" <rjones(a)redhat.com>
> >
> > If set, this causes <seclabel model=selinux relabel=no> to be added to
> > the disk element in the libvirt XML.
> >
> > It has no effect *except* on the libvirt attach method when SELinux
> > and sVirt is being used.
> > ---
> > generator/actions.ml | 8 +++++++-
> > src/guestfs-internal.h | 1 +
> > src/launch-libvirt.c | 20 ++++++++++++++++++++
> > src/launch.c | 21 ++++++++++++++-------
> > 4 files changed, 42 insertions(+), 8 deletions(-)
>
> Is there any instance where we would *not* want to do this? Off the top
> of my head, I can't think of a good reason for libguestfs to be in the
> business of relabelling drives. Can we not just hard-code this off?
>
> If, on the other hand, there is a good reason to make this option, I
> think it should be explained in the docs below.
It's a tricky situation, but if libguestfs's appliance is confined by
sVirt, and if we're accessing an ordinary, non-shared disk, then you
have to have libvirt label the disk otherwise qemu won't be able to open it.
The situation here is different: we want to confine libguestfs's
appliance with sVirt, and access a shared disk which already has some
other label, so we label the appliance to conform to the disk. The
reason we don't want libvirt to label the disk is really a bug in
libvirt: when the libvirt connection is closed, it resets the label
*not* to what it was before, but to some other label. We want to tell
libvirt to leave it alone and not do this. So you can argue that the
need for this flag is to work around a bug in libvirt.
Another way to do this would be not to use svirt_t at all, but to have
some other process label (sguestfs_t) which has fewer restrictions
than svirt_t. The trick would be to make the confinement meaningful,
since the whole point of using sVirt with libguestfs is to confine the
appliance against the threat of rogue disk images. Also I guess this
would need deep changes to libvirt.
There are several distinct aspects to this - the question of labelling,
the process domain and the process MCS category.
Before this patch, everything the libguestfs VM did was under a random
MCS level, which obviously clashed with what the existing running VM
required.
IMHO we want the libguestfs appliances to be as confined as is possible.
So if the libguestfs handle is using disks for a particular VM, then
policy must only allow it to access the disks for that specific VM.
Thus, from a policy POV, the key iss is that we need the MCS level of
the libguestfs VM to match the MCS level of the running VM.
What we're doing here with using a static label which 100% matches
the label of the running VM achieves that, at a cost though. The
cost is that the libguestfs VM is being given full read-write access
to every resource associated with the running VM, when it really only
needs to have read-only access to disks. The libguesfs VM monitor
sockets ends up with the same label sas the main VM monitor socket
which is also sub-optimal from a security POV. libguestfs should only
needs read-write access to its own qcow2 overlay image and its own
monitor socket.
I don't believe we can lock this down without a new policy domain for
the libguestfs VM and a libvirt enhancement.
We would need
- A domain type 'svirt_guestfs_t'
- A disk type 'svirt_guesfs_image_t'
- A process with type svirt_guestfs_t:s0:c123,456 would have
*readonly* access to files svirt_image_t:s0:c123,456
- A process with type svirt_guestfs_t:s0:c123,456 would have
*readwrite* access to files svirt_guestfs_image_t:s0:c123,456
So in that case you'd use
<seclabel type="static" relabel="yes">
<label>svirt_guestfs_t:s0:c123,456</label>
<imagelabel>svirt_guestfs_image_t:s0:c123,456</imagelabel>
</seclabel>
You would *allow* libvirt to relabel the qcow2 overlay that libguestfs
needs write access to.
There would need to be an enhancement to libvirt XML to allow you to
tell it to *not* try to relabel the qcow2 backing file though. That
must remain on 'svirt_image_t:s0:c123,456'.
Regards,
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 :|
9:01 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, 2013-02-28 at 14:37 +0000, Daniel P. Berrange wrote:
On Thu, Feb 28, 2013 at 02:20:54PM +0000, Richard W.M. Jones wrote:
> On Thu, Feb 28, 2013 at 02:00:50PM +0000, Matthew Booth wrote:
> > On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
> > > From: "Richard W.M. Jones" <rjones(a)redhat.com>
> > >
> > > If set, this causes <seclabel model=selinux relabel=no> to be added
to
> > > the disk element in the libvirt XML.
> > >
> > > It has no effect *except* on the libvirt attach method when SELinux
> > > and sVirt is being used.
> > > ---
> > > generator/actions.ml | 8 +++++++-
> > > src/guestfs-internal.h | 1 +
> > > src/launch-libvirt.c | 20 ++++++++++++++++++++
> > > src/launch.c | 21 ++++++++++++++-------
> > > 4 files changed, 42 insertions(+), 8 deletions(-)
> >
> > Is there any instance where we would *not* want to do this? Off the top
> > of my head, I can't think of a good reason for libguestfs to be in the
> > business of relabelling drives. Can we not just hard-code this off?
> >
> > If, on the other hand, there is a good reason to make this option, I
> > think it should be explained in the docs below.
>
> It's a tricky situation, but if libguestfs's appliance is confined by
> sVirt, and if we're accessing an ordinary, non-shared disk, then you
> have to have libvirt label the disk otherwise qemu won't be able to open it.
>
> The situation here is different: we want to confine libguestfs's
> appliance with sVirt, and access a shared disk which already has some
> other label, so we label the appliance to conform to the disk. The
> reason we don't want libvirt to label the disk is really a bug in
> libvirt: when the libvirt connection is closed, it resets the label
> *not* to what it was before, but to some other label. We want to tell
> libvirt to leave it alone and not do this. So you can argue that the
> need for this flag is to work around a bug in libvirt.
>
> Another way to do this would be not to use svirt_t at all, but to have
> some other process label (sguestfs_t) which has fewer restrictions
> than svirt_t. The trick would be to make the confinement meaningful,
> since the whole point of using sVirt with libguestfs is to confine the
> appliance against the threat of rogue disk images. Also I guess this
> would need deep changes to libvirt.
There are several distinct aspects to this - the question of labelling,
the process domain and the process MCS category.
Before this patch, everything the libguestfs VM did was under a random
MCS level, which obviously clashed with what the existing running VM
required.
IMHO we want the libguestfs appliances to be as confined as is possible.
So if the libguestfs handle is using disks for a particular VM, then
policy must only allow it to access the disks for that specific VM.
Thus, from a policy POV, the key iss is that we need the MCS level of
the libguestfs VM to match the MCS level of the running VM.
What we're doing here with using a static label which 100% matches
the label of the running VM achieves that, at a cost though. The
cost is that the libguestfs VM is being given full read-write access
to every resource associated with the running VM, when it really only
needs to have read-only access to disks. The libguesfs VM monitor
sockets ends up with the same label sas the main VM monitor socket
which is also sub-optimal from a security POV. libguestfs should only
needs read-write access to its own qcow2 overlay image and its own
monitor socket.
I don't believe we can lock this down without a new policy domain for
the libguestfs VM and a libvirt enhancement.
We would need
- A domain type 'svirt_guestfs_t'
- A disk type 'svirt_guesfs_image_t'
- A process with type svirt_guestfs_t:s0:c123,456 would have
*readonly* access to files svirt_image_t:s0:c123,456
- A process with type svirt_guestfs_t:s0:c123,456 would have
*readwrite* access to files svirt_guestfs_image_t:s0:c123,456
So in that case you'd use
<seclabel type="static" relabel="yes">
<label>svirt_guestfs_t:s0:c123,456</label>
<imagelabel>svirt_guestfs_image_t:s0:c123,456</imagelabel>
</seclabel>
You would *allow* libvirt to relabel the qcow2 overlay that libguestfs
needs write access to.
There would need to be an enhancement to libvirt XML to allow you to
tell it to *not* try to relabel the qcow2 backing file though. That
must remain on 'svirt_image_t:s0:c123,456'.
A couple of observations:
Firstly, this patch is essentially exposing an internal implementation
detail to the user, one which may well change. I suspect we can infer
from available metadata whether or not we require this. If so, I think
it should be inferred rather than passed in by the user. I'm not
convinced this is something we want to be adding to the API.
Secondly, by relabelling disk images we're potentially making them
temporarily unavailable to other processes, which is something we
weren't doing before. It's possible there may be no way round this, but
if so we ought to highlight it as a major gotcha. Is there any way we
can give a file 2 different SELinux contexts? If there were, we could
leave the original untouched and only allow our ephemeral process to
access the second one. A hard link doesn't allow this, btw. A copy is
almost certainly infeasible in the general case, but if the FS allows
reflink it might fly. Thoughts?
Matt
9:39 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 14:37 +0000, Daniel P. Berrange wrote:
> On Thu, Feb 28, 2013 at 02:20:54PM +0000, Richard W.M. Jones wrote:
> > On Thu, Feb 28, 2013 at 02:00:50PM +0000, Matthew Booth wrote:
> > > On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
> > > > From: "Richard W.M. Jones" <rjones(a)redhat.com>
> > > >
> > > > If set, this causes <seclabel model=selinux relabel=no> to be
added to
> > > > the disk element in the libvirt XML.
> > > >
> > > > It has no effect *except* on the libvirt attach method when SELinux
> > > > and sVirt is being used.
> > > > ---
> > > > generator/actions.ml | 8 +++++++-
> > > > src/guestfs-internal.h | 1 +
> > > > src/launch-libvirt.c | 20 ++++++++++++++++++++
> > > > src/launch.c | 21 ++++++++++++++-------
> > > > 4 files changed, 42 insertions(+), 8 deletions(-)
> > >
> > > Is there any instance where we would *not* want to do this? Off the top
> > > of my head, I can't think of a good reason for libguestfs to be in
the
> > > business of relabelling drives. Can we not just hard-code this off?
> > >
> > > If, on the other hand, there is a good reason to make this option, I
> > > think it should be explained in the docs below.
> >
> > It's a tricky situation, but if libguestfs's appliance is confined by
> > sVirt, and if we're accessing an ordinary, non-shared disk, then you
> > have to have libvirt label the disk otherwise qemu won't be able to open
it.
> >
> > The situation here is different: we want to confine libguestfs's
> > appliance with sVirt, and access a shared disk which already has some
> > other label, so we label the appliance to conform to the disk. The
> > reason we don't want libvirt to label the disk is really a bug in
> > libvirt: when the libvirt connection is closed, it resets the label
> > *not* to what it was before, but to some other label. We want to tell
> > libvirt to leave it alone and not do this. So you can argue that the
> > need for this flag is to work around a bug in libvirt.
> >
> > Another way to do this would be not to use svirt_t at all, but to have
> > some other process label (sguestfs_t) which has fewer restrictions
> > than svirt_t. The trick would be to make the confinement meaningful,
> > since the whole point of using sVirt with libguestfs is to confine the
> > appliance against the threat of rogue disk images. Also I guess this
> > would need deep changes to libvirt.
>
> There are several distinct aspects to this - the question of labelling,
> the process domain and the process MCS category.
>
> Before this patch, everything the libguestfs VM did was under a random
> MCS level, which obviously clashed with what the existing running VM
> required.
>
> IMHO we want the libguestfs appliances to be as confined as is possible.
> So if the libguestfs handle is using disks for a particular VM, then
> policy must only allow it to access the disks for that specific VM.
>
> Thus, from a policy POV, the key iss is that we need the MCS level of
> the libguestfs VM to match the MCS level of the running VM.
>
> What we're doing here with using a static label which 100% matches
> the label of the running VM achieves that, at a cost though. The
> cost is that the libguestfs VM is being given full read-write access
> to every resource associated with the running VM, when it really only
> needs to have read-only access to disks. The libguesfs VM monitor
> sockets ends up with the same label sas the main VM monitor socket
> which is also sub-optimal from a security POV. libguestfs should only
> needs read-write access to its own qcow2 overlay image and its own
> monitor socket.
>
> I don't believe we can lock this down without a new policy domain for
> the libguestfs VM and a libvirt enhancement.
>
>
> We would need
>
> - A domain type 'svirt_guestfs_t'
>
> - A disk type 'svirt_guesfs_image_t'
>
> - A process with type svirt_guestfs_t:s0:c123,456 would have
> *readonly* access to files svirt_image_t:s0:c123,456
>
> - A process with type svirt_guestfs_t:s0:c123,456 would have
> *readwrite* access to files svirt_guestfs_image_t:s0:c123,456
>
>
> So in that case you'd use
>
> <seclabel type="static" relabel="yes">
> <label>svirt_guestfs_t:s0:c123,456</label>
> <imagelabel>svirt_guestfs_image_t:s0:c123,456</imagelabel>
> </seclabel>
>
>
> You would *allow* libvirt to relabel the qcow2 overlay that libguestfs
> needs write access to.
>
> There would need to be an enhancement to libvirt XML to allow you to
> tell it to *not* try to relabel the qcow2 backing file though. That
> must remain on 'svirt_image_t:s0:c123,456'.
A couple of observations:
Firstly, this patch is essentially exposing an internal implementation
detail to the user, one which may well change. I suspect we can infer
from available metadata whether or not we require this. If so, I think
it should be inferred rather than passed in by the user. I'm not
convinced this is something we want to be adding to the API.
I agree.
Currently selinuxnorelabel is congruent to internal_-
set_libvirt_selinux_label having been called. However in order to
overload that API a bit less, I suggest adding another internal API
which will be something like internal_set_libvirt_selinux_-
norelabel_disks. Patch to follow.
Secondly, by relabelling disk images we're potentially making
them
temporarily unavailable to other processes, which is something we
weren't doing before. It's possible there may be no way round this, but
if so we ought to highlight it as a major gotcha. Is there any way we
can give a file 2 different SELinux contexts? If there were, we could
leave the original untouched and only allow our ephemeral process to
access the second one. A hard link doesn't allow this, btw. A copy is
almost certainly infeasible in the general case, but if the FS allows
reflink it might fly. Thoughts?
I can't really see a good way to fix this (note for the peanut
gallery: all this only applies to people who use libvirt + SELinux +
sVirt).
Copying disks doesn't sound like a good idea, even if the copies are
lightweight, because you have to examine disks looking for backing
files and potentially even modify the names of backing files within
the copied disk images. It all sounds like a big mess.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
9:49 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 14:37 +0000, Daniel P. Berrange wrote:
> On Thu, Feb 28, 2013 at 02:20:54PM +0000, Richard W.M. Jones wrote:
> > On Thu, Feb 28, 2013 at 02:00:50PM +0000, Matthew Booth wrote:
> > > On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
> > > > From: "Richard W.M. Jones" <rjones(a)redhat.com>
> > > >
> > > > If set, this causes <seclabel model=selinux relabel=no> to be
added to
> > > > the disk element in the libvirt XML.
> > > >
> > > > It has no effect *except* on the libvirt attach method when SELinux
> > > > and sVirt is being used.
> > > > ---
> > > > generator/actions.ml | 8 +++++++-
> > > > src/guestfs-internal.h | 1 +
> > > > src/launch-libvirt.c | 20 ++++++++++++++++++++
> > > > src/launch.c | 21 ++++++++++++++-------
> > > > 4 files changed, 42 insertions(+), 8 deletions(-)
> > >
> > > Is there any instance where we would *not* want to do this? Off the top
> > > of my head, I can't think of a good reason for libguestfs to be in
the
> > > business of relabelling drives. Can we not just hard-code this off?
> > >
> > > If, on the other hand, there is a good reason to make this option, I
> > > think it should be explained in the docs below.
> >
> > It's a tricky situation, but if libguestfs's appliance is confined by
> > sVirt, and if we're accessing an ordinary, non-shared disk, then you
> > have to have libvirt label the disk otherwise qemu won't be able to open
it.
> >
> > The situation here is different: we want to confine libguestfs's
> > appliance with sVirt, and access a shared disk which already has some
> > other label, so we label the appliance to conform to the disk. The
> > reason we don't want libvirt to label the disk is really a bug in
> > libvirt: when the libvirt connection is closed, it resets the label
> > *not* to what it was before, but to some other label. We want to tell
> > libvirt to leave it alone and not do this. So you can argue that the
> > need for this flag is to work around a bug in libvirt.
> >
> > Another way to do this would be not to use svirt_t at all, but to have
> > some other process label (sguestfs_t) which has fewer restrictions
> > than svirt_t. The trick would be to make the confinement meaningful,
> > since the whole point of using sVirt with libguestfs is to confine the
> > appliance against the threat of rogue disk images. Also I guess this
> > would need deep changes to libvirt.
>
> There are several distinct aspects to this - the question of labelling,
> the process domain and the process MCS category.
>
> Before this patch, everything the libguestfs VM did was under a random
> MCS level, which obviously clashed with what the existing running VM
> required.
>
> IMHO we want the libguestfs appliances to be as confined as is possible.
> So if the libguestfs handle is using disks for a particular VM, then
> policy must only allow it to access the disks for that specific VM.
>
> Thus, from a policy POV, the key iss is that we need the MCS level of
> the libguestfs VM to match the MCS level of the running VM.
>
> What we're doing here with using a static label which 100% matches
> the label of the running VM achieves that, at a cost though. The
> cost is that the libguestfs VM is being given full read-write access
> to every resource associated with the running VM, when it really only
> needs to have read-only access to disks. The libguesfs VM monitor
> sockets ends up with the same label sas the main VM monitor socket
> which is also sub-optimal from a security POV. libguestfs should only
> needs read-write access to its own qcow2 overlay image and its own
> monitor socket.
>
> I don't believe we can lock this down without a new policy domain for
> the libguestfs VM and a libvirt enhancement.
>
>
> We would need
>
> - A domain type 'svirt_guestfs_t'
>
> - A disk type 'svirt_guesfs_image_t'
>
> - A process with type svirt_guestfs_t:s0:c123,456 would have
> *readonly* access to files svirt_image_t:s0:c123,456
>
> - A process with type svirt_guestfs_t:s0:c123,456 would have
> *readwrite* access to files svirt_guestfs_image_t:s0:c123,456
>
>
> So in that case you'd use
>
> <seclabel type="static" relabel="yes">
> <label>svirt_guestfs_t:s0:c123,456</label>
> <imagelabel>svirt_guestfs_image_t:s0:c123,456</imagelabel>
> </seclabel>
>
>
> You would *allow* libvirt to relabel the qcow2 overlay that libguestfs
> needs write access to.
>
> There would need to be an enhancement to libvirt XML to allow you to
> tell it to *not* try to relabel the qcow2 backing file though. That
> must remain on 'svirt_image_t:s0:c123,456'.
A couple of observations:
Firstly, this patch is essentially exposing an internal implementation
detail to the user, one which may well change. I suspect we can infer
from available metadata whether or not we require this. If so, I think
it should be inferred rather than passed in by the user. I'm not
convinced this is something we want to be adding to the API.
Secondly, by relabelling disk images we're potentially making them
temporarily unavailable to other processes, which is something we
weren't doing before. It's possible there may be no way round this, but
if so we ought to highlight it as a major gotcha. Is there any way we
can give a file 2 different SELinux contexts? If there were, we could
leave the original untouched and only allow our ephemeral process to
access the second one. A hard link doesn't allow this, btw. A copy is
almost certainly infeasible in the general case, but if the FS allows
reflink it might fly. Thoughts?
There will never be any way to give one file multiple SELinux labels.
It is explicitly rejected as a concept by security people.
What I describe here though is explicitly about *not* having to make
libguestfs relabel other disk images. The point is to define a policy
such that libguestfs can access the other VMs' disks without any
relabelling needing doing to them.
Any relabelling would only be needing on any qcow2 snapshot that
libguestfs has above the original disks.
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 :|
9:52 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, 2013-02-28 at 15:49 +0000, Daniel P. Berrange wrote:
On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
> Secondly, by relabelling disk images we're potentially making them
> temporarily unavailable to other processes, which is something we
> weren't doing before. It's possible there may be no way round this, but
> if so we ought to highlight it as a major gotcha. Is there any way we
> can give a file 2 different SELinux contexts? If there were, we could
> leave the original untouched and only allow our ephemeral process to
> access the second one. A hard link doesn't allow this, btw. A copy is
> almost certainly infeasible in the general case, but if the FS allows
> reflink it might fly. Thoughts?
There will never be any way to give one file multiple SELinux labels.
It is explicitly rejected as a concept by security people.
What I describe here though is explicitly about *not* having to make
libguestfs relabel other disk images. The point is to define a policy
such that libguestfs can access the other VMs' disks without any
relabelling needing doing to them.
Any relabelling would only be needing on any qcow2 snapshot that
libguestfs has above the original disks.
I wasn't talking about whether libguestfs or libvirt does the
relabelling, I was talking about the problems of any relabelling
happening at all to the input file.
Matt
10:01 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, Feb 28, 2013 at 03:52:32PM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 15:49 +0000, Daniel P. Berrange wrote:
> On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
> > Secondly, by relabelling disk images we're potentially making them
> > temporarily unavailable to other processes, which is something we
> > weren't doing before. It's possible there may be no way round this,
but
> > if so we ought to highlight it as a major gotcha. Is there any way we
> > can give a file 2 different SELinux contexts? If there were, we could
> > leave the original untouched and only allow our ephemeral process to
> > access the second one. A hard link doesn't allow this, btw. A copy is
> > almost certainly infeasible in the general case, but if the FS allows
> > reflink it might fly. Thoughts?
>
> There will never be any way to give one file multiple SELinux labels.
> It is explicitly rejected as a concept by security people.
>
> What I describe here though is explicitly about *not* having to make
> libguestfs relabel other disk images. The point is to define a policy
> such that libguestfs can access the other VMs' disks without any
> relabelling needing doing to them.
>
> Any relabelling would only be needing on any qcow2 snapshot that
> libguestfs has above the original disks.
I wasn't talking about whether libguestfs or libvirt does the
relabelling, I was talking about the problems of any relabelling
happening at all to the input file.
Sorry, what do you mean by "input file" here ? What files in particular
are you worried about
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 :|
10:06 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, 2013-02-28 at 16:01 +0000, Daniel P. Berrange wrote:
On Thu, Feb 28, 2013 at 03:52:32PM +0000, Matthew Booth wrote:
> On Thu, 2013-02-28 at 15:49 +0000, Daniel P. Berrange wrote:
> > On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
> > > Secondly, by relabelling disk images we're potentially making them
> > > temporarily unavailable to other processes, which is something we
> > > weren't doing before. It's possible there may be no way round
this, but
> > > if so we ought to highlight it as a major gotcha. Is there any way we
> > > can give a file 2 different SELinux contexts? If there were, we could
> > > leave the original untouched and only allow our ephemeral process to
> > > access the second one. A hard link doesn't allow this, btw. A copy is
> > > almost certainly infeasible in the general case, but if the FS allows
> > > reflink it might fly. Thoughts?
> >
> > There will never be any way to give one file multiple SELinux labels.
> > It is explicitly rejected as a concept by security people.
> >
> > What I describe here though is explicitly about *not* having to make
> > libguestfs relabel other disk images. The point is to define a policy
> > such that libguestfs can access the other VMs' disks without any
> > relabelling needing doing to them.
> >
> > Any relabelling would only be needing on any qcow2 snapshot that
> > libguestfs has above the original disks.
>
> I wasn't talking about whether libguestfs or libvirt does the
> relabelling, I was talking about the problems of any relabelling
> happening at all to the input file.
Sorry, what do you mean by "input file" here ? What files in particular
are you worried about
If we open an image file read-only, ideally we wouldn't modify it in any
way, including its permissions/context.
Matt
10:15 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, Feb 28, 2013 at 04:06:30PM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 16:01 +0000, Daniel P. Berrange wrote:
> On Thu, Feb 28, 2013 at 03:52:32PM +0000, Matthew Booth wrote:
> > On Thu, 2013-02-28 at 15:49 +0000, Daniel P. Berrange wrote:
> > > On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
> > > > Secondly, by relabelling disk images we're potentially making
them
> > > > temporarily unavailable to other processes, which is something we
> > > > weren't doing before. It's possible there may be no way round
this, but
> > > > if so we ought to highlight it as a major gotcha. Is there any way
we
> > > > can give a file 2 different SELinux contexts? If there were, we
could
> > > > leave the original untouched and only allow our ephemeral process to
> > > > access the second one. A hard link doesn't allow this, btw. A
copy is
> > > > almost certainly infeasible in the general case, but if the FS
allows
> > > > reflink it might fly. Thoughts?
> > >
> > > There will never be any way to give one file multiple SELinux labels.
> > > It is explicitly rejected as a concept by security people.
> > >
> > > What I describe here though is explicitly about *not* having to make
> > > libguestfs relabel other disk images. The point is to define a policy
> > > such that libguestfs can access the other VMs' disks without any
> > > relabelling needing doing to them.
> > >
> > > Any relabelling would only be needing on any qcow2 snapshot that
> > > libguestfs has above the original disks.
> >
> > I wasn't talking about whether libguestfs or libvirt does the
> > relabelling, I was talking about the problems of any relabelling
> > happening at all to the input file.
>
> Sorry, what do you mean by "input file" here ? What files in particular
> are you worried about
If we open an image file read-only, ideally we wouldn't modify it in any
way, including its permissions/context.
If that's what you want, then you're basically saying libguestfs KVM security
context needs to be allowed to read any file on the system, which is a
pretty lax requirement from a security POV. It is much more secure to have
libvirt/libguesfs relabel just the files which the libguesfs VM is intended
to access and have a strict policy for QEMU.
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 :|
10:31 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
On Thu, 2013-02-28 at 16:15 +0000, Daniel P. Berrange wrote:
On Thu, Feb 28, 2013 at 04:06:30PM +0000, Matthew Booth wrote:
> On Thu, 2013-02-28 at 16:01 +0000, Daniel P. Berrange wrote:
> > On Thu, Feb 28, 2013 at 03:52:32PM +0000, Matthew Booth wrote:
> > > On Thu, 2013-02-28 at 15:49 +0000, Daniel P. Berrange wrote:
> > > > On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
> > > > > Secondly, by relabelling disk images we're potentially
making them
> > > > > temporarily unavailable to other processes, which is something
we
> > > > > weren't doing before. It's possible there may be no way
round this, but
> > > > > if so we ought to highlight it as a major gotcha. Is there any
way we
> > > > > can give a file 2 different SELinux contexts? If there were, we
could
> > > > > leave the original untouched and only allow our ephemeral
process to
> > > > > access the second one. A hard link doesn't allow this, btw.
A copy is
> > > > > almost certainly infeasible in the general case, but if the FS
allows
> > > > > reflink it might fly. Thoughts?
> > > >
> > > > There will never be any way to give one file multiple SELinux
labels.
> > > > It is explicitly rejected as a concept by security people.
> > > >
> > > > What I describe here though is explicitly about *not* having to make
> > > > libguestfs relabel other disk images. The point is to define a
policy
> > > > such that libguestfs can access the other VMs' disks without any
> > > > relabelling needing doing to them.
> > > >
> > > > Any relabelling would only be needing on any qcow2 snapshot that
> > > > libguestfs has above the original disks.
> > >
> > > I wasn't talking about whether libguestfs or libvirt does the
> > > relabelling, I was talking about the problems of any relabelling
> > > happening at all to the input file.
> >
> > Sorry, what do you mean by "input file" here ? What files in
particular
> > are you worried about
>
> If we open an image file read-only, ideally we wouldn't modify it in any
> way, including its permissions/context.
If that's what you want, then you're basically saying libguestfs KVM security
context needs to be allowed to read any file on the system, which is a
pretty lax requirement from a security POV. It is much more secure to have
libvirt/libguesfs relabel just the files which the libguesfs VM is intended
to access and have a strict policy for QEMU.
Right, hence the talk of copying/multiple labels, because it's still
less than ideal to modify the input file. We might make it temporarily
unreadable by some other process.
Matt
11:45 a.m.
New subject: [PATCH 3/7] add_drive: Add selinuxnorelabel optional boolean.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 02/28/2013 11:15 AM, Daniel P. Berrange wrote:
On Thu, Feb 28, 2013 at 04:06:30PM +0000, Matthew Booth wrote:
> On Thu, 2013-02-28 at 16:01 +0000, Daniel P. Berrange wrote:
>> On Thu, Feb 28, 2013 at 03:52:32PM +0000, Matthew Booth wrote:
>>> On Thu, 2013-02-28 at 15:49 +0000, Daniel P. Berrange wrote:
>>>> On Thu, Feb 28, 2013 at 03:01:20PM +0000, Matthew Booth wrote:
>>>>> Secondly, by relabelling disk images we're potentially making
>>>>> them temporarily unavailable to other processes, which is
>>>>> something we weren't doing before. It's possible there may be
no
>>>>> way round this, but if so we ought to highlight it as a major
>>>>> gotcha. Is there any way we can give a file 2 different SELinux
>>>>> contexts? If there were, we could leave the original untouched
>>>>> and only allow our ephemeral process to access the second one. A
>>>>> hard link doesn't allow this, btw. A copy is almost certainly
>>>>> infeasible in the general case, but if the FS allows reflink it
>>>>> might fly. Thoughts?
>>>>
>>>> There will never be any way to give one file multiple SELinux
>>>> labels. It is explicitly rejected as a concept by security people.
>>>>
>>>> What I describe here though is explicitly about *not* having to
>>>> make libguestfs relabel other disk images. The point is to define a
>>>> policy such that libguestfs can access the other VMs' disks without
>>>> any relabelling needing doing to them.
>>>>
>>>> Any relabelling would only be needing on any qcow2 snapshot that
>>>> libguestfs has above the original disks.
>>>
>>> I wasn't talking about whether libguestfs or libvirt does the
>>> relabelling, I was talking about the problems of any relabelling
>>> happening at all to the input file.
>>
>> Sorry, what do you mean by "input file" here ? What files in
>> particular are you worried about
>
> If we open an image file read-only, ideally we wouldn't modify it in any
> way, including its permissions/context.
If that's what you want, then you're basically saying libguestfs KVM
security context needs to be allowed to read any file on the system, which
is a pretty lax requirement from a security POV. It is much more secure to
have libvirt/libguesfs relabel just the files which the libguesfs VM is
intended to access and have a strict policy for QEMU.
Daniel
Seems like this might be better handled in an IRC. I don't understand the
entire problem.
You have process labels and file system labels. The confinement is on two
parts of the label, the TYPE field and the MCS Field.
MCS Labels allow your process to read file labels that match or are s0.
svirt_t:s0:c1,c2 is allowed to manage svirt_image_t:s0:c1,c2
We could launch libguest with svirt_guestfs_t:s0:c1,c2 and allow it to
read/only svirt_image_t:s0:c1,c2
Then I guess you want svirt_guestfs_t to be able to write to some other
location, correct?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlEvl8IACgkQrlYvE4MpobP4OQCgjn8NWqeX6hl740FugDQ+IK5S
G24An0Uq9qQNUhPwyP+U9oVuh7SpcHp7
=IQ98
-----END PGP SIGNATURE-----
4:57 a.m.
New subject: [PATCH 4/7] launch: libvirt: Allow the SELinux label to be set on qcow2 overlay files.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
When a disk is opened readonly, the libvirt attach-method privately
creates a qcow2 overlay on top.
This commit lets that overlay get an SELinux label, and sets it to the
label specified by guestfs_internal_set_libvirt_selinux_label.
We have to adjust the SELinux label (which is a process label) to make
it applicable to images. We do this by changing the role from
'system_r' to 'object_r', and the type from 'svirt_t' to
'svirt_image_t'.
The above only applies to the libvirt attach-method.
---
src/launch-libvirt.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 45950e2..5c14155 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -133,8 +133,8 @@ static int is_custom_qemu (guestfs_h *g);
static int is_blk (const char *path);
static int random_chars (char *ret, size_t len);
static void ignore_errors (void *ignore, virErrorPtr ignore2);
-static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format);
-static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv);
+static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format, bool
selinux_label);
+static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, bool
selinux_label);
static void drive_free_priv (void *);
static void set_socket_create_context (guestfs_h *g);
static void clear_socket_create_context (guestfs_h *g);
@@ -235,13 +235,13 @@ launch_libvirt (guestfs_h *g, const char *libvirt_uri)
* Note that appliance can be NULL if using the old-style appliance.
*/
if (appliance) {
- params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw");
+ params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw",
false);
if (!params.appliance_overlay)
goto cleanup;
}
ITER_DRIVES (g, i, drv) {
- if (make_qcow2_overlay_for_drive (g, drv) == -1)
+ if (make_qcow2_overlay_for_drive (g, drv, true) == -1)
goto cleanup;
}
@@ -1350,7 +1350,8 @@ ignore_errors (void *ignore, virErrorPtr ignore2)
/* Create a temporary qcow2 overlay on top of 'path'. */
static char *
-make_qcow2_overlay (guestfs_h *g, const char *path, const char *format)
+make_qcow2_overlay (guestfs_h *g, const char *path, const char *format,
+ bool selinux_label)
{
char *tmpfile = NULL;
CLEANUP_CMD_CLOSE struct command *cmd = guestfs___new_command (g);
@@ -1381,6 +1382,15 @@ make_qcow2_overlay (guestfs_h *g, const char *path, const char
*format)
goto error;
}
+#if HAVE_LIBSELINUX
+ if (selinux_label && g->virt_selinux_imagelabel) {
+ debug (g, "setting SELinux label on %s to %s",
+ tmpfile, g->virt_selinux_imagelabel);
+ if (setfilecon (tmpfile, g->virt_selinux_imagelabel) == -1)
+ selinux_warning (g, __func__, "setfilecon", tmpfile);
+ }
+#endif
+
return tmpfile; /* caller frees */
error:
@@ -1390,7 +1400,8 @@ make_qcow2_overlay (guestfs_h *g, const char *path, const char
*format)
}
static int
-make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv)
+make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv,
+ bool selinux_label)
{
char *path;
struct drive_libvirt *drv_priv;
@@ -1414,7 +1425,7 @@ make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv)
drv_priv->format = drv->format ? safe_strdup (g, drv->format) : NULL;
}
else {
- drv_priv->path = make_qcow2_overlay (g, path, drv->format);
+ drv_priv->path = make_qcow2_overlay (g, path, drv->format, selinux_label);
free (path);
if (!drv_priv->path)
return -1;
@@ -1531,7 +1542,7 @@ hot_add_drive_libvirt (guestfs_h *g, struct drive *drv, size_t
drv_index)
/* Create overlay for read-only drive. This works around lack of
* support for <transient/> disks in libvirt.
*/
- if (make_qcow2_overlay_for_drive (g, drv) == -1)
+ if (make_qcow2_overlay_for_drive (g, drv, true) == -1)
return -1;
/* Create the XML for the new disk. */
--
1.8.1.2
8:14 a.m.
New subject: [PATCH 4/7] launch: libvirt: Allow the SELinux label to be set on qcow2 overlay files.
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)redhat.com>
When a disk is opened readonly, the libvirt attach-method privately
creates a qcow2 overlay on top.
This commit lets that overlay get an SELinux label, and sets it to the
label specified by guestfs_internal_set_libvirt_selinux_label.
We have to adjust the SELinux label (which is a process label) to make
it applicable to images. We do this by changing the role from
'system_r' to 'object_r', and the type from 'svirt_t' to
'svirt_image_t'.
The code below doesn't do this. Are you saying that the caller must set
the process context manually?
The above only applies to the libvirt attach-method.
---
src/launch-libvirt.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 45950e2..5c14155 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -133,8 +133,8 @@ static int is_custom_qemu (guestfs_h *g);
static int is_blk (const char *path);
static int random_chars (char *ret, size_t len);
static void ignore_errors (void *ignore, virErrorPtr ignore2);
-static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format);
-static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv);
+static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char *format,
bool selinux_label);
+static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, bool
selinux_label);
static void drive_free_priv (void *);
static void set_socket_create_context (guestfs_h *g);
static void clear_socket_create_context (guestfs_h *g);
@@ -235,13 +235,13 @@ launch_libvirt (guestfs_h *g, const char *libvirt_uri)
* Note that appliance can be NULL if using the old-style appliance.
*/
if (appliance) {
- params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw");
+ params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw",
false);
if (!params.appliance_overlay)
goto cleanup;
}
I'm surprised the appliance image overlay doesn't require a label. If
libvirt launches the appliance in a confined context, I wouldn't expect
it to be able to read anything which wasn't appropriately labelled,
which would include the image. I assume you've tested it and it works,
but I would like to understand why this is possible. Relabelling the
appliance could get complicated wrt multiple appliances running
simultaneously.
Matt
8:24 a.m.
New subject: [PATCH 4/7] launch: libvirt: Allow the SELinux label to be set on qcow2 overlay files.
On Thu, Feb 28, 2013 at 02:14:42PM +0000, Matthew Booth wrote:
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones(a)redhat.com>
>
> When a disk is opened readonly, the libvirt attach-method privately
> creates a qcow2 overlay on top.
>
> This commit lets that overlay get an SELinux label, and sets it to the
> label specified by guestfs_internal_set_libvirt_selinux_label.
>
> We have to adjust the SELinux label (which is a process label) to make
> it applicable to images. We do this by changing the role from
> 'system_r' to 'object_r', and the type from 'svirt_t' to
'svirt_image_t'.
The code below doesn't do this. Are you saying that the caller must set
the process context manually?
Yes you're correct. I've fixed the commit message to match what the
code does :-)
>
> The above only applies to the libvirt attach-method.
> ---
> src/launch-libvirt.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
> index 45950e2..5c14155 100644
> --- a/src/launch-libvirt.c
> +++ b/src/launch-libvirt.c
> @@ -133,8 +133,8 @@ static int is_custom_qemu (guestfs_h *g);
> static int is_blk (const char *path);
> static int random_chars (char *ret, size_t len);
> static void ignore_errors (void *ignore, virErrorPtr ignore2);
> -static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char
*format);
> -static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv);
> +static char *make_qcow2_overlay (guestfs_h *g, const char *path, const char
*format, bool selinux_label);
> +static int make_qcow2_overlay_for_drive (guestfs_h *g, struct drive *drv, bool
selinux_label);
> static void drive_free_priv (void *);
> static void set_socket_create_context (guestfs_h *g);
> static void clear_socket_create_context (guestfs_h *g);
> @@ -235,13 +235,13 @@ launch_libvirt (guestfs_h *g, const char *libvirt_uri)
> * Note that appliance can be NULL if using the old-style appliance.
> */
> if (appliance) {
> - params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw");
> + params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw",
false);
> if (!params.appliance_overlay)
> goto cleanup;
> }
I'm surprised the appliance image overlay doesn't require a label. If
libvirt launches the appliance in a confined context, I wouldn't expect
it to be able to read anything which wasn't appropriately labelled,
which would include the image. I assume you've tested it and it works,
but I would like to understand why this is possible.
It's because libvirt relabels the overlay (and its backing file) in
this case. Note that we *don't* set <seclabel model=selinux relabel=no/>
on the appliance disk.
Relabelling the appliance could get complicated wrt multiple
appliances running simultaneously.
Right -- I suspect this is buggy actually, but it's quite hard to test
it since I need to run up lots of guests and run virt-df in parallel
on them. Also we put <shareable/> on the appliance disk, and I'm not
sure what libvirt does in that case.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
8:31 a.m.
New subject: [PATCH 4/7] launch: libvirt: Allow the SELinux label to be set on qcow2 overlay files.
On Thu, Feb 28, 2013 at 02:24:30PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 28, 2013 at 02:14:42PM +0000, Matthew Booth wrote:
> Relabelling the appliance could get complicated wrt multiple
> appliances running simultaneously.
Right -- I suspect this is buggy actually, but it's quite hard to test
it since I need to run up lots of guests and run virt-df in parallel
on them. Also we put <shareable/> on the appliance disk, and I'm not
sure what libvirt does in that case.
I spun up a few libvirt guests and surprisingly this all seems to
work. I have no idea why :-) I agree with you that it shouldn't work
because the labels should get stomped on by the different connections,
but it seems libvirt is being cleverer somehow.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
8:41 a.m.
New subject: [PATCH 4/7] launch: libvirt: Allow the SELinux label to be set on qcow2 overlay files.
On Thu, Feb 28, 2013 at 02:31:23PM +0000, Richard W.M. Jones wrote:
On Thu, Feb 28, 2013 at 02:24:30PM +0000, Richard W.M. Jones wrote:
> On Thu, Feb 28, 2013 at 02:14:42PM +0000, Matthew Booth wrote:
> > Relabelling the appliance could get complicated wrt multiple
> > appliances running simultaneously.
>
> Right -- I suspect this is buggy actually, but it's quite hard to test
> it since I need to run up lots of guests and run virt-df in parallel
> on them. Also we put <shareable/> on the appliance disk, and I'm not
> sure what libvirt does in that case.
I spun up a few libvirt guests and surprisingly this all seems to
work. I have no idea why :-) I agree with you that it shouldn't work
because the labels should get stomped on by the different connections,
but it seems libvirt is being cleverer somehow.
For any disk with <sharable/> set, libvirt ignores the VM MCS level
and labels the disk with a simple svirt_image_t label which allows
*every* VM running on the host to have read-write access to the
disk.
You can't share disks between VMs, if they have MCS levels attached,
unless the VMs all have the same MCS level themselves.
Regards,
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 :|
9:18 a.m.
New subject: [PATCH 4/7] launch: libvirt: Allow the SELinux label to be set on qcow2 overlay files.
On Thu, 2013-02-28 at 14:41 +0000, Daniel P. Berrange wrote:
On Thu, Feb 28, 2013 at 02:31:23PM +0000, Richard W.M. Jones wrote:
> On Thu, Feb 28, 2013 at 02:24:30PM +0000, Richard W.M. Jones wrote:
> > On Thu, Feb 28, 2013 at 02:14:42PM +0000, Matthew Booth wrote:
> > > Relabelling the appliance could get complicated wrt multiple
> > > appliances running simultaneously.
> >
> > Right -- I suspect this is buggy actually, but it's quite hard to test
> > it since I need to run up lots of guests and run virt-df in parallel
> > on them. Also we put <shareable/> on the appliance disk, and I'm
not
> > sure what libvirt does in that case.
>
> I spun up a few libvirt guests and surprisingly this all seems to
> work. I have no idea why :-) I agree with you that it shouldn't work
> because the labels should get stomped on by the different connections,
> but it seems libvirt is being cleverer somehow.
For any disk with <sharable/> set, libvirt ignores the VM MCS level
and labels the disk with a simple svirt_image_t label which allows
*every* VM running on the host to have read-write access to the
disk.
You can't share disks between VMs, if they have MCS levels attached,
unless the VMs all have the same MCS level themselves.
Presumably, therefore, a <shareable/> disk is only guaranteed to be
shareable between processes with the same SELinux context. I'm guessing
that <seclabel/> defaults to svirt_t, but is configurable? If I'm right
and you set it to something else, which would also require custom
policy, this would fail. Presumably the behaviour of <shareable/> must
also have some prior knowledge of the behaviour of the policy in order
to be able to create a label which will work. How does it do this?
I think the only way you can guarantee this to work is to create an
appliance per user/selinux label instead of just per user. With per-VM
categories this would create a lot of appliances, though.
Matt
4:57 a.m.
New subject: [PATCH 5/7] add-domain: Refactor domain XML parsing code.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
This is just code motion.
---
src/libvirt-domain.c | 71 +++++++++++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 37 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index ab776b3..3f5b1ed 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -40,7 +40,8 @@
#if defined(HAVE_LIBVIRT) && defined(HAVE_LIBXML2)
-static ssize_t for_each_disk (guestfs_h *g, virDomainPtr dom, int (*f) (guestfs_h *g,
const char *filename, const char *format, int readonly, void *data), void *data);
+static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom);
+static ssize_t for_each_disk (guestfs_h *g, xmlDocPtr doc, int (*f) (guestfs_h *g, const
char *filename, const char *format, int readonly, void *data), void *data);
static void
ignore_errors (void *ignore, virErrorPtr ignore2)
@@ -167,6 +168,7 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
enum readonlydisk readonlydisk = readonlydisk_write;
size_t ckp;
struct add_disk_data data;
+ CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
readonly =
optargs->bitmask & GUESTFS___ADD_LIBVIRT_DOM_READONLY_BITMASK
@@ -228,6 +230,10 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
}
}
+ /* Domain XML. */
+ if ((doc = get_domain_xml (g, dom)) == NULL)
+ return -1;
+
/* Add the disks. */
data.optargs.bitmask = 0;
data.readonly = readonly;
@@ -241,7 +247,7 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
* all disks are added or none are added.
*/
ckp = guestfs___checkpoint_drives (g);
- r = for_each_disk (g, dom, add_disk, &data);
+ r = for_each_disk (g, doc, add_disk, &data);
if (r == -1)
guestfs___rollback_drives (g, ckp);
@@ -303,29 +309,15 @@ static int
connect_live (guestfs_h *g, virDomainPtr dom)
{
int i;
- virErrorPtr err;
CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL;
- CLEANUP_FREE char *xml = NULL, *path = NULL, *attach_method = NULL;
+ CLEANUP_FREE char *path = NULL, *attach_method = NULL;
xmlNodeSetPtr nodes;
/* Domain XML. */
- xml = virDomainGetXMLDesc (dom, 0);
-
- if (!xml) {
- err = virGetLastError ();
- error (g, _("error reading libvirt XML information: %s"),
- err->message);
+ if ((doc = get_domain_xml (g, dom)) == NULL)
return -1;
- }
-
- /* Parse XML to document. */
- doc = xmlParseMemory (xml, strlen (xml));
- if (doc == NULL) {
- error (g, _("unable to parse XML information returned by libvirt"));
- return -1;
- }
xpathCtx = xmlXPathNewContext (doc);
if (xpathCtx == NULL) {
@@ -388,7 +380,7 @@ connect_live (guestfs_h *g, virDomainPtr dom)
*/
static ssize_t
for_each_disk (guestfs_h *g,
- virDomainPtr dom,
+ xmlDocPtr doc,
int (*f) (guestfs_h *g,
const char *filename, const char *format,
int readonly,
@@ -396,31 +388,13 @@ for_each_disk (guestfs_h *g,
void *data)
{
size_t i, nr_added = 0, nr_nodes;
- virErrorPtr err;
- CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL;
- CLEANUP_FREE char *xml = NULL;
xmlNodeSetPtr nodes;
- /* Domain XML. */
- xml = virDomainGetXMLDesc (dom, 0);
-
- if (!xml) {
- err = virGetLastError ();
- error (g, _("error reading libvirt XML information: %s"),
err->message);
- return -1;
- }
-
/* Now the horrible task of parsing out the fields we need from the XML.
* http://www.xmlsoft.org/examples/xpath1.c
*/
- doc = xmlParseMemory (xml, strlen (xml));
- if (doc == NULL) {
- error (g, _("unable to parse XML information returned by libvirt"));
- return -1;
- }
-
xpathCtx = xmlXPathNewContext (doc);
if (xpathCtx == NULL) {
error (g, _("unable to create new XPath context"));
@@ -533,6 +507,29 @@ for_each_disk (guestfs_h *g,
return nr_added;
}
+static xmlDocPtr
+get_domain_xml (guestfs_h *g, virDomainPtr dom)
+{
+ virErrorPtr err;
+ xmlDocPtr doc;
+
+ CLEANUP_FREE char *xml = virDomainGetXMLDesc (dom, 0);
+ if (!xml) {
+ err = virGetLastError ();
+ error (g, _("error reading libvirt XML information: %s"),
err->message);
+ return NULL;
+ }
+
+ /* Parse the domain XML into an XML document. */
+ doc = xmlParseMemory (xml, strlen (xml));
+ if (doc == NULL) {
+ error (g, _("unable to parse XML information returned by libvirt"));
+ return NULL;
+ }
+
+ return doc;
+}
+
#else /* no libvirt or libxml2 at compile time */
#define NOT_IMPL(r) \
--
1.8.1.2
9:22 a.m.
New subject: [PATCH 5/7] add-domain: Refactor domain XML parsing code.
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)redhat.com>
This is just code motion.
---
src/libvirt-domain.c | 71 +++++++++++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 37 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index ab776b3..3f5b1ed 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -40,7 +40,8 @@
#if defined(HAVE_LIBVIRT) && defined(HAVE_LIBXML2)
-static ssize_t for_each_disk (guestfs_h *g, virDomainPtr dom, int (*f) (guestfs_h *g,
const char *filename, const char *format, int readonly, void *data), void *data);
+static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom);
+static ssize_t for_each_disk (guestfs_h *g, xmlDocPtr doc, int (*f) (guestfs_h *g, const
char *filename, const char *format, int readonly, void *data), void *data);
static void
ignore_errors (void *ignore, virErrorPtr ignore2)
@@ -167,6 +168,7 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
enum readonlydisk readonlydisk = readonlydisk_write;
size_t ckp;
struct add_disk_data data;
+ CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
readonly =
optargs->bitmask & GUESTFS___ADD_LIBVIRT_DOM_READONLY_BITMASK
@@ -228,6 +230,10 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
}
}
+ /* Domain XML. */
+ if ((doc = get_domain_xml (g, dom)) == NULL)
+ return -1;
+
/* Add the disks. */
data.optargs.bitmask = 0;
data.readonly = readonly;
@@ -241,7 +247,7 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
* all disks are added or none are added.
*/
ckp = guestfs___checkpoint_drives (g);
- r = for_each_disk (g, dom, add_disk, &data);
+ r = for_each_disk (g, doc, add_disk, &data);
if (r == -1)
guestfs___rollback_drives (g, ckp);
@@ -303,29 +309,15 @@ static int
connect_live (guestfs_h *g, virDomainPtr dom)
{
int i;
- virErrorPtr err;
CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL;
- CLEANUP_FREE char *xml = NULL, *path = NULL, *attach_method = NULL;
+ CLEANUP_FREE char *path = NULL, *attach_method = NULL;
xmlNodeSetPtr nodes;
/* Domain XML. */
- xml = virDomainGetXMLDesc (dom, 0);
-
- if (!xml) {
- err = virGetLastError ();
- error (g, _("error reading libvirt XML information: %s"),
- err->message);
+ if ((doc = get_domain_xml (g, dom)) == NULL)
return -1;
- }
-
- /* Parse XML to document. */
- doc = xmlParseMemory (xml, strlen (xml));
- if (doc == NULL) {
- error (g, _("unable to parse XML information returned by libvirt"));
- return -1;
- }
xpathCtx = xmlXPathNewContext (doc);
if (xpathCtx == NULL) {
@@ -388,7 +380,7 @@ connect_live (guestfs_h *g, virDomainPtr dom)
*/
static ssize_t
for_each_disk (guestfs_h *g,
- virDomainPtr dom,
+ xmlDocPtr doc,
int (*f) (guestfs_h *g,
const char *filename, const char *format,
int readonly,
@@ -396,31 +388,13 @@ for_each_disk (guestfs_h *g,
void *data)
{
size_t i, nr_added = 0, nr_nodes;
- virErrorPtr err;
- CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL;
- CLEANUP_FREE char *xml = NULL;
xmlNodeSetPtr nodes;
- /* Domain XML. */
- xml = virDomainGetXMLDesc (dom, 0);
-
- if (!xml) {
- err = virGetLastError ();
- error (g, _("error reading libvirt XML information: %s"),
err->message);
- return -1;
- }
-
/* Now the horrible task of parsing out the fields we need from the XML.
* http://www.xmlsoft.org/examples/xpath1.c
*/
- doc = xmlParseMemory (xml, strlen (xml));
- if (doc == NULL) {
- error (g, _("unable to parse XML information returned by libvirt"));
- return -1;
- }
-
xpathCtx = xmlXPathNewContext (doc);
if (xpathCtx == NULL) {
error (g, _("unable to create new XPath context"));
@@ -533,6 +507,29 @@ for_each_disk (guestfs_h *g,
return nr_added;
}
+static xmlDocPtr
+get_domain_xml (guestfs_h *g, virDomainPtr dom)
+{
+ virErrorPtr err;
+ xmlDocPtr doc;
+
+ CLEANUP_FREE char *xml = virDomainGetXMLDesc (dom, 0);
+ if (!xml) {
+ err = virGetLastError ();
+ error (g, _("error reading libvirt XML information: %s"),
err->message);
+ return NULL;
+ }
+
+ /* Parse the domain XML into an XML document. */
+ doc = xmlParseMemory (xml, strlen (xml));
+ if (doc == NULL) {
+ error (g, _("unable to parse XML information returned by libvirt"));
+ return NULL;
+ }
+
+ return doc;
+}
+
#else /* no libvirt or libxml2 at compile time */
#define NOT_IMPL(r) \
ACK.
Matt
4:57 a.m.
New subject: [PATCH 6/7] add-domain: Pass SELinux label from guest to appliance (RHBZ#912499).
From: "Richard W.M. Jones" <rjones(a)redhat.com>
When adding a domain (ie. guestfs_add_domain), read the SELinux
<label/> and <imagelabel/> from the guest and use them for the
appliance. The appliance is statically labelled the same as the
guest, so it is able to read its disks.
However tell libvirt not to try relabelling the disks, to prevent
libvirt from disturbing the existing labels on the disks (in
particular when the libvirt connection is closed, we don't want
libvirt to try to restore some other label on the disks).
---
src/libvirt-domain.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 78 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 3f5b1ed..15be611 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -20,6 +20,7 @@
#include <stdio.h>
#include <stdlib.h>
+#include <stdbool.h>
#include <assert.h>
#ifdef HAVE_LIBVIRT
@@ -42,6 +43,7 @@
static xmlDocPtr get_domain_xml (guestfs_h *g, virDomainPtr dom);
static ssize_t for_each_disk (guestfs_h *g, xmlDocPtr doc, int (*f) (guestfs_h *g, const
char *filename, const char *format, int readonly, void *data), void *data);
+static int libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc, char **label_rtn, char
**imagelabel_rtn);
static void
ignore_errors (void *ignore, virErrorPtr ignore2)
@@ -169,6 +171,8 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
size_t ckp;
struct add_disk_data data;
CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
+ CLEANUP_FREE char *label = NULL, *imagelabel = NULL;
+ int selinuxnorelabel;
readonly =
optargs->bitmask & GUESTFS___ADD_LIBVIRT_DOM_READONLY_BITMASK
@@ -234,6 +238,16 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
if ((doc = get_domain_xml (g, dom)) == NULL)
return -1;
+ /* Find and pass the SELinux security label to the libvirt back end. */
+ if (libvirt_selinux_label (g, doc, &label, &imagelabel) == -1)
+ return -1;
+ if (label && imagelabel) {
+ guestfs_internal_set_libvirt_selinux_label (g, label, imagelabel);
+ selinuxnorelabel = 1;
+ }
+ else
+ selinuxnorelabel = 0;
+
/* Add the disks. */
data.optargs.bitmask = 0;
data.readonly = readonly;
@@ -242,6 +256,10 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
data.optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_IFACE_BITMASK;
data.optargs.iface = iface;
}
+ if (selinuxnorelabel) {
+ data.optargs.bitmask |= GUESTFS_ADD_DRIVE_OPTS_SELINUXNORELABEL_BITMASK;
+ data.optargs.selinuxnorelabel = 1;
+ }
/* Checkpoint the command line around the operation so that either
* all disks are added or none are added.
@@ -374,6 +392,66 @@ connect_live (guestfs_h *g, virDomainPtr dom)
return guestfs_set_attach_method (g, attach_method);
}
+/* Find the <seclabel/> element in the libvirt XML, and if it exists
+ * get the SELinux process label and image label from it.
+ *
+ * The reason for all this is because of sVirt:
+ * https://bugzilla.redhat.com/show_bug.cgi?id=912499#c7
+ */
+static int
+libvirt_selinux_label (guestfs_h *g, xmlDocPtr doc,
+ char **label_rtn, char **imagelabel_rtn)
+{
+ CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
+ CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL;
+ xmlNodeSetPtr nodes;
+ size_t nr_nodes;
+ xmlNodePtr node, child;
+ bool gotlabel = 0, gotimagelabel = 0;
+
+ xpathCtx = xmlXPathNewContext (doc);
+ if (xpathCtx == NULL) {
+ error (g, _("unable to create new XPath context"));
+ return -1;
+ }
+
+ /* This gives us a set of all the <seclabel/> nodes, hopefully only one. */
+ xpathObj = xmlXPathEvalExpression (BAD_CAST "/domain/seclabel",
+ xpathCtx);
+ if (xpathObj == NULL) {
+ error (g, _("unable to evaluate XPath expression"));
+ return -1;
+ }
+
+ nodes = xpathObj->nodesetval;
+ nr_nodes = nodes->nodeNr;
+
+ if (nr_nodes == 0 || nr_nodes > 1)
+ return 0;
+
+ node = nodes->nodeTab[0];
+ if (node->type != XML_ELEMENT_NODE) {
+ error (g, _("expected <seclabel/> to be an XML element"));
+ return -1;
+ }
+
+ /* Find the <label/> and <imagelabel/> child nodes. */
+ for (child = node->children; child != NULL; child = child->next) {
+ if (!gotlabel && STREQ ((char *) child->name, "label")) {
+ /* Get the label content. */
+ *label_rtn = (char *) xmlNodeGetContent (child);
+ gotlabel = 1;
+ }
+ if (!gotimagelabel && STREQ ((char *) child->name,
"imagelabel")) {
+ /* Get the imagelabel content. */
+ *imagelabel_rtn = (char *) xmlNodeGetContent (child);
+ gotimagelabel = 1;
+ }
+ }
+
+ return 0;
+}
+
/* The callback function 'f' is called once for each disk.
*
* Returns number of disks, or -1 if there was an error.
--
1.8.1.2
9:46 a.m.
New subject: [PATCH 6/7] add-domain: Pass SELinux label from guest to appliance (RHBZ#912499).
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)redhat.com>
When adding a domain (ie. guestfs_add_domain), read the SELinux
<label/> and <imagelabel/> from the guest and use them for the
appliance. The appliance is statically labelled the same as the
guest, so it is able to read its disks.
However tell libvirt not to try relabelling the disks, to prevent
libvirt from disturbing the existing labels on the disks (in
particular when the libvirt connection is closed, we don't want
libvirt to try to restore some other label on the disks).
---
src/libvirt-domain.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 78 insertions(+)
From a previous email, it sounds like you're reworking this one.
Matt
4:57 a.m.
New subject: [PATCH 7/7] add-domain: Move 'connect_live' function.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
This is just code motion.
---
src/libvirt-domain.c | 138 +++++++++++++++++++++++++--------------------------
1 file changed, 69 insertions(+), 69 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 15be611..22d9803 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -323,75 +323,6 @@ add_disk (guestfs_h *g,
return guestfs__add_drive_opts (g, filename, &optargs);
}
-static int
-connect_live (guestfs_h *g, virDomainPtr dom)
-{
- int i;
- CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
- CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
- CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL;
- CLEANUP_FREE char *path = NULL, *attach_method = NULL;
- xmlNodeSetPtr nodes;
-
- /* Domain XML. */
- if ((doc = get_domain_xml (g, dom)) == NULL)
- return -1;
-
- xpathCtx = xmlXPathNewContext (doc);
- if (xpathCtx == NULL) {
- error (g, _("unable to create new XPath context"));
- return -1;
- }
-
- /* This gives us a set of all the <channel> nodes related to the
- * guestfsd virtio-serial channel.
- */
- xpathObj = xmlXPathEvalExpression (BAD_CAST
- "//devices/channel[@type=\"unix\" and "
- "./source/@mode=\"bind\" and "
- "./source/@path and "
- "./target/@type=\"virtio\" and "
-
"./target/(a)name=\"org.libguestfs.channel.0\"]",
- xpathCtx);
- if (xpathObj == NULL) {
- error (g, _("unable to evaluate XPath expression"));
- return -1;
- }
-
- nodes = xpathObj->nodesetval;
- for (i = 0; i < nodes->nodeNr; ++i) {
- CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xppath = NULL;
- xmlAttrPtr attr;
-
- /* See note in function above. */
- xpathCtx->node = nodes->nodeTab[i];
-
- /* The path is in <source path=..> attribute. */
- xppath = xmlXPathEvalExpression (BAD_CAST "./source/@path", xpathCtx);
- if (xppath == NULL ||
- xppath->nodesetval == NULL ||
- xppath->nodesetval->nodeNr == 0) {
- xmlXPathFreeObject (xppath);
- continue; /* no type attribute, skip it */
- }
- assert (xppath->nodesetval->nodeTab[0]);
- assert (xppath->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE);
- attr = (xmlAttrPtr) xppath->nodesetval->nodeTab[0];
- path = (char *) xmlNodeListGetString (doc, attr->children, 1);
- break;
- }
-
- if (path == NULL) {
- error (g, _("this guest has no libvirt <channel> definition for
guestfsd\n"
- "See ATTACHING TO RUNNING DAEMONS in guestfs(3) for further
information."));
- return -1;
- }
-
- /* Got a path. */
- attach_method = safe_asprintf (g, "unix:%s", path);
- return guestfs_set_attach_method (g, attach_method);
-}
-
/* Find the <seclabel/> element in the libvirt XML, and if it exists
* get the SELinux process label and image label from it.
*
@@ -585,6 +516,75 @@ for_each_disk (guestfs_h *g,
return nr_added;
}
+static int
+connect_live (guestfs_h *g, virDomainPtr dom)
+{
+ int i;
+ CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
+ CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
+ CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xpathObj = NULL;
+ CLEANUP_FREE char *path = NULL, *attach_method = NULL;
+ xmlNodeSetPtr nodes;
+
+ /* Domain XML. */
+ if ((doc = get_domain_xml (g, dom)) == NULL)
+ return -1;
+
+ xpathCtx = xmlXPathNewContext (doc);
+ if (xpathCtx == NULL) {
+ error (g, _("unable to create new XPath context"));
+ return -1;
+ }
+
+ /* This gives us a set of all the <channel> nodes related to the
+ * guestfsd virtio-serial channel.
+ */
+ xpathObj = xmlXPathEvalExpression (BAD_CAST
+ "//devices/channel[@type=\"unix\" and "
+ "./source/@mode=\"bind\" and "
+ "./source/@path and "
+ "./target/@type=\"virtio\" and "
+
"./target/(a)name=\"org.libguestfs.channel.0\"]",
+ xpathCtx);
+ if (xpathObj == NULL) {
+ error (g, _("unable to evaluate XPath expression"));
+ return -1;
+ }
+
+ nodes = xpathObj->nodesetval;
+ for (i = 0; i < nodes->nodeNr; ++i) {
+ CLEANUP_XMLXPATHFREEOBJECT xmlXPathObjectPtr xppath = NULL;
+ xmlAttrPtr attr;
+
+ /* See note in function above. */
+ xpathCtx->node = nodes->nodeTab[i];
+
+ /* The path is in <source path=..> attribute. */
+ xppath = xmlXPathEvalExpression (BAD_CAST "./source/@path", xpathCtx);
+ if (xppath == NULL ||
+ xppath->nodesetval == NULL ||
+ xppath->nodesetval->nodeNr == 0) {
+ xmlXPathFreeObject (xppath);
+ continue; /* no type attribute, skip it */
+ }
+ assert (xppath->nodesetval->nodeTab[0]);
+ assert (xppath->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE);
+ attr = (xmlAttrPtr) xppath->nodesetval->nodeTab[0];
+ path = (char *) xmlNodeListGetString (doc, attr->children, 1);
+ break;
+ }
+
+ if (path == NULL) {
+ error (g, _("this guest has no libvirt <channel> definition for
guestfsd\n"
+ "See ATTACHING TO RUNNING DAEMONS in guestfs(3) for further
information."));
+ return -1;
+ }
+
+ /* Got a path. */
+ attach_method = safe_asprintf (g, "unix:%s", path);
+ return guestfs_set_attach_method (g, attach_method);
+}
+
static xmlDocPtr
get_domain_xml (guestfs_h *g, virDomainPtr dom)
{
--
1.8.1.2
9:48 a.m.
New subject: [PATCH 7/7] add-domain: Move 'connect_live' function.
On Thu, 2013-02-28 at 10:57 +0000, Richard W.M. Jones wrote:
From: "Richard W.M. Jones" <rjones(a)redhat.com>
This is just code motion.
---
src/libvirt-domain.c | 138 +++++++++++++++++++++++++--------------------------
1 file changed, 69 insertions(+), 69 deletions(-)
ACK.
Matt
4099
days inactive
4099
days old
30 comments
4 participants
participants (4)
-
Daniel J Walsh -
Daniel P. Berrange -
Matthew Booth -
Richard W.M. Jones