| Message ID | 20210119112001.11651-1-amikan@ilbers.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] isar-bootstrap: Run gpg-agent before starting apt-key | expand |
On 19.01.21 12:20, Anton Mikanovich wrote: > From: Yuri Adamov <yadamov@ilbers.de> > > Building rpi-stretch natively (under qemu) sometimes fails with: > > gpg: can't connect to the agent: IPC connect call failed > > gpg starts gpg-agent and times out after 5 s. This value is hard-coded. > > Besides, leaving running gpg-agent processes is not clean and prevents > unmounting of filesystems. > > This patch starts and stops the agent manually. > > gnupg now appended to package list unconditionally because gpg-agent is > used in every isar_bootstrap run. Looks good - except that I do not get why makingthis unconditionally. That looks still like a lazy approach to me. Or do I miss some technical need for that (which is not documented here)? Jan > > Signed-off-by: Yuri Adamov <yadamov@ilbers.de> > Signed-off-by: Anton Mikanovich <amikan@ilbers.de> > --- > Changes since v1: > - Removed unnecessary sleeping. > - Removed -9 in kill. > - Commented unconditionally gnupg package append. > - Removed unused OVERRIDES_append and get_distro_needs_gpg_support(). > --- > .../isar-bootstrap/isar-bootstrap.inc | 22 +++++++++---------- > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc > index 8f5f727..751980f 100644 > --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc > +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc > @@ -24,7 +24,7 @@ DISTRO_BOOTSTRAP_KEYFILES = "" > THIRD_PARTY_APT_KEYFILES = "" > DEPLOY_ISAR_BOOTSTRAP ?= "" > DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales" > -DISTRO_BOOTSTRAP_BASE_PACKAGES_append_gnupg = ",gnupg" > +DISTRO_BOOTSTRAP_BASE_PACKAGES_append = ",gnupg" > DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = "${@https_support(d)}" > > inherit deb-dl-dir > @@ -175,16 +175,6 @@ def get_distro_needs_https_support(d, is_host=False): > else: > return "" > > -def get_distro_needs_gpg_support(d): > - apt_keys = d.getVar("DISTRO_BOOTSTRAP_KEYS") or "" > - apt_keys += " " + (d.getVar("THIRD_PARTY_APT_KEYS") or "") > - apt_keys += " " + (d.getVar("BASE_REPO_KEY") or "") > - 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] > > @@ -309,14 +299,22 @@ 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" > + MY_GPGHOME=$(chroot "${ROOTFSDIR}" mktemp -d /tmp/gpghomeXXXXXXXXXX) > + echo "Created temporary directory ${MY_GPGHOME} for gpg-agent" > + chroot "${ROOTFSDIR}" gpg-agent --homedir "${MY_GPGHOME}" --daemon > find ${APT_KEYS_DIR}/ -type f | while read keyfile > do > kfn="$(basename $keyfile)" > cp $keyfile "${ROOTFSDIR}/tmp/$kfn" > chroot "${ROOTFSDIR}" /usr/bin/apt-key \ > - --keyring ${THIRD_PARTY_APT_KEYRING} add "/tmp/$kfn" > + --keyring ${THIRD_PARTY_APT_KEYRING} \ > + --homedir ${MY_GPGHOME} add "/tmp/$kfn" > rm "${ROOTFSDIR}/tmp/$kfn" > done > + GPG_AGENT_PID=$(ps -aux | grep "gpg-agent.*${MY_GPGHOME}" | grep -v grep | awk '{print $2}') > + echo "Killing gpg-agent with pid $GPG_AGENT_PID" > + /bin/kill ${GPG_AGENT_PID} > + chroot "${ROOTFSDIR}" /bin/rm -rf "${MY_GPGHOME}" > > if [ "${@get_distro_suite(d, True)}" = "stretch" ] && [ "${@get_host_release().split('.')[0]}" -lt "4" ]; then > install -v -m644 "${WORKDIR}/isar-apt-fallback.conf" \ >
19.01.2021 15:54, Jan Kiszka wrote: > On 19.01.21 12:20, Anton Mikanovich wrote: >> From: Yuri Adamov <yadamov@ilbers.de> >> >> Building rpi-stretch natively (under qemu) sometimes fails with: >> >> gpg: can't connect to the agent: IPC connect call failed >> >> gpg starts gpg-agent and times out after 5 s. This value is hard-coded. >> >> Besides, leaving running gpg-agent processes is not clean and prevents >> unmounting of filesystems. >> >> This patch starts and stops the agent manually. >> >> gnupg now appended to package list unconditionally because gpg-agent is >> used in every isar_bootstrap run. > Looks good - except that I do not get why makingthis unconditionally. > That looks still like a lazy approach to me. Or do I miss some technical > need for that (which is not documented here)? > > Jan It's used unconditionally inside isar_bootstrap(), so appended it also unconditionally.
On 19.01.21 15:24, Anton Mikanovich wrote: > 19.01.2021 15:54, Jan Kiszka wrote: >> On 19.01.21 12:20, Anton Mikanovich wrote: >>> From: Yuri Adamov <yadamov@ilbers.de> >>> >>> Building rpi-stretch natively (under qemu) sometimes fails with: >>> >>> gpg: can't connect to the agent: IPC connect call failed >>> >>> gpg starts gpg-agent and times out after 5 s. This value is hard-coded. >>> >>> Besides, leaving running gpg-agent processes is not clean and prevents >>> unmounting of filesystems. >>> >>> This patch starts and stops the agent manually. >>> >>> gnupg now appended to package list unconditionally because gpg-agent is >>> used in every isar_bootstrap run. >> Looks good - except that I do not get why makingthis unconditionally. >> That looks still like a lazy approach to me. Or do I miss some technical >> need for that (which is not documented here)? >> >> Jan > > It's used unconditionally inside isar_bootstrap(), so appended it also > unconditionally. > Yes, this is the laziness I'm talking about: I don't see WHY we need to use it unconditionally now. Jan
Am Tue, 19 Jan 2021 14:20:01 +0300 schrieb Anton Mikanovich <amikan@ilbers.de>: > From: Yuri Adamov <yadamov@ilbers.de> > > Building rpi-stretch natively (under qemu) sometimes fails with: > > gpg: can't connect to the agent: IPC connect call failed > > gpg starts gpg-agent and times out after 5 s. This value is > hard-coded. > > Besides, leaving running gpg-agent processes is not clean and prevents > unmounting of filesystems. > > This patch starts and stops the agent manually. > > gnupg now appended to package list unconditionally because gpg-agent > is used in every isar_bootstrap run. > > Signed-off-by: Yuri Adamov <yadamov@ilbers.de> > Signed-off-by: Anton Mikanovich <amikan@ilbers.de> > --- > Changes since v1: > - Removed unnecessary sleeping. > - Removed -9 in kill. > - Commented unconditionally gnupg package append. > - Removed unused OVERRIDES_append and get_distro_needs_gpg_support(). > --- > .../isar-bootstrap/isar-bootstrap.inc | 22 > +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc > b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc index > 8f5f727..751980f 100644 --- > a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc +++ > b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc @@ -24,7 +24,7 > @@ DISTRO_BOOTSTRAP_KEYFILES = "" THIRD_PARTY_APT_KEYFILES = "" > DEPLOY_ISAR_BOOTSTRAP ?= "" > DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales" > -DISTRO_BOOTSTRAP_BASE_PACKAGES_append_gnupg = ",gnupg" > +DISTRO_BOOTSTRAP_BASE_PACKAGES_append = ",gnupg" > DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = > "${@https_support(d)}" > inherit deb-dl-dir > @@ -175,16 +175,6 @@ def get_distro_needs_https_support(d, > is_host=False): else: > return "" > > -def get_distro_needs_gpg_support(d): > - apt_keys = d.getVar("DISTRO_BOOTSTRAP_KEYS") or "" > - apt_keys += " " + (d.getVar("THIRD_PARTY_APT_KEYS") or "") > - apt_keys += " " + (d.getVar("BASE_REPO_KEY") or "") > - 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] > > @@ -309,14 +299,22 @@ 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" > + MY_GPGHOME=$(chroot "${ROOTFSDIR}" mktemp -d > /tmp/gpghomeXXXXXXXXXX) > + echo "Created temporary directory ${MY_GPGHOME} for > gpg-agent" It is probably better to "export GNUPGHOME" and skip the extra argument to all the calls, this way we can not forget that argument on some calls. > + chroot "${ROOTFSDIR}" gpg-agent --homedir > "${MY_GPGHOME}" --daemon find ${APT_KEYS_DIR}/ -type f | while read > keyfile do > kfn="$(basename $keyfile)" > cp $keyfile "${ROOTFSDIR}/tmp/$kfn" > chroot "${ROOTFSDIR}" /usr/bin/apt-key \ > - --keyring ${THIRD_PARTY_APT_KEYRING} add > "/tmp/$kfn" > + --keyring ${THIRD_PARTY_APT_KEYRING} \ > + --homedir ${MY_GPGHOME} add "/tmp/$kfn" > rm "${ROOTFSDIR}/tmp/$kfn" > done > + GPG_AGENT_PID=$(ps -aux | grep > "gpg-agent.*${MY_GPGHOME}" | grep -v grep | awk '{print $2}') > + echo "Killing gpg-agent with pid $GPG_AGENT_PID" > + /bin/kill ${GPG_AGENT_PID} That kill is better done with "gpgconf --kill gpg-agent". In fact you should kill always before the first use, in case something fails and a second run finds a running agent. > + chroot "${ROOTFSDIR}" /bin/rm -rf "${MY_GPGHOME}" this should be guarded making sure MY_GPGHOME is indeed a directory with a matching name ... would be a pity if for some reason it would be i.e. "/usr" or even only "/tmp" Henning > if [ "${@get_distro_suite(d, True)}" = "stretch" ] && [ > "${@get_host_release().split('.')[0]}" -lt "4" ]; then install -v > -m644 "${WORKDIR}/isar-apt-fallback.conf" \
Am Wed, 20 Jan 2021 10:58:55 +0100 schrieb "[ext] Jan Kiszka" <jan.kiszka@siemens.com>: > On 19.01.21 15:24, Anton Mikanovich wrote: > > 19.01.2021 15:54, Jan Kiszka wrote: > >> On 19.01.21 12:20, Anton Mikanovich wrote: > >>> From: Yuri Adamov <yadamov@ilbers.de> > >>> > >>> Building rpi-stretch natively (under qemu) sometimes fails with: > >>> > >>> gpg: can't connect to the agent: IPC connect call failed > >>> > >>> gpg starts gpg-agent and times out after 5 s. This value is > >>> hard-coded. > >>> > >>> Besides, leaving running gpg-agent processes is not clean and > >>> prevents unmounting of filesystems. > >>> > >>> This patch starts and stops the agent manually. > >>> > >>> gnupg now appended to package list unconditionally because > >>> gpg-agent is used in every isar_bootstrap run. > >> Looks good - except that I do not get why makingthis > >> unconditionally. That looks still like a lazy approach to me. Or > >> do I miss some technical need for that (which is not documented > >> here)? > >> > >> Jan > > > > It's used unconditionally inside isar_bootstrap(), so appended it > > also unconditionally. > > > > Yes, this is the laziness I'm talking about: I don't see WHY we need > to use it unconditionally now. It seems like a functional change. We did use the native agent before and now never use it. But i guess the native agent is potentially still used i.e. when building packages. Not sure i like the idea of special casing this one potential gpg user. Henning > Jan >
On Wed, Jan 20, 2021 at 11:55:42AM +0100, Henning Schild wrote: > > + MY_GPGHOME=$(chroot "${ROOTFSDIR}" mktemp -d /tmp/gpghomeXXXXXXXXXX) ... > > + chroot "${ROOTFSDIR}" /bin/rm -rf "${MY_GPGHOME}" > > this should be guarded making sure MY_GPGHOME is indeed a directory > with a matching name ... would be a pity if for some reason it would be > i.e. "/usr" or even only "/tmp" That comes from mktemp -d. If that succeeds, my understanding is it's a directory. Or do I miss anything? With kind regards, Baurzhan.
Am Wed, 20 Jan 2021 12:20:08 +0100 schrieb Baurzhan Ismagulov <ibr@radix50.net>: > On Wed, Jan 20, 2021 at 11:55:42AM +0100, Henning Schild wrote: > > > + MY_GPGHOME=$(chroot "${ROOTFSDIR}" mktemp -d > > > /tmp/gpghomeXXXXXXXXXX) > ... > > > + chroot "${ROOTFSDIR}" /bin/rm -rf "${MY_GPGHOME}" > > > > this should be guarded making sure MY_GPGHOME is indeed a directory > > with a matching name ... would be a pity if for some reason it > > would be i.e. "/usr" or even only "/tmp" > > That comes from mktemp -d. If that succeeds, my understanding is it's > a directory. Or do I miss anything? That is the impression you can get when reading the code. But under the assumption that the script will indeed fail if mktemp fails, and that nothing else overwrites the variable ... even by accident. Now and in the future when people need to touch this code again. Just a safeguard suggestion, i do not feel strong about it. Henning > With kind regards, > Baurzhan.
On Wed, Jan 20, 2021 at 12:04:57PM +0100, Henning Schild wrote: > It seems like a functional change. We did use the native agent before > and now never use it. This is a good point. Yes, we've focused on the CI and not on the interactive build. What do you mean by "native", is it the agent running on my host as my desktop user? If that is required, it would add some more complexity... > But i guess the native agent is potentially still used i.e. when > building packages. This is also a good point. We need agent management, among other reasons, for clean unmounting of the filesystems. If the agent is started elsewhere, we should handle that, too. We used to dpkg-buildpackage -uc -us -- we'll have check that after this patch. With kind regards, Baurzhan.
Am Wed, 20 Jan 2021 12:35:30 +0100 schrieb Baurzhan Ismagulov <ibr@radix50.net>: > On Wed, Jan 20, 2021 at 12:04:57PM +0100, Henning Schild wrote: > > It seems like a functional change. We did use the native agent > > before and now never use it. > > This is a good point. Yes, we've focused on the CI and not on the > interactive build. What do you mean by "native", is it the agent > running on my host as my desktop user? If that is required, it would > add some more complexity... I was talking about an agent potentially already running on that host under the user calling isar. (so i guess root) > > But i guess the native agent is potentially still used i.e. when > > building packages. > > This is also a good point. We need agent management, among other > reasons, for clean unmounting of the filesystems. If the agent is > started elsewhere, we should handle that, too. We used to > dpkg-buildpackage -uc -us -- we'll have check that after this patch. I would actually assume that all the sudo and chroot stuff would automatically avoid accidental agent reuse from root on the host. But i am not sure about it. It could also be that the patch should just kill a potential agent in the chroot with the command i suggested. But as longs as its not understood where that problematic agent is really coming from, i would refrain from proposing patches that are not fully understood either. Henning > > With kind regards, > Baurzhan.
On Wed, Jan 20, 2021 at 01:15:37PM +0100, Henning Schild wrote: > But as longs as its not understood where that problematic agent is > really coming from, i would refrain from proposing patches that are not > fully understood either. The patch fixes a specific problem with starting the agent. With kind regards, Baurzhan.
diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc index 8f5f727..751980f 100644 --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc @@ -24,7 +24,7 @@ DISTRO_BOOTSTRAP_KEYFILES = "" THIRD_PARTY_APT_KEYFILES = "" DEPLOY_ISAR_BOOTSTRAP ?= "" DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales" -DISTRO_BOOTSTRAP_BASE_PACKAGES_append_gnupg = ",gnupg" +DISTRO_BOOTSTRAP_BASE_PACKAGES_append = ",gnupg" DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = "${@https_support(d)}" inherit deb-dl-dir @@ -175,16 +175,6 @@ def get_distro_needs_https_support(d, is_host=False): else: return "" -def get_distro_needs_gpg_support(d): - apt_keys = d.getVar("DISTRO_BOOTSTRAP_KEYS") or "" - apt_keys += " " + (d.getVar("THIRD_PARTY_APT_KEYS") or "") - apt_keys += " " + (d.getVar("BASE_REPO_KEY") or "") - 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] @@ -309,14 +299,22 @@ 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" + MY_GPGHOME=$(chroot "${ROOTFSDIR}" mktemp -d /tmp/gpghomeXXXXXXXXXX) + echo "Created temporary directory ${MY_GPGHOME} for gpg-agent" + chroot "${ROOTFSDIR}" gpg-agent --homedir "${MY_GPGHOME}" --daemon find ${APT_KEYS_DIR}/ -type f | while read keyfile do kfn="$(basename $keyfile)" cp $keyfile "${ROOTFSDIR}/tmp/$kfn" chroot "${ROOTFSDIR}" /usr/bin/apt-key \ - --keyring ${THIRD_PARTY_APT_KEYRING} add "/tmp/$kfn" + --keyring ${THIRD_PARTY_APT_KEYRING} \ + --homedir ${MY_GPGHOME} add "/tmp/$kfn" rm "${ROOTFSDIR}/tmp/$kfn" done + GPG_AGENT_PID=$(ps -aux | grep "gpg-agent.*${MY_GPGHOME}" | grep -v grep | awk '{print $2}') + echo "Killing gpg-agent with pid $GPG_AGENT_PID" + /bin/kill ${GPG_AGENT_PID} + chroot "${ROOTFSDIR}" /bin/rm -rf "${MY_GPGHOME}" if [ "${@get_distro_suite(d, True)}" = "stretch" ] && [ "${@get_host_release().split('.')[0]}" -lt "4" ]; then install -v -m644 "${WORKDIR}/isar-apt-fallback.conf" \