On 05/12/2016 08:26 AM, Peter Krempa wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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.
>
Ironically what I had originally; however, in an oh sh* moment I altered
things... It does mean though, that the "secret" in both instances will
either need to be "memcpy()'d" into an allocated "char *" buffer or just
cast (const char *). Is there a preference?
>> 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.
>
iSCSI uses/expects the unencoded value - that's one of the factors
behind the need for IV/AES secrets. Unfortunately, the insane way one
has to add the secret to the command line via the '-iscsi' option makes
it impossible since there's no way to add a unique "-id" as well without
a colon (and/or slash) since that's the way an IQN is described
(iqn.date.reverse-host-name:storage-specific-string).
The current algorithm for iSCSI in QEMU uses that plaintext password
instead of an encoded one. Competing requirements.
>> 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.
>
OK - easy enough to handle.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list