Re: [PATCH v2 1/3] Fix and simplify apt keyring generation

[Andreas Reichel <andreas.reichel.ext@siemens.com>]

On Fri, Mar 01, 2019 at 11:22:24AM +0100, Henning Schild wrote:
> Am Wed, 27 Feb 2019 17:12:42 +0100
> schrieb "[ext] Henning Schild" <henning.schild@siemens.com>:
> 
> > Am Wed, 27 Feb 2019 16:18:54 +0100
> > schrieb "[ext] Andreas J. Reichel" <andreas.reichel.ext@siemens.com>:
> > 
> > > From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > > 
> > > Different fetcher stored keys in different sub dirs, and we can
> > > never be sure about where downloaded files go.
> > > To avoid this without making any assumptions, we ask the fetcher
> > > where the file will be after it is downloaded. This way we also
> > > don't need to parse the URL manually.
> > > 
> > > The code is simplified by removing duplicate code and using apt-key
> > > instead of manually calling gpg.
> > > 
> > > Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > > ---
> > >  meta/classes/isar-bootstrap-helper.bbclass    | 11 ++++++
> > >  .../isar-bootstrap/isar-bootstrap.inc         | 39
> > > +++++++++---------- 2 files changed, 29 insertions(+), 21
> > > deletions(-)
> > > 
> > > diff --git a/meta/classes/isar-bootstrap-helper.bbclass
> > > b/meta/classes/isar-bootstrap-helper.bbclass index d780b85..b8c41f9
> > > 100644 --- a/meta/classes/isar-bootstrap-helper.bbclass
> > > +++ b/meta/classes/isar-bootstrap-helper.bbclass
> > > @@ -119,6 +119,16 @@ setup_root_file_system() {
> > >      export LANG=C
> > >      export LANGUAGE=C
> > >      export LC_ALL=C
> > > +
> > > +    if [ -d ${TMPDIR}/aptkeys ]; then
> > > +        for keyfile in ${TMPDIR}/aptkeys/*
> > > +        do
> > > +            kfn="tmp/$(basename $keyfile)"
> > > +            cp $keyfile "$ROOTFSDIR/$kfn"
> > > +            sudo -E chroot "$ROOTFSDIR" /usr/bin/apt-key add
> > > "/$kfn"  
> > 
> > Would probably be a good idea to work in /tmp/ as well and in a
> > subdir. Try naming your key "root" or "usr" ...
> > 
Already worked in tmp :) But factored it out of $kfn in patch v3.

> > > +            rm "$ROOTFSDIR/$kfn"
> > > +        done
> > > +    fi
> > >      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 +138,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/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > > b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc index
> > > 234d339..2ef3b1e 100644 ---
> > > a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc +++
> > > b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc @@ -23,35
> > > +23,30 @@ 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 = "locales gnupg2
> > > apt-transport-https ca-certificates"  
> > 
> > That looks more like leftovers from an experiment. Are you sure that
> > is needed?
> > 
Fixed in v3
> > 
> > In general i think that patch should be splittet into two or
> > even more patches. Because it solves at least two problems.
> 
> One possible split would be:
> 
>  - gpg -> apt-key
>  - filename.join -> fetcher.localpath
>  - adding BASE_PACKAGES depending on GPG needed
>  - ...
> 
Done in v3.

> Henning
> 
> > Henning
> > 
> > >  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
> > > "" }" inherit base-apt-helper 
> > >  python () {
> > > -    from urllib.parse import urlparse
> > >      distro_apt_keys = d.getVar("DISTRO_APT_KEYS", False)
> > > -    wd = d.getVar("WORKDIR", True)
> > > +    aptkeys = []
> > > +
> > >      if distro_apt_keys:
> > > -        d.setVar("DEBOOTSTRAP_KEYRING", "--keyring ${APTKEYRING}")
> > > -        for key in distro_apt_keys.split():
> > > -            url = urlparse(key)
> > > -            filename = ''.join([wd, url.path])
> > > -            d.appendVar("SRC_URI", " %s" % key)
> > > -            d.appendVar("APTKEYFILES", " %s" % filename)
> > > +        aptkeys += distro_apt_keys.split()
> > > +
> > >      if bb.utils.to_boolean(d.getVar('ISAR_USE_CACHED_BASE_REPO')):
> > >          own_pub_key = d.getVar("BASE_REPO_KEY", False)
> > >          if own_pub_key:
> > > -            d.setVar("DEBOOTSTRAP_KEYRING", "--keyring
> > > ${APTKEYRING}")
> > > -            for key in own_pub_key.split():
> > > -                url = urlparse(key)
> > > -                filename = ''.join([wd, url.path])
> > > -                d.appendVar("SRC_URI", " %s" % key)
> > > -                d.appendVar("APTKEYFILES", " %s" % filename)
> > > +            aptkeys += own_pub_keys.split()
> > > +
> > > +    for key in aptkeys:
> > > +        d.appendVar("SRC_URI", " %s" % key)
> > > +        fetcher = bb.fetch2.Fetch([key], d)
> > > +        filename = fetcher.localpath(key)
> > > +        d.appendVar("APTKEYFILES", " %s" % filename)
> > >  }
> > >  
> > >  def aggregate_files(d, file_list, file_out):
> > > @@ -174,13 +169,17 @@ def get_distro_components_argument(d,
> > > is_host): else:
> > >          return ""
> > >  
> > > +APTKEYTMPDIR := "${TMPDIR}/aptkeys"
> > > +
> > > +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 add "$keyfile"
> > >          done
> > >      fi
> > >  }
> > > @@ -222,7 +221,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)}"
> > > @@ -231,7 +229,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)}"  
> > 
> 

-- 
Andreas Reichel
Dipl.-Phys. (Univ.)
Software Consultant

Andreas.Reichel@tngtech.com, +49-174-3180074
TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterfoehring
Geschaeftsfuehrer: Henrik Klagges, Dr. Robert Dahlke, Gerhard Mueller
Sitz: Unterfoehring * Amtsgericht Muenchen * HRB 135082