On Thu, Aug 20, 2009 at 08:18:05PM +0200, Miloslav Trma?? wrote:
> This implementation stores the secrets in an unencrypted text file,
> for simplicity in implementation and debugging.
>
> (Symmetric encryption, e.g. using gpgme, will not be difficult to add.
> Because the TLS private key used by libvirtd is stored unencrypted,
> encrypting the secrets file does not currently provide much additional
> security.)
Looking at this in more detail now, I think we ought to change the way
the simple file store works. WIth this code all the secrets end up in
one big file
id NzFkMDY4ZjMtMzU0ZS0xYTUxLTI0MWMtNWJmNmQ1ZjY4Zjhk
ephemeral no
private yes
description TFVLUyBwYXNzcGhyYXNlIGZvciBvdXIgbWFpbCBzZXJ2ZXI=
volume L2hvbWUvYmVycmFuZ2UvbWFpbC5pbWc=
id YTY3Yzk0YWMtN2RiNi1iNjYxLWIwNGMtMDU4YzFlYWMyNTEy
ephemeral no
private yes
description TFVLUyBwYXNzcGhyYXNlIGZvciBvdXIgbWFpbCBzZXJ2ZXI=
volume L2hvbWUvYmVycmFuZ2UvbWFpbC5pbWc=
id MWI0ZmRlNmYtMDkyOS0zYmI1LWMyYWEtMGY2MmVmNzllNTJh
ephemeral no
private yes
Since this is re-written to disk everytime something is changed, it
poses quite a risk of data loss. It also means we've got yet another
file format parser/formatter.
How about doing something arguably even simpler
$LIBVIRT_CONFIG_DIR
|
+- secrets
|
+- 7c2c8591-eb89-d373-3bcf-0957b84210fc.xml
+- 7c2c8591-eb89-d373-3bcf-0957b84210fc.base64
+- 3174ee25-1be0-ba8f-1c99-d0b929226d7e.xml
+- 3174ee25-1be0-ba8f-1c99-d0b929226d7e.base64
ie, a directory 'secrets', and in that directory the 'XXX.xml' file
is just the stuff passed in from virSecretDefineXML. The XXX.base64
file contains just the bae64 encoded secret value, or simply does
not exist if no secret has been set yet.
This means we never have to worry about re-writing existing files.
Using the existing XML format for the metadata means we don't need
another file format parser. Finally having the secret value separate
in a .base64 file means we can read/write it without the secret data
being copied around all over the place by the XML parser, and can
trivially switch to gpgme later, by just using XXXX.gpg for the
secret data - which is nicely upgradable too - since code can simply
check for XXX.gpg, and fallback to XXX.base64 if it wasn't found.
> +
> +typedef struct _virSecretEntry virSecretEntry;
> +typedef virSecretEntry *virSecretEntryPtr;
> +struct _virSecretEntry {
> + virSecretEntryPtr next;
> + char *id; /* We generate UUIDs, but don't restrict user-chosen IDs */
> + unsigned char *value; /* May be NULL */
> + size_t value_size;
> + unsigned ephemeral : 1;
> + unsigned private : 1;
> + char *description; /* May be NULL */
> + char *volume; /* May be NULL */
> +};
This struct and the parse/format methods just below here...
> +
> +static virSecretEntryPtr
> +secretXMLParseNode(virConnectPtr conn, xmlDocPtr xml, xmlNodePtr root)
> +{
> + xmlXPathContextPtr ctxt = NULL;
> + virSecretEntryPtr secret = NULL, ret = NULL;
> + char *prop = NULL;
> +
> + if (!xmlStrEqual(root->name, BAD_CAST "secret")) {
> + virSecretReportError(conn, VIR_ERR_XML_ERROR, "%s",
> + _("incorrect root element"));
> + goto cleanup;
> + }
> +
> + ctxt = xmlXPathNewContext(xml);
> + if (ctxt == NULL) {
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> + ctxt->node = root;
> +
> + if (VIR_ALLOC(secret) < 0) {
> + virReportOOMError(conn);
> + goto cleanup;
> + }
> +
> + prop = virXPathString(conn, "string(./@ephemeral)", ctxt);
> + if (prop != NULL) {
> + if (STREQ(prop, "yes"))
> + secret->ephemeral = 1;
> + else if (STREQ(prop, "no"))
> + secret->ephemeral = 0;
> + else {
> + virSecretReportError(conn, VIR_ERR_XML_ERROR, "%s",
> + _("invalid value of 'ephemeral'"));
> + goto cleanup;
> + }
> + VIR_FREE(prop);
> + }
> +
> + prop = virXPathString(conn, "string(./@private)", ctxt);
> + if (prop != NULL) {
> + if (STREQ(prop, "yes"))
> + secret->private = 1;
> + else if (STREQ(prop, "no"))
> + secret->private = 0;
> + else {
> + virSecretReportError(conn, VIR_ERR_XML_ERROR, "%s",
> + _("invalid value of 'private'"));
> + goto cleanup;
> + }
> + VIR_FREE(prop);
> + }
> +
> + secret->id = virXPathString(conn, "string(./uuid)", ctxt);
> + secret->description = virXPathString(conn, "string(./description)", ctxt);
> + secret->volume = virXPathString(conn, "string(./volume)", ctxt);
> +
> + ret = secret;
> + secret = NULL;
> +
> + cleanup:
> + VIR_FREE(prop);
> + secretFree(secret);
> + xmlXPathFreeContext(ctxt);
> + return ret;
> +}
> +
> +/* Called from SAX on parsing errors in the XML. */
> +static void
> +catchXMLError(void *ctx, const char *msg ATTRIBUTE_UNUSED, ...)
> +{
> + xmlParserCtxtPtr ctxt = (xmlParserCtxtPtr) ctx;
> +
> + if (ctxt) {
> + virConnectPtr conn = ctxt->_private;
> +
> + if (virGetLastError() == NULL &&
> + ctxt->lastError.level == XML_ERR_FATAL &&
> + ctxt->lastError.message != NULL) {
> + virSecretReportError(conn, VIR_ERR_XML_DETAIL, _("at line %d: %s"),
> + ctxt->lastError.line, ctxt->lastError.message);
> + }
> + }
> +}
> +
> +static virSecretEntryPtr
> +secretXMLParseString(virConnectPtr conn, const char *xmlStr)
> +{
> + xmlParserCtxtPtr pctxt;
> + xmlDocPtr xml = NULL;
> + xmlNodePtr root;
> + virSecretEntryPtr ret = NULL;
> +
> + pctxt = xmlNewParserCtxt();
> + if (pctxt == NULL || pctxt->sax == NULL)
> + goto cleanup;
> + pctxt->sax->error = catchXMLError;
> + pctxt->_private = conn;
> +
> + xml = xmlCtxtReadDoc(pctxt, BAD_CAST xmlStr, "secret.xml", NULL,
> + XML_PARSE_NOENT | XML_PARSE_NONET |
> + XML_PARSE_NOWARNING);
> + if (xml == NULL) {
> + if (conn->err.code == VIR_ERR_NONE)
> + virSecretReportError(conn, VIR_ERR_XML_ERROR, "%s",
> + _("failed to parse xml document"));
> + goto cleanup;
> + }
> +
> + root = xmlDocGetRootElement(xml);
> + if (root == NULL) {
> + virSecretReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing root element"));
> + goto cleanup;
> + }
> +
> + ret = secretXMLParseNode(conn, xml, root);
> +
> + cleanup:
> + xmlFreeDoc(xml);
> + xmlFreeParserCtxt(pctxt);
> + return ret;
> +}
> +
> +static char *
> +secretXMLFormat(virConnectPtr conn, const virSecretEntry *secret)
> +{
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + char *tmp;
> +
> + virBufferVSprintf(&buf, "<secret ephemeral='%s' private='%s'>\n",
> + secret->ephemeral ? "yes" : "no",
> + secret->private ? "yes" : "no");
> + virBufferEscapeString(&buf, " <uuid>%s</uuid>\n", secret->id);
> + if (secret->description != NULL)
> + virBufferEscapeString(&buf, " <description>%s</description>\n",
> + secret->description);
> + if (secret->volume != NULL)
> + virBufferEscapeString(&buf, " <volume>%s</volume>\n", secret->volume);
> + virBufferAddLit(&buf, "</secret>\n");
> +
> + if (virBufferError(&buf))
> + goto no_memory;
> +
> + return virBufferContentAndReset(&buf);
> +
> + no_memory:
> + virReportOOMError(conn);
> + tmp = virBufferContentAndReset(&buf);
> + VIR_FREE(tmp);
> + return NULL;
> +}
should be moved into a separate 'secret_conf.h' and 'secret_conf.c'
file, and linked to the base libvirt_driver.la
This would let the later patches touching storage_backend.c and
qemu_driver.c to use the virSecretEntry struct directly, and
then just call the XML formattter/parsers, instead of having
to include their own copies of the XML formatting code.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list