9:23 a.m.
On Sun, Aug 16, 2009 at 10:48:00PM +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.)
What if we change our mind in some time, would there be any obstacle
to dynamically detecting things are not encrypted and switching to
a crypted file transparently? I'm just trying to make sure a decision
taken now won't become a obstacle on deployed instances if we ever
revisit the issue.
[...]
diff --git a/src/secret_driver.c b/src/secret_driver.c
new file mode 100644
index 0000000..d9e638c
--- /dev/null
+++ b/src/secret_driver.c
@@ -0,0 +1,1102 @@
[...]
+#define virSecretReportError(conn, code, fmt...) \
+ virReportErrorHelper(conn, VIR_FROM_SECRET, code, __FILE__, \
+ __FUNCTION__, __LINE__, fmt)
+
+#define secretLog(msg...) fprintf(stderr, msg)
argh, no please let's use the existing log mechanism and not push
more fprintf based debugging
#include "logging.h"
and
#define secretLog(level, msg, ...) \
virLogMessage(__FILE__, level, 0, msg, __VA_ARGS__)
and use one of the virLogPriority values for level
+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, *volume; /* May be NULL */
for structs I prefer one entry per line
char *description;
char *volume;
+};
cosmetic but in line with other code
[...]
+static virSecretEntryPtr *
+secretFind(virSecretDriverStatePtr driver, const char *uuid)
+{
+ virSecretEntryPtr *pptr, s;
+
+ for (pptr = &driver->secrets; (s = *pptr) != NULL; pptr = &s->next) {
Urgh, a test with an affectation side effect in the loop guard, please
clean this up, this is really hard to figure out
+ if (STREQ(s->id, uuid))
+ return pptr;
+ }
+ return NULL;
+}
+
+static int
+writeBase64Data(virConnectPtr conn, int fd, const char *field,
+ const void *data, size_t size)
+{
+ int ret = -1;
+ char *base64 = NULL;
+
+ if (writeString(conn, fd, field) < 0 || writeString(conn, fd, " ") <
0)
+ goto cleanup;
+
+ base64_encode_alloc(data, size, &base64);
where is base64_encode_alloc coming from ?
[...]
+static int
+saveSecrets(virConnectPtr conn, virSecretDriverStatePtr driver)
+{
+ const virSecretEntry *secret;
+ char *tmp_path = NULL;
+ int fd = -1, ret = -1;
+
+ if (virAsprintf(&tmp_path, "%sXXXXXX", driver->filename) < 0) {
+ virReportOOMError(conn);
+ goto cleanup;
+ }
+ fd = mkstemp (tmp_path);
Hum even if unencrypted, we should make sure of the mode of the file
beforehand... isn't there a safer equivalent (possibly made postable by
gnulib ?)
+ if (fd == -1) {
+ virReportSystemError (conn, errno, _("mkstemp(\"%s\")
failed"),
+ tmp_path);
+ goto cleanup;
+ }
+
+ for (secret = driver->secrets; secret != NULL;
+ secret = secret->next) {
+ if (!secret->ephemeral && writeSecret(conn, fd, secret) < 0)
+ goto cleanup;
+ }
+ close(fd);
+ fd = -1;
+ if (rename(tmp_path, driver->filename) < 0) {
+ virReportSystemError (conn, errno, _("rename(%s, %s) failed"),
tmp_path,
+ driver->filename);
+ goto cleanup;
+ }
Hum, the whole set smells fishy, we are creating a temp file, without
mode checked, then moving that file somewhere else. I would ratehr have
internal APIsdeling with a dump to memory and then a single open, safe
and directly to the driver->filename.
+static int
+parseBase64String(virConnectPtr conn, const char *base64, char **string)
+{
+ char *tmp;
+ size_t size;
+
+ if (!base64_decode_alloc(base64, strlen(base64), &tmp, &size)) {
where is base64_decode_alloc coming from ?
+ virSecretReportError(conn, VIR_ERR_INTERNAL_ERROR,
"%s",
+ _("invalid format of base64 in secret
storage"));
+ return -1;
+ }
+ if (tmp == NULL || VIR_ALLOC_N(*string, size + 1) < 0) {
+ virReportOOMError(conn);
+ VIR_FREE(tmp);
+ return -1;
+ }
+
+ memcpy(*string, tmp, size);
+ (*string)[size] = '\0';
+ VIR_FREE(tmp);
+ return 0;
+}
+static int
+parseKeyValue(virConnectPtr conn, virSecretEntryPtr *list,
+ virSecretEntryPtr *secret, const char *key, const char *value)
+{
+ virSecretEntryPtr s;
+
+ s = *secret;
+ if (s == NULL) {
+ if (VIR_ALLOC(s) < 0)
+ goto no_memory;
+ *secret = s;
+ }
+ if (STREQ(key, "id")) {
+ if (s->id != NULL) {
+ listInsert(list, s);
+ if (VIR_ALLOC(s) < 0)
+ goto no_memory;
+ *secret = s;
+ }
+ if (parseBase64String(conn, value, &s->id) < 0)
+ return -1;
+ } else if (STREQ(key, "ephemeral")) {
+ if (STREQ(value, "yes"))
+ s->ephemeral = 1;
+ else if (STREQ(value, "no"))
+ s->ephemeral = 0;
+ else
+ goto invalid;
here
+ } else if (STREQ(key, "private")) {
+ if (STREQ(value, "yes"))
+ s->private = 1;
+ else if (STREQ(value, "no"))
+ s->private = 0;
+ else
+ goto invalid;
and here, it's better to state why parsing failed rather than just let
the user guess. We have the info let's give it back
+ } else if (STREQ(key, "value")) {
+static int
+loadSecrets(virConnectPtr conn, virSecretDriverStatePtr driver,
+ virSecretEntryPtr *dest)
+{
+ int ret = -1, fd = -1;
+ struct stat st;
+ char *contents = NULL, *strtok_data = NULL, *strtok_first;
+ const char *key, *value;
+ virSecretEntryPtr secret = NULL, list = NULL;
+
+ if (stat(driver->filename, &st) < 0) {
+ if (errno == ENOENT)
+ return 0;
+ virReportSystemError (conn, errno, _("cannot stat '%s'"),
+ driver->filename);
+ goto cleanup;
+ }
+ if ((size_t)st.st_size != st.st_size || (size_t)(st.st_size + 1) == 0) {
+ virSecretReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
+ _("secrets file does not fit in memory"));
+ goto cleanup;
+ }
+
+ fd = open(driver->filename, O_RDONLY);
+ if (fd == -1) {
+ virReportSystemError (conn, errno, _("cannot open '%s'"),
+ driver->filename);
+ goto cleanup;
+ }
stat()/open() introduces a small race, I think open() and
then fdstat() is a bit cleaner, not a big deal though
+ if (VIR_ALLOC_N(contents, st.st_size + 1) < 0) {
+ virReportOOMError(conn);
+ goto cleanup;
+ }
+ if (saferead(fd, contents, st.st_size) != st.st_size) {
+ virReportSystemError (conn, errno, _("cannot read '%s'"),
+ driver->filename);
+ goto cleanup;
+ }
+ close(fd);
+ fd = -1;
contents[st.st_size] = 0;
needed here.
+ strtok_first = contents;
[...]
+static virSecretEntryPtr
+secretXMLParseNode(virConnectPtr conn, xmlDocPtr xml, xmlNodePtr root)
+{
+ xmlXPathContextPtr ctxt = NULL;
+ virSecretEntryPtr secret = NULL, ret = NULL;
+ char *prop;
+
+ 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 && STREQ(prop, "yes"))
+ secret->ephemeral = 1;
we should test for "no" and explicitely fail otherwise
if (prop != NULL) {
if (STREQ(prop, "yes")) {
secret->ephemeral = 1;
} else if (STREQ(prop, "no")) {
secret->ephemeral = 0;
} else {
raise an error
}
VIR_FREE(prop);
}
+ VIR_FREE(prop);
+
+ prop = virXPathString(conn, "string(./@private)", ctxt);
+ if (prop != NULL && STREQ(prop, "yes"))
+ secret->private = 1;
+ VIR_FREE(prop);
same here
+ 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:
+ secretFree(secret);
+ xmlXPathFreeContext(ctxt);
+ return ret;
+}
[...]
[...]
+ restore_backup:
+ /* Error - restore previous state and free new attributes */
+ shallowCopyAttributes(secret, &backup);
+ if (secret_is_new) {
+ /* "secret" was added to the head of the list above */
+ if (listUnlink(&driverState->secrets) != secret)
+ /* abort() instead? */
no we can't abort() in the library
+ virSecretReportError(conn, VIR_ERR_INTERNAL_ERROR,
"%s",
+ _("list of secrets is inconsistent"));
+ else
+ secretFree(secret);
+ }
+
+ cleanup:
+ secretFree(new_attrs);
+ secretDriverUnlock(driver);
+
+ return ret;
+}
+static int
+secretSetValue(virSecretPtr obj, const unsigned char *value,
+ size_t value_size)
+{
+ virSecretDriverStatePtr driver = obj->conn->secretPrivateData;
+ int ret = -1;
+ unsigned char *old_value, *new_value;
+ size_t old_value_size;
+ virSecretEntryPtr secret, *pptr;
+
+ if (VIR_ALLOC_N(new_value, value_size) < 0) {
+ virReportOOMError(obj->conn);
+ return -1;
+ }
+
+ secretDriverLock(driver);
+
+ pptr = secretFind(driver, obj->uuid);
+ if (pptr == NULL) {
+ virSecretReportError(obj->conn, VIR_ERR_NO_SECRET,
+ _("no secret with matching id '%s'"),
obj->uuid);
+ goto cleanup;
+ }
I would be tempted to add an immutable flag to a secret, basically
this would allow to set the value once but forbid ever changing it
we would just test here that secret->value is NULL before allowing to
set it if immutable.
But it's easy to add later if needed.
+ secret = *pptr;
+
+ old_value = secret->value;
+ old_value_size = secret->value_size;
+
So there is a few points to check IMHO, also I would like to
understand where the base64 encoding/decoding routines come from,
actually I would prefer to have them in utils.[ch] and with function
names matching the existing conventions.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/