Re: [Qemu-devel] [PATCH v2 1/3] rbd: add support for getting password from QCryptoSecret object

[Josh Durgin <jdurgin@redhat.com>]

On 12/21/2015 06:59 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 'passwordid' 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 driver=rbd,filename=rbd:pool/image:id=myname:\
>                 auth_supported=cephx,passwordid=secret0
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Looks good to me, thanks!

Reviewed-by: Josh Durgin <jdurgin@redhat.com>

> ---
>   block/rbd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index a60a19d..dc4db0a 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,27 @@ static char *qemu_rbd_parse_clientname(const char *conf, char *clientname)
>       return NULL;
>   }
>
> +
> +static int qemu_rbd_set_auth(rados_t cluster, const char *passwordid,
> +                             Error **errp)
> +{
> +    if (passwordid == 0) {
> +        return 0;
> +    }
> +
> +    gchar *secret = qcrypto_secret_lookup_as_base64(passwordid,
> +                                                    errp);
> +    if (!secret) {
> +        return -1;
> +    }
> +
> +    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 +321,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 *passwordid;
>       rados_t cluster;
>       rados_ioctx_t io_ctx;
>       int ret;
>
> +    passwordid = qemu_opt_get(opts, "passwordid");
> +
>       if (qemu_rbd_parsename(filename, pool, sizeof(pool),
>                              snap_buf, sizeof(snap_buf),
>                              name, sizeof(name),
> @@ -350,6 +375,11 @@ static int qemu_rbd_create(const char *filename, QemuOpts *opts, Error **errp)
>           return -EIO;
>       }
>
> +    if (qemu_rbd_set_auth(cluster, passwordid, errp) < 0) {
> +        rados_shutdown(cluster);
> +        return -EIO;
> +    }
> +
>       if (rados_connect(cluster) < 0) {
>           error_setg(errp, "error connecting");
>           rados_shutdown(cluster);
> @@ -423,6 +453,11 @@ static QemuOptsList runtime_opts = {
>               .type = QEMU_OPT_STRING,
>               .help = "Specification of the rbd image",
>           },
> +        {
> +            .name = "passwordid",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of secret providing the password",
> +        },
>           { /* end of list */ }
>       },
>   };
> @@ -436,6 +471,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 *passwordid;
>       QemuOpts *opts;
>       Error *local_err = NULL;
>       const char *filename;
> @@ -450,6 +486,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>
>       filename = qemu_opt_get(opts, "filename");
> +    passwordid = qemu_opt_get(opts, "passwordid");
>
>       if (qemu_rbd_parsename(filename, pool, sizeof(pool),
>                              snap_buf, sizeof(snap_buf),
> @@ -488,6 +525,11 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>           }
>       }
>
> +    if (qemu_rbd_set_auth(s->cluster, passwordid, errp) < 0) {
> +        r = -EIO;
> +        goto failed_shutdown;
> +    }
> +
>       /*
>        * Fallback to more conservative semantics if setting cache
>        * options fails. Ignore errors from setting rbd_cache because the
> @@ -919,6 +961,11 @@ static QemuOptsList qemu_rbd_create_opts = {
>               .type = QEMU_OPT_SIZE,
>               .help = "RBD object size"
>           },
> +        {
> +            .name = "passwordid",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of secret providing the password",
> +        },
>           { /* end of list */ }
>       }
>   };
>