----- "Daniel Veillard" <veillard@redhat.com> 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