[PATCH v2 1/2] package.bbclass: add prohibited-path qa test

[PATCH v2 1/2] package.bbclass: add prohibited-path qa test

[Martyn Welch]

Sometimes we wish to ensure that packages don't install files or
directories somewhere that may prove detrimental to the operation of the
system. For example, this may be the case if files are placed in a
directory that is utilised as a mount point at run time, thus making them
inaccessible once when the mount point is being utilised.

Implement the prohibited-path QA test, which enables such locations to be
specified in a "PROHIBITED_PATH" variable. This implementation allows for
exact matches and simple wildcards (paths ending with an asterisk. An
error will be raised should a match be found, or in the case of a
wildcard, for any files added below the specificed location(s).

Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

Changes since v1:
 - Correcting author and SOB.

 meta/classes/insane.bbclass  |  2 +-
 meta/classes/package.bbclass | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index def9c70..fb10681 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -33,7 +33,7 @@ ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
             perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
             split-strip packages-list pkgv-undefined var-undefined \
             version-going-backwards expanded-d invalid-chars \
-            license-checksum dev-elf file-rdeps \
+            license-checksum dev-elf file-rdeps prohibited-path \
             "
 # Add usrmerge QA check based on distro feature
 ERROR_QA_append = "${@bb.utils.contains('DISTRO_FEATURES', 'usrmerge', ' usrmerge', '', d)}"
diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
index 2053d46..721ca1e 100644
--- a/meta/classes/package.bbclass
+++ b/meta/classes/package.bbclass
@@ -1162,6 +1162,17 @@ python populate_packages () {
                 continue
             seen.append(file)
 
+            prohibited_path = d.getVar('PROHIBITED_PATH')
+            if prohibited_path is not None:
+                for p in prohibited_path.split():
+                    exactmatch = True
+                    if p.endswith("*"):
+                        p = p[:len(p)-1]
+                        exactmatch = False
+                    if file[1:].startswith(p) and ((file[1:] != p) or exactmatch) :
+                        msg = "%s is in a prohibited path.\n" % file[1:]
+                        package_qa_handle_error("prohibited-path", msg, d)
+
             def mkdir(src, dest, p):
                 src = os.path.join(src, p)
                 dest = os.path.join(dest, p)
-- 
2.1.4

[PATCH v2 2/2] ref-manual: Add documentation for prohibited-path QA test

[Martyn Welch]

Add documentation for the prohibited-path QA test and associated
PROHIBITED_PATH variable.

Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---

Changes since v1:
 - Correcting author and SOB.

 documentation/ref-manual/ref-classes.xml   |  5 +++++
 documentation/ref-manual/ref-variables.xml | 25 +++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/documentation/ref-manual/ref-classes.xml b/documentation/ref-manual/ref-classes.xml
index 5961d3e..15f5586 100644
--- a/documentation/ref-manual/ref-classes.xml
+++ b/documentation/ref-manual/ref-classes.xml
@@ -1714,6 +1714,11 @@
                 <filename>FILES_${PN} = "xyz"</filename> effectively turn into
                 <filename>FILES = "xyz"</filename>.
                 </para></listitem>
+            <listitem><para><emphasis><filename>prohibited-path:</filename></emphasis>
+                Checks that a recipe does not package and files in locations
+                specified in
+                <link linkend='var-PROHIBITED_PATH'><filename>PROHIBITED_PATH</filename></link>.
+                </para></listitem>
            <listitem><para><emphasis><filename>rpaths:</filename></emphasis>
                 Checks for rpaths in the binaries that contain build system paths such
                 as <filename>TMPDIR</filename>.
diff --git a/documentation/ref-manual/ref-variables.xml b/documentation/ref-manual/ref-variables.xml
index e31aa21..0bdbbba 100644
--- a/documentation/ref-manual/ref-variables.xml
+++ b/documentation/ref-manual/ref-variables.xml
@@ -10820,6 +10820,31 @@ recipes-graphics/xorg-font/font-alias_1.0.3.bb:PR = "${INC_PR}.3"
             </glossdef>
         </glossentry>
 
+        <glossentry id='var-PROHIBITED_PATH'><glossterm>PROHIBITED_PATH</glossterm>
+            <info>
+                PROHIBITED_PATH[doc] = "A list of paths in which recipes are prohibited from installing."
+            </info>
+            <glossdef>
+                <para role="glossdeffirst">
+<!--                <para role="glossdeffirst"><imagedata fileref="figures/define-generic.png" /> -->
+                    A list of paths in which recipes are prohibited from
+                    installing.
+                    Paths can be provided for specific locations or may include
+                    a wildcard asterisk at the end to ensure nothing is
+                    installed under the path provided.
+                </para>
+
+                <para>
+                    For example, the following
+                    <filename>PROHIBITED_PATH</filename> ensures no packages
+                    install anything under <filename>/mnt</filename>:
+                    <literallayout class='monospaced'>
+     PROHIBITED_PATH += "/mnt/*"
+                    </literallayout>
+                </para>
+            </glossdef>
+        </glossentry>
+
         <glossentry id='var-PROVIDES'><glossterm>PROVIDES</glossterm>
             <info>
                 PROVIDES[doc] = "A list of aliases that a recipe also provides. These aliases are useful for satisfying dependencies of other recipes during the build as specified by DEPENDS."
-- 
2.1.4

✗ patchtest: failure for "[v2] package.bbclass: add proh..." and 1 more

[Patchwork]

== Series Details ==

Series: "[v2] package.bbclass: add proh..." and 1 more
Revision: 1
URL   : https://patchwork.openembedded.org/series/9760/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Issue             Series sent to the wrong mailing list or some patches from the series correspond to different mailing lists [test_target_mailing_list] 
  Suggested fix    Send the series again to the correct mailing list (ML)
  Suggested ML     yocto@yoctoproject.org [http://git.yoctoproject.org/cgit/cgit.cgi/yocto-docs/]
  Patch's path:    documentation/ref-manual/ref-classes.xml

* Issue             Series does not apply on top of target branch [test_series_merge_on_head] 
  Suggested fix    Rebase your series on top of targeted branch
  Targeted branch  master (currently at a17f3ec910)



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Guidelines:     https://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe

Re: [OE-core] [PATCH v2 1/2] package.bbclass: add prohibited-path qa test

[Alexander Kanavin]

On 11/13/2017 09:19 PM, Martyn Welch wrote:
> Sometimes we wish to ensure that packages don't install files or
> directories somewhere that may prove detrimental to the operation of the
> system. For example, this may be the case if files are placed in a
> directory that is utilised as a mount point at run time, thus making them
> inaccessible once when the mount point is being utilised.
> 
> Implement the prohibited-path QA test, which enables such locations to be
> specified in a "PROHIBITED_PATH" variable. This implementation allows for
> exact matches and simple wildcards (paths ending with an asterisk. An
> error will be raised should a match be found, or in the case of a
> wildcard, for any files added below the specificed location(s).

I think this situation is an image configuration problem, not a package 
qa problem. If a package wants to install something in a location that 
is deemed inappropriate by the local config, there's a reason behind it, 
the fix may not be easy to develop, and will likely be useless or even 
unacceptable to other users of the recipe. It's better to reconfigure 
the mount points.

Can you perform this check using IMAGE_QA_COMMANDS please?

On a broader note, we've had an open issue to improve image qa testing 
for a long time, so maybe this could be a first step :)
https://bugzilla.yoctoproject.org/show_bug.cgi?id=10636

Alex

Re: [PATCH v2 1/2] package.bbclass: add prohibited-path qa test

[Alexander Kanavin]

On 11/13/2017 09:19 PM, Martyn Welch wrote:
> Sometimes we wish to ensure that packages don't install files or
> directories somewhere that may prove detrimental to the operation of the
> system. For example, this may be the case if files are placed in a
> directory that is utilised as a mount point at run time, thus making them
> inaccessible once when the mount point is being utilised.
> 
> Implement the prohibited-path QA test, which enables such locations to be
> specified in a "PROHIBITED_PATH" variable. This implementation allows for
> exact matches and simple wildcards (paths ending with an asterisk. An
> error will be raised should a match be found, or in the case of a
> wildcard, for any files added below the specificed location(s).

I think this situation is an image configuration problem, not a package 
qa problem. If a package wants to install something in a location that 
is deemed inappropriate by the local config, there's a reason behind it, 
the fix may not be easy to develop, and will likely be useless or even 
unacceptable to other users of the recipe. It's better to reconfigure 
the mount points.

Can you perform this check using IMAGE_QA_COMMANDS please?

On a broader note, we've had an open issue to improve image qa testing 
for a long time, so maybe this could be a first step :)
https://bugzilla.yoctoproject.org/show_bug.cgi?id=10636

Alex

Re: [OE-core] [PATCH v2 1/2] package.bbclass: add prohibited-path qa test

[Alexander Kanavin]

On 11/14/2017 02:47 PM, Alexander Kanavin wrote:

> I think this situation is an image configuration problem, not a package 
> qa problem. If a package wants to install something in a location that 
> is deemed inappropriate by the local config, there's a reason behind it, 
> the fix may not be easy to develop, and will likely be useless or even 
> unacceptable to other users of the recipe. It's better to reconfigure 
> the mount points.

Oh and another point I forgot: some files are created at do_rootfs time 
when packages are installed onto the image, so they won't be caught by 
this check at all. Better to do it at the last moment.

Alex

Re: [PATCH v2 1/2] package.bbclass: add prohibited-path qa test

[Alexander Kanavin]

On 11/14/2017 02:47 PM, Alexander Kanavin wrote:

> I think this situation is an image configuration problem, not a package 
> qa problem. If a package wants to install something in a location that 
> is deemed inappropriate by the local config, there's a reason behind it, 
> the fix may not be easy to develop, and will likely be useless or even 
> unacceptable to other users of the recipe. It's better to reconfigure 
> the mount points.

Oh and another point I forgot: some files are created at do_rootfs time 
when packages are installed onto the image, so they won't be caught by 
this check at all. Better to do it at the last moment.

Alex

Re: [OE-core] [PATCH v2 1/2] package.bbclass: add prohibited-path qa test

[Andreas Oberritter]

Hi Martyn,

On Mon, 13 Nov 2017 19:19:47 +0000
Martyn Welch <martyn.welch@collabora.co.uk> wrote:

> Sometimes we wish to ensure that packages don't install files or
> directories somewhere that may prove detrimental to the operation of the
> system. For example, this may be the case if files are placed in a
> directory that is utilised as a mount point at run time, thus making them
> inaccessible once when the mount point is being utilised.
> 
> Implement the prohibited-path QA test, which enables such locations to be
> specified in a "PROHIBITED_PATH" variable. This implementation allows for
> exact matches and simple wildcards (paths ending with an asterisk. An
> error will be raised should a match be found, or in the case of a
> wildcard, for any files added below the specificed location(s).
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
> 
> Changes since v1:
>  - Correcting author and SOB.
> 
>  meta/classes/insane.bbclass  |  2 +-
>  meta/classes/package.bbclass | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index def9c70..fb10681 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -33,7 +33,7 @@ ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
>              perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
>              split-strip packages-list pkgv-undefined var-undefined \
>              version-going-backwards expanded-d invalid-chars \
> -            license-checksum dev-elf file-rdeps \
> +            license-checksum dev-elf file-rdeps prohibited-path \
>              "
>  # Add usrmerge QA check based on distro feature
>  ERROR_QA_append = "${@bb.utils.contains('DISTRO_FEATURES', 'usrmerge', ' usrmerge', '', d)}"
> diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
> index 2053d46..721ca1e 100644
> --- a/meta/classes/package.bbclass
> +++ b/meta/classes/package.bbclass
> @@ -1162,6 +1162,17 @@ python populate_packages () {
>                  continue
>              seen.append(file)
>  
> +            prohibited_path = d.getVar('PROHIBITED_PATH')
> +            if prohibited_path is not None:
> +                for p in prohibited_path.split():

Paths may contain space characters. How about using colons as path separators
like in $PATH and several other path variables in OE-Core?

> +                    exactmatch = True
> +                    if p.endswith("*"):
> +                        p = p[:len(p)-1]
> +                        exactmatch = False
> +                    if file[1:].startswith(p) and ((file[1:] != p) or exactmatch) :
> +                        msg = "%s is in a prohibited path.\n" % file[1:]
> +                        package_qa_handle_error("prohibited-path", msg, d)
> +

You could use fnmatch to allow generic shell wildcards.

Why don't you compare the first character? Saved mount points are usually
absolute.

Regards,
Andreas

Re: [PATCH v2 1/2] package.bbclass: add prohibited-path qa test

[Andreas Oberritter]

Hi Martyn,

On Mon, 13 Nov 2017 19:19:47 +0000
Martyn Welch <martyn.welch@collabora.co.uk> wrote:

> Sometimes we wish to ensure that packages don't install files or
> directories somewhere that may prove detrimental to the operation of the
> system. For example, this may be the case if files are placed in a
> directory that is utilised as a mount point at run time, thus making them
> inaccessible once when the mount point is being utilised.
> 
> Implement the prohibited-path QA test, which enables such locations to be
> specified in a "PROHIBITED_PATH" variable. This implementation allows for
> exact matches and simple wildcards (paths ending with an asterisk. An
> error will be raised should a match be found, or in the case of a
> wildcard, for any files added below the specificed location(s).
> 
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
> 
> Changes since v1:
>  - Correcting author and SOB.
> 
>  meta/classes/insane.bbclass  |  2 +-
>  meta/classes/package.bbclass | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index def9c70..fb10681 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -33,7 +33,7 @@ ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch pkgconfig la \
>              perms dep-cmp pkgvarcheck perm-config perm-line perm-link \
>              split-strip packages-list pkgv-undefined var-undefined \
>              version-going-backwards expanded-d invalid-chars \
> -            license-checksum dev-elf file-rdeps \
> +            license-checksum dev-elf file-rdeps prohibited-path \
>              "
>  # Add usrmerge QA check based on distro feature
>  ERROR_QA_append = "${@bb.utils.contains('DISTRO_FEATURES', 'usrmerge', ' usrmerge', '', d)}"
> diff --git a/meta/classes/package.bbclass b/meta/classes/package.bbclass
> index 2053d46..721ca1e 100644
> --- a/meta/classes/package.bbclass
> +++ b/meta/classes/package.bbclass
> @@ -1162,6 +1162,17 @@ python populate_packages () {
>                  continue
>              seen.append(file)
>  
> +            prohibited_path = d.getVar('PROHIBITED_PATH')
> +            if prohibited_path is not None:
> +                for p in prohibited_path.split():

Paths may contain space characters. How about using colons as path separators
like in $PATH and several other path variables in OE-Core?

> +                    exactmatch = True
> +                    if p.endswith("*"):
> +                        p = p[:len(p)-1]
> +                        exactmatch = False
> +                    if file[1:].startswith(p) and ((file[1:] != p) or exactmatch) :
> +                        msg = "%s is in a prohibited path.\n" % file[1:]
> +                        package_qa_handle_error("prohibited-path", msg, d)
> +

You could use fnmatch to allow generic shell wildcards.

Why don't you compare the first character? Saved mount points are usually
absolute.

Regards,
Andreas