[libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file

Peter Krempa posted 4 patches 6 weeks ago

[libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file

Posted by Peter Krempa 6 weeks ago
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

Re: [libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file

Posted by Ján Tomko 4 weeks ago
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

Re: [libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file

Posted by Daniel P. Berrangé 4 weeks ago
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 :|

Re: [libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file

Posted by Peter Krempa 4 weeks ago
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

Re: [libvirt] [PATCH 3/4] virsh: secret: Allow setting secrets from file

Posted by Daniel P. Berrangé 4 weeks ago
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 :|