[PATCH v2 0/9] virsh: secret: Improve handling of secret value

Improve handling of secrets for security. 'secret-passwd' command was dropped in favor of '--interactive' and few other changes requested at review. Peter Krempa (9): virsh: Work around virSecretFree quirks virsh: secret: Refactor cleanup in cmdSecretSetValue virsh: secret: Refactor cleanup in cmdSecretGetValue virsh: secret: Add --plain flag for secret-get-value virsh: secret: Add --file 'filename' support for secret-set-value virsh: secret: Print warning that passing secret on command-line is insecure virsh: secret: Add --plain switch for secret-set-value tools: virsh: Add --interactive flag for secret-set-value command docs: secret: Unify and sanitize examples on how to set secret value build-aux/syntax-check.mk | 2 +- docs/formatsecret.html.in | 88 +++++++++++++-------- docs/manpages/virsh.rst | 20 ++++- tools/virsh-completer-secret.c | 3 +- tools/virsh-secret.c | 140 ++++++++++++++++++++++++--------- tools/virsh-util.c | 11 +++ tools/virsh-util.h | 5 ++ 7 files changed, 195 insertions(+), 74 deletions(-) -- 2.24.1

Similarly to other libvirt object freeing APIs the function resets the libvirt error when called and doesn't take NULL gracefully. Install the workaround and g_autoptr handlers similarly to the 'virshDomain' type. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- build-aux/syntax-check.mk | 2 +- tools/virsh-completer-secret.c | 3 ++- tools/virsh-secret.c | 22 ++++++++++------------ tools/virsh-util.c | 11 +++++++++++ tools/virsh-util.h | 5 +++++ 5 files changed, 29 insertions(+), 14 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index e5623606dd..14122476de 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1118,7 +1118,7 @@ sc_gettext_init: $(_sc_search_regexp) sc_prohibit_obj_free_apis_in_virsh: - @prohibit='\bvir(Domain|DomainSnapshot)Free\b' \ + @prohibit='\bvir(Domain|DomainSnapshot|Secret)Free\b' \ in_vc_files='virsh.*\.[ch]$$' \ exclude='sc_prohibit_obj_free_apis_in_virsh' \ halt='avoid using virDomain(Snapshot)Free in virsh, use virsh-prefixed wrappers instead' \ diff --git a/tools/virsh-completer-secret.c b/tools/virsh-completer-secret.c index a90fdb399f..505b19db84 100644 --- a/tools/virsh-completer-secret.c +++ b/tools/virsh-completer-secret.c @@ -23,6 +23,7 @@ #include "virsh-completer-secret.h" #include "viralloc.h" #include "virsh-secret.h" +#include "virsh-util.h" #include "virsh.h" #include "virstring.h" @@ -61,7 +62,7 @@ virshSecretUUIDCompleter(vshControl *ctl, cleanup: for (i = 0; i < nsecrets; i++) - virSecretFree(secrets[i]); + virshSecretFree(secrets[i]); VIR_FREE(secrets); return ret; } diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 01c62b9ce8..7123b10860 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -20,6 +20,7 @@ #include <config.h> #include "virsh-secret.h" +#include "virsh-util.h" #include "internal.h" #include "virbuffer.h" @@ -106,8 +107,7 @@ cmdSecretDefine(vshControl *ctl, const vshCmd *cmd) cleanup: VIR_FREE(buffer); - if (res) - virSecretFree(res); + virshSecretFree(res); return ret; } @@ -153,7 +153,7 @@ cmdSecretDumpXML(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - virSecretFree(secret); + virshSecretFree(secret); return ret; } @@ -215,7 +215,7 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - virSecretFree(secret); + virshSecretFree(secret); return ret; } @@ -266,7 +266,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) cleanup: VIR_DISPOSE_N(value, value_size); - virSecretFree(secret); + virshSecretFree(secret); return ret; } @@ -312,7 +312,7 @@ cmdSecretUndefine(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - virSecretFree(secret); + virshSecretFree(secret); return ret; } @@ -348,10 +348,9 @@ virshSecretListFree(virshSecretListPtr list) size_t i; if (list && list->secrets) { - for (i = 0; i < list->nsecrets; i++) { - if (list->secrets[i]) - virSecretFree(list->secrets[i]); - } + for (i = 0; i < list->nsecrets; i++) + virshSecretFree(list->secrets[i]); + VIR_FREE(list->secrets); } VIR_FREE(list); @@ -763,8 +762,7 @@ cmdSecretEvent(vshControl *ctl, const vshCmd *cmd) if (eventId >= 0 && virConnectSecretEventDeregisterAny(priv->conn, eventId) < 0) ret = false; - if (secret) - virSecretFree(secret); + virshSecretFree(secret); return ret; } diff --git a/tools/virsh-util.c b/tools/virsh-util.c index f436fbb486..932d6d0849 100644 --- a/tools/virsh-util.c +++ b/tools/virsh-util.c @@ -250,6 +250,17 @@ virshDomainSnapshotFree(virDomainSnapshotPtr snap) } +void +virshSecretFree(virSecretPtr secret) +{ + if (!secret) + return; + + vshSaveLibvirtHelperError(); + virSecretFree(secret); /* sc_prohibit_obj_free_apis_in_virsh */ +} + + int virshDomainGetXMLFromDom(vshControl *ctl, virDomainPtr dom, diff --git a/tools/virsh-util.h b/tools/virsh-util.h index 2b0447268e..72653d9735 100644 --- a/tools/virsh-util.h +++ b/tools/virsh-util.h @@ -45,6 +45,11 @@ void virshDomainFree(virDomainPtr dom); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshDomain, virshDomainFree); +typedef virSecret virshSecret; +void +virshSecretFree(virSecretPtr secret); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virshSecret, virshSecretFree); + void virshDomainCheckpointFree(virDomainCheckpointPtr chk); -- 2.24.1

On Fri, Jan 24, 2020 at 05:08:33PM +0100, Peter Krempa wrote:
Similarly to other libvirt object freeing APIs the function resets the libvirt error when called and doesn't take NULL gracefully. Install the workaround and g_autoptr handlers similarly to the 'virshDomain' type.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- build-aux/syntax-check.mk | 2 +- tools/virsh-completer-secret.c | 3 ++- tools/virsh-secret.c | 22 ++++++++++------------ tools/virsh-util.c | 11 +++++++++++ tools/virsh-util.h | 5 +++++ 5 files changed, 29 insertions(+), 14 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Automatically clean the secret object and get rid of the cleanup label and 'ret' valiable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-secret.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 7123b10860..58c9a54af6 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -188,18 +188,17 @@ static const vshCmdOptDef opts_secret_set_value[] = { static bool cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) { - virSecretPtr secret; + g_autoptr(virshSecret) secret = NULL; size_t value_size; const char *base64 = NULL; unsigned char *value; int res; - bool ret = false; if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) return false; if (vshCommandOptStringReq(ctl, cmd, "base64", &base64) < 0) - goto cleanup; + return false; value = g_base64_decode(base64, &value_size); @@ -209,14 +208,10 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) if (res != 0) { vshError(ctl, "%s", _("Failed to set secret value")); - goto cleanup; + return false; } vshPrintExtra(ctl, "%s", _("Secret value set\n")); - ret = true; - - cleanup: - virshSecretFree(secret); - return ret; + return true; } /* -- 2.24.1

On Fri, Jan 24, 2020 at 05:08:34PM +0100, Peter Krempa wrote:
Automatically clean the secret object and get rid of the cleanup label and 'ret' valiable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-secret.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Automatically clean the secret object and get rid of the cleanup label and 'ret' valiable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-secret.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 58c9a54af6..7067d13353 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -240,29 +240,23 @@ static const vshCmdOptDef opts_secret_get_value[] = { static bool cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) { - virSecretPtr secret; + g_autoptr(virshSecret) secret = NULL; VIR_AUTODISPOSE_STR base64 = NULL; unsigned char *value; size_t value_size; - bool ret = false; - secret = virshCommandOptSecret(ctl, cmd, NULL); - if (secret == NULL) + if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) return false; - value = virSecretGetValue(secret, &value_size, 0); - if (value == NULL) - goto cleanup; + if (!(value = virSecretGetValue(secret, &value_size, 0))) + return false; base64 = g_base64_encode(value, value_size); vshPrint(ctl, "%s", base64); - ret = true; - cleanup: VIR_DISPOSE_N(value, value_size); - virshSecretFree(secret); - return ret; + return true; } /* -- 2.24.1

On Fri, Jan 24, 2020 at 05:08:35PM +0100, Peter Krempa wrote:
Automatically clean the secret object and get rid of the cleanup label and 'ret' valiable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-secret.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Users might want to get the raw value instead of dealing with base64 encoding. This might be useful for redirection to file and also for simple human-readable secrets. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 6 +++++- tools/virsh-secret.c | 17 +++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index ef15c10e02..0e6eb4cf35 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6576,11 +6576,15 @@ secret-get-value .. code-block:: - secret-get-value secret + secret-get-value [--plain] secret Output the value associated with *secret* (specified by its UUID) to stdout, encoded using Base64. +If the *--plain* flag is used the value is not base64 encoded, but rather +printed raw. Note that unless virsh is started in quiet mode (*virsh -q*) it +prints a newline at the end of the command. This newline is not part of the +secret. secret-undefine --------------- diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 7067d13353..ead740dd8f 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -234,6 +234,10 @@ static const vshCmdOptDef opts_secret_get_value[] = { .help = N_("secret UUID"), .completer = virshSecretUUIDCompleter, }, + {.name = "plain", + .type = VSH_OT_BOOL, + .help = N_("get value without converting to base64") + }, {.name = NULL} }; @@ -244,6 +248,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) VIR_AUTODISPOSE_STR base64 = NULL; unsigned char *value; size_t value_size; + bool plain = vshCommandOptBool(cmd, "plain"); if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) return false; @@ -251,9 +256,17 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) if (!(value = virSecretGetValue(secret, &value_size, 0))) return false; - base64 = g_base64_encode(value, value_size); + if (plain) { + if (fwrite(value, 1, value_size, stdout) != value_size) { + VIR_DISPOSE_N(value, value_size); + vshError(ctl, "failed to write secret"); + return false; + } + } else { + base64 = g_base64_encode(value, value_size); - vshPrint(ctl, "%s", base64); + vshPrint(ctl, "%s", base64); + } VIR_DISPOSE_N(value, value_size); return true; -- 2.24.1

On Fri, Jan 24, 2020 at 05:08:36PM +0100, Peter Krempa wrote:
Users might want to get the raw value instead of dealing with base64 encoding. This might be useful for redirection to file and also for simple human-readable secrets.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 6 +++++- tools/virsh-secret.c | 17 +++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The necessity to specify the secret value as command argument is insecure. Allow reading the secret from a file. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 7 +++++-- tools/virsh-secret.c | 38 +++++++++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 0e6eb4cf35..a7551b9709 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6563,10 +6563,13 @@ secret-set-value .. code-block:: - secret-set-value secret base64 + secret-set-value secret (--file filename | base64) Set the value associated with *secret* (specified by its UUID) to the value -Base64-encoded value *base64*. +Base64-encoded value *base64* or Base-64-encoded contents of file named +*filename*. + +Note that *--file* and *base64* options are mutually exclusive. secret-get-value diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index ead740dd8f..66852173b5 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -177,9 +177,13 @@ static const vshCmdOptDef opts_secret_set_value[] = { .help = N_("secret UUID"), .completer = virshSecretUUIDCompleter, }, + {.name = "file", + .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("read secret from file"), + }, {.name = "base64", - .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, + .type = VSH_OT_STRING, .help = N_("base64-encoded secret value") }, {.name = NULL} @@ -189,22 +193,46 @@ static bool cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshSecret) secret = NULL; - size_t value_size; const char *base64 = NULL; + const char *filename = NULL; + char *file_buf = NULL; + size_t file_len = 0; unsigned char *value; + size_t value_size; int res; + VSH_EXCLUSIVE_OPTIONS("file", "base64"); + if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) return false; if (vshCommandOptStringReq(ctl, cmd, "base64", &base64) < 0) return false; + if (vshCommandOptStringReq(ctl, cmd, "file", &filename) < 0) + return false; + + if (!base64 && !filename) { + vshError(ctl, _("Input secret value is missing")); + return false; + } + + if (filename) { + ssize_t read_ret; + if ((read_ret = virFileReadAll(filename, 1024, &file_buf)) < 0) { + vshSaveLibvirtError(); + return false; + } + + file_len = read_ret; + base64 = file_buf; + } + value = g_base64_decode(base64, &value_size); res = virSecretSetValue(secret, value, value_size, 0); - memset(value, 0, value_size); - VIR_FREE(value); + VIR_DISPOSE_N(value, value_size); + VIR_DISPOSE_N(file_buf, file_len); if (res != 0) { vshError(ctl, "%s", _("Failed to set secret value")); -- 2.24.1

On Fri, Jan 24, 2020 at 05:08:37PM +0100, Peter Krempa wrote:
The necessity to specify the secret value as command argument is insecure. Allow reading the secret from a file.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 7 +++++-- tools/virsh-secret.c | 38 +++++++++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Print a warning if users pass in secrets as command line arguments and mention it in the man page. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 3 +++ tools/virsh-secret.c | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index a7551b9709..823f130f1c 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6571,6 +6571,9 @@ Base64-encoded value *base64* or Base-64-encoded contents of file named Note that *--file* and *base64* options are mutually exclusive. +Passing secrets via the *base64* option on command line is INSECURE and +deprecated. Use the *--file* option instead. + secret-get-value ---------------- diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 66852173b5..0ca08bc133 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -217,6 +217,10 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) return false; } + /* warn users that the --base64 option passed from command line is wrong */ + if (base64) + vshError(ctl, _("Passing secret value as command-line argument is insecure!")); + if (filename) { ssize_t read_ret; if ((read_ret = virFileReadAll(filename, 1024, &file_buf)) < 0) { -- 2.24.1

On Fri, Jan 24, 2020 at 05:08:38PM +0100, Peter Krempa wrote:
Print a warning if users pass in secrets as command line arguments and mention it in the man page.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 3 +++ tools/virsh-secret.c | 4 ++++ 2 files changed, 7 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Allow using the contents of --file without base64 decoding. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 5 +++-- tools/virsh-secret.c | 14 +++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 823f130f1c..dbeac9232f 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6563,11 +6563,12 @@ secret-set-value .. code-block:: - secret-set-value secret (--file filename | base64) + secret-set-value secret (--file filename [--plain] | base64) Set the value associated with *secret* (specified by its UUID) to the value Base64-encoded value *base64* or Base-64-encoded contents of file named -*filename*. +*filename*. Using the *--plain* flag is together with *--file* allows to use +the file contents directly as the secret value. Note that *--file* and *base64* options are mutually exclusive. diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 0ca08bc133..87f3cfff16 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -182,6 +182,10 @@ static const vshCmdOptDef opts_secret_set_value[] = { .flags = VSH_OFLAG_REQ_OPT, .help = N_("read secret from file"), }, + {.name = "plain", + .type = VSH_OT_BOOL, + .help = N_("read the secret from file without converting from base64") + }, {.name = "base64", .type = VSH_OT_STRING, .help = N_("base64-encoded secret value") @@ -199,9 +203,11 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) size_t file_len = 0; unsigned char *value; size_t value_size; + bool plain = vshCommandOptBool(cmd, "plain"); int res; VSH_EXCLUSIVE_OPTIONS("file", "base64"); + VSH_EXCLUSIVE_OPTIONS("plain", "base64"); if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) return false; @@ -232,7 +238,13 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) base64 = file_buf; } - value = g_base64_decode(base64, &value_size); + if (plain) { + value = g_steal_pointer(&file_buf); + value_size = file_len; + file_len = 0; + } else { + value = g_base64_decode(base64, &value_size); + } res = virSecretSetValue(secret, value, value_size, 0); VIR_DISPOSE_N(value, value_size); -- 2.24.1

On Fri, Jan 24, 2020 at 05:08:39PM +0100, Peter Krempa wrote:
Allow using the contents of --file without base64 decoding.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 5 +++-- tools/virsh-secret.c | 14 +++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Simplify human usage of secret-set-value by adding --interactive which will read the value of the secret from the terminal. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 7 +++++-- tools/virsh-secret.c | 22 +++++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index dbeac9232f..8841ae1b31 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6563,14 +6563,17 @@ secret-set-value .. code-block:: - secret-set-value secret (--file filename [--plain] | base64) + secret-set-value secret (--file filename [--plain] | --interactive | base64) Set the value associated with *secret* (specified by its UUID) to the value Base64-encoded value *base64* or Base-64-encoded contents of file named *filename*. Using the *--plain* flag is together with *--file* allows to use the file contents directly as the secret value. -Note that *--file* and *base64* options are mutually exclusive. +If *--interactive* flag is used the secret value is read as a password from the +terminal. + +Note that *--file*, *--interactive* and *base64* options are mutually exclusive. Passing secrets via the *base64* option on command line is INSECURE and deprecated. Use the *--file* option instead. diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 87f3cfff16..00a434e997 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -186,6 +186,10 @@ static const vshCmdOptDef opts_secret_set_value[] = { .type = VSH_OT_BOOL, .help = N_("read the secret from file without converting from base64") }, + {.name = "interactive", + .type = VSH_OT_BOOL, + .help = N_("read the secret from the terminal") + }, {.name = "base64", .type = VSH_OT_STRING, .help = N_("base64-encoded secret value") @@ -204,10 +208,14 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) unsigned char *value; size_t value_size; bool plain = vshCommandOptBool(cmd, "plain"); + bool interactive = vshCommandOptBool(cmd, "interactive"); int res; VSH_EXCLUSIVE_OPTIONS("file", "base64"); VSH_EXCLUSIVE_OPTIONS("plain", "base64"); + VSH_EXCLUSIVE_OPTIONS("interactive", "base64"); + VSH_EXCLUSIVE_OPTIONS("interactive", "plain"); + VSH_EXCLUSIVE_OPTIONS("interactive", "file"); if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) return false; @@ -218,7 +226,7 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "file", &filename) < 0) return false; - if (!base64 && !filename) { + if (!base64 && !filename && !interactive) { vshError(ctl, _("Input secret value is missing")); return false; } @@ -238,6 +246,18 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) base64 = file_buf; } + if (interactive) { + vshPrint(ctl, "%s", _("Enter new value for secret:")); + fflush(stdout); + + if (!(file_buf = getpass(""))) { + vshError(ctl, "%s", _("Failed to read secret")); + return false; + } + file_len = strlen(file_buf); + plain = true; + } + if (plain) { value = g_steal_pointer(&file_buf); value_size = file_len; -- 2.24.1

On Fri, Jan 24, 2020 at 05:08:40PM +0100, Peter Krempa wrote:
Simplify human usage of secret-set-value by adding --interactive which will read the value of the secret from the terminal.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 7 +++++-- tools/virsh-secret.c | 22 +++++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Discourage passing secrets as commandline arguments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatsecret.html.in | 88 +++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 31 deletions(-) diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 8f5383cf64..6c2d5e02a6 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -76,13 +76,13 @@ <pre> # virsh secret-define volume-secret.xml Secret 0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f created -# -# MYSECRET=`printf %s "open sesame" | base64` -# virsh secret-set-value 0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f $MYSECRET -Secret value set -# </pre> + <p> + See <a href="#settingSecrets">virsh secret-set-value</a> on how + to set the value of the secret. + </p> + <p> The volume type secret can be supplied either in volume XML during creation of a <a href="formatstorage.html#StorageVol">storage volume</a> @@ -103,12 +103,11 @@ Secret value set # virsh secret-define luks-secret.xml Secret f52a81b2-424e-490c-823d-6bd4235bc57 created -# -# MYSECRET=`printf %s "letmein" | base64` -# virsh secret-set-value f52a81b2-424e-490c-823d-6bd4235bc57 $MYSECRET -Secret value set -# </pre> + <p> + See <a href="#settingSecrets">virsh secret-set-value</a> on how + to set the value of the secret. + </p> <p> The volume type secret can be supplied in domain XML for a luks storage @@ -156,13 +155,11 @@ Secret 1b40a534-8301-45d5-b1aa-11894ebb1735 created UUID Usage ----------------------------------------------------------- 1b40a534-8301-45d5-b1aa-11894ebb1735 cephx ceph_example -# -# CEPHPHRASE=`printf %s "pass phrase" | base64` -# virsh secret-set-value 1b40a534-8301-45d5-b1aa-11894ebb1735 $CEPHPHRASE -Secret value set - -# </pre> + <p> + See <a href="#settingSecrets">virsh secret-set-value</a> on how + to set the value of the secret. + </p> <p> The ceph secret can then be used by UUID or by the @@ -229,7 +226,9 @@ incominguser myname mysecret <p> Next, use <code>virsh secret-define iscsi-secret.xml</code> to define - the secret and <code>virsh secret-set-value</code> using the generated + the secret and + <code><a href="#settingSecrets">virsh secret-set-value</a></code> + using the generated UUID value and a base64 generated secret value in order to define the chosen secret pass phrase. The pass phrase must match the password used in the iSCSI authentication configuration file. @@ -243,12 +242,13 @@ Secret c4dbe20b-b1a3-4ac1-b6e6-2ac97852ebb6 created ----------------------------------------------------------- c4dbe20b-b1a3-4ac1-b6e6-2ac97852ebb6 iscsi libvirtiscsi -# MYSECRET=`printf %s "mysecret" | base64` -# virsh secret-set-value c4dbe20b-b1a3-4ac1-b6e6-2ac97852ebb6 $MYSECRET -Secret value set -# </pre> + <p> + See <a href="#settingSecrets">virsh secret-set-value</a> on how + to set the value of the secret. + </p> + <p> The iSCSI secret can then be used by UUID or by the usage name via the <code><auth></code> element in a domain's @@ -313,19 +313,13 @@ Secret 718c71bd-67b5-4a2b-87ec-a24e8ca200dc created Once the secret is defined, a secret value will need to be set. The secret would be the passphrase used to access the TLS credentials. The following is a simple example of using - <code>virsh secret-set-value</code> to set the secret value. The + <code><a href="#settingSecrets">virsh secret-set-value</a></code> to set + the secret value. The <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> <code>virSecretSetValue</code></a> API may also be used to set a more secure secret without using printable/readable characters. </p> - <pre> -# MYSECRET=`printf %s "letmein" | base64` -# virsh secret-set-value 718c71bd-67b5-4a2b-87ec-a24e8ca200dc $MYSECRET -Secret value set - - </pre> - <h3><a id="vTPMUsageType">Usage type "vtpm"</a></h3> <p> @@ -370,17 +364,49 @@ Secret 6dd3e4a5-1d76-44ce-961f-f119f5aad935 created Once the secret is defined, a secret value will need to be set. The secret would be the passphrase used to decrypt the vTPM state. The following is a simple example of using - <code>virsh secret-set-value</code> to set the secret value. The + <code><a href="#settingSecrets">virsh secret-set-value</a></code> + to set the secret value. The <a href="html/libvirt-libvirt-secret.html#virSecretSetValue"> <code>virSecretSetValue</code></a> API may also be used to set a more secure secret without using printable/readable characters. </p> + <h2><a id="settingSecrets">Setting secret values in virsh</a></h2> + + <p> + To set the value of the secret you can use the following virsh commands. + If the secret is a password-like string (printable characters, no newline) + you can use: + </p> + <pre> +# virsh secret-set-value --interactive 6dd3e4a5-1d76-44ce-961f-f119f5aad935 +Enter new value for secret: +Secret value set + </pre> + + <p> + Another secure option is to read the secret from a file. This way the + secret can contain any bytes (even NUL and non-printable characters). The + length of the secret is the length of the input file. Alternatively the + <code>--plain</code> option can be omitted if the file contents are + base64-encoded. + </p> + + <pre> +# virsh secret-set-value 6dd3e4a5-1d76-44ce-961f-f119f5aad935 --file --plain secretinfile +Secret value set + </pre> + + <p> + The secret can also be set via an argument, but note that other users + may see it in the process listing output. The secret must be base64 + encoded. + </p> + <pre> # MYSECRET=`printf %s "open sesame" | base64` # virsh secret-set-value 6dd3e4a5-1d76-44ce-961f-f119f5aad935 $MYSECRET Secret value set - </pre> </body> -- 2.24.1

On 1/24/20 10:08 AM, Peter Krempa wrote:
Discourage passing secrets as commandline arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatsecret.html.in | 88 +++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 31 deletions(-)
+ <h2><a id="settingSecrets">Setting secret values in virsh</a></h2> +
+ </pre> + + <p> + The secret can also be set via an argument, but note that other users + may see it in the process listing output. The secret must be base64 + encoded.
Is this last sentence still accurate, given that you can pass --plain to avoid base64 encoding? Should the note use <b> or other formatting to call attention to the security risk of doing it this way?
+ </p> + <pre> # MYSECRET=`printf %s "open sesame" | base64` # virsh secret-set-value 6dd3e4a5-1d76-44ce-961f-f119f5aad935 $MYSECRET Secret value set - </pre>
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

On Fri, Jan 24, 2020 at 10:41:00 -0600, Eric Blake wrote:
On 1/24/20 10:08 AM, Peter Krempa wrote:
Discourage passing secrets as commandline arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatsecret.html.in | 88 +++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 31 deletions(-)
+ <h2><a id="settingSecrets">Setting secret values in virsh</a></h2> +
+ </pre> + + <p> + The secret can also be set via an argument, but note that other users + may see it in the process listing output. The secret must be base64 + encoded.
Is this last sentence still accurate, given that you can pass --plain to avoid base64 encoding?
I didn't allow to use --plain together with passing it on the command line. --plain works only in conjunction with --file. I didn't see a point of adding new features to an insecure way of doing things.
Should the note use <b> or other formatting to call attention to the security risk of doing it this way?
Yeah, I can add it.

On Mon, Jan 27, 2020 at 09:59:01 +0100, Peter Krempa wrote:
On Fri, Jan 24, 2020 at 10:41:00 -0600, Eric Blake wrote:
On 1/24/20 10:08 AM, Peter Krempa wrote:
Discourage passing secrets as commandline arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatsecret.html.in | 88 +++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 31 deletions(-)
+ <h2><a id="settingSecrets">Setting secret values in virsh</a></h2> +
+ </pre> + + <p> + The secret can also be set via an argument, but note that other users + may see it in the process listing output. The secret must be base64 + encoded.
Is this last sentence still accurate, given that you can pass --plain to avoid base64 encoding?
I didn't allow to use --plain together with passing it on the command line. --plain works only in conjunction with --file. I didn't see a point of adding new features to an insecure way of doing things.
Should the note use <b> or other formatting to call attention to the security risk of doing it this way?
Yeah, I can add it.
I propose the following change to emphasize the fact that it's insecure: diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 6c2d5e02a6..695370e150 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -398,9 +398,10 @@ Secret value set </pre> <p> - The secret can also be set via an argument, but note that other users - may see it in the process listing output. The secret must be base64 - encoded. + <b>WARNING</b>The following approach is <b>insecure</b> and deprecated. + The secret can also be set via an argument. Note that other users may see + the actual secret in the process listing! + The secret must be base64 encoded. </p>

On Mon, Jan 27, 2020 at 02:16:43PM +0100, Peter Krempa wrote:
On Mon, Jan 27, 2020 at 09:59:01 +0100, Peter Krempa wrote:
On Fri, Jan 24, 2020 at 10:41:00 -0600, Eric Blake wrote:
On 1/24/20 10:08 AM, Peter Krempa wrote:
Discourage passing secrets as commandline arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatsecret.html.in | 88 +++++++++++++++++++++++++-------------- 1 file changed, 57 insertions(+), 31 deletions(-)
+ <h2><a id="settingSecrets">Setting secret values in virsh</a></h2> +
+ </pre> + + <p> + The secret can also be set via an argument, but note that other users + may see it in the process listing output. The secret must be base64 + encoded.
Is this last sentence still accurate, given that you can pass --plain to avoid base64 encoding?
I didn't allow to use --plain together with passing it on the command line. --plain works only in conjunction with --file. I didn't see a point of adding new features to an insecure way of doing things.
Should the note use <b> or other formatting to call attention to the security risk of doing it this way?
Yeah, I can add it.
I propose the following change to emphasize the fact that it's insecure:
diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 6c2d5e02a6..695370e150 100644 --- a/docs/formatsecret.html.in +++ b/docs/formatsecret.html.in @@ -398,9 +398,10 @@ Secret value set </pre>
<p> - The secret can also be set via an argument, but note that other users - may see it in the process listing output. The secret must be base64 - encoded. + <b>WARNING</b>The following approach is <b>insecure</b> and deprecated. + The secret can also be set via an argument. Note that other users may see + the actual secret in the process listing! + The secret must be base64 encoded. </p>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé
-
Eric Blake
-
Peter Krempa