[1/2] Makefile: add tainting support

Message ID 1520326085-18337-2-git-send-email-angelo@amarulasolutions.com
State Superseded
Headers show
Series
  • [1/2] Makefile: add tainting support
Related show

Commit Message

Angelo Compagnucci March 6, 2018, 8:48 a.m.
From: Angelo Compagnucci <angelo.compagnucci@gmail.com>

Packages who harms the build reproducibility can declare
FOO_TAINTS variable.
If a package taints the build it will be added to a list
of tainting packages.
The build ends with an error when the BR2_REPRODUCIBLE
is enabled and the tainting packages list is not empty.
Moreover, legal info will show a warning in presence
of a tainting package.

Signed-off-by: Angelo Compagnucci <angelo@amarulasolutions.com>
---
 Makefile               | 14 +++++++++++++-
 package/pkg-generic.mk |  9 +++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

Thomas Petazzoni April 28, 2018, 4:53 p.m. | #1
Hello,

On Tue,  6 Mar 2018 09:48:04 +0100, Angelo Compagnucci wrote:

> +# Check here if there are packages declaring they harm
> +# the reproducibility of the build
> +.PHONY: check-tainted
> +check-tainted:
> +ifneq ($(BR2_REPRODUCIBLE),)
> +ifneq ($(BR2_TAINTED_BY),)

I don't think BR2_REPRODUCIBLE has anything to do with this 'tainting'
thing.

BR2_REPRODUCIBLE is about getting binary identical results given a
Buildroot configuration. But even with BR2_REPRODUCIBLE disabled,
Buildroot aims at building the same source code, which the nodejs npm
stuff does not allow to do. So the 'tainting' happens regardless of
whether BR2_REPRODUCIBLE=y or not.

If you remove the BR2_REPRODUCIBLE condition, then the Buildroot build
would always fail as soon as one package taints the build, so I'm not
sure a hard failure is a good approach here. Perhaps just a warning at
the end of the build ?

>  .PHONY: target-post-image
> -target-post-image: $(TARGETS_ROOTFS) target-finalize
> +target-post-image: check-tainted $(TARGETS_ROOTFS) target-finalize
>  	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \
>  		$(call MESSAGE,"Executing post-image script $(s)"); \
>  		$(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep))
> @@ -1063,6 +1074,7 @@ help:
>  	@echo '  source                 - download all sources needed for offline-build'
>  	@echo '  external-deps          - list external packages used'
>  	@echo '  legal-info             - generate info about license compliance'
> +	@echo '  check-tainted          - check if any selected package harms build reproducibility'

Not sure it is really useful to make this a first class target,
documented in the help text. A warning at the end of the build is
probably enough.

Or maybe simply mention it as a Config.in comment ?

comment "WARNING: nodejs modules will make your build non reproducible!"
	depends on BR2_PACKAGE_NODEJS_MODULES_ADDITIONAL != ""

>  	@echo '  printvars              - dump all the internal variables'
>  	@echo
>  	@echo '  make V=0|1             - 0 => quiet build (default), 1 => verbose build'
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index a2a12e7..e643ecf 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -556,6 +556,10 @@ ifndef $(2)_REDISTRIBUTE
>   endif
>  endif
>  
> +ifdef $(2)_TAINTS
> + BR2_TAINTED_BY+=$$($(2)_RAWNAME)
> +endif

I think "TAINTS" is very vague. Unfortunately, I don't have a good
proposal. In addition, it's not only about tainting the build in terms
of reproducibility, but also about the lack of integration with the
download and legal infrastructures.

I'm not sure what a good name for this would be. I've added Arnout in
Cc, he often has interesting (and creative) ideas on such topics.

> +# Save a legal warning if tainted
> +ifneq ($$(call qstrip,$$($(2)_TAINTS)),)

Implementation detail: you always use the construct:

ifneq ($(var),)

which is negative logic. Please use:

ifeq ($(var),YES)

instead. Of course, unless the variable really is empty vs. non-empty,
but that isn't the case here.

Also, your patch series was sent at a time where patchwork was missing
some patches, so your PATCH 2/2 was not recorded. It would be useful if
you could resend this patch series.

Best regards,

Thomas
Angelo Compagnucci May 3, 2018, 9:34 p.m. | #2
Dear Thomas,

2018-04-28 18:53 GMT+02:00 Thomas Petazzoni <thomas.petazzoni@bootlin.com>:
> Hello,
>
> On Tue,  6 Mar 2018 09:48:04 +0100, Angelo Compagnucci wrote:
>
>> +# Check here if there are packages declaring they harm
>> +# the reproducibility of the build
>> +.PHONY: check-tainted
>> +check-tainted:
>> +ifneq ($(BR2_REPRODUCIBLE),)
>> +ifneq ($(BR2_TAINTED_BY),)
>
> I don't think BR2_REPRODUCIBLE has anything to do with this 'tainting'
> thing.
>
> BR2_REPRODUCIBLE is about getting binary identical results given a
> Buildroot configuration. But even with BR2_REPRODUCIBLE disabled,
> Buildroot aims at building the same source code, which the nodejs npm
> stuff does not allow to do. So the 'tainting' happens regardless of
> whether BR2_REPRODUCIBLE=y or not.
>
> If you remove the BR2_REPRODUCIBLE condition, then the Buildroot build
> would always fail as soon as one package taints the build, so I'm not
> sure a hard failure is a good approach here. Perhaps just a warning at
> the end of the build ?

Fair enough, will do.

>
>>  .PHONY: target-post-image
>> -target-post-image: $(TARGETS_ROOTFS) target-finalize
>> +target-post-image: check-tainted $(TARGETS_ROOTFS) target-finalize
>>       @$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \
>>               $(call MESSAGE,"Executing post-image script $(s)"); \
>>               $(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep))
>> @@ -1063,6 +1074,7 @@ help:
>>       @echo '  source                 - download all sources needed for offline-build'
>>       @echo '  external-deps          - list external packages used'
>>       @echo '  legal-info             - generate info about license compliance'
>> +     @echo '  check-tainted          - check if any selected package harms build reproducibility'
>
> Not sure it is really useful to make this a first class target,
> documented in the help text. A warning at the end of the build is
> probably enough.

I think this should be kept especially for CI systems in which it's
useful to know if a user introduced something that harms the build.

> Or maybe simply mention it as a Config.in comment ?
>
> comment "WARNING: nodejs modules will make your build non reproducible!"
>         depends on BR2_PACKAGE_NODEJS_MODULES_ADDITIONAL != ""
>
>>       @echo '  printvars              - dump all the internal variables'
>>       @echo
>>       @echo '  make V=0|1             - 0 => quiet build (default), 1 => verbose build'
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> index a2a12e7..e643ecf 100644
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -556,6 +556,10 @@ ifndef $(2)_REDISTRIBUTE
>>   endif
>>  endif
>>
>> +ifdef $(2)_TAINTS
>> + BR2_TAINTED_BY+=$$($(2)_RAWNAME)
>> +endif
>
> I think "TAINTS" is very vague. Unfortunately, I don't have a good
> proposal. In addition, it's not only about tainting the build in terms
> of reproducibility, but also about the lack of integration with the
> download and legal infrastructures.

Yes, it's not the best name probably, but the concept is here.

>
> I'm not sure what a good name for this would be. I've added Arnout in
> Cc, he often has interesting (and creative) ideas on such topics.
>
>> +# Save a legal warning if tainted
>> +ifneq ($$(call qstrip,$$($(2)_TAINTS)),)
>
> Implementation detail: you always use the construct:
>
> ifneq ($(var),)
>
> which is negative logic. Please use:
>
> ifeq ($(var),YES)
>
> instead. Of course, unless the variable really is empty vs. non-empty,
> but that isn't the case here.
>
> Also, your patch series was sent at a time where patchwork was missing
> some patches, so your PATCH 2/2 was not recorded. It would be useful if
> you could resend this patch series.
>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Arnout Vandecappelle May 4, 2018, 6:59 p.m. | #3
On 03-05-18 23:34, Angelo Compagnucci wrote:
> Dear Thomas,
> 
> 2018-04-28 18:53 GMT+02:00 Thomas Petazzoni <thomas.petazzoni@bootlin.com>:
>> Hello,
>>
>> On Tue,  6 Mar 2018 09:48:04 +0100, Angelo Compagnucci wrote:
[snip]
>>> @@ -1063,6 +1074,7 @@ help:
>>>       @echo '  source                 - download all sources needed for offline-build'
>>>       @echo '  external-deps          - list external packages used'
>>>       @echo '  legal-info             - generate info about license compliance'
>>> +     @echo '  check-tainted          - check if any selected package harms build reproducibility'
>>
>> Not sure it is really useful to make this a first class target,
>> documented in the help text. A warning at the end of the build is
>> probably enough.
> 
> I think this should be kept especially for CI systems in which it's
> useful to know if a user introduced something that harms the build.

 Yes, makes sense. Especially the way you did it in v2: non-fatal in the
target-finalize step, but fatal if check-tainted is called explicitly.

>> Or maybe simply mention it as a Config.in comment ?
>>
>> comment "WARNING: nodejs modules will make your build non reproducible!"
>>         depends on BR2_PACKAGE_NODEJS_MODULES_ADDITIONAL != ""
>>
>>>       @echo '  printvars              - dump all the internal variables'
>>>       @echo
>>>       @echo '  make V=0|1             - 0 => quiet build (default), 1 => verbose build'
>>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>>> index a2a12e7..e643ecf 100644
>>> --- a/package/pkg-generic.mk
>>> +++ b/package/pkg-generic.mk
>>> @@ -556,6 +556,10 @@ ifndef $(2)_REDISTRIBUTE
>>>   endif
>>>  endif
>>>
>>> +ifdef $(2)_TAINTS
>>> + BR2_TAINTED_BY+=$$($(2)_RAWNAME)
>>> +endif
>>
>> I think "TAINTS" is very vague. Unfortunately, I don't have a good
>> proposal. In addition, it's not only about tainting the build in terms
>> of reproducibility, but also about the lack of integration with the
>> download and legal infrastructures.
> 
> Yes, it's not the best name probably, but the concept is here.

 In the kernel the 'taint' name is also used for things that are not really
literally tainting, so why not.

 Regards,
 Arnout


>> I'm not sure what a good name for this would be. I've added Arnout in
>> Cc, he often has interesting (and creative) ideas on such topics.
[snip]

Patch

diff --git a/Makefile b/Makefile
index 1b34818..afa5f29 100644
--- a/Makefile
+++ b/Makefile
@@ -761,8 +761,19 @@  endif
 		$(call MESSAGE,"Executing post-build script $(s)"); \
 		$(EXTRA_ENV) $(s) $(TARGET_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep))
 
+# Check here if there are packages declaring they harm
+# the reproducibility of the build
+.PHONY: check-tainted
+check-tainted:
+ifneq ($(BR2_REPRODUCIBLE),)
+ifneq ($(BR2_TAINTED_BY),)
+	$(error Buildroot is tainted (by: $(BR2_TAINTED_BY)). Cannot do a reproducible build.)
+	@exit 1
+endif
+endif
+
 .PHONY: target-post-image
-target-post-image: $(TARGETS_ROOTFS) target-finalize
+target-post-image: check-tainted $(TARGETS_ROOTFS) target-finalize
 	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \
 		$(call MESSAGE,"Executing post-image script $(s)"); \
 		$(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep))
@@ -1063,6 +1074,7 @@  help:
 	@echo '  source                 - download all sources needed for offline-build'
 	@echo '  external-deps          - list external packages used'
 	@echo '  legal-info             - generate info about license compliance'
+	@echo '  check-tainted          - check if any selected package harms build reproducibility'
 	@echo '  printvars              - dump all the internal variables'
 	@echo
 	@echo '  make V=0|1             - 0 => quiet build (default), 1 => verbose build'
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index a2a12e7..e643ecf 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -556,6 +556,10 @@  ifndef $(2)_REDISTRIBUTE
  endif
 endif
 
+ifdef $(2)_TAINTS
+ BR2_TAINTED_BY+=$$($(2)_RAWNAME)
+endif
+
 $(2)_REDISTRIBUTE		?= YES
 
 $(2)_REDIST_SOURCES_DIR = $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))/$$($(2)_RAW_BASE_NAME)
@@ -869,6 +873,11 @@  else
 	$(Q)$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$($(2)_RAW_BASE_NAME),$$($(2)_PKGDIR),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
 endif # license files
 
+# Save a legal warning if tainted
+ifneq ($$(call qstrip,$$($(2)_TAINTS)),)
+	$(Q)$$(call legal-warning-pkg,$$($(2)_RAWNAME),unknown license for additional modules or dependencies)
+endif
+
 ifeq ($$($(2)_SITE_METHOD),local)
 # Packages without a tarball: don't save and warn
 	@$$(call legal-warning-nosource,$$($(2)_RAWNAME),local)