public inbox for isar-users@googlegroups.com
 help / color / mirror / Atom feed
* [PATCH v4 0/6] Fixes usage of additional apt keys and repos
@ 2019-03-07 14:22 Andreas J. Reichel
  2019-03-07 14:22 ` [PATCH v4 1/6] meta: refactored flock usage Andreas J. Reichel
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Andreas J. Reichel @ 2019-03-07 14:22 UTC (permalink / raw)
  To: isar-users; +Cc: Andreas Reichel

From: Andreas Reichel <andreas.reichel.ext@siemens.com>

Diff to v3:
	* Remove apt-get update in target root file systems,
	  and use the isar-apt again as the only apt source, to support
	  consistent target root file systems without intermediate
	  updates.
	* Add the keyring in the right place (isar-bootstrap function
	  in isar-bootstrap.inc, which needs Claudius' flock patch
	  to fix the reversed double-ticks usage, which breaks
	  everything
	* only add gnupg if we add apt-keys
	* better commit message for revert commit
	* include Claudius' patch to make it clear that it should
	  be merged before
	* isar keyring file is specified in bitbake.conf, where
	  it should be to be available to all image generation
	  steps

What this series does:

Enable the user to really ADD bootstrap repos and keys without
replacing existing ones.

The existing keyring is not replaced but keys are added to
/ect/apt/trusted.gpg.d/isar.gpg instead, where debootstrap and any apt
call can find it.

Furthermore, the code to add keys is simplified by removing duplicate
code and not manually parsing URIs and guessing about download locations
as well as not manually handling gpg and giving apt config overrides.

It is much simpler by using `apt-key` and default apt keyring paths.

The patch of Claudius is included here because it is not yet merged
but required (fix of awful flock usage).

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>


Andreas Reichel (5):
  Revert "isar-bootstrap: Allow to set local keys in DISTRO_APT_KEYS"
  Remove duplicate code from apt-keyring generation
  Fix fetched key location in apt-keyring generator
  Use apt-key to generate apt-keyring
  If we use a custom keyring debootstrap may fall to https

Claudius Heine (1):
  meta: refactored flock usage

 .../conf/multiconfig/qemuamd64-buster.conf    |  1 -
 .../conf/multiconfig/qemuamd64-jessie.conf    |  1 -
 meta/classes/buildchroot.bbclass              |  6 +-
 meta/classes/isar-bootstrap-helper.bbclass    |  2 +
 meta/classes/isar-image.bbclass               |  1 +
 meta/classes/wic-img.bbclass                  |  6 +-
 meta/conf/bitbake.conf                        |  1 +
 .../isar-bootstrap/isar-bootstrap.inc         | 76 +++++++++++++------
 8 files changed, 65 insertions(+), 29 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 1/6] meta: refactored flock usage
  2019-03-07 14:22 [PATCH v4 0/6] Fixes usage of additional apt keys and repos Andreas J. Reichel
@ 2019-03-07 14:22 ` Andreas J. Reichel
  2019-03-07 14:23 ` [PATCH v4 2/6] Revert "isar-bootstrap: Allow to set local keys in DISTRO_APT_KEYS" Andreas J. Reichel
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Andreas J. Reichel @ 2019-03-07 14:22 UTC (permalink / raw)
  To: isar-users; +Cc: Claudius Heine

From: Claudius Heine <ch@denx.de>

Currently much care has to be taken in order to correctly escape strings
inside flock commands. And there is also on instance where this was
incorrectly used (isar-bootstrap.inc).

The usage of flock was changed to no longer require single or double
ticks. Instead commands are run inside a subshell.

Signed-off-by: Claudius Heine <ch@denx.de>
---
 meta/classes/buildchroot.bbclass                    |  6 ++++--
 meta/classes/wic-img.bbclass                        |  6 ++++--
 meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 10 +++++++---
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/meta/classes/buildchroot.bbclass b/meta/classes/buildchroot.bbclass
index 0d4ff4e..c017b25 100644
--- a/meta/classes/buildchroot.bbclass
+++ b/meta/classes/buildchroot.bbclass
@@ -22,7 +22,8 @@ python __anonymous() {
 MOUNT_LOCKFILE = "${BUILDCHROOT_DIR}/mount.lock"
 
 buildchroot_do_mounts() {
-    sudo flock ${MOUNT_LOCKFILE} -c ' \
+    sudo -s <<'EOSUDO'
+        ( flock 9
         set -e
         if ! grep -q ${BUILDCHROOT_DIR}/isar-apt /proc/mounts; then
             mount --bind ${REPO_ISAR_DIR}/${DISTRO} ${BUILDCHROOT_DIR}/isar-apt
@@ -36,5 +37,6 @@ buildchroot_do_mounts() {
 
         # Refresh /etc/resolv.conf at this chance
         cp -L /etc/resolv.conf ${BUILDCHROOT_DIR}/etc
-        '
+        ) 9>${MOUNT_LOCKFILE}
+EOSUDO
 }
diff --git a/meta/classes/wic-img.bbclass b/meta/classes/wic-img.bbclass
index c9d90a9..ac85254 100644
--- a/meta/classes/wic-img.bbclass
+++ b/meta/classes/wic-img.bbclass
@@ -136,14 +136,16 @@ do_build[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
 
 do_wic_image() {
     buildchroot_do_mounts
-    sudo flock ${MOUNT_LOCKFILE} -c ' \
+    sudo -s <<'EOSUDO'
+        ( flock 9
         for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
             mkdir -p ${BUILDCHROOT_DIR}/$dir
             if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1; then
                 mount --bind --make-private $dir ${BUILDCHROOT_DIR}/$dir
             fi
         done
-        '
+        ) 9>${MOUNT_LOCKFILE}
+EOSUDO
     export FAKEROOTCMD=${FAKEROOTCMD}
     export BUILDDIR=${BUILDDIR}
     export MTOOLS_SKIP_CHECK=1
diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
index 234d339..9c82184 100644
--- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
+++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
@@ -207,14 +207,16 @@ isar_bootstrap() {
         esac
         shift
     done
-    debootstrap_args="--verbose --variant=minbase --include='${DISTRO_BOOTSTRAP_BASE_PACKAGES}'"
+    debootstrap_args="--verbose --variant=minbase --include=${DISTRO_BOOTSTRAP_BASE_PACKAGES}"
     if [ "${ISAR_USE_CACHED_BASE_REPO}" = "1" ]; then
         if [ -z "${BASE_REPO_KEY}" ] ; then
             debootstrap_args="$debootstrap_args --no-check-gpg"
         fi
     fi
     E="${@bb.utils.export_proxies(d)}"
-    sudo -E flock "${ISAR_BOOTSTRAP_LOCK}" -c "\
+    export IS_HOST debootstrap_args E
+    sudo -E -s <<'EOSUDO'
+        ( flock 9
         set -e
         if [ ! -e "${DEPLOY_ISAR_BOOTSTRAP}" ]; then
             rm -rf "${ROOTFSDIR}"
@@ -295,7 +297,9 @@ isar_bootstrap() {
 
             # Finalize debootstrap by setting the link in deploy
             ln -Tfsr "${ROOTFSDIR}" "${DEPLOY_ISAR_BOOTSTRAP}"
-        fi"
+        fi
+        ) 9>'${ISAR_BOOTSTRAP_LOCK}'
+EOSUDO
 }
 
 CLEANFUNCS = "clean_deploy"
-- 
2.21.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 2/6] Revert "isar-bootstrap: Allow to set local keys in DISTRO_APT_KEYS"
  2019-03-07 14:22 [PATCH v4 0/6] Fixes usage of additional apt keys and repos Andreas J. Reichel
  2019-03-07 14:22 ` [PATCH v4 1/6] meta: refactored flock usage Andreas J. Reichel
@ 2019-03-07 14:23 ` Andreas J. Reichel
  2019-03-07 14:23 ` [PATCH v4 3/6] Remove duplicate code from apt-keyring generation Andreas J. Reichel
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Andreas J. Reichel @ 2019-03-07 14:23 UTC (permalink / raw)
  To: isar-users; +Cc: Andreas Reichel

From: Andreas Reichel <andreas.reichel.ext@siemens.com>

This reverts commit af983a13b6f4cee5d4af5e5cf6318231e02775c9.

This commit broke usage of remote keys, where they usually come from.
If fetching "http://example.com/dir1/dir2/key", the file is fetched
into the subdir WORKDIR/dir1/dir2/, which breaks with this code.
However it succeeds with absolute paths.
We do not want to guess where the downloaded file will be. This does
not work anymore if the key is downloaded from remote with a URL.
Furthermore, a user could specify "subdir" as fetcher parameter
or other things, which break this.

This is really fixed in a follow-up commit.

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
---
 meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
index 9c82184..9506741 100644
--- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
+++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
@@ -40,7 +40,7 @@ python () {
         d.setVar("DEBOOTSTRAP_KEYRING", "--keyring ${APTKEYRING}")
         for key in distro_apt_keys.split():
             url = urlparse(key)
-            filename = ''.join([wd, url.path])
+            filename = os.path.basename(url.path)
             d.appendVar("SRC_URI", " %s" % key)
             d.appendVar("APTKEYFILES", " %s" % filename)
     if bb.utils.to_boolean(d.getVar('ISAR_USE_CACHED_BASE_REPO')):
-- 
2.21.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 3/6] Remove duplicate code from apt-keyring generation
  2019-03-07 14:22 [PATCH v4 0/6] Fixes usage of additional apt keys and repos Andreas J. Reichel
  2019-03-07 14:22 ` [PATCH v4 1/6] meta: refactored flock usage Andreas J. Reichel
  2019-03-07 14:23 ` [PATCH v4 2/6] Revert "isar-bootstrap: Allow to set local keys in DISTRO_APT_KEYS" Andreas J. Reichel
@ 2019-03-07 14:23 ` Andreas J. Reichel
  2019-03-07 14:46   ` Henning Schild
  2019-03-07 14:23 ` [PATCH v4 4/6] Fix fetched key location in apt-keyring generator Andreas J. Reichel
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Andreas J. Reichel @ 2019-03-07 14:23 UTC (permalink / raw)
  To: isar-users; +Cc: Andreas Reichel

From: Andreas Reichel <andreas.reichel.ext@siemens.com>

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
---
 .../isar-bootstrap/isar-bootstrap.inc         | 24 +++++++++----------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
index 9506741..2d6b761 100644
--- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
+++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
@@ -35,23 +35,21 @@ 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 = os.path.basename(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_key.split()
+
+    d.setVar("DEBOOTSTRAP_KEYRING", "--keyring ${APTKEYRING}")
+    for key in aptkeys:
+        url = urlparse(key)
+        filename = os.path.basename(url.path)
+        d.appendVar("SRC_URI", " %s" % key)
+        d.appendVar("APTKEYFILES", " %s" % filename)
 }
 
 def aggregate_files(d, file_list, file_out):
-- 
2.21.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 4/6] Fix fetched key location in apt-keyring generator
  2019-03-07 14:22 [PATCH v4 0/6] Fixes usage of additional apt keys and repos Andreas J. Reichel
                   ` (2 preceding siblings ...)
  2019-03-07 14:23 ` [PATCH v4 3/6] Remove duplicate code from apt-keyring generation Andreas J. Reichel
@ 2019-03-07 14:23 ` Andreas J. Reichel
  2019-03-07 14:23 ` [PATCH v4 5/6] Use apt-key to generate apt-keyring Andreas J. Reichel
  2019-03-07 14:23 ` [PATCH v4 6/6] If we use a custom keyring debootstrap may fall to https Andreas J. Reichel
  5 siblings, 0 replies; 18+ messages in thread
From: Andreas J. Reichel @ 2019-03-07 14:23 UTC (permalink / raw)
  To: isar-users; +Cc: Andreas Reichel

From: Andreas Reichel <andreas.reichel.ext@siemens.com>

Use bb.fetch2.Fetch to retrieve the final filename after it is
downloaded. This way we don't have to guess (wrongly), and also
additional SRC_URI parameters like subdir or filename are usable now.

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
---
 meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
index 2d6b761..dbc3938 100644
--- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
+++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
@@ -33,7 +33,6 @@ DISTRO_APT_PREMIRRORS ?= "${@ "http://ftp\.(\S+\.)?debian.org  file:///${REPO_BA
 inherit base-apt-helper
 
 python () {
-    from urllib.parse import urlparse
     distro_apt_keys = d.getVar("DISTRO_APT_KEYS", False)
     aptkeys = []
 
@@ -46,9 +45,9 @@ python () {
 
     d.setVar("DEBOOTSTRAP_KEYRING", "--keyring ${APTKEYRING}")
     for key in aptkeys:
-        url = urlparse(key)
-        filename = os.path.basename(url.path)
         d.appendVar("SRC_URI", " %s" % key)
+        fetcher = bb.fetch2.Fetch([key], d)
+        filename = fetcher.localpath(key)
         d.appendVar("APTKEYFILES", " %s" % filename)
 }
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 5/6] Use apt-key to generate apt-keyring
  2019-03-07 14:22 [PATCH v4 0/6] Fixes usage of additional apt keys and repos Andreas J. Reichel
                   ` (3 preceding siblings ...)
  2019-03-07 14:23 ` [PATCH v4 4/6] Fix fetched key location in apt-keyring generator Andreas J. Reichel
@ 2019-03-07 14:23 ` Andreas J. Reichel
  2019-03-07 14:51   ` Henning Schild
  2019-03-07 14:58   ` Claudius Heine
  2019-03-07 14:23 ` [PATCH v4 6/6] If we use a custom keyring debootstrap may fall to https Andreas J. Reichel
  5 siblings, 2 replies; 18+ messages in thread
From: Andreas J. Reichel @ 2019-03-07 14:23 UTC (permalink / raw)
  To: isar-users; +Cc: Andreas Reichel

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}"
 }
 
 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"
 
 # 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"
+
+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/*
+                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" \
-- 
2.21.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v4 6/6] If we use a custom keyring debootstrap may fall to https
  2019-03-07 14:22 [PATCH v4 0/6] Fixes usage of additional apt keys and repos Andreas J. Reichel
                   ` (4 preceding siblings ...)
  2019-03-07 14:23 ` [PATCH v4 5/6] Use apt-key to generate apt-keyring Andreas J. Reichel
@ 2019-03-07 14:23 ` Andreas J. Reichel
  2019-03-07 14:54   ` Henning Schild
  5 siblings, 1 reply; 18+ messages in thread
From: Andreas J. Reichel @ 2019-03-07 14:23 UTC (permalink / raw)
  To: isar-users; +Cc: Andreas Reichel

From: Andreas Reichel <andreas.reichel.ext@siemens.com>

See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=891009

So if we have something in aptkeyring, append https-support to
OVERRIDES.

Furthermore, the conditional append for https-support was missing
in qemuamd64-stretch.conf, thus, remove this from all the distros
and put it into the isar-bootstrap.inc.

Furthermore, packages are comma-, not space-separated.

Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
---
 meta-isar/conf/multiconfig/qemuamd64-buster.conf    |  1 -
 meta-isar/conf/multiconfig/qemuamd64-jessie.conf    |  1 -
 meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 11 +++++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/meta-isar/conf/multiconfig/qemuamd64-buster.conf b/meta-isar/conf/multiconfig/qemuamd64-buster.conf
index 63df75c..da90993 100644
--- a/meta-isar/conf/multiconfig/qemuamd64-buster.conf
+++ b/meta-isar/conf/multiconfig/qemuamd64-buster.conf
@@ -18,4 +18,3 @@ QEMU_MACHINE ?= "q35"
 QEMU_CPU ?= ""
 QEMU_DISK_ARGS ?= "-hda ##ROOTFS_IMAGE## -bios /usr/local/share/ovmf/OVMF.fd"
 
-DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = " apt-transport-https ca-certificates"
diff --git a/meta-isar/conf/multiconfig/qemuamd64-jessie.conf b/meta-isar/conf/multiconfig/qemuamd64-jessie.conf
index d1335ff..42c71df 100644
--- a/meta-isar/conf/multiconfig/qemuamd64-jessie.conf
+++ b/meta-isar/conf/multiconfig/qemuamd64-jessie.conf
@@ -15,4 +15,3 @@ QEMU_MACHINE ?= "pc"
 QEMU_CPU ?= ""
 QEMU_DISK_ARGS ?= "-hda ##ROOTFS_IMAGE##"
 
-DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = " apt-transport-https ca-certificates"
diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
index 2fb5c5b..4c10633 100644
--- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
+++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
@@ -26,6 +26,7 @@ APTKEYFILES = ""
 DEPLOY_ISAR_BOOTSTRAP ?= ""
 DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales"
 DISTRO_BOOTSTRAP_BASE_PACKAGES_append_gnupg = ",gnupg2"
+DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = ",apt-transport-https,ca-certificates"
 
 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 "" }"
 
@@ -42,6 +43,12 @@ python () {
         if own_pub_key:
             aptkeys += own_pub_key.split()
 
+    if len(aptkeys) > 0:
+        # debootstrap falls back to https if there is no
+        # 'reliable' keyring, whatever that means, but it happened
+        # https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=891009
+        d.setVar("HAVE_CUSTOM_APT_KEYS", "True")
+
     for key in aptkeys:
         d.appendVar("SRC_URI", " %s" % key)
         fetcher = bb.fetch2.Fetch([key], d)
@@ -151,6 +158,10 @@ def get_distro_have_https_source(d, is_host=False):
     return any(source[2].startswith("https://") for source in generate_distro_sources(d, is_host))
 
 def get_distro_needs_https_support(d, is_host=False):
+    apt_keys = d.getVar("HAVE_CUSTOM_APT_KEYS", False)
+    if apt_keys:
+        return "https-support"
+
     if get_distro_have_https_source(d, is_host):
         return "https-support"
     else:
-- 
2.21.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/6] Remove duplicate code from apt-keyring generation
  2019-03-07 14:23 ` [PATCH v4 3/6] Remove duplicate code from apt-keyring generation Andreas J. Reichel
@ 2019-03-07 14:46   ` Henning Schild
  2019-03-18 10:14     ` Andreas Reichel
  0 siblings, 1 reply; 18+ messages in thread
From: Henning Schild @ 2019-03-07 14:46 UTC (permalink / raw)
  To: [ext] Andreas J. Reichel; +Cc: isar-users

Am Thu, 7 Mar 2019 15:23:01 +0100
schrieb "[ext] Andreas J. Reichel" <andreas.reichel.ext@siemens.com>:

> From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> 
> Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> ---
>  .../isar-bootstrap/isar-bootstrap.inc         | 24
> +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc index
> 9506741..2d6b761 100644 ---
> a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc +++
> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc @@ -35,23
> +35,21 @@ 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 = os.path.basename(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_key.split()
> +
> +    d.setVar("DEBOOTSTRAP_KEYRING", "--keyring ${APTKEYRING}")
> +    for key in aptkeys:
> +        url = urlparse(key)
> +        filename = os.path.basename(url.path)
> +        d.appendVar("SRC_URI", " %s" % key)
> +        d.appendVar("APTKEYFILES", " %s" % filename)
>  }
>  
>  def aggregate_files(d, file_list, file_out):

Now i get where the duplication came from in the first place.

And as said in another thread, we need this here in a central location:
DISTRO_APT_KEYS_append = "${BASE_REPO_KEY}"

I kind of feel sorry to discover other patch mistakes in reviewing your
series ....

Henning


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 5/6] Use apt-key to generate apt-keyring
  2019-03-07 14:23 ` [PATCH v4 5/6] Use apt-key to generate apt-keyring Andreas J. Reichel
@ 2019-03-07 14:51   ` Henning Schild
  2019-03-18 12:57     ` Andreas Reichel
  2019-03-07 14:58   ` Claudius Heine
  1 sibling, 1 reply; 18+ messages in thread
From: Henning Schild @ 2019-03-07 14:51 UTC (permalink / raw)
  To: [ext] Andreas J. Reichel; +Cc: isar-users

Am Thu, 7 Mar 2019 15:23:03 +0100
schrieb "[ext] Andreas J. Reichel" <andreas.reichel.ext@siemens.com>:

> 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}"
>  }
>  
>  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"
>  
>  # 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 ""

This will probably become DISTRO_APT_KEYS ... and in this series
HAVE_CUSTOM_APT_KEYS does not even exist yet.

Henning

> +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"
> +
> +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/*
> +                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" \


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 6/6] If we use a custom keyring debootstrap may fall to https
  2019-03-07 14:23 ` [PATCH v4 6/6] If we use a custom keyring debootstrap may fall to https Andreas J. Reichel
@ 2019-03-07 14:54   ` Henning Schild
  2019-03-18 10:09     ` Andreas Reichel
  0 siblings, 1 reply; 18+ messages in thread
From: Henning Schild @ 2019-03-07 14:54 UTC (permalink / raw)
  To: [ext] Andreas J. Reichel; +Cc: isar-users

Am Thu, 7 Mar 2019 15:23:04 +0100
schrieb "[ext] Andreas J. Reichel" <andreas.reichel.ext@siemens.com>:

> From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> 
> See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=891009
> 
> So if we have something in aptkeyring, append https-support to
> OVERRIDES.
> 
> Furthermore, the conditional append for https-support was missing
> in qemuamd64-stretch.conf, thus, remove this from all the distros
> and put it into the isar-bootstrap.inc.
> 
> Furthermore, packages are comma-, not space-separated.
> 
> Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> ---
>  meta-isar/conf/multiconfig/qemuamd64-buster.conf    |  1 -
>  meta-isar/conf/multiconfig/qemuamd64-jessie.conf    |  1 -
>  meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 11 +++++++++++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/meta-isar/conf/multiconfig/qemuamd64-buster.conf
> b/meta-isar/conf/multiconfig/qemuamd64-buster.conf index
> 63df75c..da90993 100644 ---
> a/meta-isar/conf/multiconfig/qemuamd64-buster.conf +++
> b/meta-isar/conf/multiconfig/qemuamd64-buster.conf @@ -18,4 +18,3 @@
> QEMU_MACHINE ?= "q35" QEMU_CPU ?= ""
>  QEMU_DISK_ARGS ?= "-hda ##ROOTFS_IMAGE##
> -bios /usr/local/share/ovmf/OVMF.fd" 
> -DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = "
> apt-transport-https ca-certificates" diff --git
> a/meta-isar/conf/multiconfig/qemuamd64-jessie.conf
> b/meta-isar/conf/multiconfig/qemuamd64-jessie.conf index
> d1335ff..42c71df 100644 ---
> a/meta-isar/conf/multiconfig/qemuamd64-jessie.conf +++
> b/meta-isar/conf/multiconfig/qemuamd64-jessie.conf @@ -15,4 +15,3 @@
> QEMU_MACHINE ?= "pc" QEMU_CPU ?= "" QEMU_DISK_ARGS ?= "-hda
> ##ROOTFS_IMAGE##" 
> -DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = "
> apt-transport-https ca-certificates" diff --git
> a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc index
> 2fb5c5b..4c10633 100644 ---
> a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc +++
> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc @@ -26,6 +26,7
> @@ APTKEYFILES = "" DEPLOY_ISAR_BOOTSTRAP ?= ""
> DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales"
> DISTRO_BOOTSTRAP_BASE_PACKAGES_append_gnupg = ",gnupg2"
> +DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support =
> ",apt-transport-https,ca-certificates" 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 "" }"
> @@ -42,6 +43,12 @@ python () { if own_pub_key:
>              aptkeys += own_pub_key.split()
>  
> +    if len(aptkeys) > 0:
> +        # debootstrap falls back to https if there is no
> +        # 'reliable' keyring, whatever that means, but it happened
> +        # https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=891009
> +        d.setVar("HAVE_CUSTOM_APT_KEYS", "True")
> +
>      for key in aptkeys:
>          d.appendVar("SRC_URI", " %s" % key)
>          fetcher = bb.fetch2.Fetch([key], d)
> @@ -151,6 +158,10 @@ def get_distro_have_https_source(d,
> is_host=False): return any(source[2].startswith("https://") for
> source in generate_distro_sources(d, is_host)) 
>  def get_distro_needs_https_support(d, is_host=False):

As i said somewhere else. Something like 
get_distro_overrides
Could be a nice place to just deal with both in one function. So even
having that weird implication to https, we have it all in one place.

Right now we have to such functions depending on keys, otherwise we
would have two functions returning "https-support"

Henning

> +    apt_keys = d.getVar("HAVE_CUSTOM_APT_KEYS", False)
> +    if apt_keys:
> +        return "https-support"
> +
>      if get_distro_have_https_source(d, is_host):
>          return "https-support"
>      else:


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 5/6] Use apt-key to generate apt-keyring
  2019-03-07 14:23 ` [PATCH v4 5/6] Use apt-key to generate apt-keyring Andreas J. Reichel
  2019-03-07 14:51   ` Henning Schild
@ 2019-03-07 14:58   ` Claudius Heine
  2019-03-18 10:21     ` Andreas Reichel
  1 sibling, 1 reply; 18+ messages in thread
From: Claudius Heine @ 2019-03-07 14:58 UTC (permalink / raw)
  To: [ext] Andreas J. Reichel, isar-users

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 6/6] If we use a custom keyring debootstrap may fall to https
  2019-03-07 14:54   ` Henning Schild
@ 2019-03-18 10:09     ` Andreas Reichel
  0 siblings, 0 replies; 18+ messages in thread
From: Andreas Reichel @ 2019-03-18 10:09 UTC (permalink / raw)
  To: Henning Schild; +Cc: isar-users

On Thu, Mar 07, 2019 at 03:54:52PM +0100, Henning Schild wrote:
> Am Thu, 7 Mar 2019 15:23:04 +0100
> schrieb "[ext] Andreas J. Reichel" <andreas.reichel.ext@siemens.com>:
> 
> > From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > 
> > See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=891009
> > 
> > @@ -151,6 +158,10 @@ def get_distro_have_https_source(d,
> > is_host=False): return any(source[2].startswith("https://") for
> > source in generate_distro_sources(d, is_host)) 
> >  def get_distro_needs_https_support(d, is_host=False):
> 
> As i said somewhere else. Something like 
> get_distro_overrides
> Could be a nice place to just deal with both in one function. So even
> having that weird implication to https, we have it all in one place.
> 
> Right now we have to such functions depending on keys, otherwise we
> would have two functions returning "https-support"
> 

Maybe it's better to change this in a follow-up patch because this would
change code that has nothing to do with my series. Then we can delete
get_distro_needs_https_support and make one function for all cases.

Andreas
> Henning
> 
> > +    apt_keys = d.getVar("HAVE_CUSTOM_APT_KEYS", False)
> > +    if apt_keys:
> > +        return "https-support"
> > +
> >      if get_distro_have_https_source(d, is_host):
> >          return "https-support"
> >      else:
> 

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 3/6] Remove duplicate code from apt-keyring generation
  2019-03-07 14:46   ` Henning Schild
@ 2019-03-18 10:14     ` Andreas Reichel
  0 siblings, 0 replies; 18+ messages in thread
From: Andreas Reichel @ 2019-03-18 10:14 UTC (permalink / raw)
  To: Henning Schild; +Cc: isar-users

On Thu, Mar 07, 2019 at 03:46:00PM +0100, Henning Schild wrote:
> Am Thu, 7 Mar 2019 15:23:01 +0100
> schrieb "[ext] Andreas J. Reichel" <andreas.reichel.ext@siemens.com>:
> 
> > From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > 
> > Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> > ---
> >  .../isar-bootstrap/isar-bootstrap.inc         | 24
> > +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc index
> > 9506741..2d6b761 100644 ---
> > a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc +++
> > b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc @@ -35,23
> > +35,21 @@ 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 = os.path.basename(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_key.split()
> > +
> > +    d.setVar("DEBOOTSTRAP_KEYRING", "--keyring ${APTKEYRING}")
> > +    for key in aptkeys:
> > +        url = urlparse(key)
> > +        filename = os.path.basename(url.path)
> > +        d.appendVar("SRC_URI", " %s" % key)
> > +        d.appendVar("APTKEYFILES", " %s" % filename)
> >  }
> >  
> >  def aggregate_files(d, file_list, file_out):
> 
> Now i get where the duplication came from in the first place.
> 
> And as said in another thread, we need this here in a central location:
> DISTRO_APT_KEYS_append = "${BASE_REPO_KEY}"
> 
Sorry I don't get this yet. Where should BASE_REPO_KEY come from? The
fetcher's destination has to be retrieved before, that's what is done in
the function above and that's why we don't use _append but python
syntax.

Andreas
> I kind of feel sorry to discover other patch mistakes in reviewing your
> series ....
> 
> Henning
> 

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 5/6] Use apt-key to generate apt-keyring
  2019-03-07 14:58   ` Claudius Heine
@ 2019-03-18 10:21     ` Andreas Reichel
  2019-03-18 11:48       ` Claudius Heine
  0 siblings, 1 reply; 18+ messages in thread
From: Andreas Reichel @ 2019-03-18 10:21 UTC (permalink / raw)
  To: Claudius Heine; +Cc: isar-users

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.

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 probably be better than to introduce different APT_KEY
variables, which might be confusing in code.

> >   }
> >   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.
> 
... as suggested before.

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

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

> 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

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 5/6] Use apt-key to generate apt-keyring
  2019-03-18 10:21     ` Andreas Reichel
@ 2019-03-18 11:48       ` Claudius Heine
  2019-03-18 11:49         ` Andreas Reichel
  2019-03-18 11:53         ` Andreas Reichel
  0 siblings, 2 replies; 18+ messages in thread
From: Claudius Heine @ 2019-03-18 11:48 UTC (permalink / raw)
  To: Andreas Reichel; +Cc: isar-users

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 5/6] Use apt-key to generate apt-keyring
  2019-03-18 11:48       ` Claudius Heine
@ 2019-03-18 11:49         ` Andreas Reichel
  2019-03-18 11:53         ` Andreas Reichel
  1 sibling, 0 replies; 18+ messages in thread
From: Andreas Reichel @ 2019-03-18 11:49 UTC (permalink / raw)
  To: Claudius Heine; +Cc: isar-users

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.
> 
Okay, then my next series will keep the keys for simplicity :)

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

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 5/6] Use apt-key to generate apt-keyring
  2019-03-18 11:48       ` Claudius Heine
  2019-03-18 11:49         ` Andreas Reichel
@ 2019-03-18 11:53         ` Andreas Reichel
  1 sibling, 0 replies; 18+ messages in thread
From: Andreas Reichel @ 2019-03-18 11:53 UTC (permalink / raw)
  To: Claudius Heine; +Cc: isar-users

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v4 5/6] Use apt-key to generate apt-keyring
  2019-03-07 14:51   ` Henning Schild
@ 2019-03-18 12:57     ` Andreas Reichel
  0 siblings, 0 replies; 18+ messages in thread
From: Andreas Reichel @ 2019-03-18 12:57 UTC (permalink / raw)
  To: Henning Schild; +Cc: isar-users

On Thu, Mar 07, 2019 at 03:51:53PM +0100, Henning Schild wrote:
> Am Thu, 7 Mar 2019 15:23:03 +0100
> schrieb "[ext] Andreas J. Reichel" <andreas.reichel.ext@siemens.com>:
> 
> > 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}"
> >  }
> >  
> >  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"
> >  
> >  # 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 ""
> 
> This will probably become DISTRO_APT_KEYS ... and in this series
> HAVE_CUSTOM_APT_KEYS does not even exist yet.
> 
This does not matter here, because I check if it exists...

> Henning
> 
> > +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"
> > +
> > +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/*
> > +                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" \
> 

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-03-18 12:58 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-07 14:22 [PATCH v4 0/6] Fixes usage of additional apt keys and repos Andreas J. Reichel
2019-03-07 14:22 ` [PATCH v4 1/6] meta: refactored flock usage Andreas J. Reichel
2019-03-07 14:23 ` [PATCH v4 2/6] Revert "isar-bootstrap: Allow to set local keys in DISTRO_APT_KEYS" Andreas J. Reichel
2019-03-07 14:23 ` [PATCH v4 3/6] Remove duplicate code from apt-keyring generation Andreas J. Reichel
2019-03-07 14:46   ` Henning Schild
2019-03-18 10:14     ` Andreas Reichel
2019-03-07 14:23 ` [PATCH v4 4/6] Fix fetched key location in apt-keyring generator Andreas J. Reichel
2019-03-07 14:23 ` [PATCH v4 5/6] Use apt-key to generate apt-keyring Andreas J. Reichel
2019-03-07 14:51   ` Henning Schild
2019-03-18 12:57     ` Andreas Reichel
2019-03-07 14:58   ` Claudius Heine
2019-03-18 10:21     ` Andreas Reichel
2019-03-18 11:48       ` Claudius Heine
2019-03-18 11:49         ` Andreas Reichel
2019-03-18 11:53         ` Andreas Reichel
2019-03-07 14:23 ` [PATCH v4 6/6] If we use a custom keyring debootstrap may fall to https Andreas J. Reichel
2019-03-07 14:54   ` Henning Schild
2019-03-18 10:09     ` Andreas Reichel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox