[libvirt] [PATCH 0/3] Secrets adjustments
Fallout from review of v3 of the IV Secrets: http://www.redhat.com/archives/libvir-list/2016-May/msg00444.html Add a new buffer escape API to handle printing a sized buffer Alter the existing fetch, store, and printing of the secret to be a "uint8_t *" rather than "(unsigned) char *". Change the name of SecretIV to SecretAES Figured I'd work through the getting the naming right first... John Ferlan (3): util: Introduce virBufferEscapeSizedBuf secret: Alter virSecretGetSecretString qemu: Change from SecretIV or _IV to SecretAES or _AES src/libvirt_private.syms | 1 + src/libxl/libxl_conf.c | 18 +++++++++------- src/qemu/qemu_command.c | 11 +++++----- src/qemu/qemu_domain.c | 11 +++++----- src/qemu/qemu_domain.h | 17 ++++++++------- src/secret/secret_util.c | 19 +++++++++++++---- src/secret/secret_util.h | 13 ++++++------ src/util/virbuffer.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 3 +++ 9 files changed, 112 insertions(+), 35 deletions(-) -- 2.5.5
Soon we will need to escape a buffer string that cannot use strlen, so introduce this API to allow printing/escaping of the entire buffer. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 3 +++ 3 files changed, 58 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a980a32..a0112cd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1215,6 +1215,7 @@ virBufferError; virBufferEscape; virBufferEscapeSexpr; virBufferEscapeShell; +virBufferEscapeSizedString; virBufferEscapeString; virBufferFreeAndReset; virBufferGetIndent; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index d582e7d..6985832 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -588,6 +588,60 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, VIR_FREE(escaped); } + +/** + * virBufferEscapeSizedString: + * @buf: the buffer to append to + * @escape: the escape character to inject + * @toescape: NUL-terminated list of characters to escape + * @format: a printf like format string but with only one %s parameter + * @bufstr: buffer of bytes which needs to be escaped + * @bufstrlen: the length of bufstr + * + * Do a formatted print with a size to a buffer. Any characters + * in the provided list that are contained in @bufstr are escaped with the + * given escape. Escaping is not applied to characters specified in @format. + * Auto indentation may be applied. + */ +void +virBufferEscapeSizedString(virBufferPtr buf, + char escape, + const char *toescape, + const char *format, + const uint8_t *bufstr, + size_t bufstrlen) +{ + char *escaped, *out; + const uint8_t *cur; + + if ((format == NULL) || (buf == NULL) || (bufstr == NULL)) + return; + + if (buf->error) + return; + + if (xalloc_oversized(2, bufstrlen) || + VIR_ALLOC_N_QUIET(escaped, 2 * bufstrlen + 1) < 0) { + virBufferSetError(buf, errno); + return; + } + + cur = bufstr; + out = escaped; + while (bufstrlen) { + if (strchr(toescape, *cur)) + *out++ = escape; + *out++ = *cur; + cur++; + bufstrlen--; + } + *out = 0; + + virBufferAsprintf(buf, format, escaped); + VIR_FREE(escaped); +} + + /** * virBufferURIEncodeString: * @buf: the buffer to append to diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index 144a1ba..6513d3e 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -87,6 +87,9 @@ void virBufferEscapeString(virBufferPtr buf, const char *format, void virBufferEscapeSexpr(virBufferPtr buf, const char *format, const char *str); void virBufferEscapeShell(virBufferPtr buf, const char *str); +void virBufferEscapeSizedString(virBufferPtr buf, char escape, + const char *toescape, const char *format, + const uint8_t *bufstr, size_t bufstrlen); void virBufferURIEncodeString(virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ -- 2.5.5
On Thu, May 12, 2016 at 07:49:30 -0400, John Ferlan wrote:
Soon we will need to escape a buffer string that cannot use strlen, so introduce this API to allow printing/escaping of the entire buffer.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 3 +++ 3 files changed, 58 insertions(+)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index d582e7d..6985832 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -588,6 +588,60 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, VIR_FREE(escaped); }
+ +/** + * virBufferEscapeSizedString:
I don't think this makes much sense. If the string contains nul '\0' bytes this won't help at all since it will copy the \0 byte to the resulting string. If the string doesn't contain those, then the previously existing code would work just fine. If you want to put such stuff to a commandline you need to encode it to base 64 anyways.
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); } else { virBufferAddLit(&buf, ":auth_supported=none"); } @@ -1018,7 +1019,8 @@ static int libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) { virConnectPtr conn = NULL; - char *secret = NULL; + uint8_t *secret = NULL; + size_t secretlen; char *username = NULL; int ret = -1; @@ -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))) 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); 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, virStorageAuthDefPtr authdef, - virSecretUsageType secretUsageType) + virSecretUsageType secretUsageType, + size_t *ret_secret_size) { size_t secret_size; virSecretPtr sec = NULL; char *secret = NULL; char uuidStr[VIR_UUID_STRING_BUFLEN]; + uint8_t *ret = NULL; /* look up secret */ switch (authdef->secretType) { @@ -105,7 +108,7 @@ virSecretGetSecretString(virConnectPtr conn, if (encoded) { char *base64 = NULL; - base64_encode_alloc(secret, secret_size, &base64); + secret_size = base64_encode_alloc(secret, secret_size, &base64); VIR_FREE(secret); if (!base64) { virReportOOMError(); @@ -114,7 +117,15 @@ virSecretGetSecretString(virConnectPtr conn, secret = base64; } + if (VIR_ALLOC_N(ret, secret_size) < 0) + goto cleanup; + + memcpy(ret, secret, secret_size); + *ret_secret_size = secret_size; + cleanup: virObjectUnref(sec); - return secret; + memset(secret, 0, secret_size); + VIR_FREE(secret); + return ret; } diff --git a/src/secret/secret_util.h b/src/secret/secret_util.h index c707599..4ac6031 100644 --- a/src/secret/secret_util.h +++ b/src/secret/secret_util.h @@ -25,11 +25,12 @@ # include "internal.h" # include "virstoragefile.h" -char *virSecretGetSecretString(virConnectPtr conn, - const char *scheme, - bool encoded, - virStorageAuthDefPtr authdef, - virSecretUsageType secretUsageType) +uint8_t *virSecretGetSecretString(virConnectPtr conn, + const char *scheme, + bool encoded, + virStorageAuthDefPtr authdef, + virSecretUsageType secretUsageType, + size_t *ret_secret_size) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) - ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(6) ATTRIBUTE_RETURN_CHECK; #endif /* __VIR_SECRET_H__ */ -- 2.5.5
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.
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. Peter
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
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@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
The preferred name will be AES not IV, change current references Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 6 +++--- src/qemu/qemu_domain.h | 14 +++++++------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fd7ce72..1375d96 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -638,7 +638,7 @@ qemuBuildGeneralSecinfoURI(virURIPtr uri, } break; - case VIR_DOMAIN_SECRET_INFO_TYPE_IV: + case VIR_DOMAIN_SECRET_INFO_TYPE_AES: case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: return -1; } @@ -677,7 +677,7 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, secinfo->s.plain.secretlen); break; - case VIR_DOMAIN_SECRET_INFO_TYPE_IV: + case VIR_DOMAIN_SECRET_INFO_TYPE_AES: case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: return -1; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 98ab55fc..62dea1a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -737,7 +737,7 @@ qemuDomainSecretPlainFree(qemuDomainSecretPlain secret) static void -qemuDomainSecretIVFree(qemuDomainSecretIV secret) +qemuDomainSecretAESFree(qemuDomainSecretAES secret) { VIR_FREE(secret.username); VIR_FREE(secret.alias); @@ -757,8 +757,8 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) qemuDomainSecretPlainFree((*secinfo)->s.plain); break; - case VIR_DOMAIN_SECRET_INFO_TYPE_IV: - qemuDomainSecretIVFree((*secinfo)->s.iv); + case VIR_DOMAIN_SECRET_INFO_TYPE_AES: + qemuDomainSecretAESFree((*secinfo)->s.aes); break; case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index a03bdc5..5e825b3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -242,7 +242,7 @@ struct _qemuDomainObjPrivate { /* Type of domain secret */ typedef enum { VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0, - VIR_DOMAIN_SECRET_INFO_TYPE_IV, + VIR_DOMAIN_SECRET_INFO_TYPE_AES, /* utilize GNUTLS_CIPHER_AES_256_CBC */ VIR_DOMAIN_SECRET_INFO_TYPE_LAST } qemuDomainSecretInfoType; @@ -255,11 +255,11 @@ struct _qemuDomainSecretPlain { size_t secretlen; }; -# define QEMU_DOMAIN_IV_KEY_LEN 16 /* 16 bytes for 128 bit random */ - /* initialization vector key */ -typedef struct _qemuDomainSecretIV qemuDomainSecretIV; -typedef struct _qemuDomainSecretIV *qemuDomainSecretIVPtr; -struct _qemuDomainSecretIV { +# define QEMU_DOMAIN_AES_KEY_LEN 16 /* 16 bytes for 128 bit random */ + /* initialization vector key */ +typedef struct _qemuDomainSecretAES qemuDomainSecretAES; +typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr; +struct _qemuDomainSecretAES { char *username; char *alias; /* generated alias for secret */ char *iv; /* base64 encoded initialization vector */ @@ -272,7 +272,7 @@ struct _qemuDomainSecretInfo { qemuDomainSecretInfoType type; union { qemuDomainSecretPlain plain; - qemuDomainSecretIV iv; + qemuDomainSecretAES aes; } s; }; -- 2.5.5
On Thu, May 12, 2016 at 07:49:32 -0400, John Ferlan wrote:
The preferred name will be AES not IV, change current references
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 6 +++--- src/qemu/qemu_domain.h | 14 +++++++------- 3 files changed, 12 insertions(+), 12 deletions(-)
[...]
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index a03bdc5..5e825b3 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h
[...]
@@ -255,11 +255,11 @@ struct _qemuDomainSecretPlain { size_t secretlen; };
-# define QEMU_DOMAIN_IV_KEY_LEN 16 /* 16 bytes for 128 bit random */ - /* initialization vector key */ -typedef struct _qemuDomainSecretIV qemuDomainSecretIV; -typedef struct _qemuDomainSecretIV *qemuDomainSecretIVPtr; -struct _qemuDomainSecretIV { +# define QEMU_DOMAIN_AES_KEY_LEN 16 /* 16 bytes for 128 bit random */
This should be QEMU_DOMAIN_AES_IV_LEN. Since it's the length of the initialization vector.
+ /* initialization vector key */
s/key// The word key is reserved for the shared secret. The IV is shared but not secret.
+typedef struct _qemuDomainSecretAES qemuDomainSecretAES; +typedef struct _qemuDomainSecretAES *qemuDomainSecretAESPtr; +struct _qemuDomainSecretAES { char *username; char *alias; /* generated alias for secret */ char *iv; /* base64 encoded initialization vector */
Here it's used correctly.
@@ -272,7 +272,7 @@ struct _qemuDomainSecretInfo { qemuDomainSecretInfoType type; union { qemuDomainSecretPlain plain; - qemuDomainSecretIV iv; + qemuDomainSecretAES aes; } s; };
ACK with the tweaks above. Peter
participants (2)
-
John Ferlan -
Peter Krempa