[LEDE-DEV,2/2] download.mk: introduce a new variable SKIPHASH

Message ID 20171026095059.6592-2-git@bitsofnetworks.org
State Superseded
Delegated to: John Crispin
Headers show
Series
  • [LEDE-DEV,1/2] scripts/download.pl: Add a --skip-hash option
Related show

Commit Message

Baptiste Jonglez Oct. 26, 2017, 9:50 a.m.
When calling a download target, hash verification is now completely
skipped if the SKIPHASH variable is set.

This allows to easily bump package version:

    # Update PKG_VERSION in the package Makefile
    $ make package/<mypackage>/download SKIPHASH=1 V=s
    $ make package/<mypackage>/check FIXUP=1 V=s

This will download the new version of the package, and then automatically
update PKG_HASH with the hash of the new version.  Of course, it is still
the responsibility of the packager to ensure that the new tarball is
legitimate, because it is downloaded from a possibly untrusted source.

Fixes: b30ba14e ("scripts/download.pl: fail loudly if provided hash is unsupported")
Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org>
---
 include/download.mk | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Karl Palsson Oct. 26, 2017, 10:35 a.m. | #1
Baptiste Jonglez <git@bitsofnetworks.org> wrote:
> When calling a download target, hash verification is now
> completely skipped if the SKIPHASH variable is set.
> 
> This allows to easily bump package version:
> 
>     # Update PKG_VERSION in the package Makefile
>     $ make package/<mypackage>/download SKIPHASH=1 V=s
>     $ make package/<mypackage>/check FIXUP=1 V=s
> 
> This will download the new version of the package, and then
> automatically update PKG_HASH with the hash of the new version.
> Of course, it is still the responsibility of the packager to
> ensure that the new tarball is legitimate, because it is
> downloaded from a possibly untrusted source.
> 
> Fixes: b30ba14e ("scripts/download.pl: fail loudly if provided
> hash is unsupported") Signed-off-by: Baptiste Jonglez
> <git@bitsofnetworks.org>


Until/if your new documentation project takes off, please
consider adding this to the "how to package" doc pages. none of
these extra magical parameters are discoverable in any way right
now.
Andrey Jr. Melnikov Oct. 26, 2017, 1:01 p.m. | #2
Baptiste Jonglez <git@bitsofnetworks.org> wrote:
> When calling a download target, hash verification is now completely
> skipped if the SKIPHASH variable is set.

> This allows to easily bump package version:

>     # Update PKG_VERSION in the package Makefile
>     $ make package/<mypackage>/download SKIPHASH=1 V=s
>     $ make package/<mypackage>/check FIXUP=1 V=s

Maybe better introduce DL_OPT and use it?

ifdef SKIPHASH
DL_OPT += --skip-hash
endif

...

$(SCRIPT_DIR)/download.pl $(DL_OPT) "$(DL_DIR)" "$(FILE)" "$(HASH)" "$(URL_FILE)" $(foreach url,$(URL),"$(url)") \
Yousong Zhou Oct. 27, 2017, 1:56 a.m. | #3
On 26 October 2017 at 17:50, Baptiste Jonglez <git@bitsofnetworks.org> wrote:
> When calling a download target, hash verification is now completely
> skipped if the SKIPHASH variable is set.
>
> This allows to easily bump package version:
>
>     # Update PKG_VERSION in the package Makefile
>     $ make package/<mypackage>/download SKIPHASH=1 V=s
>     $ make package/<mypackage>/check FIXUP=1 V=s
>
> This will download the new version of the package, and then automatically
> update PKG_HASH with the hash of the new version.  Of course, it is still
> the responsibility of the packager to ensure that the new tarball is
> legitimate, because it is downloaded from a possibly untrusted source.

Introducing another knob to the build system seems cubersome.  I
remembered that hash checking would be skipped if PKG_MD5SUM var was
empty and the behaviour is very likely the same with PKG_HASH.  The
workflow can be simply emptying PKG_HASH var while bumping the
versions, then do the download and hash fixup on the second command.
This should eliminate the need for SKIPHASH var.

Regards,
                yousong
Baptiste Jonglez Oct. 27, 2017, 7:11 a.m. | #4
On 26-10-17, Karl Palsson wrote:
> Until/if your new documentation project takes off, please
> consider adding this to the "how to package" doc pages. none of
> these extra magical parameters are discoverable in any way right
> now.

Fully agreed, all this is magical enough :)  While writing this patch, I even
discovered some features of download.pl I didn't know about and are not
documented anywhere...

By the way, FIXUP=1 is already documented here: https://wiki.openwrt.org/doc/devel/packages#use_source_repository

Baptiste
Baptiste Jonglez Oct. 27, 2017, 8:27 a.m. | #5
On 27-10-17, Yousong Zhou wrote:
> On 26 October 2017 at 17:50, Baptiste Jonglez <git@bitsofnetworks.org> wrote:
> > When calling a download target, hash verification is now completely
> > skipped if the SKIPHASH variable is set.
> >
> > This allows to easily bump package version:
> >
> >     # Update PKG_VERSION in the package Makefile
> >     $ make package/<mypackage>/download SKIPHASH=1 V=s
> >     $ make package/<mypackage>/check FIXUP=1 V=s
> >
> > This will download the new version of the package, and then automatically
> > update PKG_HASH with the hash of the new version.  Of course, it is still
> > the responsibility of the packager to ensure that the new tarball is
> > legitimate, because it is downloaded from a possibly untrusted source.
> 
> Introducing another knob to the build system seems cubersome.  I
> remembered that hash checking would be skipped if PKG_MD5SUM var was
> empty and the behaviour is very likely the same with PKG_HASH.  The
> workflow can be simply emptying PKG_HASH var while bumping the
> versions, then do the download and hash fixup on the second command.
> This should eliminate the need for SKIPHASH var.

In my opinion, it's bad practice to implicitly skip hash verification in
some special cases.  Before my first patch [1], any typo in the hash would
result in verification being silently skipped.  You can also imagine a
typo in the variable name (e.g. "PKGHASH" instead of "PKG_HASH") and this
would inadvertently result in an empty hash.

When we're talking about bypassing security features, it's much better to
have an explicit knob to avoid mistakes.

[1] http://patchwork.ozlabs.org/patch/809259/

Baptiste
Baptiste Jonglez Nov. 8, 2017, 11:14 a.m. | #6
Hi Stijn,

What is your opinion on this patch?  There has been a bit of feedback, but
you were the one requesting the change in the first place :)

Thanks,
Baptiste

On 26-10-17, Baptiste Jonglez wrote:
> When calling a download target, hash verification is now completely
> skipped if the SKIPHASH variable is set.
> 
> This allows to easily bump package version:
> 
>     # Update PKG_VERSION in the package Makefile
>     $ make package/<mypackage>/download SKIPHASH=1 V=s
>     $ make package/<mypackage>/check FIXUP=1 V=s
> 
> This will download the new version of the package, and then automatically
> update PKG_HASH with the hash of the new version.  Of course, it is still
> the responsibility of the packager to ensure that the new tarball is
> legitimate, because it is downloaded from a possibly untrusted source.
> 
> Fixes: b30ba14e ("scripts/download.pl: fail loudly if provided hash is unsupported")
> Signed-off-by: Baptiste Jonglez <git@bitsofnetworks.org>
> ---
>  include/download.mk | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/download.mk b/include/download.mk
> index 0a25641738..a6821b5304 100644
> --- a/include/download.mk
> +++ b/include/download.mk
> @@ -102,12 +102,18 @@ check_md5 = \
>  hash_var = $(if $(filter-out x,$(1)),MD5SUM,HASH)
>  endif
>  
> +ifdef SKIPHASH
> +DOWNLOAD_CMD = $(SCRIPT_DIR)/download.pl --skip-hash
> +else
> +DOWNLOAD_CMD = $(SCRIPT_DIR)/download.pl
> +endif
> +
>  define DownloadMethod/unknown
>  	echo "ERROR: No download method available"; false
>  endef
>  
>  define DownloadMethod/default
> -	$(SCRIPT_DIR)/download.pl "$(DL_DIR)" "$(FILE)" "$(HASH)" "$(URL_FILE)" $(foreach url,$(URL),"$(url)") \
> +	$(DOWNLOAD_CMD) "$(DL_DIR)" "$(FILE)" "$(HASH)" "$(URL_FILE)" $(foreach url,$(URL),"$(url)") \
>  	$(if $(filter check,$(1)), \
>  		$(call check_hash,$(FILE),$(HASH),$(2)$(call hash_var,$(MD5SUM))) \
>  		$(call check_md5,$(MD5SUM),$(2)MD5SUM,$(2)HASH) \
Stijn Tintel Nov. 29, 2017, 7:19 p.m. | #7
On 08-11-17 12:14, Baptiste Jonglez wrote:
> Hi Stijn,
>
> What is your opinion on this patch?  There has been a bit of feedback, but
> you were the one requesting the change in the first place :)
It seems most feedback simply ignores the part where b30ba14e actually
broke existing functionality. I keep running into this breakage when
using the script to bump kernel version, or when bumping packages I
maintain. While I agree that adding another knob seems cumbersome, I
would like to be able to use the broken functionality, so

Acked-by: Stijn Tintel <stijn@linux-ipv6.be>
Jo-Philipp Wich Dec. 7, 2017, 2:56 p.m. | #8
Hi Baptiste,

we've been discussing this patch again on IRC today and I came up with
an alternate suggestion that does not require a new variable.

-- 8< --
diff --git a/scripts/download.pl b/scripts/download.pl
index 775408934a..ad9c480c67 100755
--- a/scripts/download.pl
+++ b/scripts/download.pl
@@ -88,7 +88,7 @@ sub download_cmd($) {
 }

 my $hash_cmd = hash_cmd();
-$hash_cmd or die "Cannot find appropriate hash command, ensure the
provided hash is either a MD5 or SHA256 checksum.\n";
+$hash_cmd or ($file_hash eq "none") or die "Cannot find appropriate
hash command, ensure the provided hash is either a MD5 or SHA256
checksum.\n";

 sub download
 {
-- >8 --

Using the change above one can issue a

  "make package/mypackage/download PKG_HASH=none"

to download ignoring the Makefile checksum while preserving the error
semantics of unset PKG_HASH cases.

Ideally I'd like to push the "none" variant but wanted to share the idea
on the list first to see what others think about it.

Regards,
Jo
Stijn Tintel Dec. 7, 2017, 6:38 p.m. | #9
On 07-12-17 15:56, Jo-Philipp Wich wrote:
> Hi Baptiste,
>
> we've been discussing this patch again on IRC today and I came up with
> an alternate suggestion that does not require a new variable.
>
> -- 8< --
> diff --git a/scripts/download.pl b/scripts/download.pl
> index 775408934a..ad9c480c67 100755
> --- a/scripts/download.pl
> +++ b/scripts/download.pl
> @@ -88,7 +88,7 @@ sub download_cmd($) {
>  }
>
>  my $hash_cmd = hash_cmd();
> -$hash_cmd or die "Cannot find appropriate hash command, ensure the
> provided hash is either a MD5 or SHA256 checksum.\n";
> +$hash_cmd or ($file_hash eq "none") or die "Cannot find appropriate
> hash command, ensure the provided hash is either a MD5 or SHA256
> checksum.\n";
>
>  sub download
>  {
> -- >8 --
>
> Using the change above one can issue a
>
>   "make package/mypackage/download PKG_HASH=none"
>
> to download ignoring the Makefile checksum while preserving the error
> semantics of unset PKG_HASH cases.
>
> Ideally I'd like to push the "none" variant but wanted to share the idea
> on the list first to see what others think about it.
>
I prefer this over using an extra variable.

Acked-by: Stijn Tintel <stijn@linux-ipv6.be>
Ted Hess Dec. 7, 2017, 11:59 p.m. | #10
Ack from me for PKG_HASH=none. Much easier to use.

/ted

-----Original Message----- 
From: Jo-Philipp Wich 
Sent: Thursday, December 07, 2017 9:56 AM 
To: lede-dev@lists.infradead.org 
Subject: Re: [LEDE-DEV] [PATCH 2/2] download.mk: introduce a new variable SKIPHASH 

Hi Baptiste,

we've been discussing this patch again on IRC today and I came up with
an alternate suggestion that does not require a new variable.

-- 8< --
diff --git a/scripts/download.pl b/scripts/download.pl
index 775408934a..ad9c480c67 100755
--- a/scripts/download.pl
+++ b/scripts/download.pl
@@ -88,7 +88,7 @@ sub download_cmd($) {
}

my $hash_cmd = hash_cmd();
-$hash_cmd or die "Cannot find appropriate hash command, ensure the
provided hash is either a MD5 or SHA256 checksum.\n";
+$hash_cmd or ($file_hash eq "none") or die "Cannot find appropriate
hash command, ensure the provided hash is either a MD5 or SHA256
checksum.\n";

sub download
{
-- >8 --

Using the change above one can issue a

  "make package/mypackage/download PKG_HASH=none"

to download ignoring the Makefile checksum while preserving the error
semantics of unset PKG_HASH cases.

Ideally I'd like to push the "none" variant but wanted to share the idea
on the list first to see what others think about it.

Regards,
Jo

Patch

diff --git a/include/download.mk b/include/download.mk
index 0a25641738..a6821b5304 100644
--- a/include/download.mk
+++ b/include/download.mk
@@ -102,12 +102,18 @@  check_md5 = \
 hash_var = $(if $(filter-out x,$(1)),MD5SUM,HASH)
 endif
 
+ifdef SKIPHASH
+DOWNLOAD_CMD = $(SCRIPT_DIR)/download.pl --skip-hash
+else
+DOWNLOAD_CMD = $(SCRIPT_DIR)/download.pl
+endif
+
 define DownloadMethod/unknown
 	echo "ERROR: No download method available"; false
 endef
 
 define DownloadMethod/default
-	$(SCRIPT_DIR)/download.pl "$(DL_DIR)" "$(FILE)" "$(HASH)" "$(URL_FILE)" $(foreach url,$(URL),"$(url)") \
+	$(DOWNLOAD_CMD) "$(DL_DIR)" "$(FILE)" "$(HASH)" "$(URL_FILE)" $(foreach url,$(URL),"$(url)") \
 	$(if $(filter check,$(1)), \
 		$(call check_hash,$(FILE),$(HASH),$(2)$(call hash_var,$(MD5SUM))) \
 		$(call check_md5,$(MD5SUM),$(2)MD5SUM,$(2)HASH) \