Re: [libvirt] [PATCH 2/3] secret: Alter virSecretGetSecretString

On Thu, May 12, 2016 at 07:49:31 -0400, John Ferlan wrote: > Rather than returning a "char *" indicating perhaps some sized set of > characters that is NUL terminated, return the value as "uint8_t *" > indicating a stream of raw bytes. In doing so, we also need to return > the size of the secret returned. > > Alter the callers to handle the adjusted model. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > src/libxl/libxl_conf.c | 18 +++++++++++------- > src/qemu/qemu_command.c | 7 ++++--- > src/qemu/qemu_domain.c | 5 +++-- > src/qemu/qemu_domain.h | 3 ++- > src/secret/secret_util.c | 19 +++++++++++++++---- > src/secret/secret_util.h | 13 +++++++------ > 6 files changed, 42 insertions(+), 23 deletions(-) > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index d927b37..e7ea320 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -939,7 +939,8 @@ libxlDomainGetEmulatorType(const virDomainDef *def) > static char * > libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, > const char *username, > - const char *secret) > + const uint8_t *secret, > + size_t secretlen) > { > char *ret = NULL; > virBuffer buf = VIR_BUFFER_INITIALIZER; > @@ -974,9 +975,9 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, > > if (username) { > virBufferEscape(&buf, '\\', ":", ":id=%s", username); > - virBufferEscape(&buf, '\\', ":", > - ":key=%s:auth_supported=cephx\\;none", > - secret); > + virBufferEscapeSizedString(&buf, '\\', ":", > + ":key=%s:auth_supported=cephx\\;none", > + secret, secretlen); This is base64 encoded thus not binary. So it's definitely not necessary here. > } else { > virBufferAddLit(&buf, ":auth_supported=none"); > } > @@ -1034,11 +1036,13 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) > protocol, > true, > src->auth, > - VIR_SECRET_USAGE_TYPE_CEPH))) > + VIR_SECRET_USAGE_TYPE_CEPH, > + &secretlen))) > goto cleanup; > } > > - if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, secret))) > + if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, > + secret, secretlen))) Not needed here, you can just discard the length.

> goto cleanup; > > ret = 0; > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 7e39b8a..fd7ce72 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -671,9 +671,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, > case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: > virBufferEscape(buf, '\\', ":", ":id=%s", > secinfo->s.plain.username); > - virBufferEscape(buf, '\\', ":", > - ":key=%s:auth_supported=cephx\\;none", > - secinfo->s.plain.secret); > + virBufferEscapeSizedString(buf, '\\', ":", > + ":key=%s:auth_supported=cephx\\;none", > + secinfo->s.plain.secret, > + secinfo->s.plain.secretlen); Same here. It makes no sense to use it here. > break; > > case VIR_DOMAIN_SECRET_INFO_TYPE_IV: > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 3da0079..98ab55fc 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -731,7 +731,7 @@ static void > qemuDomainSecretPlainFree(qemuDomainSecretPlain secret) > { > VIR_FREE(secret.username); > - memset(secret.secret, 0, strlen(secret.secret)); > + memset(secret.secret, 0, secret.secretlen); > VIR_FREE(secret.secret); > } > > @@ -886,7 +886,8 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, > > if (!(secinfo->s.plain.secret = > virSecretGetSecretString(conn, protocolstr, encode, > - authdef, secretType))) > + authdef, secretType, > + &secinfo->s.plain.secretlen))) > return -1; > > return 0; > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index c711188..a03bdc5 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h > @@ -251,7 +251,8 @@ typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain; > typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr; > struct _qemuDomainSecretPlain { > char *username; > - char *secret; > + uint8_t *secret; > + size_t secretlen; > }; > > # define QEMU_DOMAIN_IV_KEY_LEN 16 /* 16 bytes for 128 bit random */ > diff --git a/src/secret/secret_util.c b/src/secret/secret_util.c > index 217584f..edc1104 100644 > --- a/src/secret/secret_util.c > +++ b/src/secret/secret_util.c > @@ -41,6 +41,7 @@ VIR_LOG_INIT("secret.secret_util"); > * @encoded: Whether the returned secret needs to be base64 encoded > * @authdef: Pointer to the disk storage authentication > * @secretUsageType: Type of secret usage for authdef lookup > + * @ret_secret_size: Return size of the secret - either raw text or base64 > * > * Lookup the secret for the authdef usage type and return it either as > * raw text or encoded based on the caller's need. > @@ -48,17 +49,19 @@ VIR_LOG_INIT("secret.secret_util"); > * Returns a pointer to memory that needs to be cleared and free'd after > * usage or NULL on error. > */ > -char * > +uint8_t * > virSecretGetSecretString(virConnectPtr conn, > const char *scheme, > bool encoded, Shouldn't we drop this argument altogether and encode the data to base64 when it's used? That way this will work for all general secrets without the duality that the returned bufer is either binary or base64 encoded and for other encodings it will be encoded at the point where it's used.

> virStorageAuthDefPtr authdef, > - virSecretUsageType secretUsageType) > + virSecretUsageType secretUsageType, > + size_t *ret_secret_size) I think that function should at this point return an int and both the buffer and the size of the data should be returned using arguments.