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

[Eric Blake]

On Thu, Sep 30, 2021 at 11:54:58AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 9/30/21 11:47, 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 <rjones@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> this should be Reviewed-by?

In this case, I think S-o-b is actually correct: I did make some
tweaks to Rich's v2 while preparing my pull request, so Rich is
crediting my addition to his work.  And at the time of my pull request
that included his v2 (before it got dropped for 32-bit build
problems), I had not actually sent my R-b, because I was already
trusting the R-b present from other reviewers.

Oddly enough, even if Rich had dropped my S-o-b line, it will still
eventually reappear, since I'll be queuing this patch through my NBD
tree which requires me to touch it again.  So already having it now
doesn't hurt.

[Many of the patches that go through my tree end up with both my R-b
and S-o-b; but there are patches where I have S-o-b but not R-b,
because I trusted the review of others, and view the act of a careful
review as orthogonal from the responsibility of touching a patch
enough to include it in a pull request]

> 
> > ---
> >   configure                                     |  8 +++-
> >   meson.build                                   | 10 ++++-
> >   meson_options.txt                             |  3 ++
> >   qemu-nbd.c                                    | 39 +++++++++++++++++++
> 
> [..]
> 
> >       }
> > @@ -938,6 +952,19 @@ int main(int argc, char **argv)
> >           } else {
> >               backlog = MIN(shared, SOMAXCONN);
> >           }
> > +        if (sockpath && selinux_label) {
> 
> 1. Why only for unix sockets?
> 
> 2. If [1] is intentional, why silently ignore the new option for ip sockets, 
> shouldn't error-out instead?
>

Good point, and I missed it in v2, in spite of my touching that patch
to avoid silent ignoring when selinux support was not compiled in.

So at this point, I'm less certain whether it is smarter to reject
--selinux-label on non-unix sockets, or whether we try to do the
labeling regardless of socket type; and thus consider it premature for
me to give R-b until we have that resolved.

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