Re: [Qemu-devel] [PATCH 03/17] rbd: add support for getting password from QCryptoSecret object

[Josh Durgin]

On 10/19/2015 08:09 AM, Daniel P. Berrange wrote:
> Currently RBD passwords must be provided on the command line
> via
>
>    $QEMU -drive file=rbd:pool/image:id=myname:\
>      key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\
>      auth_supported=cephx
>
> This is insecure because the key is visible in the OS process
> listing.
>
> This adds support for an 'authsecret' parameter in the RBD
> parameters that can be used with the QCryptoSecret object to
> provide the password via a file:
>
>    echo "QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=" > poolkey.b64
>    $QEMU -object secret,id=secret0,file=poolkey.b64,format=base64 \
>        -drive file=rbd:pool/image:id=myname:\
>        auth_supported=cephx,authsecret=secret0
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---

Looks good in general, thanks for fixing this! Just one thing to fix
below.

>   block/rbd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index a60a19d..0acf777 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -16,6 +16,7 @@
>   #include "qemu-common.h"
>   #include "qemu/error-report.h"
>   #include "block/block_int.h"
> +#include "crypto/secret.h"
>
>   #include <rbd/librbd.h>
>
> @@ -228,6 +229,23 @@ static char *qemu_rbd_parse_clientname(const char *conf, 
> char *clientname)
>       return NULL;
>   }
>
> +
> +static int qemu_rbd_set_auth(rados_t cluster, const char *secretid,
> +                             Error **errp)
> +{
> +    gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
> +                                                    errp);
> +    if (!secret) {
> +        return -1;
> +    }

It looks like this fails if no authsecret is provided. Ceph auth can be
disabled, so it seems like we should skip the qemu_rbd_set_auth() calls
in this case.

> +
> +    rados_conf_set(cluster, "key", secret);
> +    g_free(secret);
> +
> +    return 0;
> +}
> +
> +
>   static int qemu_rbd_set_conf(rados_t cluster, const char *conf,
>                                bool only_read_conf_file,
>                                Error **errp)
> @@ -299,10 +317,13 @@ static int qemu_rbd_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>       char conf[RBD_MAX_CONF_SIZE];
>       char clientname_buf[RBD_MAX_CONF_SIZE];
>       char *clientname;
> +    const char *secretid;
>       rados_t cluster;
>       rados_ioctx_t io_ctx;
>       int ret;
>
> +    secretid = qemu_opt_get(opts, "authsecret");
> +
>       if (qemu_rbd_parsename(filename, pool, sizeof(pool),
>                              snap_buf, sizeof(snap_buf),
>                              name, sizeof(name),
> @@ -350,6 +371,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>           return -EIO;
>       }
>
> +    if (qemu_rbd_set_auth(cluster, secretid, errp) < 0) {
> +        rados_shutdown(cluster);
> +        return -EIO;
> +    }
> +
>       if (rados_connect(cluster) < 0) {
>           error_setg(errp, "error connecting");
>           rados_shutdown(cluster);
> @@ -423,6 +449,11 @@ static QemuOptsList runtime_opts = {
>               .type = QEMU_OPT_STRING,
>               .help = "Specification of the rbd image",
>           },
> +        {
> +            .name = "authsecret",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of secret providing the password",
> +        },
>           { /* end of list */ }
>       },
>   };
> @@ -436,6 +467,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>       char conf[RBD_MAX_CONF_SIZE];
>       char clientname_buf[RBD_MAX_CONF_SIZE];
>       char *clientname;
> +    const char *secretid;
>       QemuOpts *opts;
>       Error *local_err = NULL;
>       const char *filename;
> @@ -450,6 +482,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>       }
>
>       filename = qemu_opt_get(opts, "filename");
> +    secretid = qemu_opt_get(opts, "authsecret");
>
>       if (qemu_rbd_parsename(filename, pool, sizeof(pool),
>                              snap_buf, sizeof(snap_buf),
> @@ -488,6 +521,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>           }
>       }
>
> +    if (qemu_rbd_set_auth(s->cluster, secretid, errp) < 0) {
> +        goto failed_shutdown;
> +    }
> +
>       /*
>        * Fallback to more conservative semantics if setting cache
>        * options fails. Ignore errors from setting rbd_cache because the
> @@ -919,6 +956,11 @@ static QemuOptsList qemu_rbd_create_opts = {
>               .type = QEMU_OPT_SIZE,
>               .help = "RBD object size"
>           },
> +        {
> +            .name = "authsecret",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of secret providing the password",
> +        },
>           { /* end of list */ }
>       }
>   };