[PATCH 0/7] Second batch of fixes reported during Fedora package review

[PATCH 0/7] Second batch of fixes reported during Fedora package review

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

This series fixes one issue in passt itself, reported by Daniel from a
rpmlint warning, and implements six recommendations in the spec file,
also from Daniel.

Stefano Brivio (7):
  util: Drop any supplementary group before dropping privileges
  fedora: Adopt versioning guideline for snapshots
  fedora: Drop SPDX identifier from spec file
  fedora: Drop comment stating the spec file is an example file
  fedora: Define git_hash in spec file and reuse it
  fedora: Use full versioning for SELinux subpackage Requires: tag
  fedora: Pass explicit bindir, mandir, docdir, and drop OpenSUSE
    override

 contrib/fedora/passt.spec  | 18 +++++++-----------
 contrib/fedora/rpkg.macros |  7 +++++--
 util.c                     |  2 +-
 3 files changed, 13 insertions(+), 14 deletions(-)

-- 
2.35.1

[PATCH 1/7] util: Drop any supplementary group before dropping privileges

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

Commit a951e0b9efcb ("conf: Add --runas option, changing to given UID
and GID if started as root") dropped the call to initgroups() that
used to add supplementary groups corresponding to the user we'll
eventually run as -- we don't need those.

However, if the original user belongs to supplementary groups
(usually not the case, if started as root), we don't drop those,
now, and rpmlint says:

  passt.x86_64: E: missing-call-to-setgroups-before-setuid /usr/bin/passt
  passt.x86_64: E: missing-call-to-setgroups-before-setuid /usr/bin/passt.avx2

Add a call to setgroups() with an empty set, to drop any
supplementary group we might currently have, before changing GID
and UID.

Reported-by: Daniel P. Berrangé <berrange(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util.c b/util.c
index 9b87b65..7e10deb 100644
--- a/util.c
+++ b/util.c
@@ -525,7 +525,7 @@ void check_root(struct ctx *c)
 #endif
 	}
 
-	if (!setgid(c->gid) && !setuid(c->uid))
+	if (!setgroups(0, NULL) && !setgid(c->gid) && !setuid(c->uid))
 		return;
 
 	fprintf(stderr, "Can't change user/group, exiting");
-- 
@@ -525,7 +525,7 @@ void check_root(struct ctx *c)
 #endif
 	}
 
-	if (!setgid(c->gid) && !setuid(c->uid))
+	if (!setgroups(0, NULL) && !setgid(c->gid) && !setuid(c->uid))
 		return;
 
 	fprintf(stderr, "Can't change user/group, exiting");
-- 
2.35.1

Re: [PATCH 1/7] util: Drop any supplementary group before dropping privileges

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

On Mon, Aug 29, 2022 at 05:17:03PM +0200, Stefano Brivio wrote:
> Commit a951e0b9efcb ("conf: Add --runas option, changing to given UID
> and GID if started as root") dropped the call to initgroups() that
> used to add supplementary groups corresponding to the user we'll
> eventually run as -- we don't need those.
> 
> However, if the original user belongs to supplementary groups
> (usually not the case, if started as root), we don't drop those,
> now, and rpmlint says:
> 
>   passt.x86_64: E: missing-call-to-setgroups-before-setuid /usr/bin/passt
>   passt.x86_64: E: missing-call-to-setgroups-before-setuid /usr/bin/passt.avx2
> 
> Add a call to setgroups() with an empty set, to drop any
> supplementary group we might currently have, before changing GID
> and UID.
> 
> Reported-by: Daniel P. Berrangé <berrange(a)redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>

Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>

I'll keep this in mind for the rework I plan in this area.

> ---
>  util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util.c b/util.c
> index 9b87b65..7e10deb 100644
> --- a/util.c
> +++ b/util.c
> @@ -525,7 +525,7 @@ void check_root(struct ctx *c)
>  #endif
>  	}
>  
> -	if (!setgid(c->gid) && !setuid(c->uid))
> +	if (!setgroups(0, NULL) && !setgid(c->gid) && !setuid(c->uid))
>  		return;
>  
>  	fprintf(stderr, "Can't change user/group, exiting");

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/7] fedora: Adopt versioning guideline for snapshots

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

The "Simple versioning" scheme:
  https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_simple_versioning

probably doesn't apply to passt, given that upstream git tags are
not really releases. Switch to the "Snapshots" versioning scheme:
  https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_simple_versioning

Suggested-by: Daniel P. Berrangé <berrange(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 contrib/fedora/rpkg.macros | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/contrib/fedora/rpkg.macros b/contrib/fedora/rpkg.macros
index 2032034..df9dfc5 100644
--- a/contrib/fedora/rpkg.macros
+++ b/contrib/fedora/rpkg.macros
@@ -12,7 +12,10 @@
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
 function git_version {
-	printf "0.git.%s.%s" "$(date -u -I | tr - _)" "$(git rev-parse --short HEAD)"
+	__commit="$(git rev-parse --short "${1:-HEAD}")"
+	__date="$(git log --pretty="format:%cI" "${__commit}" -1)"
+
+	printf "0^%s.g%s" "$(date -uI -d "${__date}" | tr -d -)" "${__commit}"
 }
 
 function git_head {
@@ -28,7 +31,7 @@ function passt_git_changelog_entry {
 	__date="$(git log --pretty="format:%cI" "${__to}" -1)"
 	__author="$(git log -1 --pretty="format:%an <%ae>" ${__to} -- contrib/fedora)"
 
-	printf "* %s %s - %s\n" "$(date "+%a %b %e %Y" -d "${__date}")" "${__author}" "0.git.${1}-0"
+	printf "* %s %s - %s\n" "$(date "+%a %b %e %Y" -d "${__date}")" "${__author}" "$(git_version "${__to}")-1"
 
 	IFS='
 '
-- 
@@ -12,7 +12,10 @@
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
 function git_version {
-	printf "0.git.%s.%s" "$(date -u -I | tr - _)" "$(git rev-parse --short HEAD)"
+	__commit="$(git rev-parse --short "${1:-HEAD}")"
+	__date="$(git log --pretty="format:%cI" "${__commit}" -1)"
+
+	printf "0^%s.g%s" "$(date -uI -d "${__date}" | tr -d -)" "${__commit}"
 }
 
 function git_head {
@@ -28,7 +31,7 @@ function passt_git_changelog_entry {
 	__date="$(git log --pretty="format:%cI" "${__to}" -1)"
 	__author="$(git log -1 --pretty="format:%an <%ae>" ${__to} -- contrib/fedora)"
 
-	printf "* %s %s - %s\n" "$(date "+%a %b %e %Y" -d "${__date}")" "${__author}" "0.git.${1}-0"
+	printf "* %s %s - %s\n" "$(date "+%a %b %e %Y" -d "${__date}")" "${__author}" "$(git_version "${__to}")-1"
 
 	IFS='
 '
-- 
2.35.1

Re: [PATCH 2/7] fedora: Adopt versioning guideline for snapshots

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

On Mon, 29 Aug 2022 17:17:04 +0200
Stefano Brivio <sbrivio(a)redhat.com> wrote:

> The "Simple versioning" scheme:
>   https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_simple_versioning
> 
> probably doesn't apply to passt, given that upstream git tags are
> not really releases. Switch to the "Snapshots" versioning scheme:
>   https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_simple_versioning

https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots

-- 
Stefano

[PATCH 3/7] fedora: Drop SPDX identifier from spec file

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

...which makes it fall under MIT licensing terms. Daniel reports that
it's very unusual for spec files to contain explicit licensing terms
and might cause minor inconveniences later on, on mass changes to
spec files.

I originally added licensing information using SPDX identifiers to
make the project fully compliant with the REUSE Specification 3.0
(https://reuse.software/spec/), but there are anyway a few more files
not including explicit licensing information. It might be worth to
fix that later on, in any case.

Suggested-by: Daniel P. Berrangé <berrange(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 contrib/fedora/passt.spec | 2 --
 1 file changed, 2 deletions(-)

diff --git a/contrib/fedora/passt.spec b/contrib/fedora/passt.spec
index 87c3e93..4f9be34 100644
--- a/contrib/fedora/passt.spec
+++ b/contrib/fedora/passt.spec
@@ -1,5 +1,3 @@
-# SPDX-License-Identifier: AGPL-3.0-or-later
-#
 # PASST - Plug A Simple Socket Transport
 #  for qemu/UNIX domain socket mode
 #
-- 
@@ -1,5 +1,3 @@
-# SPDX-License-Identifier: AGPL-3.0-or-later
-#
 # PASST - Plug A Simple Socket Transport
 #  for qemu/UNIX domain socket mode
 #
-- 
2.35.1

[PATCH 4/7] fedora: Drop comment stating the spec file is an example file

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

...as this ends up in the actual spec file.

Suggested-by: Daniel P. Berrangé <berrange(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 contrib/fedora/passt.spec | 2 --
 1 file changed, 2 deletions(-)

diff --git a/contrib/fedora/passt.spec b/contrib/fedora/passt.spec
index 4f9be34..9356858 100644
--- a/contrib/fedora/passt.spec
+++ b/contrib/fedora/passt.spec
@@ -4,8 +4,6 @@
 # PASTA - Pack A Subtle Tap Abstraction
 #  for network namespace/tap device mode
 #
-# contrib/fedora/passt.spec - Example spec file for fedora
-#
 # Copyright (c) 2022 Red Hat GmbH
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
-- 
@@ -4,8 +4,6 @@
 # PASTA - Pack A Subtle Tap Abstraction
 #  for network namespace/tap device mode
 #
-# contrib/fedora/passt.spec - Example spec file for fedora
-#
 # Copyright (c) 2022 Red Hat GmbH
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
-- 
2.35.1

[PATCH 5/7] fedora: Define git_hash in spec file and reuse it

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

...as it's used twice. The short version, however, appears hardcoded
only once in the output, and it comes straight from the rpkg macro
building the version string -- leave that macro as it is.

Suggested-by: Daniel P. Berrangé <berrange(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 contrib/fedora/passt.spec | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/fedora/passt.spec b/contrib/fedora/passt.spec
index 9356858..6125a3b 100644
--- a/contrib/fedora/passt.spec
+++ b/contrib/fedora/passt.spec
@@ -7,6 +7,8 @@
 # Copyright (c) 2022 Red Hat GmbH
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
+%global git_hash {{{ git_head }}}
+
 Name:		passt
 Version:	{{{ git_version }}}
 Release:	1%{?dist}
@@ -14,7 +16,7 @@ Summary:	User-mode networking daemons for virtual machines and namespaces
 License:	AGPLv3+ and BSD
 Group:		System Environment/Daemons
 URL:		https://passt.top/
-Source:		https://passt.top/passt/snapshot/passt-{{{ git_head }}}.tar.xz
+Source:		https://passt.top/passt/snapshot/passt-%{git_hash}.tar.xz
 
 BuildRequires:	gcc, make, checkpolicy, selinux-policy-devel
 
@@ -40,7 +42,7 @@ Requires(preun): policycoreutils, %{name}
 This package adds SELinux enforcement to passt(1) and pasta(1).
 
 %prep
-%setup -q -n passt-{{{ git_head }}}
+%setup -q -n passt-%{git_hash}
 
 %build
 %set_build_flags
-- 
@@ -7,6 +7,8 @@
 # Copyright (c) 2022 Red Hat GmbH
 # Author: Stefano Brivio <sbrivio(a)redhat.com>
 
+%global git_hash {{{ git_head }}}
+
 Name:		passt
 Version:	{{{ git_version }}}
 Release:	1%{?dist}
@@ -14,7 +16,7 @@ Summary:	User-mode networking daemons for virtual machines and namespaces
 License:	AGPLv3+ and BSD
 Group:		System Environment/Daemons
 URL:		https://passt.top/
-Source:		https://passt.top/passt/snapshot/passt-{{{ git_head }}}.tar.xz
+Source:		https://passt.top/passt/snapshot/passt-%{git_hash}.tar.xz
 
 BuildRequires:	gcc, make, checkpolicy, selinux-policy-devel
 
@@ -40,7 +42,7 @@ Requires(preun): policycoreutils, %{name}
 This package adds SELinux enforcement to passt(1) and pasta(1).
 
 %prep
-%setup -q -n passt-{{{ git_head }}}
+%setup -q -n passt-%{git_hash}
 
 %build
 %set_build_flags
-- 
2.35.1

[PATCH 6/7] fedora: Use full versioning for SELinux subpackage Requires: tag

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

...as recommended in:
  https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package

Reported-by: Daniel P. Berrangé <berrange(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 contrib/fedora/passt.spec | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/fedora/passt.spec b/contrib/fedora/passt.spec
index 6125a3b..a8af326 100644
--- a/contrib/fedora/passt.spec
+++ b/contrib/fedora/passt.spec
@@ -34,7 +34,7 @@ requiring any capabilities or privileges.
 %package    selinux
 BuildArch:  noarch
 Summary:    SELinux support for passt and pasta
-Requires:   %{name} = %{version}
+Requires:   %{name} = %{version}-%{release}
 Requires(post): policycoreutils, %{name}
 Requires(preun): policycoreutils, %{name}
 
-- 
@@ -34,7 +34,7 @@ requiring any capabilities or privileges.
 %package    selinux
 BuildArch:  noarch
 Summary:    SELinux support for passt and pasta
-Requires:   %{name} = %{version}
+Requires:   %{name} = %{version}-%{release}
 Requires(post): policycoreutils, %{name}
 Requires(preun): policycoreutils, %{name}
 
-- 
2.35.1

[PATCH 7/7] fedora: Pass explicit bindir, mandir, docdir, and drop OpenSUSE override

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

Fedora's parameters currently match the ones from the Makefile (which
is based on GNU recommendations), but that's not necessarily
guaranteed.

This should make the OpenSUSE Tumbleweed override for docdir
unnecessary: drop it.

Suggested-by: Daniel P. Berrangé <berrange(a)redhat.com>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 contrib/fedora/passt.spec | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/contrib/fedora/passt.spec b/contrib/fedora/passt.spec
index a8af326..ca22d38 100644
--- a/contrib/fedora/passt.spec
+++ b/contrib/fedora/passt.spec
@@ -49,10 +49,8 @@ This package adds SELinux enforcement to passt(1) and pasta(1).
 %make_build
 
 %install
-%if 0%{?suse_version} > 910
-%make_install DESTDIR=%{buildroot} prefix=%{_prefix} docdir=%{_prefix}/share/doc/packages/passt
-%else
-%make_install DESTDIR=%{buildroot} prefix=%{_prefix}
+%make_install DESTDIR=%{buildroot} prefix=%{_prefix} bindir=%{_bindir} mandir=%{_mandir} docdir=%{_docdir}/passt
+
 %endif
 %ifarch x86_64
 ln -sr %{buildroot}%{_mandir}/man1/passt.1 %{buildroot}%{_mandir}/man1/passt.avx2.1
-- 
@@ -49,10 +49,8 @@ This package adds SELinux enforcement to passt(1) and pasta(1).
 %make_build
 
 %install
-%if 0%{?suse_version} > 910
-%make_install DESTDIR=%{buildroot} prefix=%{_prefix} docdir=%{_prefix}/share/doc/packages/passt
-%else
-%make_install DESTDIR=%{buildroot} prefix=%{_prefix}
+%make_install DESTDIR=%{buildroot} prefix=%{_prefix} bindir=%{_bindir} mandir=%{_mandir} docdir=%{_docdir}/passt
+
 %endif
 %ifarch x86_64
 ln -sr %{buildroot}%{_mandir}/man1/passt.1 %{buildroot}%{_mandir}/man1/passt.avx2.1
-- 
2.35.1

Re: [PATCH 7/7] fedora: Pass explicit bindir, mandir, docdir, and drop OpenSUSE override

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

On Mon, 29 Aug 2022 17:17:09 +0200
Stefano Brivio <sbrivio(a)redhat.com> wrote:

> Fedora's parameters currently match the ones from the Makefile (which
> is based on GNU recommendations), but that's not necessarily
> guaranteed.
> 
> This should make the OpenSUSE Tumbleweed override for docdir
> unnecessary: drop it.
> 
> Suggested-by: Daniel P. Berrangé <berrange(a)redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
> ---
>  contrib/fedora/passt.spec | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/fedora/passt.spec b/contrib/fedora/passt.spec
> index a8af326..ca22d38 100644
> --- a/contrib/fedora/passt.spec
> +++ b/contrib/fedora/passt.spec
> @@ -49,10 +49,8 @@ This package adds SELinux enforcement to passt(1) and pasta(1).
>  %make_build
>  
>  %install
> -%if 0%{?suse_version} > 910
> -%make_install DESTDIR=%{buildroot} prefix=%{_prefix} docdir=%{_prefix}/share/doc/packages/passt
> -%else
> -%make_install DESTDIR=%{buildroot} prefix=%{_prefix}
> +%make_install DESTDIR=%{buildroot} prefix=%{_prefix} bindir=%{_bindir} mandir=%{_mandir} docdir=%{_docdir}/passt
> +
>  %endif

Oops, I forgot to complete a rebase before sending this: this %endif
goes away of course.

-- 
Stefano