Re: [Qemu-block] [Qemu-devel] [PATCH 18/18] rbd: New parameter key-secret

[Daniel P . Berrangé]

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 <address@hidden>
> ---
>  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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|