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?

> +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 ?

> + 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.