[Buildroot] [PATCH 0/5] core: check hashes of license files

[Buildroot] [PATCH 0/5] core: check hashes of license files

[Yann E. MORIN]

Hello All!

This small series is a proposal to check the hashes of the license files
during legal-info, to catch the packages whose license changes but where
the text of the new license is in the same file.

This was suggested by Rahul recently:
    http://lists.busybox.net/pipermail/buildroot/2017-June/194425.html

Changes v1 -> v2:
  - call MESSAGE to display current package name  (Luca)
  - don't mention weak hashes  (Luca)
  - fix support script check-package  (Ricardo)


Regards,
Yann E. MORIN.


The following changes since commit 0672f3f2133294b6248af0740bb7bb78f97bccf6

  uboot-tools: drop unreferenced empty variable (2017-06-25 15:54:09 +0200)


are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to 48ad498d1bf7e08c8120090942e711a1d2320c32

  docs/manual: document hashes for license files (2017-06-25 23:56:31 +0200)


----------------------------------------------------------------
Yann E. MORIN (5):
      support/check-package: don't check filenames of hashes
      core/pkg-generic: call MESSAGE when saving package legal-info
      core/pkg-util: pass package directory and name when saving license files
      core/pkg-utils: check hashes of license files
      docs/manual: document hashes for license files

 Makefile                                    |  2 +-
 docs/manual/adding-packages-directory.txt   | 15 +++++++++++++--
 package/pkg-generic.mk                      |  4 +++-
 package/pkg-utils.mk                        | 11 ++++++++---
 support/scripts/checkpackagelib/lib_hash.py | 16 ----------------
 5 files changed, 25 insertions(+), 23 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/5] support/check-package: don't check filenames of hashes

[Yann E. MORIN]

Currently, we check that the filenames in hash lists do not contain
a slash '/' character, because all we are checking so far are the
downloaded archives, and we explicitly need the filename to not contain
a directory component at all.

However, we're soon to also check the hashes of the license files in
packages sources, and those license files may be at any arbitrary
directory-depth in the packages source tree, and thus two or more
license files may have the same basename.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Peter Korsgaard <peter@korsgaard.com>
Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
 support/scripts/checkpackagelib/lib_hash.py | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/support/scripts/checkpackagelib/lib_hash.py b/support/scripts/checkpackagelib/lib_hash.py
index c76abb43f1..e4beb0404f 100644
--- a/support/scripts/checkpackagelib/lib_hash.py
+++ b/support/scripts/checkpackagelib/lib_hash.py
@@ -17,22 +17,6 @@ def _empty_line_or_comment(text):
     return text.strip() == "" or text.startswith("#")
 
 
-class HashFilename(_CheckFunction):
-    def check_line(self, lineno, text):
-        if _empty_line_or_comment(text):
-            return
-
-        fields = text.split()
-        if len(fields) < 3:
-            return
-
-        if '/' in fields[2]:
-            return ["{}:{}: use filename without directory component"
-                    " ({}#adding-packages-hash)"
-                    .format(self.filename, lineno, self.url_to_manual),
-                    text]
-
-
 class HashNumberOfFields(_CheckFunction):
     def check_line(self, lineno, text):
         if _empty_line_or_comment(text):
-- 
2.11.0

[Buildroot] [PATCH 1/5] support/check-package: don't check filenames of hashes

[Ricardo Martincoski]

Hello,

On Sun, Jun 25, 2017 at 07:03 PM, Yann E. MORIN wrote:

> Currently, we check that the filenames in hash lists do not contain
> a slash '/' character, because all we are checking so far are the
> downloaded archives, and we explicitly need the filename to not contain
> a directory component at all.
> 
> However, we're soon to also check the hashes of the license files in
> packages sources, and those license files may be at any arbitrary
> directory-depth in the packages source tree, and thus two or more

Small nit: the explanation could end at "source tree". The removed code was
testing only for the presence of a slash, so having 2 entries with the same
basename shouldn't matter.

> license files may have the same basename.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Either with the nit fixed or not,
Acked-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Regards,
Ricardo

[Buildroot] [PATCH 1/5] support/check-package: don't check filenames of hashes

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Currently, we check that the filenames in hash lists do not contain
 > a slash '/' character, because all we are checking so far are the
 > downloaded archives, and we explicitly need the filename to not contain
 > a directory component at all.

 > However, we're soon to also check the hashes of the license files in
 > packages sources, and those license files may be at any arbitrary
 > directory-depth in the packages source tree, and thus two or more
 > license files may have the same basename.

Committed after adjusting the commit message as suggested by Richardo,
thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 2/5] core/pkg-generic: call MESSAGE when saving package legal-info

[Yann E. MORIN]

Currently, the per-package legal-info is mostly silent, but we're soon
to add a check for the hashes of the license files.

In that case, and when there is a hash mis-match, we want a user to know
what package had a changed license file.

So, we add a call to MESSAGE to display the package we're currently
saving the legal-info of, like so:

    >>> busybox 1.26.2 Collecting legal info

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Peter Korsgaard <peter@korsgaard.com>
Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
---
 package/pkg-generic.mk | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index f474704980..22330edc5b 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -835,7 +835,9 @@ endif
 endif
 
 # legal-info: produce legally relevant info.
+$(1)-legal-info: PKG=$(2)
 $(1)-legal-info:
+	@$$(call MESSAGE,"Collecting legal info")
 # Packages without a source are assumed to be part of Buildroot, skip them.
 	$$(foreach hook,$$($(2)_PRE_LEGAL_INFO_HOOKS),$$(call $$(hook))$$(sep))
 ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
-- 
2.11.0

[Buildroot] [PATCH 2/5] core/pkg-generic: call MESSAGE when saving package legal-info

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Currently, the per-package legal-info is mostly silent, but we're soon
 > to add a check for the hashes of the license files.

 > In that case, and when there is a hash mis-match, we want a user to know
 > what package had a changed license file.

 > So, we add a call to MESSAGE to display the package we're currently
 > saving the legal-info of, like so:

 >>>> busybox 1.26.2 Collecting legal info

I wonder if we still need the general "Collecting legal info" message,
as it now looks something like:

>>>   Collecting legal info
>>> toolchain-buildroot  Collecting legal info
>>> host-gcc-final 6.3.0 Collecting legal info
>>> host-binutils 2.27 Collecting legal info
>>> host-gmp 6.1.2 Collecting legal info

Though, with the entire patch series applied it might be handy to match
the warnings, E.G.:

>>>   Collecting legal info
WARNING: no hash file for COPYING
>>> toolchain-buildroot  Collecting legal info

Presumably this warning is from legal-info-prepare. I'm not sure exactly
how we should get rid of this warning? A buildroot.hash with an entry
for our COPYING?

That can be fixed later though - Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 3/5] core/pkg-util: pass package directory and name when saving license files

[Yann E. MORIN]

This will be usefull when checking the hashes of the license files.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Peter Korsgaard <peter@korsgaard.com>
Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
---
 Makefile               | 2 +-
 package/pkg-generic.mk | 2 +-
 package/pkg-utils.mk   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 88d98e0405..1c272d009c 100644
--- a/Makefile
+++ b/Makefile
@@ -740,7 +740,7 @@ legal-info-clean:
 .PHONY: legal-info-prepare
 legal-info-prepare: $(LEGAL_INFO_DIR)
 	@$(call MESSAGE,"Collecting legal info")
-	@$(call legal-license-file,buildroot,COPYING,COPYING,HOST)
+	@$(call legal-license-file,buildroot,buildroot,,COPYING,COPYING,HOST)
 	@$(call legal-manifest,PACKAGE,VERSION,LICENSE,LICENSE FILES,SOURCE ARCHIVE,SOURCE SITE,TARGET)
 	@$(call legal-manifest,PACKAGE,VERSION,LICENSE,LICENSE FILES,SOURCE ARCHIVE,SOURCE SITE,HOST)
 	@$(call legal-manifest,buildroot,$(BR2_VERSION_FULL),GPL-2.0+,COPYING,not saved,not saved,HOST)
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 22330edc5b..2916a7bbad 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -851,7 +851,7 @@ ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
 ifeq ($$(call qstrip,$$($(2)_LICENSE_FILES)),)
 	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),cannot save license ($(2)_LICENSE_FILES not defined))
 else
-	@$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAW_BASE_NAME),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
+	@$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_NAME),$$($(2)_RAW_BASE_NAME),$$($(2)_PKGDIR),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
 endif # license files
 
 ifeq ($$($(2)_SITE_METHOD),local)
diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index c95e77953b..e9ac56276f 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -83,7 +83,7 @@ define legal-manifest # pkg, version, license, license-files, source, url, {HOST
 	echo '"$(1)","$(2)","$(3)","$(4)","$(5)","$(6)"' >>$(LEGAL_MANIFEST_CSV_$(7))
 endef
 
-define legal-license-file # pkg, filename, file-fullpath, {HOST|TARGET}
-	mkdir -p $(LICENSE_FILES_DIR_$(4))/$(1)/$(dir $(2)) && \
-	cp $(3) $(LICENSE_FILES_DIR_$(4))/$(1)/$(2)
+define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
+	mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \
+	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
 endef
-- 
2.11.0

[Buildroot] [PATCH 3/5] core/pkg-util: pass package directory and name when saving license files

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > This will be usefull when checking the hashes of the license files.
 > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > Cc: Luca Ceresoli <luca@lucaceresoli.net>
 > Cc: Peter Korsgaard <peter@korsgaard.com>
 > Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
 > ---
 >  Makefile               | 2 +-
 >  package/pkg-generic.mk | 2 +-
 >  package/pkg-utils.mk   | 6 +++---
 >  3 files changed, 5 insertions(+), 5 deletions(-)

 > diff --git a/Makefile b/Makefile
 > index 88d98e0405..1c272d009c 100644
 > --- a/Makefile
 > +++ b/Makefile
 > @@ -740,7 +740,7 @@ legal-info-clean:
 >  .PHONY: legal-info-prepare
 >  legal-info-prepare: $(LEGAL_INFO_DIR)
 >  	@$(call MESSAGE,"Collecting legal info")
 > -	@$(call legal-license-file,buildroot,COPYING,COPYING,HOST)
 > +	@$(call legal-license-file,buildroot,buildroot,,COPYING,COPYING,HOST)

I believe the third argument should be '.' (or some subdirectory if we
want to hide the file) as check-hash is called with $(3)/$(1).hash

So I've fixed that and committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 4/5] core/pkg-utils: check hashes of license files

[Yann E. MORIN]

This will help catch a change of license even if the filename does
not change.

For now, a missing hash for the license files is not a fatal error, to
let people catch up and add them. When we switch to make it mandatory,
we can simplify the code by just removing the case statement.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Peter Korsgaard <peter@korsgaard.com>
Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
---
 package/pkg-utils.mk | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index e9ac56276f..accf48c464 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -85,5 +85,10 @@ endef
 
 define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET}
 	mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \
+	{ \
+		support/download/check-hash $(3)/$(1).hash $(5) $(4); \
+		ret=$${?}; \
+		case $${ret} in (0|3) ;; (*) exit 1;; esac; \
+	} && \
 	cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4)
 endef
-- 
2.11.0

[Buildroot] [PATCH 4/5] core/pkg-utils: check hashes of license files

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > This will help catch a change of license even if the filename does
 > not change.

 > For now, a missing hash for the license files is not a fatal error, to
 > let people catch up and add them. When we switch to make it mandatory,
 > we can simplify the code by just removing the case statement.

 > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > Cc: Luca Ceresoli <luca@lucaceresoli.net>
 > Cc: Peter Korsgaard <peter@korsgaard.com>
 > Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 5/5] docs/manual: document hashes for license files

[Yann E. MORIN]

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Peter Korsgaard <peter@korsgaard.com>
Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>

---
Changes v1 -> v2:
  - don't mention that a weak hash is OK  (Luca)
---
 docs/manual/adding-packages-directory.txt | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt
index 08f5d42f91..b75b91a28c 100644
--- a/docs/manual/adding-packages-directory.txt
+++ b/docs/manual/adding-packages-directory.txt
@@ -443,7 +443,7 @@ Optionally, you can add a third file, named +libfoo.hash+, that contains
 the hashes of the downloaded files for the +libfoo+ package.
 
 The hashes stored in that file are used to validate the integrity of the
-downloaded files.
+downloaded files and of the license files.
 
 The format of this file is one line for each file for which to check the
 hash, each line being space-separated, with these three fields:
@@ -458,7 +458,10 @@ hash, each line being space-separated, with these three fields:
 ** for +sha256+, 64 hexadecimal characters
 ** for +sha384+, 96 hexadecimal characters
 ** for +sha512+, 128 hexadecimal characters
-* the name of the file, without any directory component
+* the name of the file:
+** for a source archive: the basename of the file, without any directory
+   component,
+** for a license file: the path as it appears in +FOO_LICENSE_FILES+.
 
 Lines starting with a +#+ sign are considered comments, and ignored. Empty
 lines are ignored.
@@ -476,6 +479,10 @@ strong hash yourself (preferably +sha256+, but not +md5+), and mention
 this in a comment line above the hashes.
 
 .Note
+The hashes for license files are used to detect a license change when a
+package version is bumped.
+
+.Note
 The number of spaces does not matter, so one can use spaces (or tabs) to
 properly align the different fields.
 
@@ -501,6 +508,10 @@ sha256 ff52101fb90bbfc3fe9475e425688c660f46216d7e751c4bbdb1dc85cdccacb9 libfoo-f
 
 # No hash for 1234:
 none   xxx                                                              libfoo-1234.tar.gz
+
+# Hash for license files:
+sha1  c47a888f2be626e1197b6f651dee966ef882077d  COPYING
+sha1  e101765734390d664b59325b2d644d80d9a6bd9a  doc/COPYING.LGPL
 ----
 
 If the +.hash+ file is present, and it contains one or more hashes for a
-- 
2.11.0

[Buildroot] [PATCH 5/5] docs/manual: document hashes for license files

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
 > Cc: Luca Ceresoli <luca@lucaceresoli.net>
 > Cc: Peter Korsgaard <peter@korsgaard.com>
 > Cc: Rahul Bedarkar <rahulbedarkar89@gmail.com>
 > Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>

 >  # No hash for 1234:
 >  none   xxx                                                              libfoo-1234.tar.gz
 > +
 > +# Hash for license files:
 > +sha1  c47a888f2be626e1197b6f651dee966ef882077d  COPYING
 > +sha1  e101765734390d664b59325b2d644d80d9a6bd9a  doc/COPYING.LGPL

I think it makes more sense to recommend sha256 like for tarballs, so
I've changed this to sha256 and committed, thanks.

-- 
Bye, Peter Korsgaard