[libvirt] [PATCH v2] secret: Alter virSecretGetSecretString
John Ferlan
jferlan at redhat.com
Thu May 12 15:43:39 UTC 2016
Rather than returning a "char *" indicating perhaps some sized set of
characters that is NUL terminated, alter the function to return 0 or -1
for success/failure and add two parameters to handle returning the
buffer and it's size.
The function no longer encodes the returned secret, rather it returns
the unencoded secret forcing callers to make the necessary adjustments.
Alter the callers to handle the adjusted model.
Add a new function virStringBufferIsPrintable to handle checking if
the plaintext secret that is about to be printed on the command line
has non-printable characters and cause failure if it does.
Signed-off-by: John Ferlan <jferlan at redhat.com>
---
v1: http://www.redhat.com/archives/libvir-list/2016-May/msg00870.html
Changes since v1:
libvirt_private.syms, virstring.c, virstring.h:
- Introduce virStringBufferIsPrintable. I could make it a separate patch
if so desired.
libxl_conf.c, qemu_domain.c, qemu_command.c
- Use new format of virSecretGetSecretString
- Encode the secret after the call now when necessary
- Use virStringBufferIsPrintable before printing iSCSI password
secret_util.c:
- Return int instead of uint8_t *
- Remove 'encode' parameter
- Add parameter "*ret_secret", alloc and return the secret in uint8_t * buf
src/libvirt_private.syms | 1 +
src/libxl/libxl_conf.c | 24 +++++++++++++++++-------
src/qemu/qemu_command.c | 18 +++++++++++++++++-
src/qemu/qemu_domain.c | 17 +++++------------
src/qemu/qemu_domain.h | 3 ++-
src/secret/secret_util.c | 45 ++++++++++++++++++++++-----------------------
src/secret/secret_util.h | 15 ++++++++-------
src/util/virstring.c | 19 +++++++++++++++++++
src/util/virstring.h | 1 +
9 files changed, 92 insertions(+), 51 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a980a32..b76d9d5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2301,6 +2301,7 @@ virSkipSpacesBackwards;
virStrcpy;
virStrdup;
virStringArrayHasString;
+virStringBufferIsPrintable;
virStringFreeList;
virStringFreeListCount;
virStringGetFirstWithPrefix;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index d927b37..2dfd03d 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -47,6 +47,7 @@
#include "libxl_utils.h"
#include "virstoragefile.h"
#include "secret_util.h"
+#include "base64.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -1018,7 +1019,9 @@ static int
libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
{
virConnectPtr conn = NULL;
- char *secret = NULL;
+ uint8_t *secret = NULL;
+ char *base64secret = NULL;
+ size_t secretlen;
char *username = NULL;
int ret = -1;
@@ -1030,21 +1033,28 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr)
if (!(conn = virConnectOpen("xen:///system")))
goto cleanup;
- if (!(secret = virSecretGetSecretString(conn,
- protocol,
- true,
- src->auth,
- VIR_SECRET_USAGE_TYPE_CEPH)))
+ if (virSecretGetSecretString(conn, protocol, src->auth,
+ VIR_SECRET_USAGE_TYPE_CEPH,
+ &secret, &secretlen) < 0)
goto cleanup;
+
+ /* RBD expects an encoded secret */
+ base64_encode_alloc((const char *)secret, secretlen, &base64secret);
+ memset(secret, 0, secretlen);
+ if (!base64secret) {
+ virReportOOMError();
+ goto cleanup;
+ }
}
- if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, secret)))
+ if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret)))
goto cleanup;
ret = 0;
cleanup:
VIR_FREE(secret);
+ VIR_FREE(base64secret);
virObjectUnref(conn);
return ret;
}
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0d6d5f8..18b268a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -56,6 +56,7 @@
#include "virscsi.h"
#include "virnuma.h"
#include "virgic.h"
+#include "base64.h"
#if defined(__linux__)
# include <linux/capability.h>
#endif
@@ -628,6 +629,12 @@ qemuBuildGeneralSecinfoURI(virURIPtr uri,
switch ((qemuDomainSecretInfoType) secinfo->type) {
case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN:
if (secinfo->s.plain.secret) {
+ if (!virStringBufferIsPrintable(secinfo->s.plain.secret,
+ secinfo->s.plain.secretlen)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("found non printable characters in secret"));
+ return -1;
+ }
if (virAsprintf(&uri->user, "%s:%s",
secinfo->s.plain.username,
secinfo->s.plain.secret) < 0)
@@ -662,6 +669,8 @@ static int
qemuBuildRBDSecinfoURI(virBufferPtr buf,
qemuDomainSecretInfoPtr secinfo)
{
+ char *base64secret = NULL;
+
if (!secinfo) {
virBufferAddLit(buf, ":auth_supported=none");
return 0;
@@ -669,11 +678,18 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf,
switch ((qemuDomainSecretInfoType) secinfo->type) {
case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN:
+ base64_encode_alloc((const char *)secinfo->s.plain.secret,
+ secinfo->s.plain.secretlen, &base64secret);
+ if (!base64secret) {
+ virReportOOMError();
+ return -1;
+ }
virBufferEscape(buf, '\\', ":", ":id=%s",
secinfo->s.plain.username);
virBufferEscape(buf, '\\', ":",
":key=%s:auth_supported=cephx\\;none",
- secinfo->s.plain.secret);
+ base64secret);
+ VIR_FREE(base64secret);
break;
case VIR_DOMAIN_SECRET_INFO_TYPE_AES:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 39a50e6..2f94f23 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);
}
@@ -870,7 +870,6 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
virStorageNetProtocol protocol,
virStorageAuthDefPtr authdef)
{
- bool encode = false;
int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
const char *protocolstr = virStorageNetProtocolTypeToString(protocol);
@@ -878,18 +877,12 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
if (VIR_STRDUP(secinfo->s.plain.username, authdef->username) < 0)
return -1;
- if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
- /* qemu requires the secret to be encoded for RBD */
- encode = true;
+ if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
secretType = VIR_SECRET_USAGE_TYPE_CEPH;
- }
-
- if (!(secinfo->s.plain.secret =
- virSecretGetSecretString(conn, protocolstr, encode,
- authdef, secretType)))
- return -1;
- return 0;
+ return virSecretGetSecretString(conn, protocolstr, authdef, secretType,
+ &secinfo->s.plain.secret,
+ &secinfo->s.plain.secretlen);
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 331ade0..f074ca5 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_AES_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..5dff57c 100644
--- a/src/secret/secret_util.c
+++ b/src/secret/secret_util.c
@@ -27,7 +27,6 @@
#include "virlog.h"
#include "virobject.h"
#include "viruuid.h"
-#include "base64.h"
#include "datatypes.h"
#define VIR_FROM_THIS VIR_FROM_SECRET
@@ -38,27 +37,30 @@ VIR_LOG_INIT("secret.secret_util");
/* virSecretGetSecretString:
* @conn: Pointer to the connection driver to make secret driver call
* @scheme: Unique enough string for error message to help determine cause
- * @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: returned secret as a sized stream of unsigned chars
+ * @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.
+ * Lookup the secret for the authdef usage type and return it as raw text.
+ * It is up to the caller to encode the secret further.
*
- * Returns a pointer to memory that needs to be cleared and free'd after
- * usage or NULL on error.
+ * Returns 0 on success, -1 on failure. On success the memory in ret_secret
+ * needs to be cleared and free'd after usage.
*/
-char *
+int
virSecretGetSecretString(virConnectPtr conn,
const char *scheme,
- bool encoded,
virStorageAuthDefPtr authdef,
- virSecretUsageType secretUsageType)
+ virSecretUsageType secretUsageType,
+ uint8_t **ret_secret,
+ size_t *ret_secret_size)
{
size_t secret_size;
virSecretPtr sec = NULL;
- char *secret = NULL;
+ unsigned char *secret = NULL;
char uuidStr[VIR_UUID_STRING_BUFLEN];
+ int ret = -1;
/* look up secret */
switch (authdef->secretType) {
@@ -85,8 +87,8 @@ virSecretGetSecretString(virConnectPtr conn,
goto cleanup;
}
- secret = (char *)conn->secretDriver->secretGetValue(sec, &secret_size, 0,
- VIR_SECRET_GET_VALUE_INTERNAL_CALL);
+ secret = conn->secretDriver->secretGetValue(sec, &secret_size, 0,
+ VIR_SECRET_GET_VALUE_INTERNAL_CALL);
if (!secret) {
if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -102,19 +104,16 @@ virSecretGetSecretString(virConnectPtr conn,
goto cleanup;
}
- if (encoded) {
- char *base64 = NULL;
+ if (VIR_ALLOC_N(*ret_secret, secret_size) < 0)
+ goto cleanup;
- base64_encode_alloc(secret, secret_size, &base64);
- VIR_FREE(secret);
- if (!base64) {
- virReportOOMError();
- goto cleanup;
- }
- secret = base64;
- }
+ memcpy(*ret_secret, secret, secret_size);
+ *ret_secret_size = secret_size;
+ ret = 0;
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..0520db3 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)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4)
- ATTRIBUTE_RETURN_CHECK;
+int virSecretGetSecretString(virConnectPtr conn,
+ const char *scheme,
+ virStorageAuthDefPtr authdef,
+ virSecretUsageType secretUsageType,
+ uint8_t **ret_secret,
+ size_t *ret_secret_size)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) ATTRIBUTE_RETURN_CHECK;
#endif /* __VIR_SECRET_H__ */
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 735e65b..1679266 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -1066,3 +1066,22 @@ virStringIsPrintable(const char *str)
return true;
}
+
+
+/**
+ * virBufferIsPrintable:
+ *
+ * Returns true if @buf of @buflen contains only printable characters
+ */
+bool
+virStringBufferIsPrintable(const uint8_t *buf,
+ size_t buflen)
+{
+ size_t i;
+
+ for (i = 0; i < buflen; i++)
+ if (!c_isprint(buf[i]))
+ return false;
+
+ return true;
+}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index fd2868a..9203aa3 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -276,5 +276,6 @@ bool virStringHasControlChars(const char *str);
void virStringStripControlChars(char *str);
bool virStringIsPrintable(const char *str);
+bool virStringBufferIsPrintable(const uint8_t *buf, size_t buflen);
#endif /* __VIR_STRING_H__ */
--
2.5.5
More information about the libvir-list
mailing list