| Message ID | 20190416012441.24625-1-aatteka@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] rhel: if rpms were built without libcapng then let processrs to run as root | expand |
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
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
>
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 >> >
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
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
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
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
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
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(-)