[gentoo-portage-dev] [PATCH] install-qa-check.d: Add a QA check for installing xattrs

[gentoo-portage-dev] [PATCH] install-qa-check.d: Add a QA check for installing xattrs

[Michał Górny]

Warn the developers if ebuilds install files with xattrs to ${ED}.
The xattrs may or may not be preserved when installing the package,
making them unreliable on one hand, and somewhat suprising in other
cases (e.g. when they unintentionally leak from developer's system).

This is the first step towards restoring PMS compliance and *not*
preserving extended metadata.

Signed-off-by: Michał Górny <mgorny@gentoo.org>
---
 bin/install-qa-check.d/95xattr | 54 ++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 bin/install-qa-check.d/95xattr

diff --git a/bin/install-qa-check.d/95xattr b/bin/install-qa-check.d/95xattr
new file mode 100644
index 000000000..07d8042a8
--- /dev/null
+++ b/bin/install-qa-check.d/95xattr
@@ -0,0 +1,54 @@
+# Check for xattrs.
+
+xattr_check() {
+	type -P getfattr >/dev/null || return
+
+	pushd "${ED}" >/dev/null || die
+	local x file= keys
+	local -A data=()
+	while read -r x; do
+		case ${x} in
+			"# file: "*)
+				file=${x#*: }
+				file=/${file#.}
+				;;
+			btrfs.*)
+				# ignore btrfs xattrs, they're implicit fs metadata
+				;;
+			security.capability)
+				# don't report caps if we have fcaps.eclass inherited
+				if ! has fcaps ${INHERITED}; then
+					data[${file}]+=" ${x}"
+				fi
+				;;
+			?*)
+				data[${file}]+=" ${x}"
+				;;
+		esac
+	done < <(getfattr -R -h -m - . 2>/dev/null)
+	popd >/dev/null || die
+
+	if [[ ${data[@]} ]]; then
+		eqawarn "One or more files in \${ED} include extended attributes."
+		eqawarn
+
+		for file in "${!data[@]}"; do
+			keys=( ${data[${file}]} )
+			for x in "${keys[@]}"; do
+				eqatag xattr "key=${x}" "${file}"
+			done
+			eqawarn "  ${file} (${keys[*]})"
+		done
+
+		eqawarn
+		eqawarn "It is impossible to reliably guarantee that the extended attributes"
+		eqawarn "will be reliably preserved while merging.  Please ensure that any"
+		eqawarn "extended metadata necessary is applied in pkg_postinst() phase,"
+		eqawarn "and that the implementation includes a fallback if necessary."
+	fi
+}
+
+xattr_check
+: # guarantee successful exit
+
+# vim:ft=sh
-- 
2.33.0

Re: [gentoo-portage-dev] [PATCH] install-qa-check.d: Add a QA check for installing xattrs

[Mike Gilbert]

On Mon, Sep 27, 2021 at 1:20 PM Michał Górny <mgorny@gentoo.org> wrote:
>
> Warn the developers if ebuilds install files with xattrs to ${ED}.
> The xattrs may or may not be preserved when installing the package,
> making them unreliable on one hand, and somewhat suprising in other
> cases (e.g. when they unintentionally leak from developer's system).
>
> This is the first step towards restoring PMS compliance and *not*
> preserving extended metadata.

How does preserving xattrs conflict with PMS?

Is there a bug report you could reference?

Re: [gentoo-portage-dev] [PATCH] install-qa-check.d: Add a QA check for installing xattrs

[Mike Gilbert]

On Mon, Sep 27, 2021 at 1:20 PM Michał Górny <mgorny@gentoo.org> wrote:
> +               eqawarn
> +               eqawarn "It is impossible to reliably guarantee that the extended attributes"
> +               eqawarn "will be reliably preserved while merging.  Please ensure that any"
> +               eqawarn "extended metadata necessary is applied in pkg_postinst() phase,"
> +               eqawarn "and that the implementation includes a fallback if necessary."

This message suggests that applying xattrs in pkg_postinst is
acceptable. However, your patch offers no way to disable the QA
warning for ebuilds that do so.

Re: [gentoo-portage-dev] [PATCH] install-qa-check.d: Add a QA check for installing xattrs

[Michał Górny]

On Mon, 2021-09-27 at 21:03 -0400, Mike Gilbert wrote:
> On Mon, Sep 27, 2021 at 1:20 PM Michał Górny <mgorny@gentoo.org> wrote:
> > 
> > Warn the developers if ebuilds install files with xattrs to ${ED}.
> > The xattrs may or may not be preserved when installing the package,
> > making them unreliable on one hand, and somewhat suprising in other
> > cases (e.g. when they unintentionally leak from developer's system).
> > 
> > This is the first step towards restoring PMS compliance and *not*
> > preserving extended metadata.
> 
> How does preserving xattrs conflict with PMS?

The PMS doesn't specify that xattrs, ACLs, caps etc. are preserved.
By doing that, Portage allows developers to commit ebuilds that are not
going to work reliably without even realizing it.  In fact, this can't
even work reliably inside Portage itself, depending on the filesystem
used for $D.

Furthermore, doexe preserving stuff goes contrary to common sense.  Why
would helpers preserve xattrs when they are supposed to reset things
like mode and ownership by design?

> Is there a bug report you could reference?

It starts with https://bugs.gentoo.org/814857.



-- 
Best regards,
Michał Górny

Re: [gentoo-portage-dev] [PATCH] install-qa-check.d: Add a QA check for installing xattrs

[Michał Górny]

On Mon, 2021-09-27 at 21:09 -0400, Mike Gilbert wrote:
> On Mon, Sep 27, 2021 at 1:20 PM Michał Górny <mgorny@gentoo.org> wrote:
> > +               eqawarn
> > +               eqawarn "It is impossible to reliably guarantee that the extended attributes"
> > +               eqawarn "will be reliably preserved while merging.  Please ensure that any"
> > +               eqawarn "extended metadata necessary is applied in pkg_postinst() phase,"
> > +               eqawarn "and that the implementation includes a fallback if necessary."
> 
> This message suggests that applying xattrs in pkg_postinst is
> acceptable. However, your patch offers no way to disable the QA
> warning for ebuilds that do so.

We'll cross that bridge when we get there.  Ideally, we wouldn't need to
silence the check because no packages would do that.  If they do, then
we'll probably want to work on an eclass like fcaps.eclas.

-- 
Best regards,
Michał Górny

Re: [gentoo-portage-dev] [PATCH] install-qa-check.d: Add a QA check for installing xattrs

[Mike Gilbert]

On Tue, Sep 28, 2021 at 2:25 AM Michał Górny <mgorny@gentoo.org> wrote:
>
> On Mon, 2021-09-27 at 21:09 -0400, Mike Gilbert wrote:
> > On Mon, Sep 27, 2021 at 1:20 PM Michał Górny <mgorny@gentoo.org> wrote:
> > > +               eqawarn
> > > +               eqawarn "It is impossible to reliably guarantee that the extended attributes"
> > > +               eqawarn "will be reliably preserved while merging.  Please ensure that any"
> > > +               eqawarn "extended metadata necessary is applied in pkg_postinst() phase,"
> > > +               eqawarn "and that the implementation includes a fallback if necessary."
> >
> > This message suggests that applying xattrs in pkg_postinst is
> > acceptable. However, your patch offers no way to disable the QA
> > warning for ebuilds that do so.
>
> We'll cross that bridge when we get there.  Ideally, we wouldn't need to
> silence the check because no packages would do that.  If they do, then
> we'll probably want to work on an eclass like fcaps.eclas.

We need a way to silence this thing when false positives pop up and/or
ebuilds are adjusted. That needs to be there from day 1, not when we
cross some bridge later.

An immediate example: packages that call pax-mark in src_compile
because the need to disable MPROTECT on binary that is called a
compile time will end up with extended attributes in ${D} due to
install-xattr. We can adjust them to also call pax-mark in
pkg_postinst, but that won't magically make them go away in ${D}.

Re: [gentoo-portage-dev] [PATCH] install-qa-check.d: Add a QA check for installing xattrs

[Arfrever Frehtes Taifersar Arahesis]

Not relying on preservation of xatrs would be a gentoo.git tree policy.

If such policy is created, new QA check file would belong in
gentoo.git repository in metadata/install-qa-check.d directory, not in
Portage repository.
(And there is no need to delete any xattr-related code in Portage in future.)

Creation of such policy would need discussion on gentoo-dev mailing
list and/or formal vote in QA team.