On Tue, May 23, 2017 at 12:23 PM, Randy Witt
<randy.e.w...@linux.intel.com> wrote:
> On 05/23/2017 08:29 AM, Joshua Watt wrote:
>>
>> On Tue, May 23, 2017 at 9:37 AM, Burton, Ross <ross.bur...@intel.com>
>> wrote:
>>>
>>>
>>> On 7 May 2017 at 02:33, Joshua Watt <jpewhac...@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.
Ah, OK. That makes sense, I will update sync to specify the
files/directory explicitly.
>
>
>>>
>>>>
>>>> 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
>
>
--
_______________________________________________
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core
