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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list