[PATCH] sshd-regen-keys: Improve service, make more robust

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

This improves a number of things:

- stop the service while regenerating keys, rather than disabling its
auto-start
- fix restart test condition
- also check that /tmp is writable (better safe than sorry)
- do not disabling the regen service if it was not successful

Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

This obsoletes Quirin's patch "sshd-regen-keys: do not enable ssh server if previously disabled".

.../sshd-regen-keys/files/sshd-regen-keys.service | 2 +-
.../sshd-regen-keys/files/sshd-regen-keys.sh | 14 ++++++++------
...hd-regen-keys_0.3.bb => sshd-regen-keys_0.4.bb} | 0
3 files changed, 9 insertions(+), 7 deletions(-)
rename meta/recipes-support/sshd-regen-keys/{sshd-regen-keys_0.3.bb => sshd-regen-keys_0.4.bb} (100%)

diff --git a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
index f50d34c8..e7142e69 100644
--- a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
+++ b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
@@ -5,13 +5,13 @@ Conflicts=shutdown.target
After=systemd-remount-fs.service
Before=shutdown.target ssh.service
ConditionPathIsReadWrite=/etc
+ConditionPathIsReadWrite=/tmp

[Service]
Type=oneshot
RemainAfterExit=yes
Environment=DEBIAN_FRONTEND=noninteractive
ExecStart=/usr/sbin/sshd-regen-keys.sh
-ExecStartPost=-/bin/systemctl disable sshd-regen-keys.service
StandardOutput=syslog
StandardError=syslog

diff --git a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
index 910d879b..9b19f9d3 100644
--- a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
+++ b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
@@ -1,9 +1,9 @@
#!/usr/bin/env sh

echo -n "SSH server is "
-if systemctl is-enabled ssh; then
- SSHD_ENABLED="true"
- systemctl disable --no-reload ssh
+if systemctl is-active ssh; then
+ SSHD_ACTIVE="true"
+ systemctl stop ssh
fi

echo "Removing keys ..."
@@ -12,9 +12,11 @@ rm -v /etc/ssh/ssh_host_*_key*
echo "Regenerating keys ..."
dpkg-reconfigure openssh-server

-if test -n $SSHD_ENABLED; then
- echo "Reenabling ssh server ..."
- systemctl enable --no-reload ssh
+if test -n "$SSHD_ACTIVE"; then
+ echo "Restarting ssh server ..."
+ systemctl start ssh
fi

+systemctl disable sshd-regen-keys.service
+
sync
diff --git a/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb b/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb
similarity index 100%
rename from meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb
rename to meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb
--
2.26.2

[Henning Schild]

I am beginning to think we should fix that upstream. If the upstream
service file would generate the keys if missing ... all isar would need
to do is remove the files. Either with a package hook or with a
image-postprocess

Am Thu, 25 Mar 2021 13:54:02 +0100
schrieb Jan Kiszka <jan.k...@siemens.com>:

> From: Jan Kiszka <jan.k...@siemens.com>
>
> This improves a number of things:
>
> - stop the service while regenerating keys, rather than disabling its
> auto-start

Not sure this is going to work. There is this "Before=ssh.service"
which i would expect makes sure it should never end up being
"is-active". And that dpkg-reconfigure also plays with is-active ...
/var/lib/dpkg/info/openssh-server.postinst

The idea was to reuse the key generation code from that postinst, but
the construct we need to build to get that to work seems to be getting
out of hand and too complicated. In fact it is systemd-only, which
could be an issue for some.

Maybe running after ssh
- remove
- "create with own code"
- "copy those few ssh-keygen lines"
- or "source openssh-server.postinst && create_keys"
- killall -HUP sshd (systemctl reload ssh)
might turn out to be the simpler and easier to maintain version.

For sure Harald should be involved, did add him to Cc.

Henning

[Jan Kiszka]

I don't mind any simpler solution. It need to be robust as well, that's
all. The one we have so far once again fell apart today and costed me
hours to understand and resolve (because it was slow to reproduce).

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Henning Schild]

Am Thu, 25 Mar 2021 19:53:46 +0100

What i proposed should hopefully be more robust and simpler, but i have
no time to implement and test it.

What could be even simpler

/etc/systemd/system/sshd.service.d/generate-missing-keys.conf
[Service]
ExecStartPre=
ExecStartPre=/usr/bin/ssh-keygen -A
ExecStartPre=/usr/sbin/sshd -t

DEBIAN_DEPENDS="openssh-server"

postinst
rm -v /etc/ssh/ssh_host_*_key*

That ExecStartPre is what seems to be missing in the service file from
debian because they seem to assume they fully deal with keys at
installation time and never at runtime.
Unfortunately we need 3 lines because we need to prepend before the
"sshd -t". First to "overwrite", second "our content", third "content
from original"

Tried that manually on a system, with the systemd snippet you get new
keys every time the exisiting ones go missing.

regards,
Henning

[Henning Schild]

Am Fri, 26 Mar 2021 08:35:51 +0100
schrieb "[ext] Henning Schild" <henning...@siemens.com>:

Because of that prepend and having to copy existing "ExecStartPre" into
the snippet, a Before-service is probably better. Because that simply
does not care what the original service might look like.
Did send a patch.

regards,
Henning

[Henning Schild]

Switch to using "/usr/bin/ssh-keygen -A" instead of dpkg-reconfigure.
With this we would generate new host keys every time the service starts
and no keys exist. Removing the keys from openssh-server in a postinst
makes it complete so that we really only generate on the first boot.

This is easier to handle that reusing the debian package hooks for key
generation.

Signed-off-by: Henning Schild <henning...@siemens.com>
---
.../sshd-regen-keys/files/postinst | 2 ++
.../files/sshd-regen-keys.service | 4 +---
.../sshd-regen-keys/files/sshd-regen-keys.sh | 20 -------------------
.../sshd-regen-keys/sshd-regen-keys_0.3.bb | 17 ----------------
.../sshd-regen-keys/sshd-regen-keys_0.4.bb | 14 +++++++++++++
5 files changed, 17 insertions(+), 40 deletions(-)
delete mode 100644 meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
delete mode 100644 meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb
create mode 100644 meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb

diff --git a/meta/recipes-support/sshd-regen-keys/files/postinst b/meta/recipes-support/sshd-regen-keys/files/postinst
index ae722a7349a2..1c9b03e3e040 100644
--- a/meta/recipes-support/sshd-regen-keys/files/postinst
+++ b/meta/recipes-support/sshd-regen-keys/files/postinst
@@ -1,4 +1,6 @@
#!/bin/sh
set -e

+rm /etc/ssh/ssh_host_*_key*
+
systemctl enable sshd-regen-keys.service
diff --git a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
index f50d34c820d8..af98d5e9e966 100644
--- a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
+++ b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
@@ -9,9 +9,7 @@ ConditionPathIsReadWrite=/etc
[Service]
Type=oneshot
RemainAfterExit=yes
-Environment=DEBIAN_FRONTEND=noninteractive
-ExecStart=/usr/sbin/sshd-regen-keys.sh
-ExecStartPost=-/bin/systemctl disable sshd-regen-keys.service
+ExecStart=/usr/bin/ssh-keygen -A

StandardOutput=syslog
StandardError=syslog

diff --git a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh

deleted file mode 100644
index 910d879ba51f..000000000000
--- a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
+++ /dev/null
@@ -1,20 +0,0 @@
-#!/usr/bin/env sh
-
-echo -n "SSH server is "

-if systemctl is-enabled ssh; then
- SSHD_ENABLED="true"
- systemctl disable --no-reload ssh

-fi
-
-echo "Removing keys ..."
-rm -v /etc/ssh/ssh_host_*_key*
-
-echo "Regenerating keys ..."
-dpkg-reconfigure openssh-server
-

-if test -n $SSHD_ENABLED; then
- echo "Reenabling ssh server ..."
- systemctl enable --no-reload ssh

-fi
-
-sync
diff --git a/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb b/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb
deleted file mode 100644
index 6f12414239a3..000000000000
--- a/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb
+++ /dev/null
@@ -1,17 +0,0 @@
-# This software is a part of ISAR.
-inherit dpkg-raw
-
-DESCRIPTION = "Systemd service to regenerate sshd keys"
-MAINTAINER = "isar-users <isar-...@googlegroups.com>"
-DEBIAN_DEPENDS = "openssh-server, systemd"
-
-SRC_URI = "file://postinst \
- file://sshd-regen-keys.service \
- file://sshd-regen-keys.sh"
-
-do_install[cleandirs] = "${D}/lib/systemd/system \
- ${D}/usr/sbin"
-do_install() {
- install -v -m 644 "${WORKDIR}/sshd-regen-keys.service" "${D}/lib/systemd/system/sshd-regen-keys.service"
- install -v -m 755 "${WORKDIR}/sshd-regen-keys.sh" "${D}/usr/sbin/sshd-regen-keys.sh"
-}
diff --git a/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb b/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb
new file mode 100644
index 000000000000..8b1cd8d4aba0
--- /dev/null
+++ b/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb
@@ -0,0 +1,14 @@
+# This software is a part of ISAR.
+inherit dpkg-raw
+
+DESCRIPTION = "Systemd service to regenerate sshd keys"
+MAINTAINER = "isar-users <isar-...@googlegroups.com>"
+DEBIAN_DEPENDS = "openssh-server, systemd"
+
+SRC_URI = "file://postinst \
+ file://sshd-regen-keys.service"
+
+do_install() {
+ install -m 0755 "${D}/lib/systemd/system"
+ install -m 0644 "${WORKDIR}/sshd-regen-keys.service" "${D}/lib/systemd/system/sshd-regen-keys.service"
+}
--
2.26.3

[Henning Schild]

This uses the same subject line as the patch from Jan, maybe i should
have used v2 or another line.

It is the outcome of the review on Jans patch but uses a different
approach on key regeneration.

Jan please test it and let me know what you think. Feel free to take
over and massage it further in case this looks like a valid approach.

regards,
Henning

Am Fri, 26 Mar 2021 09:11:08 +0100
schrieb Henning Schild <henning...@siemens.com>:

[Harald Seiler]

Hi,

On Fri, 2021-03-26 at 09:11 +0100, Henning Schild wrote:
> Switch to using "/usr/bin/ssh-keygen -A" instead of dpkg-reconfigure.
> With this we would generate new host keys every time the service starts
> and no keys exist. Removing the keys from openssh-server in a postinst
> makes it complete so that we really only generate on the first boot.
>
> This is easier to handle that reusing the debian package hooks for key
> generation.

Yes, this is a _much_ more robust solution, I agree. The debian hooks
were a mess to deal with and we had so many edge cases over time that not
relying on them here is a much better choice. This also means the package
would now work on a target where dpkg was removed for size constraints.

> Signed-off-by: Henning Schild <henning...@siemens.com>
> ---
>  .../sshd-regen-keys/files/postinst | 2 ++
>  .../files/sshd-regen-keys.service | 4 +---
>  .../sshd-regen-keys/files/sshd-regen-keys.sh | 20 -------------------
>  .../sshd-regen-keys/sshd-regen-keys_0.3.bb | 17 ----------------
>  .../sshd-regen-keys/sshd-regen-keys_0.4.bb | 14 +++++++++++++
>  5 files changed, 17 insertions(+), 40 deletions(-)
>  delete mode 100644 meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
>  delete mode 100644 meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb
>  create mode 100644 meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb
>
> diff --git a/meta/recipes-support/sshd-regen-keys/files/postinst b/meta/recipes-support/sshd-regen-keys/files/postinst
> index ae722a7349a2..1c9b03e3e040 100644
> --- a/meta/recipes-support/sshd-regen-keys/files/postinst
> +++ b/meta/recipes-support/sshd-regen-keys/files/postinst
> @@ -1,4 +1,6 @@
>  #!/bin/sh
>  set -e
>  
>
> +rm /etc/ssh/ssh_host_*_key*
> +

Just to make sure, this will always run after the openssh-server postinst
which initially generates the keys?

>  systemctl enable sshd-regen-keys.service
> diff --git a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
> index f50d34c820d8..af98d5e9e966 100644
> --- a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
> +++ b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
> @@ -9,9 +9,7 @@ ConditionPathIsReadWrite=/etc
>  [Service]
>  Type=oneshot
>  RemainAfterExit=yes
> -Environment=DEBIAN_FRONTEND=noninteractive
> -ExecStart=/usr/sbin/sshd-regen-keys.sh
> -ExecStartPost=-/bin/systemctl disable sshd-regen-keys.service
> +ExecStart=/usr/bin/ssh-keygen -A
>  StandardOutput=syslog
>  StandardError=syslog

This is also much cleaner because it no longer relies on the "self
disabling service hack". Much preferred! Not sure if worth it,
because ssh-keygen already ignores existing keys, but maybe we could add
some

ConditionPathExists=|!/etc/ssh/ssh_host_ecdsa_key
ConditionPathExists=|!/etc/ssh/ssh_host_ed25519_key
ConditionPathExists=|!/etc/ssh/ssh_host_rsa_key

(== systemd will skip the unit if all keys are present). This would also
hide the service in the startup log when all keys exist where it would
otherwise show up unconditionally.

Otherwise:

Reviewed-by: Harald Seiler <h...@denx.de>

--
Harald

[Henning Schild]

Am Fri, 26 Mar 2021 10:44:39 +0100
schrieb Harald Seiler <h...@denx.de>:

> Hi,
>
> On Fri, 2021-03-26 at 09:11 +0100, Henning Schild wrote:
> > Switch to using "/usr/bin/ssh-keygen -A" instead of
> > dpkg-reconfigure. With this we would generate new host keys every
> > time the service starts and no keys exist. Removing the keys from
> > openssh-server in a postinst makes it complete so that we really
> > only generate on the first boot.
> >
> > This is easier to handle that reusing the debian package hooks for
> > key generation.
>
> Yes, this is a _much_ more robust solution, I agree. The debian hooks
> were a mess to deal with and we had so many edge cases over time that
> not relying on them here is a much better choice. This also means
> the package would now work on a target where dpkg was removed for
> size constraints.

Thanks for the positive review.

@Jan did you get around testing this for your use-case?

Henning

[Henning Schild]

Am Fri, 26 Mar 2021 09:11:08 +0100
schrieb Henning Schild <henning...@siemens.com>:

missing "-d" will send v2

[Henning Schild]

index 000000000000..9ce1d8d88300

--- /dev/null
+++ b/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb
@@ -0,0 +1,14 @@
+# This software is a part of ISAR.
+inherit dpkg-raw
+
+DESCRIPTION = "Systemd service to regenerate sshd keys"
+MAINTAINER = "isar-users <isar-...@googlegroups.com>"
+DEBIAN_DEPENDS = "openssh-server, systemd"
+
+SRC_URI = "file://postinst \
+ file://sshd-regen-keys.service"
+
+do_install() {

+ install -d -m 0755 "${D}/lib/systemd/system"

+ install -m 0644 "${WORKDIR}/sshd-regen-keys.service" "${D}/lib/systemd/system/sshd-regen-keys.service"
+}

--
2.26.3

[Anton Mikanovich]

30.03.2021 13:17, Henning Schild wrote:
> Switch to using "/usr/bin/ssh-keygen -A" instead of dpkg-reconfigure.
> With this we would generate new host keys every time the service starts
> and no keys exist. Removing the keys from openssh-server in a postinst
> makes it complete so that we really only generate on the first boot.
>
> This is easier to handle that reusing the debian package hooks for key
> generation.
>
> Signed-off-by: Henning Schild <henning...@siemens.com>

Looks good, waiting for Jan to approve.

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

[Jan Kiszka]

On 08.04.21 10:24, Anton Mikanovich wrote:
> 30.03.2021 13:17, Henning Schild wrote:
>> Switch to using "/usr/bin/ssh-keygen -A" instead of dpkg-reconfigure.
>> With this we would generate new host keys every time the service starts
>> and no keys exist. Removing the keys from openssh-server in a postinst
>> makes it complete so that we really only generate on the first boot.
>>
>> This is easier to handle that reusing the debian package hooks for key
>> generation.
>>
>> Signed-off-by: Henning Schild <henning...@siemens.com>
>
> Looks good, waiting for Jan to approve.
>

Just checked this with my problematic setup, and the patch works as it
should.

Jan

[Anton Mikanovich]

30.03.2021 13:17, Henning Schild wrote:

> Switch to using "/usr/bin/ssh-keygen -A" instead of dpkg-reconfigure.
> With this we would generate new host keys every time the service starts
> and no keys exist. Removing the keys from openssh-server in a postinst
> makes it complete so that we really only generate on the first boot.
>
> This is easier to handle that reusing the debian package hooks for key
> generation.
>
> Signed-off-by: Henning Schild <henning...@siemens.com>

Applied to next, thanks.

[Moessbauer, Felix]

Hi,

While this patch definitely improves the situation, there are still a couple of issues:

1. Reinstalling:
When apt-get updating the package, the host's ssh-keys are removed.
IMO it would be better to create a backup in the pre-rm step and restore that in postinst.
An alternative would be, to remove the ssh keys using ISAR in a post-processing step.
Then no postinst script is required (that's similar to how the sshd-keygen@.service in fedora works).

2. Systemd dependencies:
It has to run as early as possible and anyways before the sshd-service.
On some systems like fedora, there is already a sshd-keygen@.service that takes care of re-generating the keys if they are not present (as part of the openssh-server package).
We should conflict on that, or better auto-disable in case this service is installed.
I don't know if Debian plans to add something similar.

3. Compatibility with upstream
If more distros accept the sshd-keygen service approach, we do not want to diverge here.
Maybe, it would be better to just port this approach to Debian / ISAR and deploy images without pre-installed ssh-keys.

Best regards,
Felix

> --
> You received this message because you are subscribed to the Google Groups
> "isar-users" group.
> To unsubscribe from this group and stop receiving emails from it, send an email
> to isar-users+...@googlegroups.com.
> To view this discussion on the web visit
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.g
> oogle.com%2Fd%2Fmsgid%2Fisar-users%2F20210330101722.10371-1-
> henning.schild%2540siemens.com&amp;data=04%7C01%7Cfelix.moessbauer%4
> 0siemens.com%7Ccf1624cf55db4c9c706708d8f36509a3%7C38ae3bcd95794fd4
> addab42e1495d55a%7C1%7C0%7C637526962559188131%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C1000&amp;sdata=bM6bgFd1Yq4Vo2tMGrR7GHzRWgSAQMB90vu
> %2BHOa2eZ4%3D&amp;reserved=0.

[Henning Schild]

Am Wed, 28 Apr 2021 14:21:38 +0200
schrieb "Moessbauer, Felix (T RDA IOT SES-DE)"
<felix.mo...@siemens.com>:

> Hi,
>
> While this patch definitely improves the situation, there are still a
> couple of issues:
>
> 1. Reinstalling:
> When apt-get updating the package, the host's ssh-keys are removed.
> IMO it would be better to create a backup in the pre-rm step and
> restore that in postinst. An alternative would be, to remove the ssh
> keys using ISAR in a post-processing step. Then no postinst script is
> required (that's similar to how the sshd-keygen@.service in fedora
> works).

Good catch! I will try to make something up. Problem with a backup is
that we do not want that backup on the first install, but with the
pre-rm you suggested it might just work.
The postprocess would be a clean way as well, but that would need to be
conditional if the package is installed. Because the systemd units will
not generate keys if missing and sshd will not come up. But i think
that can be done and is better than the backup cycle.

> 2. Systemd dependencies:
> It has to run as early as possible and anyways before the
> sshd-service. On some systems like fedora, there is already a
> sshd-keygen@.service that takes care of re-generating the keys if
> they are not present (as part of the openssh-server package). We
> should conflict on that, or better auto-disable in case this service
> is installed. I don't know if Debian plans to add something similar.

I guess you mean that sshd-regen-keys.service needs to finish for sure
before ssh@.service comes up. That seems to missing as you say, thanks!
A systemd dep would be the way to go, we failed with enable/disable
stuff before.

> 3. Compatibility with upstream
> If more distros accept the sshd-keygen service approach, we do not
> want to diverge here. Maybe, it would be better to just port this
> approach to Debian / ISAR and deploy images without pre-installed
> ssh-keys.

That in fact sound like the most promising way to go, but also the
hardest. If debian would simply "generate if missing" and not "generate
at install time" that would be it. I think they did that in init
scripts before systemd became a thing.

One more thing we discussed internally is "cloud-init". That solves the
same problem and is packaged in debian. In fact it might address more
that might be relevant for images that get distributed to many
machines. But we also found that it is kind of heavy, pulling in
python3 and libs.

At the moment i have no clue how to proceed and will need to think
about it. I would say that "apt-get update" is maybe not something that
most Isar users want to use. We also have severe kernel update issues
with our wic, where at least legacy is affected and kernel updates will
not work. Not a nice situation ... but it takes the pressure out of
this one a bit.

Anyone feel free to discuss further to help out.

regards,
Henning

[Gylstorff Quirin]

Is it intended that it now runs on every boot?

Quirin

[Henning Schild]

Am Thu, 18 Nov 2021 12:10:04 +0100
schrieb Gylstorff Quirin <quirin.g...@siemens.com>:

Yes. But only the first one will really do something, the others will
end up as noop.

Henning