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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
© 2016 - 2026 Red Hat, Inc.