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(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(-)

> > 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?

>> 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.