Libguestfs - Oct 2014 - [PATCH] launch: libvirt: Implement drive secrets (RHBZ#1159016).

Implement the GUESTFS_ADD_DRIVE_OPTS_SECRET argument of
guestfs_add_drive_opts.  For libvirt we have to save the secret in
libvirtd first, get a UUID, and then pass the UUID back through the
domain XML.
---
 src/launch-libvirt.c | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 224 insertions(+), 3 deletions(-)

diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 7206b33..45e215c 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -97,6 +97,25 @@ xmlBufferDetach (xmlBufferPtr buf)
 }
 #endif
 
+#ifdef HAVE_ATTRIBUTE_CLEANUP
+#define CLEANUP_VIRSECRETFREE __attribute__((cleanup(cleanup_virSecretFree)))
+
+static void
+cleanup_virSecretFree (void *ptr)
+{
+  virSecretFree (* (virSecretPtr *) ptr);
+}
+
+#else /* !HAVE_ATTRIBUTE_CLEANUP */
+#define CLEANUP_VIRSECRETFREE
+#endif
+
+/* Use to store a mapping of secret to libvirt secret UUID. */
+struct secret {
+  char *secret;
+  char uuid[VIR_UUID_STRING_BUFLEN];
+};
+
 #define DOMAIN_NAME_LEN (8+16+1) /* "guestfs-" + random + \0 */
 
 /* Per-handle data. */
@@ -110,6 +129,8 @@ struct backend_libvirt_data {
   char name[DOMAIN_NAME_LEN];   /* random name */
   bool is_kvm;                  /* false = qemu, true = kvm (from
capabilities)*/
   unsigned long qemu_version;   /* qemu version (from libvirt) */
+  struct secret *secrets;       /* list of secrets */
+  size_t nr_secrets;
 };
 
 /* Parameters passed to construct_libvirt_xml and subfunctions.  We
@@ -130,6 +151,9 @@ struct libvirt_xml_params {
 };
 
 static int parse_capabilities (guestfs_h *g, const char *capabilities_xml,
struct backend_libvirt_data *data);
+static int add_secret (guestfs_h *g, struct backend_libvirt_data *data, const
struct drive *drv);
+static int find_secret (guestfs_h *g, const struct backend_libvirt_data *data,
const struct drive *drv, const char **type, const char **uuid);
+static int have_secret (guestfs_h *g, const struct backend_libvirt_data *data,
const struct drive *drv);
 static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct
libvirt_xml_params *params);
 static void debug_appliance_permissions (guestfs_h *g);
 static void debug_socket_permissions (guestfs_h *g);
@@ -224,6 +248,8 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   CLEANUP_FREE xmlChar *xml = NULL;
   CLEANUP_FREE char *appliance = NULL;
   struct sockaddr_un addr;
+  struct drive *drv;
+  size_t i;
   int r;
   uint32_t size;
   CLEANUP_FREE void *buf = NULL;
@@ -458,6 +484,14 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
       debug (g, "cannot find group 'qemu'");
   }
 
+  /* Store any secrets in libvirtd, keeping a mapping from the secret
+   * to its UUID.
+   */
+  ITER_DRIVES (g, i, drv) {
+    if (add_secret (g, data, drv) == -1)
+      goto cleanup;
+  }
+
   /* Construct the libvirt XML. */
   if (g->verbose)
     guestfs___print_timestamped_message (g, "create libvirt XML");
@@ -1272,6 +1306,8 @@ construct_libvirt_xml_disk (guestfs_h *g,
   CLEANUP_FREE char *path = NULL;
   int is_host_device;
   CLEANUP_FREE char *format = NULL;
+  const char *type, *uuid;
+  int r;
 
   /* XXX We probably could support this if we thought about it some more. */
   if (drv->iface) {
@@ -1383,9 +1419,15 @@ construct_libvirt_xml_disk (guestfs_h *g,
         if (drv->src.username != NULL) {
           start_element ("auth") {
             attribute ("username", drv->src.username);
-            /* TODO: write the drive secret, after first storing it separately
-             * in libvirt
-             */
+            r = find_secret (g, data, drv, &type, &uuid);
+            if (r == -1)
+              return -1;
+            if (r == 1) {
+              start_element ("secret") {
+                attribute ("type", type);
+                attribute ("uuid", uuid);
+              } end_element ();
+            }
           } end_element ();
         }
         break;
@@ -1657,6 +1699,174 @@ construct_libvirt_xml_qemu_cmdline (guestfs_h *g,
 }
 
 static int
+construct_libvirt_xml_secret (guestfs_h *g,
+                              const struct backend_libvirt_data *data,
+                              const struct drive *drv,
+                              xmlTextWriterPtr xo)
+{
+  start_element ("secret") {
+    attribute ("ephemeral", "yes");
+    attribute ("private", "yes");
+    start_element ("description") {
+      string_format ("guestfs secret associated with %s %s",
+                     data->name, drv->src.u.path);
+    } end_element ();
+  } end_element ();
+
+  return 0;
+}
+
+/* If drv->src.secret != NULL, store the secret in libvirt, and save
+ * the UUID so we can retrieve it later.  Also there is some slight
+ * variation depending on the protocol.  See
+ * http://libvirt.org/formatsecret.html
+ */
+static int
+add_secret (guestfs_h *g, struct backend_libvirt_data *data, const struct drive
*drv)
+{
+  CLEANUP_XMLBUFFERFREE xmlBufferPtr xb = NULL;
+  xmlOutputBufferPtr ob;
+  CLEANUP_XMLFREETEXTWRITER xmlTextWriterPtr xo = NULL;
+  CLEANUP_FREE xmlChar *xml = NULL;
+  CLEANUP_VIRSECRETFREE virSecretPtr secret = NULL;
+  size_t i;
+
+  if (drv->src.secret == NULL)
+    return 0;
+
+  /* If it was already stored, don't create another secret. */
+  if (have_secret (g, data, drv))
+    return 0;
+
+  /* Create the XML for the secret. */
+  xb = xmlBufferCreate ();
+  if (xb == NULL) {
+    perrorf (g, "xmlBufferCreate");
+    return -1;
+  }
+  ob = xmlOutputBufferCreateBuffer (xb, NULL);
+  if (ob == NULL) {
+    perrorf (g, "xmlOutputBufferCreateBuffer");
+    return -1;
+  }
+  xo = xmlNewTextWriter (ob);
+  if (xo == NULL) {
+    perrorf (g, "xmlNewTextWriter");
+    return -1;
+  }
+
+  if (xmlTextWriterSetIndent (xo, 1) == -1 ||
+      xmlTextWriterSetIndentString (xo, BAD_CAST "  ") == -1) {
+    perrorf (g, "could not set XML indent");
+    return -1;
+  }
+  if (xmlTextWriterStartDocument (xo, NULL, NULL, NULL) == -1) {
+    perrorf (g, "xmlTextWriterStartDocument");
+    return -1;
+  }
+
+  if (construct_libvirt_xml_secret (g, data, drv, xo) == -1)
+    return -1;
+
+  if (xmlTextWriterEndDocument (xo) == -1) {
+    perrorf (g, "xmlTextWriterEndDocument");
+    return -1;
+  }
+  xml = xmlBufferDetach (xb);
+  if (xml == NULL) {
+    perrorf (g, "xmlBufferDetach");
+    return -1;
+  }
+
+  debug (g, "libvirt secret XML:\n%s", xml);
+
+  /* Pass the XML to libvirt. */
+  secret = virSecretDefineXML (data->conn, (const char *) xml, 0);
+  if (secret == NULL) {
+    libvirt_error (g, _("could not define libvirt secret"));
+    return -1;
+  }
+
+  /* Set the secret. */
+  if (virSecretSetValue (secret,
+                         (const unsigned char *) drv->src.secret,
+                         strlen (drv->src.secret),
+                         0) == -1) {
+    libvirt_error (g, _("could not set libvirt secret value"));
+    return -1;
+  }
+
+  /* Get back the UUID and save it in the private data. */
+  i = data->nr_secrets;
+  data->nr_secrets++;
+  data->secrets +    safe_realloc (g, data->secrets, sizeof (struct
secret) * data->nr_secrets);
+
+  data->secrets[i].secret = safe_strdup (g, drv->src.secret);
+
+  if (virSecretGetUUIDString (secret, data->secrets[i].uuid) == -1) {
+    libvirt_error (g, _("could not get UUID from libvirt secret"));
+    return -1;
+  }
+
+  return 0;
+}
+
+static int
+have_secret (guestfs_h *g,
+             const struct backend_libvirt_data *data, const struct drive *drv)
+{
+  size_t i;
+
+  if (drv->src.secret == NULL)
+    return 0;
+
+  for (i = 0; i < data->nr_secrets; ++i) {
+    if (STREQ (data->secrets[i].secret, drv->src.secret))
+      return 1;
+  }
+
+  return 0;
+}
+
+/* Find a secret previously stored in libvirt.  Returns the
+ * <secret type=... uuid=...> attributes.  This function returns -1
+ * if there was an error, 0 if there is no secret, and 1 if the
+ * secret was found and returned.
+ */
+static int
+find_secret (guestfs_h *g,
+             const struct backend_libvirt_data *data, const struct drive *drv,
+             const char **type, const char **uuid)
+{
+  size_t i;
+
+  if (drv->src.secret == NULL)
+    return 0;
+
+  for (i = 0; i < data->nr_secrets; ++i) {
+    if (STREQ (data->secrets[i].secret, drv->src.secret)) {
+      *uuid = data->secrets[i].uuid;
+
+      switch (drv->src.protocol) {
+      case drive_protocol_rbd:
+        *type = "ceph";
+        break;
+      case drive_protocol_iscsi:
+        *type = "iscsi";
+        break;
+      default:
+        *type = "volume"; /* ? */
+      }
+
+      return 1;
+    }
+  }
+
+  return 0;
+}
+
+static int
 is_blk (const char *path)
 {
   struct stat statbuf;
@@ -1678,6 +1888,7 @@ shutdown_libvirt (guestfs_h *g, void *datav, int
check_for_errors)
   struct backend_libvirt_data *data = datav;
   virConnectPtr conn = data->conn;
   virDomainPtr dom = data->dom;
+  size_t i;
   int ret = 0;
   int flags;
 
@@ -1710,6 +1921,12 @@ shutdown_libvirt (guestfs_h *g, void *datav, int
check_for_errors)
   free (data->network_bridge);
   data->network_bridge = NULL;
 
+  for (i = 0; i < data->nr_secrets; ++i)
+    free (data->secrets[i].secret);
+  free (data->secrets);
+  data->secrets = NULL;
+  data->nr_secrets = 0;
+
   return ret;
 }
 
@@ -1814,6 +2031,10 @@ hot_add_drive_libvirt (guestfs_h *g, void *datav,
     return -1;
   }
 
+  /* If the drive has an associated secret, store it in libvirt. */
+  if (add_secret (g, data, drv) == -1)
+    return -1;
+
   /* Create the XML for the new disk. */
   xml = construct_libvirt_xml_hot_add_disk (g, data, drv, drv_index);
   if (xml == NULL)
-- 
2.0.4

On Fri, Oct 31, 2014 at 11:04:25AM +0000, Richard W.M. Jones
wrote:
> Implement the GUESTFS_ADD_DRIVE_OPTS_SECRET argument of
> guestfs_add_drive_opts.  For libvirt we have to save the secret in
> libvirtd first, get a UUID, and then pass the UUID back through the
> domain XML.
> ---
>  src/launch-libvirt.c | 227
++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 224 insertions(+), 3 deletions(-)
> 
> diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
> index 7206b33..45e215c 100644
> --- a/src/launch-libvirt.c
> +++ b/src/launch-libvirt.c
> +static int
> +add_secret (guestfs_h *g, struct backend_libvirt_data *data, const struct
drive *drv)
> +{
> +  debug (g, "libvirt secret XML:\n%s", xml);
> +
> +  /* Pass the XML to libvirt. */
> +  secret = virSecretDefineXML (data->conn, (const char *) xml, 0);
> +  if (secret == NULL) {
> +    libvirt_error (g, _("could not define libvirt secret"));
> +    return -1;
> +  }
> +
> +  /* Set the secret. */
> +  if (virSecretSetValue (secret,
> +                         (const unsigned char *) drv->src.secret,
> +                         strlen (drv->src.secret),
> +                         0) == -1) {
> +    libvirt_error (g, _("could not set libvirt secret value"));
> +    return -1;
> +  }
So the value for 'secret' that we're passing into the
add_drive_opts()
method for the plain QEMU backend has to be base64 encoded, because
the value is passed as-is to QEMU on the CLI.

When the libvirt QEMU driver feches the secret value, it will apply
base64 encoding before adding it to the QEMU CLI. IOW, the virSecret
object must store the raw value, not the base64 value.

So I think you need to have a base64 decode step here before calling
virSecretSetValue.

We should probably also clarify the docs to say that the value passed
to add_drive_opts() should be the base64 encoded value of the secret.

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