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

[Peter Krempa]

On Thu, May 12, 2016 at 08:39:28 -0400, John Ferlan wrote:
> 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 at 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(-)

[...]

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

I think the correct approach would be that virSecretGetSecretString is
returning just the binary buffer and the code using it is handling the
correct encoding ... [1]

[...]


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

In that case you need a function that checks the buffer containing the
secret if it can be put in plaintext on the commandline. After that you
can treat it as a string and put it on the commandline. Of course if
it's not printable you need to report an error since there's no
alternate way to handle arbitrary strings.

[1] ... With the approach outlined above both iSCSI and RBD will have the
same semantics. You get it as a binary string. You encode (make sure
it's encoded) properly at the point where it's used and then pass it on
to qemu.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160512/4d2cfcb7/attachment-0001.sig>