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

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

On Mon, Mar 18, 2019 at 12:48:03PM +0100, Claudius Heine wrote:
> Hi Andreas,
> 
> On 18/03/2019 11.21, Andreas Reichel wrote:
> > On Thu, Mar 07, 2019 at 03:58:32PM +0100, Claudius Heine wrote:
> > > 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.
> > > > 
> > > > @@ -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 we really want to split everything up because of this or not just
> > keep all keys? If we keep keys we cannot use anymore, it does not hurt.
> 
> Right! I would prefer keeping all keys instead of removing some that might
> be needed for an update.
> 
> > Another way would be to specify the keys we want to keep like with
> > ";keep=yes" in the fetcher URI and parse this. What do you think?
> 
> That would be fine, if that works.
> 
> > That would probably be better than to introduce different APT_KEY
> > variables, which might be confusing in code.
> 
> Well you can always come up with a better name for variables to avoid
> confusion.
> 
> [...]
> 
> > > >    # 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?
> > > 
> > Because we throw it way. We don't throw things in WORKDIR away.
> 
> Sorry, I don't understand this. What do you mean with 'throw it away'?
> 
> And even if you mean that that this is only used in a intermediate step, I
> would prefer having it in the WORKDIR.
> 
> What happens if two isar-bootstraps are run in parallel with a different
> configuration? Could that not cause conflict?

Good point...
> 
> > 
> > > > +
> > > > +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.
> > 
> > True
> > > 
> > > 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.
> > > 
> > Do we know this beforehand? It is always allowed to put anything
> > temporary in TMPDIR and it is temporary because we copy the keys around.
> 
> Well the WORKDIR is inside the TMPDIR as well... And things in the WORKDIR

:) Oh yes. I will change it.

> are copied around as well... I think I currently have trouble understanding
> you here...
> 
> Claudius
> 
> 
> -- 
> 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

-- 
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