* [PATCH 0/2] Fix gpg-agent management
@ 2021-09-03 16:31 Anton Mikanovich
2021-09-03 16:31 ` [PATCH 1/2] isar-bootstrap: Fix GPG need check function Anton Mikanovich
2021-09-03 16:31 ` [PATCH 2/2] isar-bootstrap: Do not let gpg-agent to stay running Anton Mikanovich
0 siblings, 2 replies; 6+ messages in thread
From: Anton Mikanovich @ 2021-09-03 16:31 UTC (permalink / raw)
To: isar-users; +Cc: Anton Mikanovich
Management logic of starting and stopping gpg-agent in isar-bootstrap
was still not working properly. Starting of gpg-agent happened for every
bitbake run, and stopping did not happen at all. This is not a big issue
when building Isar inside kas, but should be fixed for chroot
environment.
Anton Mikanovich (2):
isar-bootstrap: Fix GPG need check function
isar-bootstrap: Do not let gpg-agent to stay running
.../isar-bootstrap/isar-bootstrap.inc | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] isar-bootstrap: Fix GPG need check function
2021-09-03 16:31 [PATCH 0/2] Fix gpg-agent management Anton Mikanovich
@ 2021-09-03 16:31 ` Anton Mikanovich
2021-09-03 16:31 ` [PATCH 2/2] isar-bootstrap: Do not let gpg-agent to stay running Anton Mikanovich
1 sibling, 0 replies; 6+ messages in thread
From: Anton Mikanovich @ 2021-09-03 16:31 UTC (permalink / raw)
To: isar-users; +Cc: Anton Mikanovich
After adding BASE_REPO_KEY in 3895266 this check was broken.
Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
index c65f2cb..e87c091 100644
--- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
+++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
@@ -187,12 +187,12 @@ def get_distro_needs_https_support(d):
OVERRIDES_append = ":${@get_distro_needs_https_support(d)}"
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 != " ":
+ if d.getVar("DISTRO_BOOTSTRAP_KEYS") or \
+ d.getVar("THIRD_PARTY_APT_KEYS") or \
+ d.getVar("BASE_REPO_KEY"):
return "gnupg"
- return ""
+ else:
+ return ""
OVERRIDES_append = ":${@get_distro_needs_gpg_support(d)}"
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] isar-bootstrap: Do not let gpg-agent to stay running
2021-09-03 16:31 [PATCH 0/2] Fix gpg-agent management Anton Mikanovich
2021-09-03 16:31 ` [PATCH 1/2] isar-bootstrap: Fix GPG need check function Anton Mikanovich
@ 2021-09-03 16:31 ` Anton Mikanovich
2021-09-03 17:30 ` Henning Schild
1 sibling, 1 reply; 6+ messages in thread
From: Anton Mikanovich @ 2021-09-03 16:31 UTC (permalink / raw)
To: isar-users; +Cc: Anton Mikanovich
If running gpg-agent as a daemon we make it to run apt-key as a child
of gpg-agent. After the finish of apt-key parent gpg-agent will be also
finished in that mode.
This will allow us not to control start-stop of gpg-agent (which was
actually broken before this commit) and get rid of any possible issues
caused by left gpg-agent processes.
Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
index e87c091..5f87f10 100644
--- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
+++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
@@ -335,20 +335,18 @@ do_bootstrap() {
MY_GPGHOME="$(chroot "${ROOTFSDIR}" mktemp -d /tmp/gpghomeXXXXXXXXXX)"
echo "Created temporary directory ${MY_GPGHOME} for gpg-agent"
export GNUPGHOME="${MY_GPGHOME}"
- chroot "${ROOTFSDIR}" gpg-agent --daemon
APT_KEY_APPEND="--homedir ${MY_GPGHOME}"
fi
find ${APT_KEYS_DIR}/ -type f | while read keyfile
do
kfn="$(basename $keyfile)"
cp $keyfile "${ROOTFSDIR}/tmp/$kfn"
- chroot "${ROOTFSDIR}" /usr/bin/apt-key \
+ chroot "${ROOTFSDIR}" /usr/bin/gpg-agent --daemon -- /usr/bin/apt-key \
--keyring ${THIRD_PARTY_APT_KEYRING} ${APT_KEY_APPEND} add "/tmp/$kfn"
rm "${ROOTFSDIR}/tmp/$kfn"
done
- if [ -d "${MY_GPGHOME}" ]; then
- echo "Killing gpg-agent for ${MY_GPGHOME}"
- chroot "${ROOTFSDIR}" gpgconf --kill gpg-agent && /bin/rm -rf "${MY_GPGHOME}"
+ if [ "${@get_distro_needs_gpg_support(d)}" = "gnupg" -a -d "${ROOTFSDIR}${MY_GPGHOME}" ]; then
+ rm -rf "${ROOTFSDIR}${MY_GPGHOME}"
fi
if [ "${@get_distro_suite(d)}" = "stretch" ] && [ "${@get_host_release().split('.')[0]}" -lt "4" ]; then
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] isar-bootstrap: Do not let gpg-agent to stay running
2021-09-03 16:31 ` [PATCH 2/2] isar-bootstrap: Do not let gpg-agent to stay running Anton Mikanovich
@ 2021-09-03 17:30 ` Henning Schild
2021-09-06 9:07 ` Anton Mikanovich
0 siblings, 1 reply; 6+ messages in thread
From: Henning Schild @ 2021-09-03 17:30 UTC (permalink / raw)
To: Anton Mikanovich; +Cc: isar-users, Venkata.Pyla
Am Fri, 3 Sep 2021 19:31:05 +0300
schrieb Anton Mikanovich <amikan@ilbers.de>:
> If running gpg-agent as a daemon we make it to run apt-key as a child
> of gpg-agent. After the finish of apt-key parent gpg-agent will be
> also finished in that mode.
> This will allow us not to control start-stop of gpg-agent (which was
> actually broken before this commit) and get rid of any possible issues
> caused by left gpg-agent processes.
>
> Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
> ---
> meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc index
> e87c091..5f87f10 100644 ---
> a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc +++
> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc @@ -335,20
> +335,18 @@ do_bootstrap() { MY_GPGHOME="$(chroot "${ROOTFSDIR}"
> mktemp -d /tmp/gpghomeXXXXXXXXXX)" echo "Created temporary directory
> ${MY_GPGHOME} for gpg-agent" export GNUPGHOME="${MY_GPGHOME}"
> - chroot "${ROOTFSDIR}" gpg-agent --daemon
> APT_KEY_APPEND="--homedir ${MY_GPGHOME}"
> fi
> find ${APT_KEYS_DIR}/ -type f | while read keyfile
> do
> kfn="$(basename $keyfile)"
> cp $keyfile "${ROOTFSDIR}/tmp/$kfn"
> - chroot "${ROOTFSDIR}" /usr/bin/apt-key \
> + chroot "${ROOTFSDIR}" /usr/bin/gpg-agent --daemon --
> /usr/bin/apt-key \ --keyring ${THIRD_PARTY_APT_KEYRING}
> ${APT_KEY_APPEND} add "/tmp/$kfn" rm "${ROOTFSDIR}/tmp/$kfn"
> done
> - if [ -d "${MY_GPGHOME}" ]; then
> - echo "Killing gpg-agent for ${MY_GPGHOME}"
> - chroot "${ROOTFSDIR}" gpgconf --kill gpg-agent &&
> /bin/rm -rf "${MY_GPGHOME}"
Venkata found that /tmp/gpghomeXXXXXXX to be part of the rootfs. It not
being deleted had the same reason the agent was never killed.
> + if [ "${@get_distro_needs_gpg_support(d)}" = "gnupg" -a -d
> "${ROOTFSDIR}${MY_GPGHOME}" ]; then
Why that new condition on get_distr_needs_gpg_support? I think this can
go away. In fact i think the whole if can go since the rm has a -f.
If the condition needs to stay ... i think
-n "${@get_distro_needs_gpg_support(d)}"
would be better than "= gnupg"
And in fact we can probably keep using daemon mode and pull that kill
out of the if as well
chroot "${ROOTFSDIR}" gpgconf --kill gpg-agent || true
rm -rf "${ROOTFSDIR}${MY_GPGHOME}"
Could be a little more efficient, but probably not worth too much
investigation. Just choose what seems more readable and maintainable.
Henning
> + rm -rf "${ROOTFSDIR}${MY_GPGHOME}"
> fi
>
> if [ "${@get_distro_suite(d)}" = "stretch" ] && [
> "${@get_host_release().split('.')[0]}" -lt "4" ]; then
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] isar-bootstrap: Do not let gpg-agent to stay running
2021-09-03 17:30 ` Henning Schild
@ 2021-09-06 9:07 ` Anton Mikanovich
2021-09-06 9:35 ` Henning Schild
0 siblings, 1 reply; 6+ messages in thread
From: Anton Mikanovich @ 2021-09-06 9:07 UTC (permalink / raw)
To: Henning Schild; +Cc: isar-users, Venkata.Pyla
[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]
03.09.2021 20:30, Henning Schild wrote:
> Venkata found that /tmp/gpghomeXXXXXXX to be part of the rootfs. It not
> being deleted had the same reason the agent was never killed.
Even if incorrect check will not fail and gpg-agent will be killed, the
second part of the original line
>chroot "${ROOTFSDIR}" gpgconf --kill gpg-agent && /bin/rm -rf
"${MY_GPGHOME}"
will be executed outside the chroot, so /tmp/gpghome* will not be
removed because of wrong path.
Executing rm outside the chroot with corrected path (`rm -rf
"${ROOTFSDIR}${MY_GPGHOME}"`) is more efficient and will fix the issue.
>> + if [ "${@get_distro_needs_gpg_support(d)}" = "gnupg" -a -d
>> "${ROOTFSDIR}${MY_GPGHOME}" ]; then
> Why that new condition on get_distr_needs_gpg_support? I think this can
> go away. In fact i think the whole if can go since the rm has a -f.
>
> If the condition needs to stay ... i think
>
> -n "${@get_distro_needs_gpg_support(d)}"
>
> would be better than "= gnupg"
This one is needed because original `if [ -d "${ROOTFSDIR}${MY_GPGHOME}"
]` will be true in case there is no need in gpg-agent (MY_GPGHOME empty,
but ROOTFSDIR exists). And that's why we also can't leave just `rm -rf
"${ROOTFSDIR}${MY_GPGHOME}"` there.
The check `= gnupg` was already used above, but yes I can rebuild
previous check also.
> And in fact we can probably keep using daemon mode and pull that kill
> out of the if as well
>
> chroot "${ROOTFSDIR}" gpgconf --kill gpg-agent || true
> rm -rf "${ROOTFSDIR}${MY_GPGHOME}"
>
> Could be a little more efficient, but probably not worth too much
> investigation. Just choose what seems more readable and maintainable.
In the original code the number of chroot executions was
2+[keys_number], the number of gpg-agent executions was 1. Now it is
just [keys_number] for both. I don't think there will be critical
performance drop.
The first priority of this rebuild was it's stability. gpg-agent should
not stays running even in case apt-key fails.
It is also possible to move the loop code inside gpg-agent run command,
but that will be much less readable.
--
Anton Mikanovich
Promwad Ltd.
External service provider of ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn, Germany
+49 (89) 122 67 24-0
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov
[-- Attachment #2: Type: text/html, Size: 4078 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] isar-bootstrap: Do not let gpg-agent to stay running
2021-09-06 9:07 ` Anton Mikanovich
@ 2021-09-06 9:35 ` Henning Schild
0 siblings, 0 replies; 6+ messages in thread
From: Henning Schild @ 2021-09-06 9:35 UTC (permalink / raw)
To: Anton Mikanovich; +Cc: isar-users, Venkata.Pyla
Am Mon, 6 Sep 2021 12:07:09 +0300
schrieb Anton Mikanovich <amikan@ilbers.de>:
> 03.09.2021 20:30, Henning Schild wrote:
> > Venkata found that /tmp/gpghomeXXXXXXX to be part of the rootfs. It
> > not being deleted had the same reason the agent was never killed.
> Even if incorrect check will not fail and gpg-agent will be killed,
> the second part of the original line
> >chroot "${ROOTFSDIR}" gpgconf --kill gpg-agent && /bin/rm -rf
> "${MY_GPGHOME}"
> will be executed outside the chroot, so /tmp/gpghome* will not be
> removed because of wrong path.
> Executing rm outside the chroot with corrected path (`rm -rf
> "${ROOTFSDIR}${MY_GPGHOME}"`) is more efficient and will fix the
> issue.
>
> >> + if [ "${@get_distro_needs_gpg_support(d)}" = "gnupg" -a -d
> >> "${ROOTFSDIR}${MY_GPGHOME}" ]; then
> > Why that new condition on get_distr_needs_gpg_support? I think this
> > can go away. In fact i think the whole if can go since the rm has a
> > -f.
> >
> > If the condition needs to stay ... i think
> >
> > -n "${@get_distro_needs_gpg_support(d)}"
> >
> > would be better than "= gnupg"
> This one is needed because original `if [ -d
> "${ROOTFSDIR}${MY_GPGHOME}" ]` will be true in case there is no need
> in gpg-agent (MY_GPGHOME empty, but ROOTFSDIR exists). And that's why
> we also can't leave just `rm -rf "${ROOTFSDIR}${MY_GPGHOME}"` there.
> The check `= gnupg` was already used above, but yes I can rebuild
> previous check also.
In fact we really need a [ -n MY_GPGHOME] before we "rm -rf". Otherwise
we could delete ${ROOTFSDIR}. Because otherwise we assume that when
get_distro_needs_gpg_support is true that variable is filled for sure.
So i would say.
chroot "${ROOTFSDIR}" gpgconf --kill gpg-agent || true
if [ -n "${MY_GPGHOME}" ]; then
rm -rf "${ROOTFSDIR}"
fi
Henning
>
> > And in fact we can probably keep using daemon mode and pull that
> > kill out of the if as well
> >
> > chroot "${ROOTFSDIR}" gpgconf --kill gpg-agent || true
> > rm -rf "${ROOTFSDIR}"
> >
> > Could be a little more efficient, but probably not worth too much
> > investigation. Just choose what seems more readable and
> > maintainable.
> In the original code the number of chroot executions was
> 2+[keys_number], the number of gpg-agent executions was 1. Now it is
> just [keys_number] for both. I don't think there will be critical
> performance drop.
> The first priority of this rebuild was it's stability. gpg-agent
> should not stays running even in case apt-key fails.
> It is also possible to move the loop code inside gpg-agent run
> command, but that will be much less readable.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-06 9:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-03 16:31 [PATCH 0/2] Fix gpg-agent management Anton Mikanovich
2021-09-03 16:31 ` [PATCH 1/2] isar-bootstrap: Fix GPG need check function Anton Mikanovich
2021-09-03 16:31 ` [PATCH 2/2] isar-bootstrap: Do not let gpg-agent to stay running Anton Mikanovich
2021-09-03 17:30 ` Henning Schild
2021-09-06 9:07 ` Anton Mikanovich
2021-09-06 9:35 ` Henning Schild
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox