| Message ID | 20180612125821.4229-19-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Configuration fixes and rbd authentication | expand |
On Tue, Jun 12, 2018 at 02:58:21PM +0200, Markus Armbruster wrote: > Legacy -drive supports "password-secret" parameter that isn't > available with -blockdev / blockdev-add. That's because we backed out > our first try to provide it there due to interface design doubts, in > commit 577d8c9a811, v2.9.0. > > This is the second try. It brings back the parameter, except it's > named "key-secret" now. > > Let's review our reasons for backing out the first try, as stated in > the commit message: > > * BlockdevOptionsRbd member @password-secret isn't actually a > password, it's a key generated by Ceph. I thought about that when I first added password-secret, but felt that it is still effectively acting as a password to authenticate to the server, and calling it password-secret made it clearer that it was related to the authentication phase, and not for example, disk encryption. > > Addressed by the rename. > > * We're not sure where member @password-secret belongs (see the > previous commit). > > See previous commit. > > * How @password-secret interacts with settings from a configuration > file specified with @conf is undocumented. > > Not actually true, the documentation for @conf says "Values in the > configuration file will be overridden by options specified via QAPI", > and we've tested this. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/rbd.c | 41 +++++++++++++++++++++++++---------------- > qapi/block-core.json | 6 ++++++ > 2 files changed, 31 insertions(+), 16 deletions(-) > > diff --git a/block/rbd.c b/block/rbd.c > index ea0575d068..f2c6965418 100644 > --- a/block/rbd.c > +++ b/block/rbd.c > @@ -239,24 +239,25 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp) > } > > > -static int qemu_rbd_set_auth(rados_t cluster, const char *secretid, > - BlockdevOptionsRbd *opts, > +static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts, > Error **errp) > { > - char *acr; > + char *key, *acr; > int r; > GString *accu; > RbdAuthModeList *auth; > > - if (secretid) { > - gchar *secret = qcrypto_secret_lookup_as_base64(secretid, > - errp); > - if (!secret) { > - return -1; > + if (opts->key_secret) { > + key = qcrypto_secret_lookup_as_base64(opts->key_secret, errp); > + if (!key) { > + return -EIO; > + } > + r = rados_conf_set(cluster, "key", key); > + g_free(key); > + if (r < 0) { > + error_setg_errno(errp, -r, "Could not set 'key'"); > + return r; > } > - > - rados_conf_set(cluster, "key", secret); > - g_free(secret); > } > > if (opts->has_auth_client_required) { > @@ -367,9 +368,7 @@ static QemuOptsList runtime_opts = { > }, > }; > > -/* FIXME Deprecate and remove keypairs or make it available in QMP. > - * password_secret should eventually be configurable in opts->location. Support > - * for it in .bdrv_open will make it work here as well. */ > +/* FIXME Deprecate and remove keypairs or make it available in QMP. */ > static int qemu_rbd_do_create(BlockdevCreateOptions *options, > const char *keypairs, const char *password_secret, > Error **errp) > @@ -575,6 +574,16 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, > Error *local_err = NULL; > int r; > > + if (secretid) { > + if (opts->key_secret) { > + error_setg(errp, > + "Legacy 'password-secret' clashes with 'key-secret'"); > + return -EINVAL; > + } > + opts->key_secret = g_strdup(secretid); > + opts->has_key_secret = true; > + } > + > mon_host = qemu_rbd_mon_host(opts, &local_err); > if (local_err) { > error_propagate(errp, local_err); > @@ -607,8 +616,8 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, > } > } > > - if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) { > - r = -EIO; > + r = qemu_rbd_set_auth(*cluster, opts, errp); > + if (r < 0) { > goto failed_shutdown; > } > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 841d196a21..3eb536de5b 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3120,6 +3120,11 @@ > # This maps to Ceph configuration option > # "auth_client_required". (Since 3.0) > # > +# @key-secret: ID of a QCryptoSecret object providing a key > +# for cephx authentication. > +# This maps to Ceph configuration option > +# "key". (Since 3.0) > +# > # @server: Monitor host address and port. This maps > # to the "mon_host" Ceph option. > # > @@ -3132,6 +3137,7 @@ > '*snapshot': 'str', > '*user': 'str', > '*auth-client-required': ['RbdAuthMode'], > + '*key-secret': 'str', > '*server': ['InetSocketAddressBase'] } } > > ## > -- > 2.17.1 > > Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Jun 12, 2018 at 02:58:21PM +0200, Markus Armbruster wrote: >> Legacy -drive supports "password-secret" parameter that isn't >> available with -blockdev / blockdev-add. That's because we backed out >> our first try to provide it there due to interface design doubts, in >> commit 577d8c9a811, v2.9.0. >> >> This is the second try. It brings back the parameter, except it's >> named "key-secret" now. >> >> Let's review our reasons for backing out the first try, as stated in >> the commit message: >> >> * BlockdevOptionsRbd member @password-secret isn't actually a >> password, it's a key generated by Ceph. > > I thought about that when I first added password-secret, but felt > that it is still effectively acting as a password to authenticate > to the server, and calling it password-secret made it clearer that > it was related to the authentication phase, and not for example, > disk encryption. I feel it's best to stick to the names Ceph uses, and Ceph calls it "key". [...]
diff --git a/block/rbd.c b/block/rbd.c index ea0575d068..f2c6965418 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -239,24 +239,25 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp) } -static int qemu_rbd_set_auth(rados_t cluster, const char *secretid, - BlockdevOptionsRbd *opts, +static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts, Error **errp) { - char *acr; + char *key, *acr; int r; GString *accu; RbdAuthModeList *auth; - if (secretid) { - gchar *secret = qcrypto_secret_lookup_as_base64(secretid, - errp); - if (!secret) { - return -1; + if (opts->key_secret) { + key = qcrypto_secret_lookup_as_base64(opts->key_secret, errp); + if (!key) { + return -EIO; + } + r = rados_conf_set(cluster, "key", key); + g_free(key); + if (r < 0) { + error_setg_errno(errp, -r, "Could not set 'key'"); + return r; } - - rados_conf_set(cluster, "key", secret); - g_free(secret); } if (opts->has_auth_client_required) { @@ -367,9 +368,7 @@ static QemuOptsList runtime_opts = { }, }; -/* FIXME Deprecate and remove keypairs or make it available in QMP. - * password_secret should eventually be configurable in opts->location. Support - * for it in .bdrv_open will make it work here as well. */ +/* FIXME Deprecate and remove keypairs or make it available in QMP. */ static int qemu_rbd_do_create(BlockdevCreateOptions *options, const char *keypairs, const char *password_secret, Error **errp) @@ -575,6 +574,16 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, Error *local_err = NULL; int r; + if (secretid) { + if (opts->key_secret) { + error_setg(errp, + "Legacy 'password-secret' clashes with 'key-secret'"); + return -EINVAL; + } + opts->key_secret = g_strdup(secretid); + opts->has_key_secret = true; + } + mon_host = qemu_rbd_mon_host(opts, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -607,8 +616,8 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t *io_ctx, } } - if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) { - r = -EIO; + r = qemu_rbd_set_auth(*cluster, opts, errp); + if (r < 0) { goto failed_shutdown; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 841d196a21..3eb536de5b 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3120,6 +3120,11 @@ # This maps to Ceph configuration option # "auth_client_required". (Since 3.0) # +# @key-secret: ID of a QCryptoSecret object providing a key +# for cephx authentication. +# This maps to Ceph configuration option +# "key". (Since 3.0) +# # @server: Monitor host address and port. This maps # to the "mon_host" Ceph option. # @@ -3132,6 +3137,7 @@ '*snapshot': 'str', '*user': 'str', '*auth-client-required': ['RbdAuthMode'], + '*key-secret': 'str', '*server': ['InetSocketAddressBase'] } } ##
Legacy -drive supports "password-secret" parameter that isn't available with -blockdev / blockdev-add. That's because we backed out our first try to provide it there due to interface design doubts, in commit 577d8c9a811, v2.9.0. This is the second try. It brings back the parameter, except it's named "key-secret" now. Let's review our reasons for backing out the first try, as stated in the commit message: * BlockdevOptionsRbd member @password-secret isn't actually a password, it's a key generated by Ceph. Addressed by the rename. * We're not sure where member @password-secret belongs (see the previous commit). See previous commit. * How @password-secret interacts with settings from a configuration file specified with @conf is undocumented. Not actually true, the documentation for @conf says "Values in the configuration file will be overridden by options specified via QAPI", and we've tested this. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/rbd.c | 41 +++++++++++++++++++++++++---------------- qapi/block-core.json | 6 ++++++ 2 files changed, 31 insertions(+), 16 deletions(-)