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

Message ID 20171026095059.6592-2-git@bitsofnetworks.org
State New
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) \

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) \