[libvirt] [PATCH 0/4] virsh: secret: Improve handling of secret value
The currently existing virsh APIs for secrets are awful for human use and don't promote security. Peter Krempa (4): virsh: secret: Add 'secret-passwd' command virsh: secret: Allow getting secret's value without base64 encoding virsh: secret: Allow setting secrets from file docs: secret: Unify and sanitize examples on how to set secret value docs/formatsecret.html.in | 86 ++++++++++++++++++---------- docs/manpages/virsh.rst | 26 ++++++++- tools/virsh-secret.c | 116 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 189 insertions(+), 39 deletions(-) -- 2.24.1
Add a command which allows to read a secret value from terminal. 'secret-passwd' is chosen as a name as the password has limitations as passwords do have (printable, terminated by newline which is not contained in the value). This makes it way more user-friendly to use the secret driver with virsh when setting a value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 15 +++++++++ tools/virsh-secret.c | 70 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 6446a903ca..03364684b5 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6564,6 +6564,21 @@ Set the value associated with *secret* (specified by its UUID) to the value Base64-encoded value *base64*. +secret-passwd +---------------- + +**Syntax:** + +.. code-block:: + + secret-passwd secret + +Set the value associated with *secret* (specified by its UUID) to a string +read from stdin. Note that input is terminated by a newline and the secret +can't contain non-printable characters. Use *secret-set-value* for generic +secrets. Note that this requires a terminal associated with virsh to read +the password. + secret-get-value ---------------- diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 66369a25dc..9f64be6b14 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -219,6 +219,70 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) return ret; } + +/* + * "secret-passwd" command + */ +static const vshCmdInfo info_secret_passwd[] = { + {.name = "help", + .data = N_("set a secret value from stdin") + }, + {.name = "desc", + .data = N_("Set a secret value from stdin") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_secret_passwd[] = { + {.name = "secret", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("secret UUID"), + .completer = virshSecretUUIDCompleter, + }, + {.name = NULL} +}; + +static bool +cmdSecretPasswd(vshControl *ctl, + const vshCmd *cmd) +{ + virSecretPtr secret; + g_autofree char *value = NULL; + int res; + bool ret = false; + + if (!ctl->istty) { + vshError(ctl, "%s", _("secret-passwd requires a terminal")); + return false; + } + + if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) + return false; + + vshPrint(ctl, "%s", _("Enter new value for secret:")); + fflush(stdout); + + if (!(value = getpass(""))) { + vshError(ctl, "%s", _("Failed to read secret")); + goto cleanup; + } + + res = virSecretSetValue(secret, (unsigned char *) value, strlen(value), 0); + + if (res != 0) { + vshError(ctl, "%s", _("Failed to set secret value")); + goto cleanup; + } + vshPrintExtra(ctl, "%s", _("Secret value set\n")); + ret = true; + + cleanup: + virSecretFree(secret); + return ret; +} + + /* * "secret-get-value" command */ @@ -805,6 +869,12 @@ const vshCmdDef secretCmds[] = { .info = info_secret_set_value, .flags = 0 }, + {.name = "secret-passwd", + .handler = cmdSecretPasswd, + .opts = opts_secret_passwd, + .info = info_secret_passwd, + .flags = 0 + }, {.name = "secret-undefine", .handler = cmdSecretUndefine, .opts = opts_secret_undefine, -- 2.24.1
On Fri, Jan 10, 2020 at 04:42:41PM +0100, Peter Krempa wrote:
Add a command which allows to read a secret value from terminal. 'secret-passwd' is chosen as a name as the password has limitations as passwords do have (printable, terminated by newline which is not contained in the value). This makes it way more user-friendly to use the secret driver with virsh when setting a value.
In a later patch you already extend secret-set-value to have a new "--filename PATH" arg. I think we should do the same for interactive usage, eg $ virsh secret-set-value --prompt or --interactive / -i
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 15 +++++++++ tools/virsh-secret.c | 70 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 6446a903ca..03364684b5 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6564,6 +6564,21 @@ Set the value associated with *secret* (specified by its UUID) to the value Base64-encoded value *base64*.
+secret-passwd +---------------- + +**Syntax:** + +.. code-block:: + + secret-passwd secret + +Set the value associated with *secret* (specified by its UUID) to a string +read from stdin. Note that input is terminated by a newline and the secret +can't contain non-printable characters. Use *secret-set-value* for generic +secrets. Note that this requires a terminal associated with virsh to read +the password. + secret-get-value ----------------
diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 66369a25dc..9f64be6b14 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -219,6 +219,70 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) return ret; }
+ +/* + * "secret-passwd" command + */ +static const vshCmdInfo info_secret_passwd[] = { + {.name = "help", + .data = N_("set a secret value from stdin") + }, + {.name = "desc", + .data = N_("Set a secret value from stdin") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_secret_passwd[] = { + {.name = "secret", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("secret UUID"), + .completer = virshSecretUUIDCompleter, + }, + {.name = NULL} +}; + +static bool +cmdSecretPasswd(vshControl *ctl, + const vshCmd *cmd) +{ + virSecretPtr secret; + g_autofree char *value = NULL; + int res; + bool ret = false; + + if (!ctl->istty) { + vshError(ctl, "%s", _("secret-passwd requires a terminal")); + return false; + } + + if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) + return false; + + vshPrint(ctl, "%s", _("Enter new value for secret:")); + fflush(stdout); + + if (!(value = getpass(""))) { + vshError(ctl, "%s", _("Failed to read secret")); + goto cleanup; + } + + res = virSecretSetValue(secret, (unsigned char *) value, strlen(value), 0); + + if (res != 0) { + vshError(ctl, "%s", _("Failed to set secret value")); + goto cleanup; + } + vshPrintExtra(ctl, "%s", _("Secret value set\n")); + ret = true; + + cleanup: + virSecretFree(secret); + return ret; +} + + /* * "secret-get-value" command */ @@ -805,6 +869,12 @@ const vshCmdDef secretCmds[] = { .info = info_secret_set_value, .flags = 0 }, + {.name = "secret-passwd", + .handler = cmdSecretPasswd, + .opts = opts_secret_passwd, + .info = info_secret_passwd, + .flags = 0 + }, {.name = "secret-undefine", .handler = cmdSecretUndefine, .opts = opts_secret_undefine, -- 2.24.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
On Tue, Jan 21, 2020 at 13:34:27 +0000, Daniel Berrange wrote:
On Fri, Jan 10, 2020 at 04:42:41PM +0100, Peter Krempa wrote:
Add a command which allows to read a secret value from terminal. 'secret-passwd' is chosen as a name as the password has limitations as passwords do have (printable, terminated by newline which is not contained in the value). This makes it way more user-friendly to use the secret driver with virsh when setting a value.
In a later patch you already extend secret-set-value to have a new "--filename PATH" arg. I think we should do the same for interactive usage, eg
$ virsh secret-set-value --prompt
or --interactive / -i
Hmm, yeah. Originally I've added this one first for simplicity and then dealt with the --filename thing, but you are right that this is the better option.
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 | 16 ++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 03364684b5..fcc8ef6758 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6586,11 +6586,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 9f64be6b14..6d95ed9d5d 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -303,6 +303,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} }; @@ -313,6 +317,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) VIR_AUTODISPOSE_STR base64 = NULL; unsigned char *value; size_t value_size; + bool plain = vshCommandOptBool(cmd, "plain"); bool ret = false; secret = virshCommandOptSecret(ctl, cmd, NULL); @@ -323,9 +328,16 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) if (value == NULL) goto cleanup; - base64 = g_base64_encode(value, value_size); + if (plain) { + if (fwrite(value, 1, value_size, stdout) != value_size) { + vshError(ctl, "failed to write secret"); + goto cleanup; + } + } else { + base64 = g_base64_encode(value, value_size); - vshPrint(ctl, "%s", base64); + vshPrint(ctl, "%s", base64); + } ret = true; cleanup: -- 2.24.1
On Fri, Jan 10, 2020 at 04:42:42PM +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 | 16 ++++++++++++++-- 2 files changed, 19 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 | 5 +++-- tools/virsh-secret.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index fcc8ef6758..992b1daf90 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6558,10 +6558,11 @@ 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 from file named *filename*. Note that *--file* +and *base64* options are mutually exclusive. secret-passwd diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 6d95ed9d5d..dd6bf75705 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} @@ -191,17 +195,37 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) virSecretPtr secret; size_t value_size; const char *base64 = NULL; + const char *filename = NULL; unsigned char *value; int res; bool ret = false; + VSH_EXCLUSIVE_OPTIONS("file", "base64"); + if (!(secret = virshCommandOptSecret(ctl, cmd, NULL))) return false; if (vshCommandOptStringReq(ctl, cmd, "base64", &base64) < 0) goto cleanup; - value = g_base64_decode(base64, &value_size); + if (vshCommandOptStringReq(ctl, cmd, "file", &filename) < 0) + goto cleanup; + + if (!base64 && !filename) { + vshError(ctl, _("Input secret value is missing")); + goto cleanup; + } + + if (filename) { + ssize_t read_len; + if ((read_len = virFileReadAll(filename, 1024, (char **) &value)) < 0) { + vshSaveLibvirtError(); + goto cleanup; + } + value_size = read_len; + } else { + value = g_base64_decode(base64, &value_size); + } res = virSecretSetValue(secret, value, value_size, 0); memset(value, 0, value_size); -- 2.24.1
On Fri, Jan 10, 2020 at 04:42:43PM +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 | 5 +++-- tools/virsh-secret.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index fcc8ef6758..992b1daf90 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6558,10 +6558,11 @@ 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 from file named *filename*. Note that *--file* +and *base64* options are mutually exclusive.
You added a --plain option to secret-get-value. It would naturally suggest that we do the same here, then we can support secret-set-value $BASE64STR secret-set-value --plain $RAWSTR secret-set-value --file FILENAME-WITH-BASE64-STR secret-set-value --plain --file FILENAME-WITH-RAW-STR 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 :|
On Tue, Jan 21, 2020 at 13:38:13 +0000, Daniel Berrange wrote:
On Fri, Jan 10, 2020 at 04:42:43PM +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 | 5 +++-- tools/virsh-secret.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index fcc8ef6758..992b1daf90 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6558,10 +6558,11 @@ 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 from file named *filename*. Note that *--file* +and *base64* options are mutually exclusive.
You added a --plain option to secret-get-value.
It would naturally suggest that we do the same here, then we can support
secret-set-value $BASE64STR secret-set-value --plain $RAWSTR
I think that both of the above should not have existed in the first place. Adding the possibility to add plain secrets via argument looks to me as a step back. If I could do it, I'd remove the base64 via command line arguments as well.
secret-set-value --file FILENAME-WITH-BASE64-STR
This seems a bit pointless to me.
secret-set-value --plain --file FILENAME-WITH-RAW-STR
On Tue, Jan 21, 2020 at 02:43:44PM +0100, Peter Krempa wrote:
On Tue, Jan 21, 2020 at 13:38:13 +0000, Daniel Berrange wrote:
On Fri, Jan 10, 2020 at 04:42:43PM +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 | 5 +++-- tools/virsh-secret.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index fcc8ef6758..992b1daf90 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6558,10 +6558,11 @@ 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 from file named *filename*. Note that *--file* +and *base64* options are mutually exclusive.
You added a --plain option to secret-get-value.
It would naturally suggest that we do the same here, then we can support
secret-set-value $BASE64STR secret-set-value --plain $RAWSTR
I think that both of the above should not have existed in the first place. Adding the possibility to add plain secrets via argument looks to me as a step back. If I could do it, I'd remove the base64 via command line arguments as well.
True, we probably should forbid --plain RAWSTR. Removing existing option is not viable, but perhaps we can at least print a warning message to stderr
secret-set-value --file FILENAME-WITH-BASE64-STR
This seems a bit pointless to me.
Bear in mind that the place you are getting the secret from may already be giving it to you in base64 format, so it is useful to write it to the file as-is, as you would do today when using set-value base64
secret-set-value --plain --file FILENAME-WITH-RAW-STR
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 :|
On Fri, Jan 10, 2020 at 04:42:43PM +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 | 5 +++-- tools/virsh-secret.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index fcc8ef6758..992b1daf90 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6558,10 +6558,11 @@ 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 from file named *filename*. Note that *--file* +and *base64* options are mutually exclusive.
secret-passwd
Please include a way to read the secret from an EBCDIC-encoded file, just for completeness. Jano
Discourage passing secrets as commandline arguments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatsecret.html.in | 86 +++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 31 deletions(-) diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 8f5383cf64..61a8396682 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,47 @@ 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-passwd 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. + </p> + + <pre> +# virsh secret-set-value 6dd3e4a5-1d76-44ce-961f-f119f5aad935 --file 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 Fri, Jan 10, 2020 at 04:42:44PM +0100, Peter Krempa wrote:
Discourage passing secrets as commandline arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatsecret.html.in | 86 +++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 31 deletions(-)
diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in index 8f5383cf64..61a8396682 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,47 @@ 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-passwd 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. + </p> + + <pre> +# virsh secret-set-value 6dd3e4a5-1d76-44ce-961f-f119f5aad935 --file 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>
This paragraph will need updating based on my prev patch comments. Overall this patch looks good though. 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 :|
On Fri, Jan 10, 2020 at 16:42:40 +0100, Peter Krempa wrote:
The currently existing virsh APIs for secrets are awful for human use and don't promote security.
Peter Krempa (4): virsh: secret: Add 'secret-passwd' command virsh: secret: Allow getting secret's value without base64 encoding virsh: secret: Allow setting secrets from file docs: secret: Unify and sanitize examples on how to set secret value
Ping
On 1/10/20 12:42 PM, Peter Krempa wrote:
The currently existing virsh APIs for secrets are awful for human use and don't promote security.
Peter Krempa (4): virsh: secret: Add 'secret-passwd' command virsh: secret: Allow getting secret's value without base64 encoding virsh: secret: Allow setting secrets from file docs: secret: Unify and sanitize examples on how to set secret value
docs/formatsecret.html.in | 86 ++++++++++++++++++---------- docs/manpages/virsh.rst | 26 ++++++++- tools/virsh-secret.c | 116 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 189 insertions(+), 39 deletions(-)
Code-wise LGTM. I have a question about the design though. Shouldn't we ask for a password confirmation when setting the secret via secret-passwd? This would be more on par with how 'passwd' works in Linux, and can also help to prevent user typos when setting a secret. Thanks, DHB
On Tue, Jan 21, 2020 at 09:57:22 -0300, Daniel Henrique Barboza wrote:
On 1/10/20 12:42 PM, Peter Krempa wrote:
The currently existing virsh APIs for secrets are awful for human use and don't promote security.
Peter Krempa (4): virsh: secret: Add 'secret-passwd' command virsh: secret: Allow getting secret's value without base64 encoding virsh: secret: Allow setting secrets from file docs: secret: Unify and sanitize examples on how to set secret value
docs/formatsecret.html.in | 86 ++++++++++++++++++---------- docs/manpages/virsh.rst | 26 ++++++++- tools/virsh-secret.c | 116 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 189 insertions(+), 39 deletions(-)
Code-wise LGTM. I have a question about the design though.
Shouldn't we ask for a password confirmation when setting the secret via secret-passwd? This would be more on par with how 'passwd' works in Linux, and can also help to prevent user typos when setting a secret.
I don't really think that it's necessary. When you mess up changing of your account password you won't be able to log in thus it's a good idea to make more sure that you didn't miss-type it. With libvirt secrets, it doesn't prevent you from fixing it later as you aren't even asked for the previous value of the secret.
On 1/21/20 10:03 AM, Peter Krempa wrote:
On Tue, Jan 21, 2020 at 09:57:22 -0300, Daniel Henrique Barboza wrote:
On 1/10/20 12:42 PM, Peter Krempa wrote:
The currently existing virsh APIs for secrets are awful for human use and don't promote security.
Peter Krempa (4): virsh: secret: Add 'secret-passwd' command virsh: secret: Allow getting secret's value without base64 encoding virsh: secret: Allow setting secrets from file docs: secret: Unify and sanitize examples on how to set secret value
docs/formatsecret.html.in | 86 ++++++++++++++++++---------- docs/manpages/virsh.rst | 26 ++++++++- tools/virsh-secret.c | 116 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 189 insertions(+), 39 deletions(-)
Code-wise LGTM. I have a question about the design though.
Shouldn't we ask for a password confirmation when setting the secret via secret-passwd? This would be more on par with how 'passwd' works in Linux, and can also help to prevent user typos when setting a secret.
I don't really think that it's necessary. When you mess up changing of your account password you won't be able to log in thus it's a good idea to make more sure that you didn't miss-type it.
With libvirt secrets, it doesn't prevent you from fixing it later as you aren't even asked for the previous value of the secret.
Those are fair points. I agree. All patches: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
participants (4)
-
Daniel Henrique Barboza -
Daniel P. Berrangé -
Ján Tomko -
Peter Krempa