In case something goes wrong during host keys generation and there are zero-sized files which should contain keys left in the filesystem, ssh-keygen run with -A option is not trying to re-generate those keys. As a consequence sshd daemon is unable to start because of corrupted keys and users cannot access the machine remotely through ssh. We have observed lots of such situations during large-scale deployments. The root cause for corrupting keys is yet to be determined, but the ssh-keygen -A should take care of re-generating evidently broken keys anyway. Simple fix would be to check not only for key file existence, but also for it's size, and re-generate the key if it does not exist or it exists but the file size is equal to zero. The best approach would be to not only detect existence and size, but also verifying if the key is not corrupted.
Created attachment 2804 [details] check that key files are loadable in ssh-keygen -A This patch checks that public and private key files can be loaded and will re-generate keys if they can't. Changes to authfile.c/sshkey.c required to allow running sshkey_load_public()/sshkey_load_private_type() with a NULL key argument.
Comment on attachment 2804 [details] check that key files are loadable in ssh-keygen -A I am not sure this is a good idea. It has the possibility of overwriting the private key if it can't be loaded due to any of: file system problem, ssh-keygen bugs, encrypted private keys, bugs in libcraypto.
Created attachment 2805 [details] overwrite zero-length key files, move keys into place atomically Darren correctly points out that the previous diff could clobber valid keys under some circumstances. This diff is much less aggressive: it only overwrites zero-length private keys, and uses rename() to atomically move finished private keys into position so there should be fewer circumstances under which it leaves partial keys at valid keys path names.
retarget unfinished bugs to next release
OpenSSH 7.4 release is closing; punt the bugs to 7.5
Move incomplete bugs to openssh-7.6 target since 7.5 shipped a while back. To calibrate expectations, there's little chance all of these are going to make 7.6.
remove 7.5 target
Created attachment 3003 [details] update to current Update to HEAD, remove appending '/' to path since the paths we later append start with '/' anyway.
Comment on attachment 3003 [details] update to current >+ public = private = NULL; >+ xasprintf(&prv_tmp, "%s%s.XXXXXXXXXX", >+ identity_file, key_types[i].path); >+ xasprintf(&pub_tmp, "%s%s.pub.XXXXXXXXXX", >+ identity_file, key_types[i].path); >+ xasprintf(&prv_file, "%s%s", >+ identity_file, key_types[i].path); >+ xasprintf(&pub_file, "%s%s.pub", >+ identity_file, key_types[i].path); >+ >+ if (stat(prv_file, &st) == 0) { >+ if (st.st_size != 0) >+ continue; This will leak prv_tmp and friends in the case where the key already exists. The next iteration of the loop will allocate them again, leaking the first.
Created attachment 3004 [details] revised diff incorporating feedback: skip good keys early, NULL filenames to avoid risk of double-free, check fwrite/fclose errors
Created attachment 3009 [details] fix double-fclose
Patch applied - this will be in OpenSSH 7.6
Close all resolved bugs after release of OpenSSH 7.7.