Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-09-27 Thread Richard W.M. Jones 
On Mon, Sep 27, 2021 at 04:18:34PM -0500, Eric Blake wrote:
> On Fri, Jul 23, 2021 at 11:33:03AM +0100, Richard W.M. Jones wrote:
> > Under SELinux, Unix domain sockets have two labels.  One is on the
> > disk and can be set with commands such as chcon(1).  There is a
> > different label stored in memory (called the process label).  This can
> > only be set by the process creating the socket.  When using SELinux +
> > SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> > you must set both labels correctly first.
> > 
> > For qemu-nbd the options to set the second label are awkward.  You can
> > create the socket in a wrapper program and then exec into qemu-nbd.
> > Or you could try something with LD_PRELOAD.
> > 
> > This commit adds the ability to set the label straightforwardly on the
> > command line, via the new --selinux-label flag.  (The name of the flag
> > is the same as the equivalent nbdkit option.)
> > 
> > A worked example showing how to use the new option can be found in
> > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > Signed-off-by: Richard W.M. Jones 
> > ---
> 
> I'm making one tweak to your patch before sending the pull request:
> 
> > +++ b/qemu-nbd.c
> > @@ -64,6 +68,7 @@
> >  #define QEMU_NBD_OPT_FORK  263
> >  #define QEMU_NBD_OPT_TLSAUTHZ  264
> >  #define QEMU_NBD_OPT_PID_FILE  265
> > +#define QEMU_NBD_OPT_SELINUX_LABEL 266
> >  
> >  #define MBR_SIZE 512
> >  
> > @@ -116,6 +121,9 @@ static void usage(const char *name)
> >  "  --forkfork off the server process and exit the 
> > parent\n"
> >  "once the server is running\n"
> >  "  --pid-file=PATH   store the server's process ID in the given 
> > file\n"
> > +#ifdef CONFIG_SELINUX
> > +"  --selinux-label=LABEL set SELinux process label on listening 
> > socket\n"
> > +#endif
> 
> The new option is only conditionally advertised under --help (qemu-nbd
> lacks a stable machine-parseable output, so scraping --help output
> will have to do for now)...
> 
> >  #if HAVE_NBD_DEVICE
> >  "\n"
> >  "Kernel NBD client support:\n"
> > @@ -532,6 +540,8 @@ int main(int argc, char **argv)
> >  { "trace", required_argument, NULL, 'T' },
> >  { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
> >  { "pid-file", required_argument, NULL, QEMU_NBD_OPT_PID_FILE },
> > +{ "selinux-label", required_argument, NULL,
> > +  QEMU_NBD_OPT_SELINUX_LABEL },
> 
> ...but is unconditionally supported as a long option even when support
> was not compiled in...
> 
> >  { NULL, 0, NULL, 0 }
> >  };
> >  int ch;
> > @@ -558,6 +568,7 @@ int main(int argc, char **argv)
> >  int old_stderr = -1;
> >  unsigned socket_activation;
> >  const char *pid_file_name = NULL;
> > +const char *selinux_label = NULL;
> >  BlockExportOptions *export_opts;
> >  
> >  #ifdef CONFIG_POSIX
> > @@ -747,6 +758,9 @@ int main(int argc, char **argv)
> >  case QEMU_NBD_OPT_PID_FILE:
> >  pid_file_name = optarg;
> >  break;
> > +case QEMU_NBD_OPT_SELINUX_LABEL:
> > +selinux_label = optarg;
> > +break;
> >  }
> >  }
> >  
> > @@ -938,6 +952,16 @@ int main(int argc, char **argv)
> >  } else {
> >  backlog = MIN(shared, SOMAXCONN);
> >  }
> > +if (sockpath && selinux_label) {
> > +#ifdef CONFIG_SELINUX
> > +if (setsockcreatecon_raw(selinux_label) == -1) {
> > +error_report("Cannot set SELinux socket create context "
> > + "to %s: %s",
> > + selinux_label, strerror(errno));
> > +exit(EXIT_FAILURE);
> > +}
> > +#endif
> 
> ...but here we silently ignore it if support is not compiled in.
> Better is to issue an error message about using an unsupported option,
> so I'll squash this in:
> 
> diff --git i/qemu-nbd.c w/qemu-nbd.c
> index 5dc82c419255..94f8ec07c064 100644
> --- i/qemu-nbd.c
> +++ w/qemu-nbd.c
> @@ -962,6 +962,9 @@ int main(int argc, char **argv)
>   selinux_label, strerror(errno));
>  exit(EXIT_FAILURE);
>  }
> +#else
> +error_report("SELinux support not enabled in this binary");
> +exit(EXIT_FAILURE);
>  #endif
>  }
>  saddr = nbd_build_socket_address(sockpath, bindto, port);
> @@ -978,6 +981,9 @@ int main(int argc, char **argv)
>   strerror(errno));
>  exit(EXIT_FAILURE);
>  }
> +#else
> +error_report("SELinux support not enabled in this binary");
> +exit(EXIT_FAILURE);
>  #endif
>  }
>  } else {
> 

Good idea, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming

Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-09-27 Thread Eric Blake 
On Fri, Jul 23, 2021 at 11:33:03AM +0100, Richard W.M. Jones wrote:
> Under SELinux, Unix domain sockets have two labels.  One is on the
> disk and can be set with commands such as chcon(1).  There is a
> different label stored in memory (called the process label).  This can
> only be set by the process creating the socket.  When using SELinux +
> SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> you must set both labels correctly first.
> 
> For qemu-nbd the options to set the second label are awkward.  You can
> create the socket in a wrapper program and then exec into qemu-nbd.
> Or you could try something with LD_PRELOAD.
> 
> This commit adds the ability to set the label straightforwardly on the
> command line, via the new --selinux-label flag.  (The name of the flag
> is the same as the equivalent nbdkit option.)
> 
> A worked example showing how to use the new option can be found in
> this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> Signed-off-by: Richard W.M. Jones 
> ---

I'm making one tweak to your patch before sending the pull request:

> +++ b/qemu-nbd.c
> @@ -64,6 +68,7 @@
>  #define QEMU_NBD_OPT_FORK  263
>  #define QEMU_NBD_OPT_TLSAUTHZ  264
>  #define QEMU_NBD_OPT_PID_FILE  265
> +#define QEMU_NBD_OPT_SELINUX_LABEL 266
>  
>  #define MBR_SIZE 512
>  
> @@ -116,6 +121,9 @@ static void usage(const char *name)
>  "  --forkfork off the server process and exit the 
> parent\n"
>  "once the server is running\n"
>  "  --pid-file=PATH   store the server's process ID in the given 
> file\n"
> +#ifdef CONFIG_SELINUX
> +"  --selinux-label=LABEL set SELinux process label on listening socket\n"
> +#endif

The new option is only conditionally advertised under --help (qemu-nbd
lacks a stable machine-parseable output, so scraping --help output
will have to do for now)...

>  #if HAVE_NBD_DEVICE
>  "\n"
>  "Kernel NBD client support:\n"
> @@ -532,6 +540,8 @@ int main(int argc, char **argv)
>  { "trace", required_argument, NULL, 'T' },
>  { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
>  { "pid-file", required_argument, NULL, QEMU_NBD_OPT_PID_FILE },
> +{ "selinux-label", required_argument, NULL,
> +  QEMU_NBD_OPT_SELINUX_LABEL },

...but is unconditionally supported as a long option even when support
was not compiled in...

>  { NULL, 0, NULL, 0 }
>  };
>  int ch;
> @@ -558,6 +568,7 @@ int main(int argc, char **argv)
>  int old_stderr = -1;
>  unsigned socket_activation;
>  const char *pid_file_name = NULL;
> +const char *selinux_label = NULL;
>  BlockExportOptions *export_opts;
>  
>  #ifdef CONFIG_POSIX
> @@ -747,6 +758,9 @@ int main(int argc, char **argv)
>  case QEMU_NBD_OPT_PID_FILE:
>  pid_file_name = optarg;
>  break;
> +case QEMU_NBD_OPT_SELINUX_LABEL:
> +selinux_label = optarg;
> +break;
>  }
>  }
>  
> @@ -938,6 +952,16 @@ int main(int argc, char **argv)
>  } else {
>  backlog = MIN(shared, SOMAXCONN);
>  }
> +if (sockpath && selinux_label) {
> +#ifdef CONFIG_SELINUX
> +if (setsockcreatecon_raw(selinux_label) == -1) {
> +error_report("Cannot set SELinux socket create context "
> + "to %s: %s",
> + selinux_label, strerror(errno));
> +exit(EXIT_FAILURE);
> +}
> +#endif

...but here we silently ignore it if support is not compiled in.
Better is to issue an error message about using an unsupported option,
so I'll squash this in:

diff --git i/qemu-nbd.c w/qemu-nbd.c
index 5dc82c419255..94f8ec07c064 100644
--- i/qemu-nbd.c
+++ w/qemu-nbd.c
@@ -962,6 +962,9 @@ int main(int argc, char **argv)
  selinux_label, strerror(errno));
 exit(EXIT_FAILURE);
 }
+#else
+error_report("SELinux support not enabled in this binary");
+exit(EXIT_FAILURE);
 #endif
 }
 saddr = nbd_build_socket_address(sockpath, bindto, port);
@@ -978,6 +981,9 @@ int main(int argc, char **argv)
  strerror(errno));
 exit(EXIT_FAILURE);
 }
+#else
+error_report("SELinux support not enabled in this binary");
+exit(EXIT_FAILURE);
 #endif
 }
 } else {


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-09-27 Thread Daniel P . Berrangé 
On Wed, Aug 25, 2021 at 02:35:04PM -0500, Eric Blake wrote:
> On Fri, Jul 23, 2021 at 05:38:06PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote:
> > > Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben:
> > > > Under SELinux, Unix domain sockets have two labels.  One is on the
> > > > disk and can be set with commands such as chcon(1).  There is a
> > > > different label stored in memory (called the process label).  This can
> > > > only be set by the process creating the socket.  When using SELinux +
> > > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> > > > you must set both labels correctly first.
> > > > 
> > > > For qemu-nbd the options to set the second label are awkward.  You can
> > > > create the socket in a wrapper program and then exec into qemu-nbd.
> > > > Or you could try something with LD_PRELOAD.
> > > > 
> > > > This commit adds the ability to set the label straightforwardly on the
> > > > command line, via the new --selinux-label flag.  (The name of the flag
> > > > is the same as the equivalent nbdkit option.)
> > > > 
> > > > A worked example showing how to use the new option can be found in
> > > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > > > 
> > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > > > Signed-off-by: Richard W.M. Jones 
> > > 
> > > I suppose this would also be relevant for the built-in NBD server,
> > > especially in the context of qemu-storage-daemon?
> > 
> > It depends on the usage scenario really. nbdkit / qemu-nbd are
> > not commonly run under any SELinux policy, so then end up being
> > unconfined_t. A QEMU NBD client can't connect to an unconfined_t
> > socket, so we need to override it with this arg.
> > 
> > In the case of qemu system emulator, under libvirt, it will
> > already have a svirt_t type, so in that case there is no need
> > to override the type for the socket.
> > 
> > For qsd there's not really any strong practice established
> > but i expect most current usage is unconfined_t too and
> > would benefit from setting label.
> > 
> > > If so, is this something specific to NBD sockets, or would it actually
> > > make sense to have it as a generic option in UnixSocketAddress?
> > 
> > It is applicable to inet sockets too in fact.
> 
> So now that 6.2 is open, should I queue the patch as is, or wait for a
> v3 that makes the option more generic to all socket usage?

My gut feeling is that it makes sense to have a more generic option,
with the selinux label specified in the "SocketAddress" QAPI type,
and then have util/qemu-sockets.h be setting the context in
socket_listen().

I don't think this invalidates the patch that Richard proprosed
here, as we'll still need the command line argument he's added.
All that will differ is that the setsockcreatecon_raw will get
moved down.

So from that POV, I don't think we need the general solution to
be a blocker, it can be additive.

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: [PATCH v2] nbd/server: Add --selinux-label option

2021-09-27 Thread Vladimir Sementsov-Ogievskiy 

24.09.2021 22:23, Eric Blake wrote:

Ping

On Wed, Aug 25, 2021 at 02:35:04PM -0500, Eric Blake wrote:

On Fri, Jul 23, 2021 at 05:38:06PM +0100, Daniel P. Berrangé wrote:

On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote:

Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben:

Under SELinux, Unix domain sockets have two labels.  One is on the
disk and can be set with commands such as chcon(1).  There is a
different label stored in memory (called the process label).  This can
only be set by the process creating the socket.  When using SELinux +
SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
you must set both labels correctly first.

For qemu-nbd the options to set the second label are awkward.  You can
create the socket in a wrapper program and then exec into qemu-nbd.
Or you could try something with LD_PRELOAD.

This commit adds the ability to set the label straightforwardly on the
command line, via the new --selinux-label flag.  (The name of the flag
is the same as the equivalent nbdkit option.)

A worked example showing how to use the new option can be found in
this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
Signed-off-by: Richard W.M. Jones 


I suppose this would also be relevant for the built-in NBD server,
especially in the context of qemu-storage-daemon?


It depends on the usage scenario really. nbdkit / qemu-nbd are
not commonly run under any SELinux policy, so then end up being
unconfined_t. A QEMU NBD client can't connect to an unconfined_t
socket, so we need to override it with this arg.

In the case of qemu system emulator, under libvirt, it will
already have a svirt_t type, so in that case there is no need
to override the type for the socket.

For qsd there's not really any strong practice established
but i expect most current usage is unconfined_t too and
would benefit from setting label.


If so, is this something specific to NBD sockets, or would it actually
make sense to have it as a generic option in UnixSocketAddress?


It is applicable to inet sockets too in fact.


If so, should patch at least be changed to call setsockcreatecon_raw() for inet 
sockets as well?

With current code selinux_label is silently ignored in that case.



So now that 6.2 is open, should I queue the patch as is, or wait for a
v3 that makes the option more generic to all socket usage?





--
Best regards,
Vladimir

Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-09-24 Thread Eric Blake 
Ping

On Wed, Aug 25, 2021 at 02:35:04PM -0500, Eric Blake wrote:
> On Fri, Jul 23, 2021 at 05:38:06PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote:
> > > Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben:
> > > > Under SELinux, Unix domain sockets have two labels.  One is on the
> > > > disk and can be set with commands such as chcon(1).  There is a
> > > > different label stored in memory (called the process label).  This can
> > > > only be set by the process creating the socket.  When using SELinux +
> > > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> > > > you must set both labels correctly first.
> > > > 
> > > > For qemu-nbd the options to set the second label are awkward.  You can
> > > > create the socket in a wrapper program and then exec into qemu-nbd.
> > > > Or you could try something with LD_PRELOAD.
> > > > 
> > > > This commit adds the ability to set the label straightforwardly on the
> > > > command line, via the new --selinux-label flag.  (The name of the flag
> > > > is the same as the equivalent nbdkit option.)
> > > > 
> > > > A worked example showing how to use the new option can be found in
> > > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > > > 
> > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > > > Signed-off-by: Richard W.M. Jones 
> > > 
> > > I suppose this would also be relevant for the built-in NBD server,
> > > especially in the context of qemu-storage-daemon?
> > 
> > It depends on the usage scenario really. nbdkit / qemu-nbd are
> > not commonly run under any SELinux policy, so then end up being
> > unconfined_t. A QEMU NBD client can't connect to an unconfined_t
> > socket, so we need to override it with this arg.
> > 
> > In the case of qemu system emulator, under libvirt, it will
> > already have a svirt_t type, so in that case there is no need
> > to override the type for the socket.
> > 
> > For qsd there's not really any strong practice established
> > but i expect most current usage is unconfined_t too and
> > would benefit from setting label.
> > 
> > > If so, is this something specific to NBD sockets, or would it actually
> > > make sense to have it as a generic option in UnixSocketAddress?
> > 
> > It is applicable to inet sockets too in fact.
> 
> So now that 6.2 is open, should I queue the patch as is, or wait for a
> v3 that makes the option more generic to all socket usage?
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-08-25 Thread Eric Blake 
On Fri, Jul 23, 2021 at 05:38:06PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote:
> > Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben:
> > > Under SELinux, Unix domain sockets have two labels.  One is on the
> > > disk and can be set with commands such as chcon(1).  There is a
> > > different label stored in memory (called the process label).  This can
> > > only be set by the process creating the socket.  When using SELinux +
> > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> > > you must set both labels correctly first.
> > > 
> > > For qemu-nbd the options to set the second label are awkward.  You can
> > > create the socket in a wrapper program and then exec into qemu-nbd.
> > > Or you could try something with LD_PRELOAD.
> > > 
> > > This commit adds the ability to set the label straightforwardly on the
> > > command line, via the new --selinux-label flag.  (The name of the flag
> > > is the same as the equivalent nbdkit option.)
> > > 
> > > A worked example showing how to use the new option can be found in
> > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > > 
> > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > > Signed-off-by: Richard W.M. Jones 
> > 
> > I suppose this would also be relevant for the built-in NBD server,
> > especially in the context of qemu-storage-daemon?
> 
> It depends on the usage scenario really. nbdkit / qemu-nbd are
> not commonly run under any SELinux policy, so then end up being
> unconfined_t. A QEMU NBD client can't connect to an unconfined_t
> socket, so we need to override it with this arg.
> 
> In the case of qemu system emulator, under libvirt, it will
> already have a svirt_t type, so in that case there is no need
> to override the type for the socket.
> 
> For qsd there's not really any strong practice established
> but i expect most current usage is unconfined_t too and
> would benefit from setting label.
> 
> > If so, is this something specific to NBD sockets, or would it actually
> > make sense to have it as a generic option in UnixSocketAddress?
> 
> It is applicable to inet sockets too in fact.

So now that 6.2 is open, should I queue the patch as is, or wait for a
v3 that makes the option more generic to all socket usage?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-07-26 Thread Eric Blake 
On Fri, Jul 23, 2021 at 11:47:51AM +0100, Daniel P. Berrangé wrote:
> On Fri, Jul 23, 2021 at 11:33:03AM +0100, Richard W.M. Jones wrote:
> > Under SELinux, Unix domain sockets have two labels.  One is on the
> > disk and can be set with commands such as chcon(1).  There is a
> > different label stored in memory (called the process label).  This can
> > only be set by the process creating the socket.  When using SELinux +
> > SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> > you must set both labels correctly first.
> > 
> > For qemu-nbd the options to set the second label are awkward.  You can
> > create the socket in a wrapper program and then exec into qemu-nbd.
> > Or you could try something with LD_PRELOAD.
> > 
> > This commit adds the ability to set the label straightforwardly on the
> > command line, via the new --selinux-label flag.  (The name of the flag
> > is the same as the equivalent nbdkit option.)
> > 
> > A worked example showing how to use the new option can be found in
> > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > Signed-off-by: Richard W.M. Jones 
> > ---
> >  configure |  9 -
> >  meson.build   | 10 +-
> >  meson_options.txt |  3 ++
> >  qemu-nbd.c| 33 +++
> >  tests/docker/dockerfiles/centos8.docker   |  1 +
> >  tests/docker/dockerfiles/fedora.docker|  1 +
> >  tests/docker/dockerfiles/opensuse-leap.docker |  1 +
> >  tests/docker/dockerfiles/ubuntu1804.docker|  1 +
> >  tests/docker/dockerfiles/ubuntu2004.docker|  1 +
> >  9 files changed, 58 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé 

Thanks. This is a new feature, so it doesn't qualify for inclusion in
6.1, but I'm queuing it through my NBD tree to go in as soon as
upstream reopens for 6.2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-07-23 Thread Daniel P . Berrangé 
On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote:
> Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben:
> > Under SELinux, Unix domain sockets have two labels.  One is on the
> > disk and can be set with commands such as chcon(1).  There is a
> > different label stored in memory (called the process label).  This can
> > only be set by the process creating the socket.  When using SELinux +
> > SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> > you must set both labels correctly first.
> > 
> > For qemu-nbd the options to set the second label are awkward.  You can
> > create the socket in a wrapper program and then exec into qemu-nbd.
> > Or you could try something with LD_PRELOAD.
> > 
> > This commit adds the ability to set the label straightforwardly on the
> > command line, via the new --selinux-label flag.  (The name of the flag
> > is the same as the equivalent nbdkit option.)
> > 
> > A worked example showing how to use the new option can be found in
> > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > Signed-off-by: Richard W.M. Jones 
> 
> I suppose this would also be relevant for the built-in NBD server,
> especially in the context of qemu-storage-daemon?

It depends on the usage scenario really. nbdkit / qemu-nbd are
not commonly run under any SELinux policy, so then end up being
unconfined_t. A QEMU NBD client can't connect to an unconfined_t
socket, so we need to override it with this arg.

In the case of qemu system emulator, under libvirt, it will
already have a svirt_t type, so in that case there is no need
to override the type for the socket.

For qsd there's not really any strong practice established
but i expect most current usage is unconfined_t too and
would benefit from setting label.

> If so, is this something specific to NBD sockets, or would it actually
> make sense to have it as a generic option in UnixSocketAddress?

It is applicable to inet sockets too in fact.


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: [PATCH v2] nbd/server: Add --selinux-label option

2021-07-23 Thread Richard W.M. Jones 
On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote:
> Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben:
> > Under SELinux, Unix domain sockets have two labels.  One is on the
> > disk and can be set with commands such as chcon(1).  There is a
> > different label stored in memory (called the process label).  This can
> > only be set by the process creating the socket.  When using SELinux +
> > SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> > you must set both labels correctly first.
> > 
> > For qemu-nbd the options to set the second label are awkward.  You can
> > create the socket in a wrapper program and then exec into qemu-nbd.
> > Or you could try something with LD_PRELOAD.
> > 
> > This commit adds the ability to set the label straightforwardly on the
> > command line, via the new --selinux-label flag.  (The name of the flag
> > is the same as the equivalent nbdkit option.)
> > 
> > A worked example showing how to use the new option can be found in
> > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > Signed-off-by: Richard W.M. Jones 
> 
> I suppose this would also be relevant for the built-in NBD server,
> especially in the context of qemu-storage-daemon?
>
> If so, is this something specific to NBD sockets, or would it actually
> make sense to have it as a generic option in UnixSocketAddress?

For other NBD sockets, most likely.  I'm not sure about Unix sockets
in general (as in: I know they also have the two label thing, but I
don't know if there's a situation where SVirt protects other sockets
apart from NBD sockets).  I'm sure Dan will know ...

By the way although it appears that setsockcreatecon_raw is setting a
global flag, it seems to actually use a thread-local variable, so
implementing this (although still ugly) would not require locks.

https://github.com/SELinuxProject/selinux/blob/32611aea6543e3a8f32635857e37b4332b0b5c99/libselinux/src/procattr.c#L347

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-07-23 Thread Kevin Wolf 
Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben:
> Under SELinux, Unix domain sockets have two labels.  One is on the
> disk and can be set with commands such as chcon(1).  There is a
> different label stored in memory (called the process label).  This can
> only be set by the process creating the socket.  When using SELinux +
> SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> you must set both labels correctly first.
> 
> For qemu-nbd the options to set the second label are awkward.  You can
> create the socket in a wrapper program and then exec into qemu-nbd.
> Or you could try something with LD_PRELOAD.
> 
> This commit adds the ability to set the label straightforwardly on the
> command line, via the new --selinux-label flag.  (The name of the flag
> is the same as the equivalent nbdkit option.)
> 
> A worked example showing how to use the new option can be found in
> this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> Signed-off-by: Richard W.M. Jones 

I suppose this would also be relevant for the built-in NBD server,
especially in the context of qemu-storage-daemon?

If so, is this something specific to NBD sockets, or would it actually
make sense to have it as a generic option in UnixSocketAddress?

Kevin

Re: [PATCH v2] nbd/server: Add --selinux-label option

2021-07-23 Thread Daniel P . Berrangé 
On Fri, Jul 23, 2021 at 11:33:03AM +0100, Richard W.M. Jones wrote:
> Under SELinux, Unix domain sockets have two labels.  One is on the
> disk and can be set with commands such as chcon(1).  There is a
> different label stored in memory (called the process label).  This can
> only be set by the process creating the socket.  When using SELinux +
> SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> you must set both labels correctly first.
> 
> For qemu-nbd the options to set the second label are awkward.  You can
> create the socket in a wrapper program and then exec into qemu-nbd.
> Or you could try something with LD_PRELOAD.
> 
> This commit adds the ability to set the label straightforwardly on the
> command line, via the new --selinux-label flag.  (The name of the flag
> is the same as the equivalent nbdkit option.)
> 
> A worked example showing how to use the new option can be found in
> this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> Signed-off-by: Richard W.M. Jones 
> ---
>  configure |  9 -
>  meson.build   | 10 +-
>  meson_options.txt |  3 ++
>  qemu-nbd.c| 33 +++
>  tests/docker/dockerfiles/centos8.docker   |  1 +
>  tests/docker/dockerfiles/fedora.docker|  1 +
>  tests/docker/dockerfiles/opensuse-leap.docker |  1 +
>  tests/docker/dockerfiles/ubuntu1804.docker|  1 +
>  tests/docker/dockerfiles/ubuntu2004.docker|  1 +
>  9 files changed, 58 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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 :|

[PATCH v2] nbd/server: Add --selinux-label option

2021-07-23 Thread Richard W.M. Jones 
Under SELinux, Unix domain sockets have two labels.  One is on the
disk and can be set with commands such as chcon(1).  There is a
different label stored in memory (called the process label).  This can
only be set by the process creating the socket.  When using SELinux +
SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
you must set both labels correctly first.

For qemu-nbd the options to set the second label are awkward.  You can
create the socket in a wrapper program and then exec into qemu-nbd.
Or you could try something with LD_PRELOAD.

This commit adds the ability to set the label straightforwardly on the
command line, via the new --selinux-label flag.  (The name of the flag
is the same as the equivalent nbdkit option.)

A worked example showing how to use the new option can be found in
this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
Signed-off-by: Richard W.M. Jones 
---
 configure |  9 -
 meson.build   | 10 +-
 meson_options.txt |  3 ++
 qemu-nbd.c| 33 +++
 tests/docker/dockerfiles/centos8.docker   |  1 +
 tests/docker/dockerfiles/fedora.docker|  1 +
 tests/docker/dockerfiles/opensuse-leap.docker |  1 +
 tests/docker/dockerfiles/ubuntu1804.docker|  1 +
 tests/docker/dockerfiles/ubuntu2004.docker|  1 +
 9 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index b5965b159f..7e04bd485f 100755
--- a/configure
+++ b/configure
@@ -443,6 +443,7 @@ fuse="auto"
 fuse_lseek="auto"
 multiprocess="auto"
 slirp_smbd="$default_feature"
+selinux="auto"
 
 malloc_trim="auto"
 gio="$default_feature"
@@ -1578,6 +1579,10 @@ for opt do
   ;;
   --disable-slirp-smbd) slirp_smbd=no
   ;;
+  --enable-selinux) selinux="enabled"
+  ;;
+  --disable-selinux) selinux="disabled"
+  ;;
   *)
   echo "ERROR: unknown option $opt"
   echo "Try '$0 --help' for more information"
@@ -1965,6 +1970,7 @@ disabled with --disable-FEATURE, default is enabled if 
available
   multiprocessOut of process device emulation support
   gio libgio support
   slirp-smbd  use smbd (at path --smbd=*) in slirp networking
+  selinux SELinux support in qemu-nbd
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -5220,7 +5226,8 @@ if test "$skip_meson" = no; then
 -Dattr=$attr -Ddefault_devices=$default_devices 
-Dvirglrenderer=$virglrenderer \
 -Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
 -Dvhost_user_blk_server=$vhost_user_blk_server 
-Dmultiprocess=$multiprocess \
--Dfuse=$fuse -Dfuse_lseek=$fuse_lseek 
-Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
+-Dfuse=$fuse -Dfuse_lseek=$fuse_lseek 
-Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf \
+-Dselinux=$selinux \
 $(if test "$default_features" = no; then echo 
"-Dauto_features=disabled"; fi) \
-Dtcg_interpreter=$tcg_interpreter \
 $cross_arg \
diff --git a/meson.build b/meson.build
index 2f377098d7..2d7206233e 100644
--- a/meson.build
+++ b/meson.build
@@ -1064,6 +1064,11 @@ keyutils = dependency('libkeyutils', required: false,
 
 has_gettid = cc.has_function('gettid')
 
+# libselinux
+selinux = dependency('libselinux',
+ required: get_option('selinux'),
+ method: 'pkg-config', kwargs: static_kwargs)
+
 # Malloc tests
 
 malloc = []
@@ -1291,6 +1296,7 @@ config_host_data.set('CONFIG_FUSE', fuse.found())
 config_host_data.set('CONFIG_FUSE_LSEEK', fuse_lseek.found())
 config_host_data.set('CONFIG_X11', x11.found())
 config_host_data.set('CONFIG_CFI', get_option('cfi'))
+config_host_data.set('CONFIG_SELINUX', selinux.found())
 config_host_data.set('QEMU_VERSION', '"@0@"'.format(meson.project_version()))
 config_host_data.set('QEMU_VERSION_MAJOR', 
meson.project_version().split('.')[0])
 config_host_data.set('QEMU_VERSION_MINOR', 
meson.project_version().split('.')[1])
@@ -2739,7 +2745,8 @@ if have_tools
   qemu_io = executable('qemu-io', files('qemu-io.c'),
  dependencies: [block, qemuutil], install: true)
   qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
-   dependencies: [blockdev, qemuutil, gnutls], install: true)
+   dependencies: [blockdev, qemuutil, gnutls, selinux],
+   install: true)
 
   subdir('storage-daemon')
   subdir('contrib/rdmacm-mux')
@@ -3104,6 +3111,7 @@ summary_info += {'libpmem support':   libpmem.found()}
 summary_info += {'libdaxctl support': libdaxctl.found()}
 summary_info += {'libudev':   libudev.found()}
 summary_info += {'FUSE lseek':fuse_lseek.found()}
+summary_info += {'selinux':   selinux.found()}
 summary(summary_info, bool_yn: true, section: 'Dependencies')
 
 if not supported_cpus.contains(cpu)

[PATCH v2] nbd/server: Add --selinux-label option

2021-07-23 Thread Richard W.M. Jones 
v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2021-07/threads.html#00713

v2 adds the changes to CI docker files as suggested by
Dan Berrange in his review.

Rich.