We switched to opening mode='bind' sockets ourselves:
commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5
qemu: support passing pre-opened UNIX socket listen FD
in v4.5.0-rc1~251
Then fixed qemuBuildChrChardevStr to change libvirtd's label
while creating the socket:
commit b0c6300fc42bbc3e5eb0b236392f7344581c5810
qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
v4.5.0-rc1~52
Also add labeling of these sockets to the DAC driver.
Instead of trying to figure out which one was created by libvirt,
label it if it exists.
https://bugzilla.redhat.com/show_bug.cgi?id=1633389
Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
src/security/security_dac.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 62442745dd..da4a6c72fe 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
break;
case VIR_DOMAIN_CHR_TYPE_UNIX:
- if (!dev_source->data.nix.listen) {
+ if (!dev_source->data.nix.listen ||
+ (dev_source->data.nix.path &&
+ virFileExists(dev_source->data.nix.path))) {
+ /* Also label mode='bind' sockets if they exist,
+ * e.g. because they were created by libvirt
+ * and passed via FD */
if (virSecurityDACSetOwnership(mgr, NULL,
dev_source->data.nix.path,
user, group) < 0)
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote:
> We switched to opening mode='bind' sockets ourselves:
> commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5
> qemu: support passing pre-opened UNIX socket listen FD
> in v4.5.0-rc1~251
>
> Then fixed qemuBuildChrChardevStr to change libvirtd's label
> while creating the socket:
> commit b0c6300fc42bbc3e5eb0b236392f7344581c5810
> qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
> v4.5.0-rc1~52
>
> Also add labeling of these sockets to the DAC driver.
> Instead of trying to figure out which one was created by libvirt,
> label it if it exists.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1633389
>
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
> src/security/security_dac.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 62442745dd..da4a6c72fe 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
> break;
>
> case VIR_DOMAIN_CHR_TYPE_UNIX:
> - if (!dev_source->data.nix.listen) {
> + if (!dev_source->data.nix.listen ||
> + (dev_source->data.nix.path &&
> + virFileExists(dev_source->data.nix.path))) {
> + /* Also label mode='bind' sockets if they exist,
> + * e.g. because they were created by libvirt
> + * and passed via FD */
> if (virSecurityDACSetOwnership(mgr, NULL,
> dev_source->data.nix.path,
> user, group) < 0)
So we're labeling path even if nix.listen == false, shouldn't we check for the
file's existence too? Or have we already done it and I missed this fact?
Basically what I'm aiming at is that nix.path will always be set at this point,
either explicitly by the user (most cases) or it would have been generated by
us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. I'm just simply wondering whether the
condition cannot be further simplified to:
if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0)
goto done;
ret = 0;
break;
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 01, 2018 at 10:22:59AM +0200, Erik Skultety wrote:
>On Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote:
>> We switched to opening mode='bind' sockets ourselves:
>> commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5
>> qemu: support passing pre-opened UNIX socket listen FD
>> in v4.5.0-rc1~251
>>
>> Then fixed qemuBuildChrChardevStr to change libvirtd's label
>> while creating the socket:
>> commit b0c6300fc42bbc3e5eb0b236392f7344581c5810
>> qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
>> v4.5.0-rc1~52
>>
>> Also add labeling of these sockets to the DAC driver.
>> Instead of trying to figure out which one was created by libvirt,
>> label it if it exists.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1633389
>>
>> Signed-off-by: Ján Tomko <jtomko@redhat.com>
>> ---
>> src/security/security_dac.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>> index 62442745dd..da4a6c72fe 100644
>> --- a/src/security/security_dac.c
>> +++ b/src/security/security_dac.c
>> @@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
>> break;
>>
>> case VIR_DOMAIN_CHR_TYPE_UNIX:
>> - if (!dev_source->data.nix.listen) {
>> + if (!dev_source->data.nix.listen ||
>> + (dev_source->data.nix.path &&
>> + virFileExists(dev_source->data.nix.path))) {
>> + /* Also label mode='bind' sockets if they exist,
>> + * e.g. because they were created by libvirt
>> + * and passed via FD */
>> if (virSecurityDACSetOwnership(mgr, NULL,
>> dev_source->data.nix.path,
>> user, group) < 0)
>
>So we're labeling path even if nix.listen == false, shouldn't we check for the
>file's existence too? Or have we already done it and I missed this fact?
For mode='connect', the path not existing is fatal, so we can safely
call virSecurityDACSetOwnership and let it fail.
For mode='bind', the path not existing is valid for QEMUs which do not
support FD passing for chardevs. If QEMU supports FD passing, the
path should exist because libvirt created it earlier. I could not think
of a nicer idea on how to pass down the logic that decides whether
libvirt pre-creates it in qemuBuildChrChardevStr (which, of course, is
an odd place to be creating sockets), so I opted to use the file's
existence as a witness of libvirt pre-creating it. Which is what I tried
to say by this commit message snippet:
>> Instead of trying to figure out which one was created by libvirt,
>> label it if it exists.
Shall I reword the message? E.g.:
Instead of duplicating the logic which decides whether libvirt should
pre-create the socket, assume an existing path means it was created by
libvirt.
>Basically what I'm aiming at is that nix.path will always be set at this point,
Yes, the dev_source->data.nix.path condition might be superfluous. I was
being overly cautious.
As evidenced by:
commit 614193fac67445a7e92bf620ffef726ed1bd6f07
conf: Fix check for chardev source path
An empty path should be caught by our validation.
>either explicitly by the user (most cases) or it would have been generated by
>us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or
>VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN.
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN |
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN is still
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. ;)
>I'm just simply wondering whether the
>condition cannot be further simplified to:
>
>if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0)
This introduces a change in behavior for mode='connect' - the DAC driver
will no longer report an error for a non-existent path.
I have not checked whether it's checked anywhere else (possibly the
SELinux driver), but it is a legitimate error and I don't see the need
to skip it.
Moreover, this hides the reason for using virFileExists in the first
place - it's only needed for type='listen' sockets.
Hope that answers your questions.
Looking forward to hearing from you.
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 01, 2018 at 01:14:34PM +0200, Ján Tomko wrote:
> On Mon, Oct 01, 2018 at 10:22:59AM +0200, Erik Skultety wrote:
> > On Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote:
> > > We switched to opening mode='bind' sockets ourselves:
> > > commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5
> > > qemu: support passing pre-opened UNIX socket listen FD
> > > in v4.5.0-rc1~251
> > >
> > > Then fixed qemuBuildChrChardevStr to change libvirtd's label
> > > while creating the socket:
> > > commit b0c6300fc42bbc3e5eb0b236392f7344581c5810
> > > qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels
> > > v4.5.0-rc1~52
> > >
> > > Also add labeling of these sockets to the DAC driver.
> > > Instead of trying to figure out which one was created by libvirt,
> > > label it if it exists.
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1633389
> > >
> > > Signed-off-by: Ján Tomko <jtomko@redhat.com>
> > > ---
> > > src/security/security_dac.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> > > index 62442745dd..da4a6c72fe 100644
> > > --- a/src/security/security_dac.c
> > > +++ b/src/security/security_dac.c
> > > @@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr,
> > > break;
> > >
> > > case VIR_DOMAIN_CHR_TYPE_UNIX:
> > > - if (!dev_source->data.nix.listen) {
> > > + if (!dev_source->data.nix.listen ||
> > > + (dev_source->data.nix.path &&
> > > + virFileExists(dev_source->data.nix.path))) {
> > > + /* Also label mode='bind' sockets if they exist,
> > > + * e.g. because they were created by libvirt
> > > + * and passed via FD */
> > > if (virSecurityDACSetOwnership(mgr, NULL,
> > > dev_source->data.nix.path,
> > > user, group) < 0)
> >
> > So we're labeling path even if nix.listen == false, shouldn't we check for the
> > file's existence too? Or have we already done it and I missed this fact?
>
> For mode='connect', the path not existing is fatal, so we can safely
> call virSecurityDACSetOwnership and let it fail.
>
> For mode='bind', the path not existing is valid for QEMUs which do not
> support FD passing for chardevs. If QEMU supports FD passing, the
> path should exist because libvirt created it earlier. I could not think
> of a nicer idea on how to pass down the logic that decides whether
> libvirt pre-creates it in qemuBuildChrChardevStr (which, of course, is
> an odd place to be creating sockets), so I opted to use the file's
> existence as a witness of libvirt pre-creating it. Which is what I tried
> to say by this commit message snippet:
> > > Instead of trying to figure out which one was created by libvirt,
> > > label it if it exists.
>
> Shall I reword the message? E.g.:
> Instead of duplicating the logic which decides whether libvirt should
> pre-create the socket, assume an existing path means it was created by
> libvirt.
s/means/meaning that
...otherwise sounds meaningful...
>
>
> > Basically what I'm aiming at is that nix.path will always be set at this point,
>
> Yes, the dev_source->data.nix.path condition might be superfluous. I was
> being overly cautious.
>
> As evidenced by:
> commit 614193fac67445a7e92bf620ffef726ed1bd6f07
> conf: Fix check for chardev source path
> An empty path should be caught by our validation.
>
> > either explicitly by the user (most cases) or it would have been generated by
> > us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or
> > VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN.
>
> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN |
> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN is still
> VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. ;)
>
> > I'm just simply wondering whether the
> > condition cannot be further simplified to:
> >
> > if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0)
>
> This introduces a change in behavior for mode='connect' - the DAC driver
> will no longer report an error for a non-existent path.
> I have not checked whether it's checked anywhere else (possibly the
> SELinux driver), but it is a legitimate error and I don't see the need
> to skip it.
>
> Moreover, this hides the reason for using virFileExists in the first
> place - it's only needed for type='listen' sockets.
Fair enough, let's got with your proposed change then (wait for after the
release though):
Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
> Hope that answers your questions.
> Looking forward to hearing from you.
>
> Jano
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/27/2018 05:02 PM, Ján Tomko wrote: > We switched to opening mode='bind' sockets ourselves: > commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5 > qemu: support passing pre-opened UNIX socket listen FD > in v4.5.0-rc1~251 > > Then fixed qemuBuildChrChardevStr to change libvirtd's label > while creating the socket: > commit b0c6300fc42bbc3e5eb0b236392f7344581c5810 > qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels > v4.5.0-rc1~52 > > Also add labeling of these sockets to the DAC driver. > Instead of trying to figure out which one was created by libvirt, > label it if it exists. > > https://bugzilla.redhat.com/show_bug.cgi?id=1633389 > > Signed-off-by: Ján Tomko <jtomko@redhat.com> > --- > src/security/security_dac.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) How come SELinux is not affected? We shouldn't rely on default policy doing the right thing. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Oct 01, 2018 at 10:34:38AM +0200, Michal Privoznik wrote: >On 09/27/2018 05:02 PM, Ján Tomko wrote: >> We switched to opening mode='bind' sockets ourselves: >> commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5 >> qemu: support passing pre-opened UNIX socket listen FD >> in v4.5.0-rc1~251 >> >> Then fixed qemuBuildChrChardevStr to change libvirtd's label >> while creating the socket: >> commit b0c6300fc42bbc3e5eb0b236392f7344581c5810 >> qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels >> v4.5.0-rc1~52 >> >> Also add labeling of these sockets to the DAC driver. >> Instead of trying to figure out which one was created by libvirt, >> label it if it exists. >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1633389 >> >> Signed-off-by: Ján Tomko <jtomko@redhat.com> >> --- >> src/security/security_dac.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) > >How come SELinux is not affected? We shouldn't rely on default policy >doing the right thing. As mentioned in the commit message, the SELinux label is set before the socket creation since the commit mentioned in the commit message. Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.