Re: [PATCH v4 5/6] Use apt-key to generate apt-keyring

[Claudius Heine <claudius.heine.ext@siemens.com>]

Hi Andreas,

On 07/03/2019 15.23, [ext] Andreas J. Reichel wrote:
> From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> 
> Use apt-key instead of manually calling gpg.
> 
> Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> ---
>   meta/classes/isar-bootstrap-helper.bbclass    |  2 ++
>   meta/classes/isar-image.bbclass               |  1 +
>   meta/conf/bitbake.conf                        |  1 +
>   .../isar-bootstrap/isar-bootstrap.inc         | 32 +++++++++++++++----
>   4 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/meta/classes/isar-bootstrap-helper.bbclass b/meta/classes/isar-bootstrap-helper.bbclass
> index d780b85..c5e39e9 100644
> --- a/meta/classes/isar-bootstrap-helper.bbclass
> +++ b/meta/classes/isar-bootstrap-helper.bbclass
> @@ -119,6 +119,7 @@ setup_root_file_system() {
>       export LANG=C
>       export LANGUAGE=C
>       export LC_ALL=C
> +
>       sudo -E chroot "$ROOTFSDIR" /usr/bin/apt-get update \
>           -o Dir::Etc::sourcelist="sources.list.d/isar-apt.list" \
>           -o Dir::Etc::sourceparts="-" \
> @@ -128,6 +129,7 @@ setup_root_file_system() {
>           sudo -E chroot "$ROOTFSDIR" /usr/bin/dpkg --add-architecture ${DISTRO_ARCH}
>           sudo -E chroot "$ROOTFSDIR" /usr/bin/apt-get update
>       fi
> +    sudo -E chroot "$ROOTFSDIR" /usr/bin/apt-key update
>       sudo -E chroot "$ROOTFSDIR" \
>           /usr/bin/apt-get ${APT_ARGS} --download-only $PACKAGES \
>               ${IMAGE_TRANSIENT_PACKAGES}
> diff --git a/meta/classes/isar-image.bbclass b/meta/classes/isar-image.bbclass
> index cdd1651..4a89bd7 100644
> --- a/meta/classes/isar-image.bbclass
> +++ b/meta/classes/isar-image.bbclass
> @@ -82,6 +82,7 @@ isar_image_cleanup() {
>           fi
>           rm -f "${IMAGE_ROOTFS}/etc/apt/sources-list"
>       '
> +    sudo rm -f "${ISARKEYRING}"

If I understand this correctly, you are removing the keys of third-party 
repositories here. Why? Aren't they needed if someone wants to update 
the image later manually via apt on a running system?

IMO that only makes sense if this file only contains keys for 
repositories like isar-apt or the cache repo.

>   }
>   
>   do_rootfs() {
> diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
> index 0e521bb..769ec9a 100644
> --- a/meta/conf/bitbake.conf
> +++ b/meta/conf/bitbake.conf
> @@ -62,6 +62,7 @@ DEBDISTRONAME = "isar"
>   # Isar apt repository paths
>   REPO_ISAR_DIR = "${DEPLOY_DIR}/isar-apt/apt"
>   REPO_ISAR_DB_DIR = "${DEPLOY_DIR}/isar-apt/db"
> +ISARKEYRING = "/etc/apt/trusted.gpg.d/isar.gpg"

I would separate third-party and isar created repo keys here. Since 
third-party keyrings should not be removed and isar created ones might 
be removed if those repos are not shared with the device while it is 
deployed.

>   
>   # Base apt repository paths
>   REPO_BASE_DIR = "${DL_DIR}/base-apt/apt"
> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> index dbc3938..2fb5c5b 100644
> --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> @@ -23,10 +23,9 @@ APTSRCS = "${WORKDIR}/apt-sources"
>   APTSRCS_INIT = "${WORKDIR}/apt-sources-init"
>   BASEAPTSRCS = "${WORKDIR}/base-apt-sources"
>   APTKEYFILES = ""
> -APTKEYRING = "${WORKDIR}/apt-keyring.gpg"
> -DEBOOTSTRAP_KEYRING = ""
>   DEPLOY_ISAR_BOOTSTRAP ?= ""
>   DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales"
> +DISTRO_BOOTSTRAP_BASE_PACKAGES_append_gnupg = ",gnupg2"
>   
>   DISTRO_APT_PREMIRRORS ?= "${@ "http://ftp\.(\S+\.)?debian.org  file:///${REPO_BASE_DIR} \n" if bb.utils.to_boolean(d.getVar('ISAR_USE_CACHED_BASE_REPO')) else "" }"
>   
> @@ -43,7 +42,6 @@ python () {
>           if own_pub_key:
>               aptkeys += own_pub_key.split()
>   
> -    d.setVar("DEBOOTSTRAP_KEYRING", "--keyring ${APTKEYRING}")
>       for key in aptkeys:
>           d.appendVar("SRC_URI", " %s" % key)
>           fetcher = bb.fetch2.Fetch([key], d)
> @@ -158,6 +156,14 @@ def get_distro_needs_https_support(d, is_host=False):
>       else:
>           return ""
>   
> +def get_distro_needs_gpg_support(d):
> +    apt_keys = d.getVar("HAVE_CUSTOM_APT_KEYS", False)
> +    if apt_keys:
> +        return "gnupg"
> +    return ""
> +
> +OVERRIDES_append = ":${@get_distro_needs_gpg_support(d)}"
> +
>   def get_distro_source(d, is_host):
>       return get_distro_primary_source_entry(d, is_host)[0]
>   
> @@ -171,13 +177,17 @@ def get_distro_components_argument(d, is_host):
>       else:
>           return ""
>    > +APTKEYTMPDIR := "${TMPDIR}/aptkeys"

Is there a reason why this is in TMPDIR and not in WORKDIR?

> +
> +do_generate_keyring[cleandirs] = "${APTKEYTMPDIR}"
>   do_generate_keyring[dirs] = "${DL_DIR}"
>   do_generate_keyring[vardeps] += "DISTRO_APT_KEYS"
>   do_generate_keyring() {
>       if [ -n "${@d.getVar("APTKEYFILES", True) or ""}" ]; then
> +        chmod 777 "${APTKEYTMPDIR}"
>           for keyfile in ${@d.getVar("APTKEYFILES", True)}; do
> -           gpg --no-default-keyring --keyring "${APTKEYRING}" \
> -               --no-tty --homedir "${DL_DIR}"  --import "$keyfile"
> +           cp "$keyfile" "${APTKEYTMPDIR}"/"$(basename "$keyfile")"
> +           sudo apt-key --keyring "${ISARKEYRING}" add "$keyfile"
>           done
>       fi
>   }
> @@ -221,7 +231,6 @@ isar_bootstrap() {
>               if [ ${IS_HOST} ]; then
>                   ${DEBOOTSTRAP} $debootstrap_args \
>                                  ${@get_distro_components_argument(d, True)} \
> -                               ${DEBOOTSTRAP_KEYRING} \
>                                  "${@get_distro_suite(d, True)}" \
>                                  "${ROOTFSDIR}" \
>                                  "${@get_distro_source(d, True)}"
> @@ -230,7 +239,6 @@ isar_bootstrap() {
>                    "${DEBOOTSTRAP}" $debootstrap_args \
>                                     --arch="${DISTRO_ARCH}" \
>                                     ${@get_distro_components_argument(d, False)} \
> -                                  ${DEBOOTSTRAP_KEYRING} \
>                                     "${@get_distro_suite(d, False)}" \
>                                     "${ROOTFSDIR}" \
>                                     "${@get_distro_source(d, False)}"
> @@ -259,6 +267,16 @@ isar_bootstrap() {
>               mkdir -p "${ROOTFSDIR}/etc/apt/apt.conf.d"
>               install -v -m644 "${WORKDIR}/isar-apt.conf" \
>                                "${ROOTFSDIR}/etc/apt/apt.conf.d/50isar.conf"
> +            if [ -d ${TMPDIR}/aptkeys ]; then
> +                for keyfile in ${TMPDIR}/aptkeys/*

Maybe use APTKEYTMPDIR here, deduplication then it is easier to find 
usage of this directory.

If the aptkeys directory is used somewhere outside of isar-bootstrap, 
then the placeing it in TMPDIR directly makes sence, but if only 
isar-bootstrap uses this directory WORKDIR would be better.

Claudius

> +                do
> +                    kfn="$(basename $keyfile)"
> +                    cp $keyfile "${ROOTFSDIR}/tmp/$kfn"
> +                    sudo -E chroot "${ROOTFSDIR}" /usr/bin/apt-key \
> +                       --keyring ${ISARKEYRING} add "/tmp/$kfn"
> +                    rm "${ROOTFSDIR}/tmp/$kfn"
> +                done
> +            fi
>   
>               if [ "${@get_distro_suite(d, True)}" = "stretch" ] && [ "${@get_host_release().split('.')[0]}" -lt "4" ]; then
>                   install -v -m644 "${WORKDIR}/isar-apt-fallback.conf" \
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de