openssh: Atomically generate host keys

Generating the host keys atomically prevents power interruptions during the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

Signed-off-by Joshua Watt <[email protected]>

---
 meta/recipes-connectivity/openssh/openssh/init     | 21 ++++------------
 .../openssh/openssh/sshd-check-key                 | 28 ++++++++++++++++++++++
 .../openssh/openssh/sshdgenkeys.service            | 16 ++++---------
 meta/recipes-connectivity/openssh/openssh_7.4p1.bb |  8 +++++++
 4 files changed, 44 insertions(+), 29 deletions(-)
 create mode 100644 meta/recipes-connectivity/openssh/openssh/sshd-check-key

meta/recipes-connectivity/openssh/openssh/init +4 -17

index 1f63725..22124a9 100644
@@ -45,23 +45,10 @@ check_config() {
 }
 
 check_keys() {
-       # create keys if necessary
-       if [ ! -f $HOST_KEY_RSA ]; then
-               echo "  generating ssh RSA key..."
-               ssh-keygen -q -f $HOST_KEY_RSA -N '' -t rsa
-       fi
-       if [ ! -f $HOST_KEY_ECDSA ]; then
-               echo "  generating ssh ECDSA key..."
-               ssh-keygen -q -f $HOST_KEY_ECDSA -N '' -t ecdsa
-       fi
-       if [ ! -f $HOST_KEY_DSA ]; then
-               echo "  generating ssh DSA key..."
-               ssh-keygen -q -f $HOST_KEY_DSA -N '' -t dsa
-       fi
-       if [ ! -f $HOST_KEY_ED25519 ]; then
-               echo "  generating ssh ED25519 key..."
-               ssh-keygen -q -f $HOST_KEY_ED25519 -N '' -t ed25519
-       fi
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_RSA rsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ECDSA ecdsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_DSA dsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ED25519 ed25519
 }
 
 export PATH="${PATH:+$PATH:}/usr/sbin:/sbin"

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +28 -0

new file mode 100644
index 0000000..3495d98
@@ -0,0 +1,28 @@
+#! /bin/sh
+set -e
+
+NAME="$1"
+TYPE="$2"
+
+if [ -z "$NAME" ] || [ -z "$TYPE" ]; then
+    echo "Usage: $0 NAME TYPE"
+    exit 1;
+fi
+
+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before renaming
+    sync
+
+    # Move (Atomically rename) files
+    # Rename the .pub file first, since the check that triggers a
+    # key generation is based on the private file.
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+    sync
+
+    mv -f "${NAME}.tmp" "${NAME}"
+    sync
+fi
+

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +4 -12

index 148e6ad..af56404 100644
@@ -1,22 +1,14 @@
 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key
 
 [Service]
 Environment="SYSCONFDIR=/etc/ssh"
 EnvironmentFile=-/etc/default/ssh
 ExecStart=@BASE_BINDIR@/mkdir -p $SYSCONFDIR
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_rsa_key -N '' -t rsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_dsa_key -N '' -t dsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ecdsa_key -N '' -t ecdsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ed25519_key -N '' -t ed25519
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_rsa_key rsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_dsa_key dsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ecdsa_key ecdsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ed25519_key ed25519
 Type=oneshot
 RemainAfterExit=yes

meta/recipes-connectivity/openssh/openssh_7.4p1.bb +8 -1

index c8093d4..ccd7a02 100644
@@ -25,6 +25,7 @@ SRC_URI = "http://ftp.openbsd.org/pub/OpenBSD/OpenSSH/portable/openssh-${PV}.tar
            file://openssh-7.1p1-conditional-compile-des-in-cipher.patch \
            file://openssh-7.1p1-conditional-compile-des-in-pkcs11.patch \
            file://fix-potential-signed-overflow-in-pointer-arithmatic.patch \
+           file://sshd-check-key \
            "
 
 PAM_SRC_URI = "file://sshd"

@@ -124,7 +125,14 @@ do_install_append () {
        sed -i -e 's,@BASE_BINDIR@,${base_bindir},g' \
                -e 's,@SBINDIR@,${sbindir},g' \
                -e 's,@BINDIR@,${bindir},g' \
+               -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
                ${D}${systemd_unitdir}/system/sshd.socket ${D}${systemd_unitdir}/system/*.service
+
+       sed -i -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
+               ${D}${sysconfdir}/init.d/sshd
+
+       install -d 644 ${D}${libexecdir}/${BPN}
+       install -m 0755 ${WORKDIR}/sshd-check-key ${D}${libexecdir}/${BPN}
 }
 
 do_install_ptest () {
--

2.9.3

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +6 -0

+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before renaming
+    sync

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -0

index 148e6ad..af56404 100644
@@ -1,22 +1,14 @@
 [Unit]
 Description=OpenSSH Key Generation

Quoted 15 lines · Joshua Watt

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +14 -0

+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before renaming
+    sync
+
+    # Move (Atomically rename) files
+    # Rename the .pub file first, since the check that triggers a
+    # key generation is based on the private file.
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+    sync
+
+    mv -f "${NAME}.tmp" "${NAME}"

Review

Quoted 3 lines

(quoted diff) +2 -0

+    sync
+fi

All of these syncs seem quite enthusiastic, are they really needed? Writing the file to a temporary name and then mving it to the real name should result in either no file or a complete file in the event of power loss, surely?

Quoted 16 lines

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -5

index 148e6ad..af56404 100644
@@ -1,22 +1,14 @@
 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service line 11

Quoted 3 lines

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -3

-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key

Can you not continue to use ConditionPathExists to only run this unit if it needs to run? You can prepend the argument with | to make them logical OR instead of logical AND, if I'm reading this documentation correctly.

Ross

Quoted 17 lines · Ross

Quoted 15 lines · Joshua Watt

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +14 -0

+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before renaming
+    sync
+
+    # Move (Atomically rename) files
+    # Rename the .pub file first, since the check that triggers a
+    # key generation is based on the private file.
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+    sync
+
+    mv -f "${NAME}.tmp" "${NAME}"

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+    sync
+fi

All of these syncs seem quite enthusiastic, are they really needed? Writing the file to a temporary name and then mving it to the real name should result in either no file or a complete file in the event of power loss, surely?

My understanding (and observation) of most journal file systems is that only metadata (i.e. directory entries and such) are journaled in typical usage. The first sync is necessary in this case to ensure that the actual file data gets put on the disk before we rename the files, otherwise in the event of interruption journaled rename might get replayed but have garbage data. The second one is more of a "force operation order" sync to make sure the public file is written before the private one, as a reordering would cause problems. The last sync is the most optional, but I've seen it take minutes for disk to sync contents if no one calls "sync", so it is very possible that all our work of regenerating keys would be for naught if power is interrupted in the meantime.

I think some of these syncs can be removed. Namely, the first and third one. The second one needs to be there, but can server double duty of syncing data to disk and enforcing the order between the public and private rename. It does mean we could get a state where the public key exists but is garbage, but this should be OK because the private key won't exist and it would be regenerated.

The third sync can be removed and I can put one final sync at the end after all keys are generated to ensure we won't go through all the effort of regenerating the (last) key again in the event of interruption shortly after, unless you would prefer I didn't.

Quoted 16 lines

Quoted 16 lines

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -5

index 148e6ad..af56404 100644
@@ -1,22 +1,14 @@
  [Unit]
  Description=OpenSSH Key Generation
  RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key

Quoted 8 lines

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service line 11

Quoted 3 lines

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -3

-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key

Can you not continue to use ConditionPathExists to only run this unit if it needs to run? You can prepend the argument with | to make them logical OR instead of logical AND, if I'm reading this documentation correctly.

I will try that. I wasn't aware that was an option since systemd conf files are somewhat new to me.

Quoted 2 lines

Ross

Quoted 53 lines · Joshua Watt

Quoted 20 lines · Ross

Quoted 17 lines · Joshua Watt

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +15 -0

+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before renaming
+    sync
+
+    # Move (Atomically rename) files
+    # Rename the .pub file first, since the check that triggers a
+    # key generation is based on the private file.
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+    sync
+
+    mv -f "${NAME}.tmp" "${NAME}"
+    sync

Quoted 8 lines

Quoted 2 lines

+fi

All of these syncs seem quite enthusiastic, are they really needed? Writing
the file to a temporary name and then mving it to the real name should result in either no file or a complete file in the event of power loss, surely?

My understanding (and observation) of most journal file systems is that only metadata (i.e. directory entries and such) are journaled in typical usage. The first sync is necessary in this case to ensure that the actual file data gets put on the disk before we rename the files, otherwise in the event of interruption journaled rename might get replayed but have garbage data. The second one is more of a "force operation order" sync to make sure the public file is written before the private one, as a reordering would cause problems. The last sync is the most optional, but I've seen it take minutes for disk to sync contents if no one calls "sync", so it is very possible that all our work of regenerating keys would be for naught if power is interrupted in the meantime.

I think some of these syncs can be removed. Namely, the first and third one. The second one needs to be there, but can server double duty of syncing data to disk and enforcing the order between the public and private rename. It does mean we could get a state where the public key exists but is garbage, but this should be OK because the private key won't exist and it would be regenerated.

Quoted 5 lines

The third sync can be removed and I can put one final sync at the end after all keys are generated to ensure we won't go through all the effort of regenerating the (last) key again in the event of interruption shortly after, unless you would prefer I didn't.

The typical convention for this is,

1. Update file data.
2. sync file
3. sync containing directory
4. mv file to new location
5. sync directory containing new file (although I've seen this step left out before)

https://lwn.net/Articles/457672/ is a good example which is linked from
https://lwn.net/Articles/457667/

But one of the important parts vs your code, is also that the example is only calling sync on the files/directory needed, vs calling "sync" with no arguments which is going to cause all data in all filesystem caches to be flushed.

Quoted 37 lines

Quoted 25 lines

Quoted 12 lines

diff --git a/meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service b/meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service index 148e6ad..af56404 100644

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -0

@@ -1,22 +1,14 @@
  [Unit]

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service line 11

Quoted 10 lines

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -8

  Description=OpenSSH Key Generation
  RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key

Quoted 5 lines

Can you not continue to use ConditionPathExists to only run this unit if it
needs to run? You can prepend the argument with | to make them logical OR
instead of logical AND, if I'm reading this documentation correctly.

I will try that. I wasn't aware that was an option since systemd conf files are somewhat new to me.

Quoted 2 lines

Ross

Quoted 72 lines · Randy Witt

Quoted 53 lines · Joshua Watt

Quoted 20 lines · Ross

Quoted 17 lines · Joshua Watt

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +15 -0

+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before renaming
+    sync
+
+    # Move (Atomically rename) files
+    # Rename the .pub file first, since the check that triggers a
+    # key generation is based on the private file.
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+    sync
+
+    mv -f "${NAME}.tmp" "${NAME}"
+    sync

Quoted 8 lines

Quoted 2 lines

+fi

All of these syncs seem quite enthusiastic, are they really needed? Writing
the file to a temporary name and then mving it to the real name should result in either no file or a complete file in the event of power loss, surely?

My understanding (and observation) of most journal file systems is that only metadata (i.e. directory entries and such) are journaled in typical usage. The first sync is necessary in this case to ensure that the actual file data gets put on the disk before we rename the files, otherwise in the event of interruption journaled rename might get replayed but have garbage data. The second one is more of a "force operation order" sync to make sure the public file is written before the private one, as a reordering would cause problems. The last sync is the most optional, but I've seen it take minutes for disk to sync contents if no one calls "sync", so it is very possible that all our work of regenerating keys would be for naught if power is interrupted in the meantime.

I think some of these syncs can be removed. Namely, the first and third one. The second one needs to be there, but can server double duty of syncing data to disk and enforcing the order between the public and private rename. It does mean we could get a state where the public key exists but is garbage, but this should be OK because the private key won't exist and it would be regenerated.

Quoted 5 lines

The third sync can be removed and I can put one final sync at the end after all keys are generated to ensure we won't go through all the effort of regenerating the (last) key again in the event of interruption shortly after, unless you would prefer I didn't.

The typical convention for this is,

1. Update file data.
2. sync file
3. sync containing directory
4. mv file to new location
5. sync directory containing new file (although I've seen this step left out before)

https://lwn.net/Articles/457672/ is a good example which is linked from
https://lwn.net/Articles/457667/

Quoted 4 lines

But one of the important parts vs your code, is also that the example is only calling sync on the files/directory needed, vs calling "sync" with no arguments which is going to cause all data in all filesystem caches to be flushed.

Ah, OK. That makes sense, I will update sync to specify the files/directory explicitly.

Quoted 40 lines

Quoted 38 lines

Quoted 26 lines

Quoted 13 lines

diff --git a/meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service b/meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service index 148e6ad..af56404 100644

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -0

@@ -1,22 +1,14 @@
  [Unit]

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service line 11

Quoted 10 lines

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -8

  Description=OpenSSH Key Generation
  RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key

Quoted 5 lines

Can you not continue to use ConditionPathExists to only run this unit if it
needs to run? You can prepend the argument with | to make them logical OR
instead of logical AND, if I'm reading this documentation correctly.

I will try that. I wasn't aware that was an option since systemd conf files are somewhat new to me.

Quoted 2 lines

Ross

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Joshua Watt
Sent: den 23 maj 2017 21:57
To: Randy Witt <[email protected]>
Cc: OE-core <[email protected]>
Subject: Re: [OE-core] [PATCH] openssh: Atomically generate host keys

Quoted 5 lines · Joshua Watt

Quoted 3 lines · Randy Witt

Quoted 2 lines · Joshua Watt

On Tue, May 23, 2017 at 9:37 AM, Burton, Ross

<[email protected]>

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines · wrote:

meta/recipes-connectivity/openssh/openssh/sshd-check-key

Quoted 7 lines · Joshua Watt

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +5 -0

+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before

renaming

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

meta/recipes-connectivity/openssh/openssh/sshd-check-key

Quoted 4 lines

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +4 -0

+    sync
+
+    # Move (Atomically rename) files
+    # Rename the .pub file first, since the check that triggers

a

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

meta/recipes-connectivity/openssh/openssh/sshd-check-key line 27

Quoted 8 lines

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +7 -0

+    # key generation is based on the private file.
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+    sync
+
+    mv -f "${NAME}.tmp" "${NAME}"
+    sync
+fi

All of these syncs seem quite enthusiastic, are they really

needed?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Writing
the file to a temporary name and then mving it to the real name

should

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

result in either no file or a complete file in the event of power

loss,

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

surely?

My understanding (and observation) of most journal file systems is that only metadata (i.e. directory entries and such) are journaled

in

Quoted 1 line

Quoted 1 line

Quoted 1 line

typical usage. The first sync is necessary in this case to ensure

that

Quoted 1 line

Quoted 1 line

Quoted 1 line

the actual file data gets put on the disk before we rename the

files,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

otherwise in the event of interruption journaled rename might get replayed but have garbage data. The second one is more of a "force operation order" sync to make sure the public file is written

before

Quoted 1 line

Quoted 1 line

Quoted 1 line

the private one, as a reordering would cause problems. The last

sync

Quoted 1 line

Quoted 1 line

Quoted 1 line

is the most optional, but I've seen it take minutes for disk to

sync

Quoted 1 line

Quoted 1 line

Quoted 1 line

contents if no one calls "sync", so it is very possible that all

our

Quoted 1 line

Quoted 1 line

Quoted 1 line

work of regenerating keys would be for naught if power is

interrupted

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

in the meantime.

I think some of these syncs can be removed. Namely, the first and third one. The second one needs to be there, but can server double duty of syncing data to disk and enforcing the order between the public and private rename. It does mean we could get a state where

the

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

public key exists but is garbage, but this should be OK because the private key won't exist and it would be regenerated.

The third sync can be removed and I can put one final sync at the

end

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

after all keys are generated to ensure we won't go through all the effort of regenerating the (last) key again in the event of interruption shortly after, unless you would prefer I didn't.

Quoted 7 lines

Quoted 7 lines

The typical convention for this is,

1. Update file data.
2. sync file
3. sync containing directory
4. mv file to new location
5. sync directory containing new file (although I've seen this step

left out

Quoted 3 lines

Quoted 3 lines

before)

https://lwn.net/Articles/457672/ is a good example which is linked

from

Quoted 3 lines

Quoted 3 lines

https://lwn.net/Articles/457667/

But one of the important parts vs your code, is also that the

example is

Quoted 1 line

Quoted 1 line

only calling sync on the files/directory needed, vs calling "sync"

with no

Quoted 1 line

Quoted 1 line

arguments which is going to cause all data in all filesystem caches

to be

Quoted 4 lines

Quoted 1 line

flushed.

Ah, OK. That makes sense, I will update sync to specify the files/directory explicitly.

FWIW, I did some tests on the sync behavior:

It appears that older versions of the sync command ignore any arguments and just call sync(). From Ubuntu 14.04:

$ strace sync foo
...
write(2, "sync: ", 6sync: )                   = 6
write(2, "ignoring all arguments", 22)  = 22
write(2, "\n", 1)                       = 1
sync()                                  = 0
...

The same is true for the (default) busybox version of sync on master (again verified with strace), but it doesn't complain nosily about it.

I looked at the coreutils code, and sync was changed to respect arguments in January of 2015 (8b2bf5295f353016d4f5e6a2317d55b6a8e7fd00) and only call fsync() on the provided arguments (if any are provided).

Either way, adding the arguments to the sync call in my patch won't hurt because it is forward compatible, even though it won't be optimally efficient currently because the sync command currently simply calls sync()

Quoted 126 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Joshua Watt
Sent: den 23 maj 2017 21:57
To: Randy Witt <[email protected]>
Cc: OE-core <[email protected]>
Subject: Re: [OE-core] [PATCH] openssh: Atomically generate host keys

Quoted 5 lines · Joshua Watt

Quoted 3 lines · Randy Witt

Quoted 2 lines · Joshua Watt

On Tue, May 23, 2017 at 9:37 AM, Burton, Ross

<[email protected]>

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines · wrote:

Review

Quoted 7 lines · Joshua Watt

(quoted diff) +5 -0

+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before

renaming

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+    sync
+
+    # Move (Atomically rename) files
+    # Rename the .pub file first, since the check that triggers

a

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Review

Quoted 8 lines

(quoted diff) +7 -0

+    # key generation is based on the private file.
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+    sync
+
+    mv -f "${NAME}.tmp" "${NAME}"
+    sync
+fi

All of these syncs seem quite enthusiastic, are they really

needed?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Writing
the file to a temporary name and then mving it to the real name

should

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

result in either no file or a complete file in the event of power

loss,

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

surely?

My understanding (and observation) of most journal file systems is that only metadata (i.e. directory entries and such) are journaled

in

Quoted 1 line

Quoted 1 line

Quoted 1 line

typical usage. The first sync is necessary in this case to ensure

that

Quoted 1 line

Quoted 1 line

Quoted 1 line

the actual file data gets put on the disk before we rename the

files,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

otherwise in the event of interruption journaled rename might get replayed but have garbage data. The second one is more of a "force operation order" sync to make sure the public file is written

before

Quoted 1 line

Quoted 1 line

Quoted 1 line

the private one, as a reordering would cause problems. The last

sync

Quoted 1 line

Quoted 1 line

Quoted 1 line

is the most optional, but I've seen it take minutes for disk to

sync

Quoted 1 line

Quoted 1 line

Quoted 1 line

contents if no one calls "sync", so it is very possible that all

our

Quoted 1 line

Quoted 1 line

Quoted 1 line

work of regenerating keys would be for naught if power is

interrupted

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

in the meantime.

I think some of these syncs can be removed. Namely, the first and third one. The second one needs to be there, but can server double duty of syncing data to disk and enforcing the order between the public and private rename. It does mean we could get a state where

the

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

public key exists but is garbage, but this should be OK because the private key won't exist and it would be regenerated.

The third sync can be removed and I can put one final sync at the

end

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

after all keys are generated to ensure we won't go through all the effort of regenerating the (last) key again in the event of interruption shortly after, unless you would prefer I didn't.

Quoted 7 lines

Quoted 7 lines

The typical convention for this is,

1. Update file data.
2. sync file
3. sync containing directory
4. mv file to new location
5. sync directory containing new file (although I've seen this step

left out

Quoted 3 lines

Quoted 3 lines

before)

https://lwn.net/Articles/457672/ is a good example which is linked

from

Quoted 3 lines

Quoted 3 lines

https://lwn.net/Articles/457667/

But one of the important parts vs your code, is also that the

example is

Quoted 1 line

Quoted 1 line

only calling sync on the files/directory needed, vs calling "sync"

with no

Quoted 1 line

Quoted 1 line

arguments which is going to cause all data in all filesystem caches

to be

Quoted 4 lines

Quoted 1 line

flushed.

Ah, OK. That makes sense, I will update sync to specify the files/directory explicitly.

FWIW, I did some tests on the sync behavior:

It appears that older versions of the sync command ignore any arguments and just call sync(). From Ubuntu 14.04:

$ strace sync foo
...
write(2, "sync: ", 6sync: )                   = 6
write(2, "ignoring all arguments", 22)  = 22
write(2, "\n", 1)                       = 1
sync()                                  = 0
...

Quoted 2 lines

The same is true for the (default) busybox version of sync on master (again verified with strace), but it doesn't complain nosily about it.

Busybox has a separate fsync applet to do file synchronization, but it is not enabled in the default OE-core configuration...

Quoted 10 lines

I looked at the coreutils code, and sync was changed to respect arguments in January of 2015 (8b2bf5295f353016d4f5e6a2317d55b6a8e7fd00) and only call fsync() on the provided arguments (if any are provided).

Either way, adding the arguments to the sync call in my patch won't hurt because it is forward compatible, even though it won't be optimally efficient currently because the sync command currently simply calls sync()

//Peter

Generating the host keys atomically prevents power interruptions during the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

---
  meta/recipes-connectivity/openssh/openssh/init     | 21 +++----------
  .../openssh/openssh/sshd-check-key                 | 36 ++++++++++++++++++++++
  .../openssh/openssh/sshdgenkeys.service            | 24 +++++++--------
  meta/recipes-connectivity/openssh/openssh_7.5p1.bb |  8 +++++
  4 files changed, 60 insertions(+), 29 deletions(-)

create mode 100644 meta/recipes-connectivity/openssh/openssh/sshd-check-key

meta/recipes-connectivity/openssh/openssh/init +4 -17

index 1f63725..22124a9 100644
@@ -45,23 +45,10 @@ check_config() {
  }
 
  check_keys() {
-	# create keys if necessary
-	if [ ! -f $HOST_KEY_RSA ]; then
-		echo "  generating ssh RSA key..."
-		ssh-keygen -q -f $HOST_KEY_RSA -N '' -t rsa
-	fi
-	if [ ! -f $HOST_KEY_ECDSA ]; then
-		echo "  generating ssh ECDSA key..."
-		ssh-keygen -q -f $HOST_KEY_ECDSA -N '' -t ecdsa
-	fi
-	if [ ! -f $HOST_KEY_DSA ]; then
-		echo "  generating ssh DSA key..."
-		ssh-keygen -q -f $HOST_KEY_DSA -N '' -t dsa
-	fi
-	if [ ! -f $HOST_KEY_ED25519 ]; then
-		echo "  generating ssh ED25519 key..."
-		ssh-keygen -q -f $HOST_KEY_ED25519 -N '' -t ed25519
-	fi
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_RSA rsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ECDSA ecdsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_DSA dsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ED25519 ed25519
  }
 
  export PATH="${PATH:+$PATH:}/usr/sbin:/sbin"

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +24 -0

new file mode 100644
index 0000000..d2613af
@@ -0,0 +1,36 @@
+#! /bin/sh
+set -e
+
+NAME="$1"
+TYPE="$2"
+
+if [ -z "$NAME" ] || [ -z "$TYPE" ]; then
+    echo "Usage: $0 NAME TYPE"
+    exit 1;
+fi
+
+DIR="$(dirname "$NAME")"
+
+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Move (Atomically rename) files
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+
+    # This sync does double duty: Ensuring that the data in the temporary
+    # private key file is on disk before the rename, and ensuring that the
+    # public key rename is completed before the private key rename, since we
+    # switch on the existence of the private key to trigger key generation.

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +2 -0

+fi
+

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +12 -12

index 148e6ad..5d08b53 100644
@@ -1,22 +1,22 @@
  [Unit]
  Description=OpenSSH Key Generation
  RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/etc/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ed25519_key
 
  [Service]
  Environment="SYSCONFDIR=/etc/ssh"
  EnvironmentFile=-/etc/default/ssh
  ExecStart=@BASE_BINDIR@/mkdir -p $SYSCONFDIR
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_rsa_key -N '' -t rsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_dsa_key -N '' -t dsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ecdsa_key -N '' -t ecdsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ed25519_key -N '' -t ed25519
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_rsa_key rsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_dsa_key dsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ecdsa_key ecdsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ed25519_key ed25519
  Type=oneshot
  RemainAfterExit=yes

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +8 -0

index 5b96745..ede8823 100644
@@ -25,6 +25,7 @@ SRC_URI = "http://ftp.openbsd.org/pub/OpenBSD/OpenSSH/portable/openssh-${PV}.tar
             file://openssh-7.1p1-conditional-compile-des-in-cipher.patch \
             file://openssh-7.1p1-conditional-compile-des-in-pkcs11.patch \
             file://fix-potential-signed-overflow-in-pointer-arithmatic.patch \
+           file://sshd-check-key \
             "
 
  PAM_SRC_URI = "file://sshd"

@@ -124,7 +125,14 @@ do_install_append () {
  	sed -i -e 's,@BASE_BINDIR@,${base_bindir},g' \
  		-e 's,@SBINDIR@,${sbindir},g' \
  		-e 's,@BINDIR@,${bindir},g' \
+		-e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
  		${D}${systemd_unitdir}/system/sshd.socket ${D}${systemd_unitdir}/system/*.service
+
+	sed -i -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
+		${D}${sysconfdir}/init.d/sshd
+
+	install -d ${D}${libexecdir}/${BPN}
+	install -m 0755 ${WORKDIR}/sshd-check-key ${D}${libexecdir}/${BPN}
  }
 
  do_install_ptest () {

Generating the host keys atomically prevents power interruptions during the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

---
 meta/recipes-connectivity/openssh/openssh/init     | 22 +++----------
 .../openssh/openssh/sshd-check-key                 | 36 ++++++++++++++++++++++
 .../openssh/openssh/sshdgenkeys.service            | 25 +++++++--------
 meta/recipes-connectivity/openssh/openssh_7.5p1.bb |  9 ++++++
 4 files changed, 63 insertions(+), 29 deletions(-)
 create mode 100644 meta/recipes-connectivity/openssh/openssh/sshd-check-key

meta/recipes-connectivity/openssh/openssh/init +5 -17

index 1f63725..e02c479 100644
@@ -45,23 +45,11 @@ check_config() {
 }
 
 check_keys() {
-       # create keys if necessary
-       if [ ! -f $HOST_KEY_RSA ]; then
-               echo "  generating ssh RSA key..."
-               ssh-keygen -q -f $HOST_KEY_RSA -N '' -t rsa
-       fi
-       if [ ! -f $HOST_KEY_ECDSA ]; then
-               echo "  generating ssh ECDSA key..."
-               ssh-keygen -q -f $HOST_KEY_ECDSA -N '' -t ecdsa
-       fi
-       if [ ! -f $HOST_KEY_DSA ]; then
-               echo "  generating ssh DSA key..."
-               ssh-keygen -q -f $HOST_KEY_DSA -N '' -t dsa
-       fi
-       if [ ! -f $HOST_KEY_ED25519 ]; then
-               echo "  generating ssh ED25519 key..."
-               ssh-keygen -q -f $HOST_KEY_ED25519 -N '' -t ed25519
-       fi
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_RSA rsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ECDSA ecdsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_DSA dsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ED25519 ed25519
+    @BASE_BINDIR@/sync
 }
 
 export PATH="${PATH:+$PATH:}/usr/sbin:/sbin"

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +0 -0

new file mode 100644
index 0000000..d2613af

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +11 -0

+if [ ! -f "$NAME" ]; then
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Move (Atomically rename) files
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+
+    # This sync does double duty: Ensuring that the data in the temporary
+    # private key file is on disk before the rename, and ensuring that the
+    # public key rename is completed before the private key rename, since we
+    # switch on the existence of the private key to trigger key generation.

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +2 -0

+fi
+

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +13 -12

index 148e6ad..23fd351 100644
@@ -1,22 +1,23 @@
 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/etc/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ed25519_key
 
 [Service]
 Environment="SYSCONFDIR=/etc/ssh"
 EnvironmentFile=-/etc/default/ssh
 ExecStart=@BASE_BINDIR@/mkdir -p $SYSCONFDIR
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_rsa_key -N '' -t rsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_dsa_key -N '' -t dsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ecdsa_key -N '' -t ecdsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ed25519_key -N '' -t ed25519
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_rsa_key rsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_dsa_key dsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ecdsa_key ecdsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ed25519_key ed25519
+ExecStart=@BASE_BINDIR@/sync
 Type=oneshot
 RemainAfterExit=yes

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +9 -1

index 5b96745..c1fcda4 100644
@@ -25,6 +25,7 @@ SRC_URI = "http://ftp.openbsd.org/pub/OpenBSD/OpenSSH/portable/openssh-${PV}.tar
            file://openssh-7.1p1-conditional-compile-des-in-cipher.patch \
            file://openssh-7.1p1-conditional-compile-des-in-pkcs11.patch \
            file://fix-potential-signed-overflow-in-pointer-arithmatic.patch \
+           file://sshd-check-key \
            "
 
 PAM_SRC_URI = "file://sshd"

@@ -124,7 +125,15 @@ do_install_append () {
        sed -i -e 's,@BASE_BINDIR@,${base_bindir},g' \
                -e 's,@SBINDIR@,${sbindir},g' \
                -e 's,@BINDIR@,${bindir},g' \
+               -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
                ${D}${systemd_unitdir}/system/sshd.socket ${D}${systemd_unitdir}/system/*.service
+
+       sed -i -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
+               -e 's,@BASE_BINDIR@,${base_bindir},g' \
+               ${D}${sysconfdir}/init.d/sshd
+
+       install -d ${D}${libexecdir}/${BPN}
+       install -m 0755 ${WORKDIR}/sshd-check-key ${D}${libexecdir}/${BPN}
 }
 
 do_install_ptest () {
--

2.9.3

--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core

Generating the host keys atomically prevents power interruptions during the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

Just a minor comment: there is no need to include any prefix besides [PATCH vN]. Also, patches either go to oe-core or meta-oe, so it makes no sense to send the patch to the oe-core and using the [meta-oe] as prefix.

Leo

Quoted 3 lines · Joshua Watt

Generating the host keys atomically prevents power interruptions during the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

Generating the host keys atomically prevents power interruptions during the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

Signed-off-by Joshua Watt <[email protected]>

Generating the host keys atomically prevents power interruptions during the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

Signed-off-by Joshua Watt <[email protected]>

---
 meta/recipes-connectivity/openssh/openssh/init     | 22 ++++----------
 .../openssh/openssh/sshd-check-key                 | 35 ++++++++++++++++++++++
 .../openssh/openssh/sshdgenkeys.service            | 25 ++++++++--------
 meta/recipes-connectivity/openssh/openssh_7.5p1.bb |  8 +++++
 4 files changed, 61 insertions(+), 29 deletions(-)
 create mode 100644 meta/recipes-connectivity/openssh/openssh/sshd-check-key

meta/recipes-connectivity/openssh/openssh/init +5 -17

index 1f63725..e02c479 100644
@@ -45,23 +45,11 @@ check_config() {
 }
 
 check_keys() {
-       # create keys if necessary
-       if [ ! -f $HOST_KEY_RSA ]; then
-               echo "  generating ssh RSA key..."
-               ssh-keygen -q -f $HOST_KEY_RSA -N '' -t rsa
-       fi
-       if [ ! -f $HOST_KEY_ECDSA ]; then
-               echo "  generating ssh ECDSA key..."
-               ssh-keygen -q -f $HOST_KEY_ECDSA -N '' -t ecdsa
-       fi
-       if [ ! -f $HOST_KEY_DSA ]; then
-               echo "  generating ssh DSA key..."
-               ssh-keygen -q -f $HOST_KEY_DSA -N '' -t dsa
-       fi
-       if [ ! -f $HOST_KEY_ED25519 ]; then
-               echo "  generating ssh ED25519 key..."
-               ssh-keygen -q -f $HOST_KEY_ED25519 -N '' -t ed25519
-       fi
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_RSA rsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ECDSA ecdsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_DSA dsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ED25519 ed25519
+    @BASE_BINDIR@/sync
 }
 
 export PATH="${PATH:+$PATH:}/usr/sbin:/sbin"

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +35 -0

new file mode 100644
index 0000000..3afdb8b
@@ -0,0 +1,35 @@
+#! /bin/sh
+NAME="$1"
+TYPE="$2"
+
+if [ -z "$NAME" ] || [ -z "$TYPE" ]; then
+    echo "Usage: $0 NAME TYPE"
+    exit 1
+fi
+
+
+if [ ! -f "$NAME" ]; then
+    DIR="$(dirname "$NAME")"
+
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Move (Atomically rename) files
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+
+    # This sync does double duty: Ensuring that the data in the temporary
+    # private key file is on disk before the rename, and ensuring that the
+    # public key rename is completed before the private key rename, since we
+    # switch on the existence of the private key to trigger key generation.
+    # This does mean it is possible for the public key to exist, but be garbage
+    # but this is OK because in that case the private key won't exist and the
+    # keys will be regenerated.
+    #
+    # In the event that sync understands arguments that limit what it tries to
+    # fsync(), we provided them. If it does not, it will simply call sync()
+    # which is just as well
+    sync "${NAME}.pub" "$DIR" "${NAME}.tmp"
+
+    mv "${NAME}.tmp" "$NAME"
+fi
+

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +13 -12

index 148e6ad..23fd351 100644
@@ -1,22 +1,23 @@
 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/etc/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ed25519_key
 
 [Service]
 Environment="SYSCONFDIR=/etc/ssh"
 EnvironmentFile=-/etc/default/ssh
 ExecStart=@BASE_BINDIR@/mkdir -p $SYSCONFDIR
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_rsa_key -N '' -t rsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_dsa_key -N '' -t dsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ecdsa_key -N '' -t ecdsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ed25519_key -N '' -t ed25519
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_rsa_key rsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_dsa_key dsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ecdsa_key ecdsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ed25519_key ed25519
+ExecStart=@BASE_BINDIR@/sync
 Type=oneshot
 RemainAfterExit=yes

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +8 -1

index 5b96745..ec4b55f 100644
@@ -25,6 +25,7 @@ SRC_URI = "http://ftp.openbsd.org/pub/OpenBSD/OpenSSH/portable/openssh-${PV}.tar
            file://openssh-7.1p1-conditional-compile-des-in-cipher.patch \
            file://openssh-7.1p1-conditional-compile-des-in-pkcs11.patch \
            file://fix-potential-signed-overflow-in-pointer-arithmatic.patch \
+           file://sshd-check-key \
            "
 
 PAM_SRC_URI = "file://sshd"

@@ -124,7 +125,14 @@ do_install_append () {
        sed -i -e 's,@BASE_BINDIR@,${base_bindir},g' \
                -e 's,@SBINDIR@,${sbindir},g' \
                -e 's,@BINDIR@,${bindir},g' \
+               -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
                ${D}${systemd_unitdir}/system/sshd.socket ${D}${systemd_unitdir}/system/*.service
+
+       sed -i -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
+               -e 's,@BASE_BINDIR@,${base_bindir},g' \
+               ${D}${sysconfdir}/init.d/sshd
+
+       install -D -m 0755 ${WORKDIR}/sshd-check-key ${D}${libexecdir}/${BPN}
 }
 
 do_install_ptest () {
--

2.9.4

Quoted 185 lines · Joshua Watt

Generating the host keys atomically prevents power interruptions during
the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

Signed-off-by Joshua Watt <[email protected]>

---
 meta/recipes-connectivity/openssh/openssh/init     | 22 ++++----
------
 .../openssh/openssh/sshd-check-key                 | 35
++++++++++++++++++++++
 .../openssh/openssh/sshdgenkeys.service            | 25 ++++++++
--------
 meta/recipes-connectivity/openssh/openssh_7.5p1.bb |  8 +++++

 4 files changed, 61 insertions(+), 29 deletions(-)

 create mode 100644 meta/recipes-connectivity/openssh/openssh/sshd- check-key

meta/recipes-connectivity/openssh/openssh/init +5 -17

index 1f63725..e02c479 100644
@@ -45,23 +45,11 @@ check_config() {
  }
 
  check_keys() {
-       # create keys if necessary
-       if [ ! -f $HOST_KEY_RSA ]; then
-               echo "  generating ssh RSA key..."
-               ssh-keygen -q -f $HOST_KEY_RSA -N '' -t rsa
-       fi
-       if [ ! -f $HOST_KEY_ECDSA ]; then
-               echo "  generating ssh ECDSA key..."
-               ssh-keygen -q -f $HOST_KEY_ECDSA -N '' -t ecdsa
-       fi
-       if [ ! -f $HOST_KEY_DSA ]; then
-               echo "  generating ssh DSA key..."
-               ssh-keygen -q -f $HOST_KEY_DSA -N '' -t dsa
-       fi
-       if [ ! -f $HOST_KEY_ED25519 ]; then
-               echo "  generating ssh ED25519 key..."
-               ssh-keygen -q -f $HOST_KEY_ED25519 -N '' -t ed25519
-       fi
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_RSA rsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ECDSA ecdsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_DSA dsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ED25519 ed25519
+    @BASE_BINDIR@/sync
  }
 

 export PATH="${PATH:+$PATH:}/usr/sbin:/sbin"

(quoted diff) +0 -0

key b/meta/recipes-connectivity/openssh/openssh/sshd-check-key new file mode 100644
index 0000000..3afdb8b

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +20 -0

@@ -0,0 +1,35 @@
+#! /bin/sh
+NAME="$1"
+TYPE="$2"
+
+if [ -z "$NAME" ] || [ -z "$TYPE" ]; then
+    echo "Usage: $0 NAME TYPE"
+    exit 1
+fi
+
+
+if [ ! -f "$NAME" ]; then
+    DIR="$(dirname "$NAME")"
+
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Move (Atomically rename) files
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+
+    # This sync does double duty: Ensuring that the data in the

temporary
+    # private key file is on disk before the rename, and ensuring that the
+    # public key rename is completed before the private key rename, since we
+    # switch on the existence of the private key to trigger key generation.
+    # This does mean it is possible for the public key to exist, but be garbage
+    # but this is OK because in that case the private key won't exist and the
+    # keys will be regenerated.
+    #
+    # In the event that sync understands arguments that limit what it tries to
+    # fsync(), we provided them. If it does not, it will simply call sync()
+    # which is just as well
+    sync "${NAME}.pub" "$DIR" "${NAME}.tmp"

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +4 -0

+
+    mv "${NAME}.tmp" "$NAME"
+fi
+

(quoted diff) +0 -0

connectivity/openssh/openssh/sshdgenkeys.service b/meta/recipes- connectivity/openssh/openssh/sshdgenkeys.service
index 148e6ad..23fd351 100644

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -0

@@ -1,22 +1,23 @@

 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +8 -8

-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/etc/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ed25519_key

 [Service]
 Environment="SYSCONFDIR=/etc/ssh"
 EnvironmentFile=-/etc/default/ssh
 ExecStart=@BASE_BINDIR@/mkdir -p $SYSCONFDIR

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -5

-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_rsa_key
-N '' -t rsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_dsa_key
-N '' -t dsa
-ExecStart=@BINDIR@/ssh-keygen -q -f

${SYSCONFDIR}/ssh_host_ecdsa_key -N '' -t ecdsa -ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ed25519_key -N '' -t ed25519 +ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_rsa_key rsa +ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_dsa_key dsa +ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ecdsa_key ecdsa +ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ed25519_key ed25519 +ExecStart=@BASE_BINDIR@/sync
 Type=oneshot
 RemainAfterExit=yes

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +0 -0

index 5b96745..ec4b55f 100644
@@ -25,6 +25,7 @@ SRC_URI = "http://ftp.openbsd.org/pub/OpenBSD/Ope

nSSH/portable/openssh-${PV}.tar file://openssh-7.1p1-conditional-compile-des-in- cipher.patch \
            file://openssh-7.1p1-conditional-compile-des-in- pkcs11.patch \
            file://fix-potential-signed-overflow-in-pointer- arithmatic.patch \
+           file://sshd-check-key \
            "

 PAM_SRC_URI = "file://sshd"

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +1 -0

@@ -124,7 +125,14 @@ do_install_append () {
         sed -i -e 's,@BASE_BINDIR@,${base_bindir},g' \
                 -e 's,@SBINDIR@,${sbindir},g' \
                 -e 's,@BINDIR@,${bindir},g' \
+               -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \

                ${D}${systemd_unitdir}/system/sshd.socket ${D}${systemd_unitdir}/system/*.service

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +6 -0

+
+       sed -i -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
+               -e 's,@BASE_BINDIR@,${base_bindir},g' \
+               ${D}${sysconfdir}/init.d/sshd
+
+       install -D -m 0755 ${WORKDIR}/sshd-check-key

${D}${libexecdir}/${BPN}
 }

Quoted 4 lines

 do_install_ptest () {
--
2.9.4

Ping?

Quoted 185 lines · Joshua Watt

Generating the host keys atomically prevents power interruptions during
the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

Signed-off-by Joshua Watt <[email protected]>

---
 meta/recipes-connectivity/openssh/openssh/init     | 22 ++++----
------
 .../openssh/openssh/sshd-check-key                 | 35
++++++++++++++++++++++
 .../openssh/openssh/sshdgenkeys.service            | 25 ++++++++
--------
 meta/recipes-connectivity/openssh/openssh_7.5p1.bb |  8 +++++

 4 files changed, 61 insertions(+), 29 deletions(-)

 create mode 100644 meta/recipes-connectivity/openssh/openssh/sshd- check-key

meta/recipes-connectivity/openssh/openssh/init +5 -17

index 1f63725..e02c479 100644
@@ -45,23 +45,11 @@ check_config() {
  }
 
  check_keys() {
-       # create keys if necessary
-       if [ ! -f $HOST_KEY_RSA ]; then
-               echo "  generating ssh RSA key..."
-               ssh-keygen -q -f $HOST_KEY_RSA -N '' -t rsa
-       fi
-       if [ ! -f $HOST_KEY_ECDSA ]; then
-               echo "  generating ssh ECDSA key..."
-               ssh-keygen -q -f $HOST_KEY_ECDSA -N '' -t ecdsa
-       fi
-       if [ ! -f $HOST_KEY_DSA ]; then
-               echo "  generating ssh DSA key..."
-               ssh-keygen -q -f $HOST_KEY_DSA -N '' -t dsa
-       fi
-       if [ ! -f $HOST_KEY_ED25519 ]; then
-               echo "  generating ssh ED25519 key..."
-               ssh-keygen -q -f $HOST_KEY_ED25519 -N '' -t ed25519
-       fi
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_RSA rsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ECDSA ecdsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_DSA dsa
+    @LIBEXECDIR@/sshd-check-key $HOST_KEY_ED25519 ed25519
+    @BASE_BINDIR@/sync
  }
 

 export PATH="${PATH:+$PATH:}/usr/sbin:/sbin"

(quoted diff) +0 -0

key b/meta/recipes-connectivity/openssh/openssh/sshd-check-key new file mode 100644
index 0000000..3afdb8b

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +20 -0

@@ -0,0 +1,35 @@
+#! /bin/sh
+NAME="$1"
+TYPE="$2"
+
+if [ -z "$NAME" ] || [ -z "$TYPE" ]; then
+    echo "Usage: $0 NAME TYPE"
+    exit 1
+fi
+
+
+if [ ! -f "$NAME" ]; then
+    DIR="$(dirname "$NAME")"
+
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Move (Atomically rename) files
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+
+    # This sync does double duty: Ensuring that the data in the

temporary
+    # private key file is on disk before the rename, and ensuring that the
+    # public key rename is completed before the private key rename, since we
+    # switch on the existence of the private key to trigger key generation.
+    # This does mean it is possible for the public key to exist, but be garbage
+    # but this is OK because in that case the private key won't exist and the
+    # keys will be regenerated.
+    #
+    # In the event that sync understands arguments that limit what it tries to
+    # fsync(), we provided them. If it does not, it will simply call sync()
+    # which is just as well
+    sync "${NAME}.pub" "$DIR" "${NAME}.tmp"

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +4 -0

+
+    mv "${NAME}.tmp" "$NAME"
+fi
+

(quoted diff) +0 -0

connectivity/openssh/openssh/sshdgenkeys.service b/meta/recipes- connectivity/openssh/openssh/sshdgenkeys.service
index 148e6ad..23fd351 100644

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -0

@@ -1,22 +1,23 @@

 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +8 -8

-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/var/run/ssh/ssh_host_ed25519_key
+ConditionPathExists=|!/etc/ssh/ssh_host_rsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_dsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ecdsa_key
+ConditionPathExists=|!/etc/ssh/ssh_host_ed25519_key

 [Service]
 Environment="SYSCONFDIR=/etc/ssh"
 EnvironmentFile=-/etc/default/ssh
 ExecStart=@BASE_BINDIR@/mkdir -p $SYSCONFDIR

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -5

-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_rsa_key
-N '' -t rsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_dsa_key
-N '' -t dsa
-ExecStart=@BINDIR@/ssh-keygen -q -f

${SYSCONFDIR}/ssh_host_ecdsa_key -N '' -t ecdsa -ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ed25519_key -N '' -t ed25519 +ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_rsa_key rsa +ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_dsa_key dsa +ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ecdsa_key ecdsa +ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ed25519_key ed25519 +ExecStart=@BASE_BINDIR@/sync
 Type=oneshot
 RemainAfterExit=yes

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +0 -0

index 5b96745..ec4b55f 100644
@@ -25,6 +25,7 @@ SRC_URI = "http://ftp.openbsd.org/pub/OpenBSD/Ope

nSSH/portable/openssh-${PV}.tar file://openssh-7.1p1-conditional-compile-des-in- cipher.patch \
            file://openssh-7.1p1-conditional-compile-des-in- pkcs11.patch \
            file://fix-potential-signed-overflow-in-pointer- arithmatic.patch \
+           file://sshd-check-key \
            "

 PAM_SRC_URI = "file://sshd"

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +1 -0

@@ -124,7 +125,14 @@ do_install_append () {
         sed -i -e 's,@BASE_BINDIR@,${base_bindir},g' \
                 -e 's,@SBINDIR@,${sbindir},g' \
                 -e 's,@BINDIR@,${bindir},g' \
+               -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \

                ${D}${systemd_unitdir}/system/sshd.socket ${D}${systemd_unitdir}/system/*.service

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +6 -0

+
+       sed -i -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
+               -e 's,@BASE_BINDIR@,${base_bindir},g' \
+               ${D}${sysconfdir}/init.d/sshd
+
+       install -D -m 0755 ${WORKDIR}/sshd-check-key

${D}${libexecdir}/${BPN}
 }

Quoted 4 lines

 do_install_ptest () {
--
2.9.4

Ping?

Generating the host keys atomically prevents power interruptions during the first boot from leaving the key files incomplete, which often prevents users from being able to ssh into the device.

Signed-off-by Joshua Watt <[email protected]>

---
 meta/recipes-connectivity/openssh/openssh/init     | 24 +++--------------
 .../openssh/openssh/sshd-check-key                 | 30 ++++++++++++++++++++++
 .../openssh/openssh/sshdgenkeys.service            | 16 +++---------
 meta/recipes-connectivity/openssh/openssh_7.5p1.bb |  8 ++++++
 4 files changed, 46 insertions(+), 32 deletions(-)
 create mode 100644 meta/recipes-connectivity/openssh/openssh/sshd-check-key

meta/recipes-connectivity/openssh/openssh/init +4 -20

index 386628a..acb35c3 100644
@@ -80,26 +80,10 @@ check_keys() {
        [ -z "${HOST_KEY_ED25519}" ] && HOST_KEY_ED25519=$SYSCONFDIR/ssh_host_ed25519_key
 
        # create keys if necessary
-       if [ ! -f $HOST_KEY_RSA ]; then
-               echo "  generating ssh RSA key..."
-               mkdir -p $(dirname $HOST_KEY_RSA)
-               ssh-keygen -q -f $HOST_KEY_RSA -N '' -t rsa
-       fi
-       if [ ! -f $HOST_KEY_ECDSA ]; then
-               echo "  generating ssh ECDSA key..."
-               mkdir -p $(dirname $HOST_KEY_ECDSA)
-               ssh-keygen -q -f $HOST_KEY_ECDSA -N '' -t ecdsa
-       fi
-       if [ ! -f $HOST_KEY_DSA ]; then
-               echo "  generating ssh DSA key..."
-               mkdir -p $(dirname $HOST_KEY_DSA)
-               ssh-keygen -q -f $HOST_KEY_DSA -N '' -t dsa
-       fi
-       if [ ! -f $HOST_KEY_ED25519 ]; then
-               echo "  generating ssh ED25519 key..."
-               mkdir -p $(dirname $HOST_KEY_ED25519)
-               ssh-keygen -q -f $HOST_KEY_ED25519 -N '' -t ed25519
-       fi
+       @LIBEXECDIR@/sshd-check-key $HOST_KEY_RSA rsa
+       @LIBEXECDIR@/sshd-check-key $HOST_KEY_ECDSA ecdsa
+       @LIBEXECDIR@/sshd-check-key $HOST_KEY_DSA dsa
+       @LIBEXECDIR@/sshd-check-key $HOST_KEY_ED25519 ed25519
 }
 
 export PATH="${PATH:+$PATH:}/usr/sbin:/sbin"

meta/recipes-connectivity/openssh/openssh/sshd-check-key new file +30 -0

new file mode 100644
index 0000000..4999af2
@@ -0,0 +1,30 @@
+#! /bin/sh
+set -e
+
+NAME="$1"
+TYPE="$2"
+
+if [ -z "$NAME" ] || [ -z "$TYPE" ]; then
+    echo "Usage: $0 NAME TYPE"
+    exit 1;
+fi
+
+if [ ! -f "$NAME" ]; then
+    mkdir -p "$(dirname "$NAME")"
+
+    echo "  generating ssh $TYPE key..."
+    ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE
+
+    # Sync to ensure data is written to temp file before renaming
+    sync
+
+    # Move (Atomically rename) files
+    # Rename the .pub file first, since the check that triggers a
+    # key generation is based on the private file.
+    mv -f "${NAME}.tmp.pub" "${NAME}.pub"
+    sync
+
+    mv -f "${NAME}.tmp" "${NAME}"
+    sync
+fi
+

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +4 -12

index 148e6ad..af56404 100644
@@ -1,22 +1,14 @@
 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key
 
 [Service]
 Environment="SYSCONFDIR=/etc/ssh"
 EnvironmentFile=-/etc/default/ssh
 ExecStart=@BASE_BINDIR@/mkdir -p $SYSCONFDIR
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_rsa_key -N '' -t rsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_dsa_key -N '' -t dsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ecdsa_key -N '' -t ecdsa
-ExecStart=@BINDIR@/ssh-keygen -q -f ${SYSCONFDIR}/ssh_host_ed25519_key -N '' -t ed25519
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_rsa_key rsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_dsa_key dsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ecdsa_key ecdsa
+ExecStart=@LIBEXECDIR@/sshd-check-key ${SYSCONFDIR}/ssh_host_ed25519_key ed25519
 Type=oneshot
 RemainAfterExit=yes

meta/recipes-connectivity/openssh/openssh_7.5p1.bb +8 -1

index 5b96745..ede8823 100644
@@ -25,6 +25,7 @@ SRC_URI = "http://ftp.openbsd.org/pub/OpenBSD/OpenSSH/portable/openssh-${PV}.tar
            file://openssh-7.1p1-conditional-compile-des-in-cipher.patch \
            file://openssh-7.1p1-conditional-compile-des-in-pkcs11.patch \
            file://fix-potential-signed-overflow-in-pointer-arithmatic.patch \
+           file://sshd-check-key \
            "
 
 PAM_SRC_URI = "file://sshd"

@@ -124,7 +125,14 @@ do_install_append () {
        sed -i -e 's,@BASE_BINDIR@,${base_bindir},g' \
                -e 's,@SBINDIR@,${sbindir},g' \
                -e 's,@BINDIR@,${bindir},g' \
+               -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
                ${D}${systemd_unitdir}/system/sshd.socket ${D}${systemd_unitdir}/system/*.service
+
+       sed -i -e 's,@LIBEXECDIR@,${libexecdir}/${BPN},g' \
+               ${D}${sysconfdir}/init.d/sshd
+
+       install -d ${D}${libexecdir}/${BPN}
+       install -m 0755 ${WORKDIR}/sshd-check-key ${D}${libexecdir}/${BPN}
 }
 
 do_install_ptest () {
--

2.9.4

Quoted 14 lines · Joshua Watt

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -5

index 148e6ad..af56404 100644
@@ -1,22 +1,14 @@
 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run
-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service line 4

Quoted 4 lines

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -3

-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key
 

Can you not continue to use ConditionPathExists to only run this unit if it needs to run? You can prepend the argument with | to make them logical OR instead of logical AND, if I'm reading this documentation correctly.

Quoted 21 lines · Ross

Quoted 20 lines · Joshua Watt

(quoted diff) +0 -0

connectivity/openssh/openssh/sshdgenkeys.service b/meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service index 148e6ad..af56404 100644
--- a/meta/recipes- connectivity/openssh/openssh/sshdgenkeys.service
+++ b/meta/recipes- connectivity/openssh/openssh/sshdgenkeys.service

(quoted diff) +0 -0

@@ -1,22 +1,14 @@

 [Unit]
 Description=OpenSSH Key Generation
 RequiresMountsFor=/var /run

meta/recipes-connectivity/openssh/openssh/sshdgenkeys.service +0 -8

-ConditionPathExists=!/var/run/ssh/ssh_host_rsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_dsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/var/run/ssh/ssh_host_ed25519_key
-ConditionPathExists=!/etc/ssh/ssh_host_rsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_dsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ecdsa_key
-ConditionPathExists=!/etc/ssh/ssh_host_ed25519_key

Quoted 8 lines

Quoted 1 line

Can you not continue to use ConditionPathExists to only run this unit if it
needs to run?  You can prepend the argument with | to make them logical OR
instead of logical AND, if I'm reading this documentation correctly.

Am I right that if we have a read-write mounted root-FS with already existing
keys in /etc/ssh the service unit will nevertheless be started on _every_ boot
now as the files which are checked for existance in /var/run/ssh are missing?

Best regards
Ulrich
--
Pengutronix
e.K.                           |                             | Industrial Linux Solutions                 | http://www.pengutronix.d
e/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917- 0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917- 5555 |