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

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

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?

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