| 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 | expand |
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.
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)") \
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
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
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
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) \
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>
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
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>
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
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) \
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(-)