[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 07/20] Secret manipulation step 7: Local driver



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 veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]