[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




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 redhat com>
>> ---
>>  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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]