Re: [PATCH] openssh: Atomically generate host keys

[Randy Witt <randy.e.witt@linux.intel.com>]

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

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.

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