| Message ID | 1445267389-21846-4-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <berrange@redhat.com> > --- 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 */ } > } > };
On Mon, Oct 19, 2015 at 03:57:29PM -0700, Josh Durgin wrote: > 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 <berrange@redhat.com> > >--- > > 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. This failure scenario happens if the user provides a key ID, but the corresponding QCryptoSecret does not exist. This is a usage error by the user, so it is appropriate to have it be a fatal error. In the case that they don't want any auth, they can just leave out the keyid parameter. Regards, Daniel
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; + } + + 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 */ } } };
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 <berrange@redhat.com> --- block/rbd.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)