[OE-core] [PATCH] openssh: Atomically generate host keys

[Joshua Watt]

On Tue, May 23, 2017 at 9:37 AM, Burton, Ross <ross.burton at intel.com> wrote:
>
> On 7 May 2017 at 02:33, Joshua Watt <jpewhacker at gmail.com> wrote:
>>
>> +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
>>
>
> 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.

>
>>
>> diff --git a/meta/recipes-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
>> @@ -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
>
>
> 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.

>
> Ross