----- "Daniel Veillard" <veillard@xxxxxxxxxx> wrote:
> 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?
Two simple ways:
1) use a different file name, e,g. secrets.gpg
2) check the start of the file, currently the file always starts with "id ";
I can add a proper magic number if necessary.
> > +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 ?
gnulib - in a later patch I'm afraid, I'll reorder it.
> > +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...
Good point.
> isn't there a safer equivalent (possibly made postable by gnulib ?)
Grepping of gnulib does not reveal any.
> > + 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.
"Somewhere else" is in the same directory. The mkstemp() + rename() is used to make sure the secrets file is replaced atomically, without losing any data if a save is interrupted in the middle.
> > +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 ?
gnulib
> > + 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
Technically there is still a race with fstat() - but fstat is a bit better.
> > + 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.
VIR_ALLOC_N automatically zeroes the memory.
I'll fix all other issues you have mentioned. Thanks for the review.
Mirek
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list