[ovs-dev] rhel: if rpms were built without libcapng then let processrs to run as root
diff mbox series

Message ID 20190416012441.24625-1-aatteka@ovn.org
State New
Headers show
Series
  • [ovs-dev] rhel: if rpms were built without libcapng then let processrs to run as root
Related show

Commit Message

Ansis Atteka April 16, 2019, 1:24 a.m. UTC
Otherwise, Open vSwitch will fail to start with the following
error "libcap-ng is not configured at compile time" when it
attempts to downgrade to Open vSwitch user.

Also, if packages were built in a way where processes are
supposed to be running only as root, then there is no point
in creating "openvswitch" user in the first place.

Signed-off-by: Ansis Atteka <aatteka@ovn.org>
---
 poc/playbook-fedora-builder.yml | 6 +++---
 rhel/openvswitch-fedora.spec.in | 8 ++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

0-day Robot April 16, 2019, 2:50 a.m. UTC | #1
Bleep bloop.  Greetings Ansis Atteka, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 89 characters long (recommended limit is 79)
#29 FILE: poc/playbook-fedora-builder.yml:102:
    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec

WARNING: Line is 89 characters long (recommended limit is 79)
#35 FILE: poc/playbook-fedora-builder.yml:107:
    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec

WARNING: Line is 87 characters long (recommended limit is 79)
#41 FILE: poc/playbook-fedora-builder.yml:112:
    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-dkms.spec

Lines checked: 91, Warnings: 3, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Darrell Ball April 16, 2019, 6:21 a.m. UTC | #2
Thanks for the fix

1/ Main changes to openvswitch-fedora.spec.in look ok to me, but we should
probably also see if there is any specific use
case concerns from others.

2/ Couple comments inline

3/ Regarding playbook-fedora-builder.yml in general, there is issue with
playbook-fedora-builder.yml, assuming I use "as is".


dball@ubuntu:~/ovs/poc/builders$ sudo vagrant up
DEPRECATION: The 'sudo' option for the Ansible provisioner is deprecated.
Please use the 'become' option instead.
The 'sudo' option will be removed in a future release of Vagrant.

Bringing machine 'fedorabuilder' up with 'virtualbox' provider...
==> fedorabuilder: Box 'fedora/27-cloud-base' could not be found.
Attempting to find and install...
    fedorabuilder: Box Provider: virtualbox
    fedorabuilder: Box Version: >= 0
==> fedorabuilder: Loading metadata for box 'fedora/27-cloud-base'
    fedorabuilder: URL: https://vagrantcloud.com/fedora/27-cloud-base
==> fedorabuilder: Adding box 'fedora/27-cloud-base' (v20171105) for
provider: virtualbox
    fedorabuilder: Downloading:
https://vagrantcloud.com/fedora/boxes/27-cloud-base/versions/20171105/providers/virtualbox.box
    fedorabuilder: Download redirected to host: download.fedoraproject.org
An error occurred while downloading the remote file. The error
message, if any, is reproduced below. Please fix this error and try
again.

The requested URL returned error: 404 Not Found

On Mon, Apr 15, 2019 at 6:26 PM Ansis Atteka <aatteka@ovn.org> wrote:

> Otherwise, Open vSwitch will fail to start with the following
> error "libcap-ng is not configured at compile time" when it
> attempts to downgrade to Open vSwitch user.
>
> Also, if packages were built in a way where processes are
> supposed to be running only as root, then there is no point
> in creating "openvswitch" user in the first place.
>
> Signed-off-by: Ansis Atteka <aatteka@ovn.org>
> ---
>  poc/playbook-fedora-builder.yml | 6 +++---
>  rhel/openvswitch-fedora.spec.in | 8 ++++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/poc/playbook-fedora-builder.yml
> b/poc/playbook-fedora-builder.yml
> index 70f0b6ff2..b955714fc 100644
> --- a/poc/playbook-fedora-builder.yml
> +++ b/poc/playbook-fedora-builder.yml
> @@ -99,17 +99,17 @@
>        - openvswitch-dkms.spec
>
>    - name: Build Open vSwitch user space rpms
> -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
> +    command: rpmbuild -bb --without check --without libcapng
> rhel/openvswitch-fedora.spec
>      args:
>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>
>    - name: Build Open vSwitch kmod rpm
> -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
> +    command: rpmbuild -bb --without check --without libcapng
> rhel/openvswitch-fedora.spec
>

Is the correct spec file openvswitch-kmod-fedora.spec ?
Hence, do we need a change here ?


>      args:
>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>
>    - name: Build Open vSwitch dkms rpm
> -    command: rpmbuild -bb --without check rhel/openvswitch-dkms.spec
> +    command: rpmbuild -bb --without check --without libcapng
> rhel/openvswitch-dkms.spec
>

Do you need this line changed ?



>      args:
>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>
> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/
> openvswitch-fedora.spec.in
> index c1cd3f4c6..ce728b4f0 100644
> --- a/rhel/openvswitch-fedora.spec.in
> +++ b/rhel/openvswitch-fedora.spec.in
> @@ -350,6 +350,7 @@ rm -rf $RPM_BUILD_ROOT
>  %endif
>
>  %pre
> +%if %{with libcapng}
>  getent group openvswitch >/dev/null || groupadd -r openvswitch
>  getent passwd openvswitch >/dev/null || \
>      useradd -r -g openvswitch -d / -s /sbin/nologin \
> @@ -359,9 +360,11 @@ getent passwd openvswitch >/dev/null || \
>      getent group hugetlbfs >/dev/null || groupadd -r hugetlbfs
>      usermod -a -G hugetlbfs openvswitch
>  %endif
> +%endif
>  exit 0
>
>  %post
> +%if %{with libcapng}
>  if [ $1 -eq 1 ]; then
>      sed -i 's:^#OVS_USER_ID=:OVS_USER_ID=:' /etc/sysconfig/openvswitch
>      sed -i 's:\(.*su\).*:\1 openvswitch openvswitch:'
> %{_sysconfdir}/logrotate.d/openvswitch
> @@ -376,6 +379,7 @@ if [ $1 -eq 1 ]; then
>      chown -R openvswitch:openvswitch /etc/openvswitch
>      chown -R openvswitch:openvswitch /var/log/openvswitch
>  fi
> +%endif
>
>  %if 0%{?systemd_post:1}
>      %systemd_post %{name}.service
> @@ -445,7 +449,11 @@ fi
>  %endif
>
>  %files
> +%if %{with libcapng}
>  %defattr(-,openvswitch,openvswitch)
> +%else
> +%defattr(-,root,root)
> +%endif
>  %dir %{_sysconfdir}/openvswitch
>  %{_sysconfdir}/openvswitch/default.conf
>  %config %ghost %{_sysconfdir}/openvswitch/conf.db
> --
> 2.14.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Darrell Ball April 16, 2019, 2:06 p.m. UTC | #3
For '3' comment below:

s/Regarding playbook-fedora-builder.yml in general, there is issue with
playbook-fedora-builder.yml, assuming I use "as is"./
Regarding POC in general, there is an issue with POC for Fedora usage,
assuming I use "as is"./

On Mon, Apr 15, 2019 at 11:21 PM Darrell Ball <dlu998@gmail.com> wrote:

> Thanks for the fix
>
> 1/ Main changes to openvswitch-fedora.spec.in look ok to me, but we
> should probably also see if there is any specific use
> case concerns from others.
>
> 2/ Couple comments inline
>
> 3/ Regarding playbook-fedora-builder.yml in general, there is issue with
> playbook-fedora-builder.yml, assuming I use "as is".
>
>
> dball@ubuntu:~/ovs/poc/builders$ sudo vagrant up
> DEPRECATION: The 'sudo' option for the Ansible provisioner is deprecated.
> Please use the 'become' option instead.
> The 'sudo' option will be removed in a future release of Vagrant.
>
> Bringing machine 'fedorabuilder' up with 'virtualbox' provider...
> ==> fedorabuilder: Box 'fedora/27-cloud-base' could not be found.
> Attempting to find and install...
>     fedorabuilder: Box Provider: virtualbox
>     fedorabuilder: Box Version: >= 0
> ==> fedorabuilder: Loading metadata for box 'fedora/27-cloud-base'
>     fedorabuilder: URL: https://vagrantcloud.com/fedora/27-cloud-base
> ==> fedorabuilder: Adding box 'fedora/27-cloud-base' (v20171105) for
> provider: virtualbox
>     fedorabuilder: Downloading:
> https://vagrantcloud.com/fedora/boxes/27-cloud-base/versions/20171105/providers/virtualbox.box
>     fedorabuilder: Download redirected to host: download.fedoraproject.org
> An error occurred while downloading the remote file. The error
> message, if any, is reproduced below. Please fix this error and try
> again.
>
> The requested URL returned error: 404 Not Found
>
> On Mon, Apr 15, 2019 at 6:26 PM Ansis Atteka <aatteka@ovn.org> wrote:
>
>> Otherwise, Open vSwitch will fail to start with the following
>> error "libcap-ng is not configured at compile time" when it
>> attempts to downgrade to Open vSwitch user.
>>
>> Also, if packages were built in a way where processes are
>> supposed to be running only as root, then there is no point
>> in creating "openvswitch" user in the first place.
>>
>> Signed-off-by: Ansis Atteka <aatteka@ovn.org>
>> ---
>>  poc/playbook-fedora-builder.yml | 6 +++---
>>  rhel/openvswitch-fedora.spec.in | 8 ++++++++
>>  2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/poc/playbook-fedora-builder.yml
>> b/poc/playbook-fedora-builder.yml
>> index 70f0b6ff2..b955714fc 100644
>> --- a/poc/playbook-fedora-builder.yml
>> +++ b/poc/playbook-fedora-builder.yml
>> @@ -99,17 +99,17 @@
>>        - openvswitch-dkms.spec
>>
>>    - name: Build Open vSwitch user space rpms
>> -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
>> +    command: rpmbuild -bb --without check --without libcapng
>> rhel/openvswitch-fedora.spec
>>      args:
>>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>>
>>    - name: Build Open vSwitch kmod rpm
>> -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
>> +    command: rpmbuild -bb --without check --without libcapng
>> rhel/openvswitch-fedora.spec
>>
>
> Is the correct spec file openvswitch-kmod-fedora.spec ?
> Hence, do we need a change here ?
>
>
>>      args:
>>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>>
>>    - name: Build Open vSwitch dkms rpm
>> -    command: rpmbuild -bb --without check rhel/openvswitch-dkms.spec
>> +    command: rpmbuild -bb --without check --without libcapng
>> rhel/openvswitch-dkms.spec
>>
>
> Do you need this line changed ?
>
>
>
>>      args:
>>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>>
>> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/
>> openvswitch-fedora.spec.in
>> index c1cd3f4c6..ce728b4f0 100644
>> --- a/rhel/openvswitch-fedora.spec.in
>> +++ b/rhel/openvswitch-fedora.spec.in
>> @@ -350,6 +350,7 @@ rm -rf $RPM_BUILD_ROOT
>>  %endif
>>
>>  %pre
>> +%if %{with libcapng}
>>  getent group openvswitch >/dev/null || groupadd -r openvswitch
>>  getent passwd openvswitch >/dev/null || \
>>      useradd -r -g openvswitch -d / -s /sbin/nologin \
>> @@ -359,9 +360,11 @@ getent passwd openvswitch >/dev/null || \
>>      getent group hugetlbfs >/dev/null || groupadd -r hugetlbfs
>>      usermod -a -G hugetlbfs openvswitch
>>  %endif
>> +%endif
>>  exit 0
>>
>>  %post
>> +%if %{with libcapng}
>>  if [ $1 -eq 1 ]; then
>>      sed -i 's:^#OVS_USER_ID=:OVS_USER_ID=:' /etc/sysconfig/openvswitch
>>      sed -i 's:\(.*su\).*:\1 openvswitch openvswitch:'
>> %{_sysconfdir}/logrotate.d/openvswitch
>> @@ -376,6 +379,7 @@ if [ $1 -eq 1 ]; then
>>      chown -R openvswitch:openvswitch /etc/openvswitch
>>      chown -R openvswitch:openvswitch /var/log/openvswitch
>>  fi
>> +%endif
>>
>>  %if 0%{?systemd_post:1}
>>      %systemd_post %{name}.service
>> @@ -445,7 +449,11 @@ fi
>>  %endif
>>
>>  %files
>> +%if %{with libcapng}
>>  %defattr(-,openvswitch,openvswitch)
>> +%else
>> +%defattr(-,root,root)
>> +%endif
>>  %dir %{_sysconfdir}/openvswitch
>>  %{_sysconfdir}/openvswitch/default.conf
>>  %config %ghost %{_sysconfdir}/openvswitch/conf.db
>> --
>> 2.14.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
Aaron Conole April 16, 2019, 5:46 p.m. UTC | #4
Ansis Atteka <aatteka@ovn.org> writes:

> Otherwise, Open vSwitch will fail to start with the following
> error "libcap-ng is not configured at compile time" when it
> attempts to downgrade to Open vSwitch user.
>
> Also, if packages were built in a way where processes are
> supposed to be running only as root, then there is no point
> in creating "openvswitch" user in the first place.
>
> Signed-off-by: Ansis Atteka <aatteka@ovn.org>
> ---

It seems strange to not provide a user-downgrade option just because we
cannot drop capabilities via libcap-ng.

Maybe it's possible instead to change daemon-unix.c to provide an
alternative fallback when building on linux without libcapng?  Something
like the untested code here.  I have no idea if all of the capabilities
will get dropped when the user/group ids are changed, but we might be
able to deal with that as well.  WDYT?

diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
index 6169763c2..cd2f66295 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -859,9 +859,10 @@ daemon_become_new_user__(bool access_datapath)
         if (LIBCAPNG) {
             daemon_become_new_user_linux(access_datapath);
         } else {
-            VLOG_FATAL("%s: fail to downgrade user using libcap-ng. "
-                       "(libcap-ng is not configured at compile time), "
-                       "aborting.", pidfile);
+            VLOG_INFO("%s: fail to downgrade user using libcap-ng. "
+                      "(libcap-ng is not configured at compile time).",
+                      pidfile);
+            daemon_become_new_user_unix();
         }
     } else {
         daemon_become_new_user_unix();


>  poc/playbook-fedora-builder.yml | 6 +++---
>  rhel/openvswitch-fedora.spec.in | 8 ++++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/poc/playbook-fedora-builder.yml b/poc/playbook-fedora-builder.yml
> index 70f0b6ff2..b955714fc 100644
> --- a/poc/playbook-fedora-builder.yml
> +++ b/poc/playbook-fedora-builder.yml
> @@ -99,17 +99,17 @@
>        - openvswitch-dkms.spec
>  
>    - name: Build Open vSwitch user space rpms
> -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
> +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
>      args:
>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>  
>    - name: Build Open vSwitch kmod rpm
> -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
> +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
>      args:
>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>  
>    - name: Build Open vSwitch dkms rpm
> -    command: rpmbuild -bb --without check rhel/openvswitch-dkms.spec
> +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-dkms.spec
>      args:
>          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>  
> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
> index c1cd3f4c6..ce728b4f0 100644
> --- a/rhel/openvswitch-fedora.spec.in
> +++ b/rhel/openvswitch-fedora.spec.in
> @@ -350,6 +350,7 @@ rm -rf $RPM_BUILD_ROOT
>  %endif
>  
>  %pre
> +%if %{with libcapng}
>  getent group openvswitch >/dev/null || groupadd -r openvswitch
>  getent passwd openvswitch >/dev/null || \
>      useradd -r -g openvswitch -d / -s /sbin/nologin \
> @@ -359,9 +360,11 @@ getent passwd openvswitch >/dev/null || \
>      getent group hugetlbfs >/dev/null || groupadd -r hugetlbfs
>      usermod -a -G hugetlbfs openvswitch
>  %endif
> +%endif
>  exit 0
>  
>  %post
> +%if %{with libcapng}
>  if [ $1 -eq 1 ]; then
>      sed -i 's:^#OVS_USER_ID=:OVS_USER_ID=:' /etc/sysconfig/openvswitch
>      sed -i 's:\(.*su\).*:\1 openvswitch openvswitch:' %{_sysconfdir}/logrotate.d/openvswitch
> @@ -376,6 +379,7 @@ if [ $1 -eq 1 ]; then
>      chown -R openvswitch:openvswitch /etc/openvswitch
>      chown -R openvswitch:openvswitch /var/log/openvswitch
>  fi
> +%endif
>  
>  %if 0%{?systemd_post:1}
>      %systemd_post %{name}.service
> @@ -445,7 +449,11 @@ fi
>  %endif
>  
>  %files
> +%if %{with libcapng}
>  %defattr(-,openvswitch,openvswitch)
> +%else
> +%defattr(-,root,root)
> +%endif
>  %dir %{_sysconfdir}/openvswitch
>  %{_sysconfdir}/openvswitch/default.conf
>  %config %ghost %{_sysconfdir}/openvswitch/conf.db
Ansis Atteka April 16, 2019, 7:36 p.m. UTC | #5
On Tue, 16 Apr 2019 at 10:46, Aaron Conole <aconole@redhat.com> wrote:
>
> Ansis Atteka <aatteka@ovn.org> writes:
>
> > Otherwise, Open vSwitch will fail to start with the following
> > error "libcap-ng is not configured at compile time" when it
> > attempts to downgrade to Open vSwitch user.
> >
> > Also, if packages were built in a way where processes are
> > supposed to be running only as root, then there is no point
> > in creating "openvswitch" user in the first place.
> >
> > Signed-off-by: Ansis Atteka <aatteka@ovn.org>
> > ---
>
> It seems strange to not provide a user-downgrade option just because we
> cannot drop capabilities via libcap-ng.

I did not look too close in the daemon-unix.c, but I believe in
current design "linux capabilities" and "linux user downgrade" go hand
in hand (i.e. you either do both or neither). At least for Linux
Kernel datapath. Not sure about other datapaths.

>
> Maybe it's possible instead to change daemon-unix.c to provide an
> alternative fallback when building on linux without libcapng?  Something
> like the untested code here.  I have no idea if all of the capabilities
> will get dropped when the user/group ids are changed, but we might be
> able to deal with that as well.  WDYT?

I can take a look at that and if there is a valid use-case I can send
another patch addressing that specific issue. However, I think we can
at this time proceed with this patch, because
1. it fixes fatal bug when OVS was built without libcap-ng support; AND
2. it does break anything when OVS was built with libcap-ng support.

Let me know what you think and if I am missing something? Is your main
concern that if there turns out to be a valid use case to not have
libcap-ng support but still user downgrade feature, then portions of
this patch will need to be reverted? If so, I am fine with that...
>
> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> index 6169763c2..cd2f66295 100644
> --- a/lib/daemon-unix.c
> +++ b/lib/daemon-unix.c
> @@ -859,9 +859,10 @@ daemon_become_new_user__(bool access_datapath)
>          if (LIBCAPNG) {
>              daemon_become_new_user_linux(access_datapath);
>          } else {
> -            VLOG_FATAL("%s: fail to downgrade user using libcap-ng. "
> -                       "(libcap-ng is not configured at compile time), "
> -                       "aborting.", pidfile);
> +            VLOG_INFO("%s: fail to downgrade user using libcap-ng. "
> +                      "(libcap-ng is not configured at compile time).",
> +                      pidfile);
> +            daemon_become_new_user_unix();
>          }
>      } else {
>          daemon_become_new_user_unix();
>
>
> >  poc/playbook-fedora-builder.yml | 6 +++---
> >  rhel/openvswitch-fedora.spec.in | 8 ++++++++
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/poc/playbook-fedora-builder.yml b/poc/playbook-fedora-builder.yml
> > index 70f0b6ff2..b955714fc 100644
> > --- a/poc/playbook-fedora-builder.yml
> > +++ b/poc/playbook-fedora-builder.yml
> > @@ -99,17 +99,17 @@
> >        - openvswitch-dkms.spec
> >
> >    - name: Build Open vSwitch user space rpms
> > -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
> > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
> >      args:
> >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
> >
> >    - name: Build Open vSwitch kmod rpm
> > -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
> > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
> >      args:
> >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
> >
> >    - name: Build Open vSwitch dkms rpm
> > -    command: rpmbuild -bb --without check rhel/openvswitch-dkms.spec
> > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-dkms.spec
> >      args:
> >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
> >
> > diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
> > index c1cd3f4c6..ce728b4f0 100644
> > --- a/rhel/openvswitch-fedora.spec.in
> > +++ b/rhel/openvswitch-fedora.spec.in
> > @@ -350,6 +350,7 @@ rm -rf $RPM_BUILD_ROOT
> >  %endif
> >
> >  %pre
> > +%if %{with libcapng}
> >  getent group openvswitch >/dev/null || groupadd -r openvswitch
> >  getent passwd openvswitch >/dev/null || \
> >      useradd -r -g openvswitch -d / -s /sbin/nologin \
> > @@ -359,9 +360,11 @@ getent passwd openvswitch >/dev/null || \
> >      getent group hugetlbfs >/dev/null || groupadd -r hugetlbfs
> >      usermod -a -G hugetlbfs openvswitch
> >  %endif
> > +%endif
> >  exit 0
> >
> >  %post
> > +%if %{with libcapng}
> >  if [ $1 -eq 1 ]; then
> >      sed -i 's:^#OVS_USER_ID=:OVS_USER_ID=:' /etc/sysconfig/openvswitch
> >      sed -i 's:\(.*su\).*:\1 openvswitch openvswitch:' %{_sysconfdir}/logrotate.d/openvswitch
> > @@ -376,6 +379,7 @@ if [ $1 -eq 1 ]; then
> >      chown -R openvswitch:openvswitch /etc/openvswitch
> >      chown -R openvswitch:openvswitch /var/log/openvswitch
> >  fi
> > +%endif
> >
> >  %if 0%{?systemd_post:1}
> >      %systemd_post %{name}.service
> > @@ -445,7 +449,11 @@ fi
> >  %endif
> >
> >  %files
> > +%if %{with libcapng}
> >  %defattr(-,openvswitch,openvswitch)
> > +%else
> > +%defattr(-,root,root)
> > +%endif
> >  %dir %{_sysconfdir}/openvswitch
> >  %{_sysconfdir}/openvswitch/default.conf
> >  %config %ghost %{_sysconfdir}/openvswitch/conf.db
Ansis Atteka April 16, 2019, 7:39 p.m. UTC | #6
On Tue, 16 Apr 2019 at 12:36, Ansis Atteka <ansisatteka@gmail.com> wrote:
>
> On Tue, 16 Apr 2019 at 10:46, Aaron Conole <aconole@redhat.com> wrote:
> >
> > Ansis Atteka <aatteka@ovn.org> writes:
> >
> > > Otherwise, Open vSwitch will fail to start with the following
> > > error "libcap-ng is not configured at compile time" when it
> > > attempts to downgrade to Open vSwitch user.
> > >
> > > Also, if packages were built in a way where processes are
> > > supposed to be running only as root, then there is no point
> > > in creating "openvswitch" user in the first place.
> > >
> > > Signed-off-by: Ansis Atteka <aatteka@ovn.org>
> > > ---
> >
> > It seems strange to not provide a user-downgrade option just because we
> > cannot drop capabilities via libcap-ng.
>
> I did not look too close in the daemon-unix.c, but I believe in
> current design "linux capabilities" and "linux user downgrade" go hand
> in hand (i.e. you either do both or neither). At least for Linux
> Kernel datapath. Not sure about other datapaths.
>
> >
> > Maybe it's possible instead to change daemon-unix.c to provide an
> > alternative fallback when building on linux without libcapng?  Something
> > like the untested code here.  I have no idea if all of the capabilities
> > will get dropped when the user/group ids are changed, but we might be
> > able to deal with that as well.  WDYT?
>
> I can take a look at that and if there is a valid use-case I can send
> another patch addressing that specific issue. However, I think we can
> at this time proceed with this patch, because
> 1. it fixes fatal bug when OVS was built without libcap-ng support; AND
> 2. it does break anything when OVS was built with libcap-ng support.

meant to say "it does NOT break anything"
>
> Let me know what you think and if I am missing something? Is your main
> concern that if there turns out to be a valid use case to not have
> libcap-ng support but still user downgrade feature, then portions of
> this patch will need to be reverted? If so, I am fine with that...
> >
> > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> > index 6169763c2..cd2f66295 100644
> > --- a/lib/daemon-unix.c
> > +++ b/lib/daemon-unix.c
> > @@ -859,9 +859,10 @@ daemon_become_new_user__(bool access_datapath)
> >          if (LIBCAPNG) {
> >              daemon_become_new_user_linux(access_datapath);
> >          } else {
> > -            VLOG_FATAL("%s: fail to downgrade user using libcap-ng. "
> > -                       "(libcap-ng is not configured at compile time), "
> > -                       "aborting.", pidfile);
> > +            VLOG_INFO("%s: fail to downgrade user using libcap-ng. "
> > +                      "(libcap-ng is not configured at compile time).",
> > +                      pidfile);
> > +            daemon_become_new_user_unix();
> >          }
> >      } else {
> >          daemon_become_new_user_unix();
> >
> >
> > >  poc/playbook-fedora-builder.yml | 6 +++---
> > >  rhel/openvswitch-fedora.spec.in | 8 ++++++++
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/poc/playbook-fedora-builder.yml b/poc/playbook-fedora-builder.yml
> > > index 70f0b6ff2..b955714fc 100644
> > > --- a/poc/playbook-fedora-builder.yml
> > > +++ b/poc/playbook-fedora-builder.yml
> > > @@ -99,17 +99,17 @@
> > >        - openvswitch-dkms.spec
> > >
> > >    - name: Build Open vSwitch user space rpms
> > > -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
> > > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
> > >      args:
> > >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
> > >
> > >    - name: Build Open vSwitch kmod rpm
> > > -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
> > > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
> > >      args:
> > >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
> > >
> > >    - name: Build Open vSwitch dkms rpm
> > > -    command: rpmbuild -bb --without check rhel/openvswitch-dkms.spec
> > > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-dkms.spec
> > >      args:
> > >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
> > >
> > > diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
> > > index c1cd3f4c6..ce728b4f0 100644
> > > --- a/rhel/openvswitch-fedora.spec.in
> > > +++ b/rhel/openvswitch-fedora.spec.in
> > > @@ -350,6 +350,7 @@ rm -rf $RPM_BUILD_ROOT
> > >  %endif
> > >
> > >  %pre
> > > +%if %{with libcapng}
> > >  getent group openvswitch >/dev/null || groupadd -r openvswitch
> > >  getent passwd openvswitch >/dev/null || \
> > >      useradd -r -g openvswitch -d / -s /sbin/nologin \
> > > @@ -359,9 +360,11 @@ getent passwd openvswitch >/dev/null || \
> > >      getent group hugetlbfs >/dev/null || groupadd -r hugetlbfs
> > >      usermod -a -G hugetlbfs openvswitch
> > >  %endif
> > > +%endif
> > >  exit 0
> > >
> > >  %post
> > > +%if %{with libcapng}
> > >  if [ $1 -eq 1 ]; then
> > >      sed -i 's:^#OVS_USER_ID=:OVS_USER_ID=:' /etc/sysconfig/openvswitch
> > >      sed -i 's:\(.*su\).*:\1 openvswitch openvswitch:' %{_sysconfdir}/logrotate.d/openvswitch
> > > @@ -376,6 +379,7 @@ if [ $1 -eq 1 ]; then
> > >      chown -R openvswitch:openvswitch /etc/openvswitch
> > >      chown -R openvswitch:openvswitch /var/log/openvswitch
> > >  fi
> > > +%endif
> > >
> > >  %if 0%{?systemd_post:1}
> > >      %systemd_post %{name}.service
> > > @@ -445,7 +449,11 @@ fi
> > >  %endif
> > >
> > >  %files
> > > +%if %{with libcapng}
> > >  %defattr(-,openvswitch,openvswitch)
> > > +%else
> > > +%defattr(-,root,root)
> > > +%endif
> > >  %dir %{_sysconfdir}/openvswitch
> > >  %{_sysconfdir}/openvswitch/default.conf
> > >  %config %ghost %{_sysconfdir}/openvswitch/conf.db
Aaron Conole April 17, 2019, 3:36 p.m. UTC | #7
Ansis Atteka <ansisatteka@gmail.com> writes:

> On Tue, 16 Apr 2019 at 12:36, Ansis Atteka <ansisatteka@gmail.com> wrote:
>>
>> On Tue, 16 Apr 2019 at 10:46, Aaron Conole <aconole@redhat.com> wrote:
>> >
>> > Ansis Atteka <aatteka@ovn.org> writes:
>> >
>> > > Otherwise, Open vSwitch will fail to start with the following
>> > > error "libcap-ng is not configured at compile time" when it
>> > > attempts to downgrade to Open vSwitch user.
>> > >
>> > > Also, if packages were built in a way where processes are
>> > > supposed to be running only as root, then there is no point
>> > > in creating "openvswitch" user in the first place.
>> > >
>> > > Signed-off-by: Ansis Atteka <aatteka@ovn.org>
>> > > ---
>> >
>> > It seems strange to not provide a user-downgrade option just because we
>> > cannot drop capabilities via libcap-ng.
>>
>> I did not look too close in the daemon-unix.c, but I believe in
>> current design "linux capabilities" and "linux user downgrade" go hand
>> in hand (i.e. you either do both or neither). At least for Linux
>> Kernel datapath. Not sure about other datapaths.

I guess this is because we need cap_net_admin, cap_net_raw, etc.,
but it's possible to keep this even without using libcap-ng (or even
needing programmatic idiom, iirc).  For example, it is possible to
attach these capabilities to the ovs-vswitchd binary on the filesystem
(for filesystems which support) and thereby provide this functionality
without needing libcap-ng.

Then we retain the ability to run as a (mostly) unprivileged user, and
still provide the control and slow-path for the kernel space side.

>> >
>> > Maybe it's possible instead to change daemon-unix.c to provide an
>> > alternative fallback when building on linux without libcapng?  Something
>> > like the untested code here.  I have no idea if all of the capabilities
>> > will get dropped when the user/group ids are changed, but we might be
>> > able to deal with that as well.  WDYT?
>>
>> I can take a look at that and if there is a valid use-case I can send
>> another patch addressing that specific issue. However, I think we can
>> at this time proceed with this patch, because
>> 1. it fixes fatal bug when OVS was built without libcap-ng support; AND
>> 2. it does break anything when OVS was built with libcap-ng support.
>
> meant to say "it does NOT break anything"
>>
>> Let me know what you think and if I am missing something? Is your main
>> concern that if there turns out to be a valid use case to not have
>> libcap-ng support but still user downgrade feature, then portions of
>> this patch will need to be reverted? If so, I am fine with that...

RHEL systems run as non-root user by default.  I think it's a valid
use-case.  I agree, we need to address the issue when building without
libcap-ng.  Can you check if it is possible to update the RPM build so
that the file capabilities are attached to the ovs-vswitchd executable
directly?  If it works, it would allow us to achieve the same end result
in an alternative path.

>> >
>> > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
>> > index 6169763c2..cd2f66295 100644
>> > --- a/lib/daemon-unix.c
>> > +++ b/lib/daemon-unix.c
>> > @@ -859,9 +859,10 @@ daemon_become_new_user__(bool access_datapath)
>> >          if (LIBCAPNG) {
>> >              daemon_become_new_user_linux(access_datapath);
>> >          } else {
>> > -            VLOG_FATAL("%s: fail to downgrade user using libcap-ng. "
>> > -                       "(libcap-ng is not configured at compile time), "
>> > -                       "aborting.", pidfile);
>> > +            VLOG_INFO("%s: fail to downgrade user using libcap-ng. "
>> > +                      "(libcap-ng is not configured at compile time).",
>> > +                      pidfile);
>> > +            daemon_become_new_user_unix();
>> >          }
>> >      } else {
>> >          daemon_become_new_user_unix();
>> >
>> >
>> > >  poc/playbook-fedora-builder.yml | 6 +++---
>> > >  rhel/openvswitch-fedora.spec.in | 8 ++++++++
>> > >  2 files changed, 11 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/poc/playbook-fedora-builder.yml b/poc/playbook-fedora-builder.yml
>> > > index 70f0b6ff2..b955714fc 100644
>> > > --- a/poc/playbook-fedora-builder.yml
>> > > +++ b/poc/playbook-fedora-builder.yml
>> > > @@ -99,17 +99,17 @@
>> > >        - openvswitch-dkms.spec
>> > >
>> > >    - name: Build Open vSwitch user space rpms
>> > > -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
>> > > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
>> > >      args:
>> > >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>> > >
>> > >    - name: Build Open vSwitch kmod rpm
>> > > -    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
>> > > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
>> > >      args:
>> > >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>> > >
>> > >    - name: Build Open vSwitch dkms rpm
>> > > -    command: rpmbuild -bb --without check rhel/openvswitch-dkms.spec
>> > > +    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-dkms.spec
>> > >      args:
>> > >          chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
>> > >
>> > > diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
>> > > index c1cd3f4c6..ce728b4f0 100644
>> > > --- a/rhel/openvswitch-fedora.spec.in
>> > > +++ b/rhel/openvswitch-fedora.spec.in
>> > > @@ -350,6 +350,7 @@ rm -rf $RPM_BUILD_ROOT
>> > >  %endif
>> > >
>> > >  %pre
>> > > +%if %{with libcapng}
>> > >  getent group openvswitch >/dev/null || groupadd -r openvswitch
>> > >  getent passwd openvswitch >/dev/null || \
>> > >      useradd -r -g openvswitch -d / -s /sbin/nologin \
>> > > @@ -359,9 +360,11 @@ getent passwd openvswitch >/dev/null || \
>> > >      getent group hugetlbfs >/dev/null || groupadd -r hugetlbfs
>> > >      usermod -a -G hugetlbfs openvswitch
>> > >  %endif
>> > > +%endif
>> > >  exit 0
>> > >
>> > >  %post
>> > > +%if %{with libcapng}
>> > >  if [ $1 -eq 1 ]; then
>> > >      sed -i 's:^#OVS_USER_ID=:OVS_USER_ID=:' /etc/sysconfig/openvswitch
>> > >      sed -i 's:\(.*su\).*:\1 openvswitch openvswitch:' %{_sysconfdir}/logrotate.d/openvswitch
>> > > @@ -376,6 +379,7 @@ if [ $1 -eq 1 ]; then
>> > >      chown -R openvswitch:openvswitch /etc/openvswitch
>> > >      chown -R openvswitch:openvswitch /var/log/openvswitch
>> > >  fi
>> > > +%endif
>> > >
>> > >  %if 0%{?systemd_post:1}
>> > >      %systemd_post %{name}.service
>> > > @@ -445,7 +449,11 @@ fi
>> > >  %endif
>> > >
>> > >  %files
>> > > +%if %{with libcapng}
>> > >  %defattr(-,openvswitch,openvswitch)
>> > > +%else
>> > > +%defattr(-,root,root)
>> > > +%endif
>> > >  %dir %{_sysconfdir}/openvswitch
>> > >  %{_sysconfdir}/openvswitch/default.conf
>> > >  %config %ghost %{_sysconfdir}/openvswitch/conf.db
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch
diff mbox series

diff --git a/poc/playbook-fedora-builder.yml b/poc/playbook-fedora-builder.yml
index 70f0b6ff2..b955714fc 100644
--- a/poc/playbook-fedora-builder.yml
+++ b/poc/playbook-fedora-builder.yml
@@ -99,17 +99,17 @@ 
       - openvswitch-dkms.spec
 
   - name: Build Open vSwitch user space rpms
-    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
+    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
     args:
         chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
 
   - name: Build Open vSwitch kmod rpm
-    command: rpmbuild -bb --without check rhel/openvswitch-fedora.spec
+    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-fedora.spec
     args:
         chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
 
   - name: Build Open vSwitch dkms rpm
-    command: rpmbuild -bb --without check rhel/openvswitch-dkms.spec
+    command: rpmbuild -bb --without check --without libcapng rhel/openvswitch-dkms.spec
     args:
         chdir: "{{SOURCE}}/openvswitch-{{version.stdout}}"
 
diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-fedora.spec.in
index c1cd3f4c6..ce728b4f0 100644
--- a/rhel/openvswitch-fedora.spec.in
+++ b/rhel/openvswitch-fedora.spec.in
@@ -350,6 +350,7 @@  rm -rf $RPM_BUILD_ROOT
 %endif
 
 %pre
+%if %{with libcapng}
 getent group openvswitch >/dev/null || groupadd -r openvswitch
 getent passwd openvswitch >/dev/null || \
     useradd -r -g openvswitch -d / -s /sbin/nologin \
@@ -359,9 +360,11 @@  getent passwd openvswitch >/dev/null || \
     getent group hugetlbfs >/dev/null || groupadd -r hugetlbfs
     usermod -a -G hugetlbfs openvswitch
 %endif
+%endif
 exit 0
 
 %post
+%if %{with libcapng}
 if [ $1 -eq 1 ]; then
     sed -i 's:^#OVS_USER_ID=:OVS_USER_ID=:' /etc/sysconfig/openvswitch
     sed -i 's:\(.*su\).*:\1 openvswitch openvswitch:' %{_sysconfdir}/logrotate.d/openvswitch
@@ -376,6 +379,7 @@  if [ $1 -eq 1 ]; then
     chown -R openvswitch:openvswitch /etc/openvswitch
     chown -R openvswitch:openvswitch /var/log/openvswitch
 fi
+%endif
 
 %if 0%{?systemd_post:1}
     %systemd_post %{name}.service
@@ -445,7 +449,11 @@  fi
 %endif
 
 %files
+%if %{with libcapng}
 %defattr(-,openvswitch,openvswitch)
+%else
+%defattr(-,root,root)
+%endif
 %dir %{_sysconfdir}/openvswitch
 %{_sysconfdir}/openvswitch/default.conf
 %config %ghost %{_sysconfdir}/openvswitch/conf.db