Re: [ptxdist] [PATCH v2 4/5] ptxd_lib_code_signing: provide consumer functions with some environment

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Wed, Sep 08, 2021 at 01:30:01PM +0200, Roland Hieber wrote:
> On Fri, Sep 03, 2021 at 02:54:31PM +0200, Michael Olbrich wrote:
> > On Mon, Aug 09, 2021 at 10:06:07AM +0200, Roland Hieber wrote:
> > > The code signing consumer functions should be able to retrieve some
> > > information about the recipe in which they were called in order to make
> > > additional checks if needed. Refactor the (shell cs_get_*, …) calls into
> > > macro calls of the form $(call ptx/cs-get-*, <PKG>, …). Let these
> > > macros look up the package name (for now) from PTX_MAP_TO_package_<PKG>
> > > before passing it to the shell functions. Using $(call world/env) here
> > > would be practical, but would also cause make to complain about
> > > recursive variable dependencies. Therefore variables must be added
> > > to ptx/cs-consumer-env manually, but additional information can be added
> > > later if needed.
> > > 
> > > Refactor the existing consumers in the code base too, and add an error
> > > message in case anyone else that still uses the old API.
> > > 
> > > Signed-off-by: Roland Hieber <rhi@pengutronix.de>
> > > ---
> > > PATCH v2:
> > >  - define multiline macros using "define"
> > > 
> > > PATCH v1: https://lore.ptxdist.org/ptxdist/20210804142330.32739-4-rhi@pengutronix.de
> > > ---
> > >  doc/dev_code_signing.rst                      |  2 +-
> > >  doc/ref_code_signing_helpers.rst              | 25 ++++++-----
> > >  rules/barebox.make                            |  2 +-
> > >  rules/image-rauc.make                         |  6 +--
> > >  rules/kernel.make                             |  6 +--
> > >  rules/pre/030-code-signing-consumers.make     | 44 +++++++++++++++++++
> > >  rules/rauc.make                               |  2 +-
> > >  .../templates/template-barebox-imx-habv4-make |  6 +--
> > >  scripts/lib/ptxd_lib_code_signing.sh          | 13 ++++++
> > >  9 files changed, 83 insertions(+), 23 deletions(-)
> > >  create mode 100644 rules/pre/030-code-signing-consumers.make
> > > 
> > > diff --git a/doc/dev_code_signing.rst b/doc/dev_code_signing.rst
> > > index b9a7c42f2a55..413f694980eb 100644
> > > --- a/doc/dev_code_signing.rst
> > > +++ b/doc/dev_code_signing.rst
> > > @@ -164,7 +164,7 @@ also via an environment variable.
> > >  .. code-block:: none
> > >  
> > >      $(call install_copy, rauc, 0, 0, 0644, \
> > > -      $(shell cs_get_ca update), \
> > > +      $(call ptx/cs-get-ca, RAUC, update), \
> > >        /etc/rauc/ca.cert.pem)
> > >  
> > >  .. note:: When code signing helper functions are used in make variables (e.g.
> > > diff --git a/doc/ref_code_signing_helpers.rst b/doc/ref_code_signing_helpers.rst
> > > index fd16ca763557..d3429778d94d 100644
> > > --- a/doc/ref_code_signing_helpers.rst
> > > +++ b/doc/ref_code_signing_helpers.rst
> > > @@ -297,19 +297,21 @@ In the example given in :ref:`cs_group_add_roles` above, this would print::
> > >  Consumer Functions
> > >  ~~~~~~~~~~~~~~~~~~
> > >  
> > > +The consumer functions are implemented as make macros.
> > >  Packages that want to sign something or need access to keys/CAs can retrieve
> > >  PKCS#11 URIs and CA keyrings with these helpers.
> > >  
> > > +.. _ptx/cs-get-uri:
> > >  .. _cs_get_uri:
> > >  
> > > -cs_get_uri
> > > -^^^^^^^^^^
> > > +ptx/cs-get-uri
> > > +^^^^^^^^^^^^^^
> > >  
> > >  Usage:
> > >  
> > > -.. code-block:: bash
> > > +.. code-block:: make
> > >  
> > > -    cs_get_uri <role>
> > > +    $(call ptx/cs-get-uri, <PKG>, <role>)
> > >  
> > >  Get PKCS#11 URI for role.
> > >  
> > > @@ -317,16 +319,17 @@ Preconditions:
> > >  
> > >  - the URI must have been set (see :ref:`cs_set_uri`)
> > >  
> > > +.. _ptx/cs-get-ca:
> > >  .. _cs_get_ca:
> > >  
> > > -cs_get_ca
> > > -^^^^^^^^^
> > > +ptx/cs-get-ca
> > > +^^^^^^^^^^^^^
> > >  
> > >  Usage:
> > >  
> > > -.. code-block:: bash
> > > +.. code-block:: make
> > >  
> > > -    cs_get_ca <role>
> > > +    $(call ptx/cs-get-ca, <PKG>, <role>)
> > >  
> > >  Get path to the CA keyring in PEM format for role.
> > >  
> > > @@ -347,7 +350,7 @@ Example:
> > >  
> > >     # set up kernel module signing, and add a trusted CA if the provider set one
> > >     KERNEL_SIGN_OPT =
> > > -   	CONFIG_MODULE_SIG_KEY='"$(shell cs_get_uri kernel-modules)"' \
> > > +   	CONFIG_MODULE_SIG_KEY='"$(call ptx/cs-get-uri, KERNEL, kernel-modules)"' \
> > >     	CONFIG_MODULE_SIG_ALL=y \
> > > -   	$(if $(shell cs_get_ca kernel-trusted), \
> > > -   		CONFIG_SYSTEM_TRUSTED_KEYS=$(shell cs_get_ca kernel-trusted))
> > > +   	$(if $(call ptx/cs-get-ca, KERNEL, kernel-trusted), \
> > > +   		CONFIG_SYSTEM_TRUSTED_KEYS=$(call ptx/cs-get-ca, KERNEL, kernel-trusted))
> > > diff --git a/rules/barebox.make b/rules/barebox.make
> > > index bea9f3adcbf8..983d34032e0d 100644
> > > --- a/rules/barebox.make
> > > +++ b/rules/barebox.make
> > > @@ -103,7 +103,7 @@ endif
> > >  ifdef PTXCONF_CODE_SIGNING
> > >  BAREBOX_MAKE_ENV = \
> > >  	$(CODE_SIGNING_ENV) \
> > > -	IMAGE_KERNEL_FIT_KEY="$(shell cs_get_uri image-kernel-fit)"
> > > +	IMAGE_KERNEL_FIT_KEY="$(call ptx/cs-get-uri, BAREBOX, image-kernel-fit)"
> > >  endif
> > >  
> > >  $(STATEDIR)/barebox.compile:
> > > diff --git a/rules/image-rauc.make b/rules/image-rauc.make
> > > index fe1b0e89be7c..c8747231f8f1 100644
> > > --- a/rules/image-rauc.make
> > > +++ b/rules/image-rauc.make
> > > @@ -32,9 +32,9 @@ IMAGE_RAUC_ENV	= \
> > >  	RAUC_BUNDLE_VERSION="$(call remove_quotes, $(PTXCONF_RAUC_BUNDLE_VERSION))" \
> > >  	RAUC_BUNDLE_BUILD=$(call ptx/sh, date +%FT%T%z) \
> > >  	RAUC_BUNDLE_DESCRIPTION=$(PTXCONF_IMAGE_RAUC_DESCRIPTION) \
> > > -	RAUC_KEY="$(shell cs_get_uri update)" \
> > > -	RAUC_CERT="$(shell cs_get_uri update)" \
> > > -	RAUC_KEYRING="$(shell cs_get_ca update)"
> > > +	RAUC_KEY="$(call ptx/cs-get-uri, IMAGE_RAUC, update)" \
> > > +	RAUC_CERT="$(call ptx/cs-get-uri, IMAGE_RAUC, update)" \
> > > +	RAUC_KEYRING="$(call ptx/cs-get-ca, IMAGE_RAUC, update)"
> > >  
> > >  $(IMAGE_RAUC_IMAGE):
> > >  	@$(call targetinfo)
> > > diff --git a/rules/kernel.make b/rules/kernel.make
> > > index 9caff677918e..e6faba82df38 100644
> > > --- a/rules/kernel.make
> > > +++ b/rules/kernel.make
> > > @@ -73,12 +73,12 @@ KERNEL_BASE_OPT		= \
> > >  
> > >  ifdef PTXCONF_KERNEL_CODE_SIGNING
> > >  KERNEL_BASE_OPT		+= \
> > > -	$(if $(shell cs_get_ca kernel-trusted), \
> > > -		CONFIG_SYSTEM_TRUSTED_KEYS=$(shell cs_get_ca kernel-trusted))
> > > +	$(if $(call ptx/cs-get-ca, KERNEL, kernel-trusted), \
> > > +		CONFIG_SYSTEM_TRUSTED_KEYS=$(call ptx/cs-get-ca, KERNEL, kernel-trusted))
> > >  endif
> > >  ifdef PTXCONF_KERNEL_MODULES_SIGN
> > >  KERNEL_BASE_OPT		+= \
> > > -	CONFIG_MODULE_SIG_KEY='"$(shell cs_get_uri kernel-modules)"'
> > > +	CONFIG_MODULE_SIG_KEY='"$(call ptx/cs-get-uri, KERNEL, kernel-modules)"'
> > >  endif
> > >  
> > >  # Intermediate option. This will be used by kernel module packages.
> > > diff --git a/rules/pre/030-code-signing-consumers.make b/rules/pre/030-code-signing-consumers.make
> > > new file mode 100644
> > > index 000000000000..909e8ebd6936
> > > --- /dev/null
> > > +++ b/rules/pre/030-code-signing-consumers.make
> > > @@ -0,0 +1,44 @@
> > > +# -*-makefile-*-
> > > +#
> > > +# Copyright (C) 2021 Roland Hieber, Pengutronix <rhi@pengutronix.de>
> > > +#
> > > +# For further information about the PTXdist project and license conditions
> > > +# see the README file.
> > > +#
> > > +#
> > > +
> > > +#
> > > +# Usage: $(call ptx/cs-consumer-env, <PKG>)
> > > +#
> > > +# We usually want to use cs-get-* macros inside a <PKG>_MAKE_OPT etc., which is
> > > +# referenced in world/env, so we cannot use world/env to set pkg_name without
> > > +# running into circular variable dependencies.
> > > +#
> > > +define ptx/cs-consumer-env
> > 
> > Use the regular 'ptx/cs-consumer-env =' for this to keep it consistent with
> > other env lists.
> 
> I don't understand, in v1 [1] you said I should use 'define' instead of
> multi-line '=' macros…?

I meant for ptx/cs-get-uri and ptx/cs-get-ca but not for
ptx/cs-consumer-env which is just a variable list. It's just for
consistency. We've had the variable list like that since the beginning.
And I prefer 'define' for other complex macros because the nested brackets
are complex enough without adding the '\' in each line.

Michael

> [1]: https://lore.ptxdist.org/ptxdist/20210804152307.GS13108@pengutronix.de/
> 
>  - Roland
> 
> > > +	pkg_name='$(PTX_MAP_TO_package_$(strip $(1)))' \
> > > +	$(CODE_SIGNING_ENV)
> > > +endef
> > > +
> > > +#
> > > +# Usage: $(call ptx/cs-get-uri, <PKG>, <role>)
> > > +#
> > > +define ptx/cs-get-uri
> > > +$(strip \
> > > +	$(shell \
> > > +		$(call ptx/cs-consumer-env, $(1))\
> > > +			cs_get_uri '$(strip $(2))'\
> > > +	)\
> > 
> > No \ needed with 'define'.
> > 
> > > +)
> > > +endef
> > > +
> > > +#
> > > +# Usage: $(call ptx/cs-get-ca, <PKG>, <role>)
> > > +#
> > > +define ptx/cs-get-ca
> > > +$(strip \
> > > +	$(shell \
> > > +		$(call ptx/cs-consumer-env, $(1))\
> > > +			cs_get_ca '$(strip $(2))'\
> > > +	)\
> > 
> > Same here.
> > 
> > > +)
> > > +endef
> > > diff --git a/rules/rauc.make b/rules/rauc.make
> > > index 08df6336a7cd..3c28befcd3ff 100644
> > > --- a/rules/rauc.make
> > > +++ b/rules/rauc.make
> > > @@ -78,7 +78,7 @@ ifdef PTXCONF_RAUC_CONFIGURATION
> > >  	@$(call install_replace, rauc, /etc/rauc/system.conf, \
> > >  		@RAUC_BUNDLE_COMPATIBLE@, \
> > >  		"$(call remove_quotes,$(PTXCONF_RAUC_COMPATIBLE))")
> > > -	@$(call install_copy, rauc, 0, 0, 0644, $(shell cs_get_ca update), \
> > > +	@$(call install_copy, rauc, 0, 0, 0644, $(call ptx/cs-get-ca, RAUC, update), \
> > >  		/etc/rauc/ca.cert.pem)
> > >  endif
> > >  
> > > diff --git a/rules/templates/template-barebox-imx-habv4-make b/rules/templates/template-barebox-imx-habv4-make
> > > index cc825dc90292..b2d5d7100fc9 100644
> > > --- a/rules/templates/template-barebox-imx-habv4-make
> > > +++ b/rules/templates/template-barebox-imx-habv4-make
> > > @@ -64,9 +64,9 @@ endif
> > >  
> > >  BAREBOX_@PACKAGE@_MAKE_ENV	= \
> > >  	$(CODE_SIGNING_ENV) \
> > > -	CSF="$(shell cs_get_uri imx-habv4-csf1)" \
> > > -	IMG="$(shell cs_get_uri imx-habv4-img1)" \
> > > -	FIT_KEY="$(shell cs_get_uri image-kernel-fit)"
> > > +	CSF="$(call ptx/cs-get-uri, BAREBOX_@PACKAGE@, imx-habv4-csf1)" \
> > > +	IMG="$(call ptx/cs-get-uri, BAREBOX_@PACKAGE@, imx-habv4-img1)" \
> > > +	FIT_KEY="$(call ptx/cs-get-uri, BAREBOX_@PACKAGE@, image-kernel-fit)"
> > >  
> > >  BAREBOX_@PACKAGE@_MAKE_OPT	:= $(BAREBOX_@PACKAGE@_CONF_OPT)
> > >  
> > > diff --git a/scripts/lib/ptxd_lib_code_signing.sh b/scripts/lib/ptxd_lib_code_signing.sh
> > > index 66a2cab81395..24730d3cf742 100644
> > > --- a/scripts/lib/ptxd_lib_code_signing.sh
> > > +++ b/scripts/lib/ptxd_lib_code_signing.sh
> > > @@ -1,6 +1,7 @@
> > >  #!/bin/bash
> > >  #
> > >  # Copyright (C) 2019 Sascha Hauer <s.hauer@pengutronix.de>
> > > +# Copyright (C) 2021 Roland Hieber, Pengutronix <rhi@pengutronix.de>
> > >  #
> > >  # For further information about the PTXdist project and license conditions
> > >  # see the README file.
> > > @@ -176,6 +177,12 @@ export -f cs_set_uri
> > >  # Get the uri from a role
> > >  #
> > >  cs_get_uri() {
> > > +    if [ -z "${pkg_name}" ]; then
> > > +	    echo ERROR_UNSUPPORTED_CS_API_CALL
> > > +	    ptxd_bailout '$(shell cs_get_uri, <role>) is no longer supported in make files.' \
> > > +		'Use $(call ptx/cs-get-uri, <PKG>, <role>) instead.'
> > > +    fi
> > > +
> > >      local role="${1}"
> > >      cs_init_variables
> > >  
> > > @@ -297,6 +304,12 @@ export -f cs_import_key_from_pem
> > >  # Get the path to the CA in pem format from a role
> > >  #
> > >  cs_get_ca() {
> > > +    if [ -z "${pkg_name}" ]; then
> > > +	    echo ERROR_UNSUPPORTED_CS_API_CALL
> > > +	    ptxd_bailout '$(shell cs_get_ca, …) is no longer supported in make files.' \
> > > +		'Use $(call ptx/cs-get-ca, <PKG>, …) instead.'
> > > +    fi
> > > +
> > >      local role="${1}"
> > >      cs_init_variables
> > >  
> > > -- 
> > > 2.30.2
> > > 
> > > 
> > > _______________________________________________
> > > ptxdist mailing list
> > > ptxdist@pengutronix.de
> > > To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de
> > 
> > -- 
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> > 
> > _______________________________________________
> > ptxdist mailing list
> > ptxdist@pengutronix.de
> > To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de
> 
> -- 
> Roland Hieber, Pengutronix e.K.          | r.hieber@pengutronix.de     |
> Steuerwalder Str. 21                     | https://www.pengutronix.de/ |
> 31137 Hildesheim, Germany                | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686         | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> ptxdist mailing list
> ptxdist@pengutronix.de
> To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de
To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de