From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41719) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoJN4-0005Vk-9S for qemu-devel@nongnu.org; Mon, 19 Oct 2015 18:57:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoJN3-0001ex-5w for qemu-devel@nongnu.org; Mon, 19 Oct 2015 18:57:38 -0400 References: <1445267389-21846-1-git-send-email-berrange@redhat.com> <1445267389-21846-4-git-send-email-berrange@redhat.com> From: Josh Durgin Message-ID: <56257559.1060001@redhat.com> Date: Mon, 19 Oct 2015 15:57:29 -0700 MIME-Version: 1.0 In-Reply-To: <1445267389-21846-4-git-send-email-berrange@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 03/17] rbd: add support for getting password from QCryptoSecret object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Kevin Wolf , Ronnie Sahlberg , qemu-block@nongnu.org, Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini 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 > --- 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 > > @@ -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 */ } > } > };