#2422 PR `merged`: Also parse SSH Include files in 501_check_ssh_keys.sh¶

Labels: enhancement, cleanup, fixed / solved / done

jsmeix opened issue at 2020-06-08 11:53:¶

Type: Enhancement Cleanup
Impact: Normal
At most "normal" impact because the
SSH_UNPROTECTED_PRIVATE_KEYS
functionality is only meant to work to some usual extent
but not for each and every special cases,
see its description in default.conf

# But SSH_UNPROTECTED_PRIVATE_KEYS="no" is not at all any guarantee
# to not have any kind of unprotected SSH key in the recovery system.
# In general to check if there are unwanted files in the recovery system
# use KEEP_BUILD_DIR="yes" and inspect the recovery system content
# in $TMPDIR/rear.XXXXXXXXXXXXXXX/rootfs/ and use COPY_AS_IS_EXCLUDE
# to exclude unwanted files from the recovery system.
SSH_UNPROTECTED_PRIVATE_KEYS='no'

Reference to related issue (URL):
https://github.com/rear/rear/issues/2421
How was this pull request tested?
A quick "rear mkrescue" test on my openSUSE Leap 15.1 system
with this in /etc/ssh/ssh_config

#   IdentityFile ~/.ssh/id_rsa
IdentityFile    ~/.ssh/id_dsa
 IdentityFile = ~/.ssh/id_ecdsa
  identityfile "~/.ssh/id_ed25519"

and that (dummy stuff) in /root/.ssh/config

#   IdentityFile ~/.ssh/id_rsaqq
IdentityFile    ~/.ssh/id_dsaqq
 IdentityFile = ~/.ssh/id_ecdsaqq
  identityfile "~/.ssh/id_ed25519qq"

Brief description of the changes in this pull request:

Overhauled how SSH config files are parsed for IdentityFile values
to find (and remove) unprotected SSH keys in the recovery system.
Now "find ./etc/ssh" ensures that SSH 'Include' config files
e.g. in /etc/ssh/ssh_config.d/ are also parsed

jsmeix commented at 2020-06-08 11:59:¶

@gdha @OliverO2
perhaps you could have a look here and try out if it also works for you.
If yes we may even merge that already into ReaR 2.6?

@gozora
by the way I think I also fixed a $ROOTFS_DIR omission
from before

... sed -n -e "s#~#root#g" ...

to now

... sed -e ... "s#~#$ROOT_HOME_DIR#g" ...

OliverO2 commented at 2020-06-08 13:25:¶

Tested successfully on Ubuntu 18.04.4 LTS (just a smoke test on a standard configuration, without extra configuration files and without unprotected private keys).

jsmeix commented at 2020-06-08 13:53:¶

I have tested
https://github.com/rear/rear/pull/2422/commits/4933de3c5e4a29f2bc519f465854cfa3d917f8cf
on the same openSUSE Leap 15.1 system as above
https://github.com/rear/rear/pull/2422#issue-431078521

Before and afterwards I get same SSH key files found and tested
(same excepts from my rear -D mkrescue logs):

# grep 'ssh-keygen -q -p -P' var/log/rear/rear-linux-h9wr.log
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/.//root/.ssh/id_dsa
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/.//root/.ssh/id_ecdsa
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/.//root/.ssh/id_ed25519
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/.//root/.ssh/id_rsa
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/root/.ssh/id_dsa
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/root/.ssh/id_ecdsa
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/root/.ssh/id_ed25519

The duplicate found files (/rootfs/.//root/ is the same as /rootfs/root/)
happen because of how my current sed code works and as a result
the sort -u in

 key_files=( $( echo "${host_key_files[@]}" "${root_key_files[@]}" "${host_identity_files[@]}" "${root_identity_files[@]}" | tr -s '[:space:]' '\n' | sort -u ) )

cannot detect them as duplicates.

I could unify the found file names by using an artificial ./ prefix

sed ... -e "s#~#./$ROOT_HOME_DIR#g"

instead of my currently more simple sed code

sed ... -e "s#~#$ROOT_HOME_DIR#g"

to make the sed result match the other code in

local root_key_files=( ... ./$ROOT_HOME_DIR/.ssh/id_* )

Should I do that?

OliverO2 commented at 2020-06-08 14:00:¶

If I understand it correctly, those duplicates would be reported to the user so it's probably best to avoid them.

jsmeix commented at 2020-06-08 14:36:¶

@OliverO2
thank you for the hint that duplicates could be reported to the user!

I think actually duplicates are not reported to the user
because when a file was removed and a duplicate apperars
in the for key_file in "${key_files[@]}" loop the
test -s "$ROOTFS_DIR/$key_file" || continue
skips the duplicate.

What happens (and I have seen them in my log file) are

Log "SSH key file '$key_file' has a passphrase and is allowed in the recovery system"

messages for duplicates.

jsmeix commented at 2020-06-08 14:40:¶

If there are no objections I would like to merge it tomorrow.

jsmeix commented at 2020-06-08 14:44:¶

With
https://github.com/rear/rear/pull/2422/commits/19eea34d3e9eb080009de9df7ec1e98d0b01ed61
I get now in my rear -D mkrescue log:

# grep 'ssh-keygen -q -p -P' var/log/rear/rear-linux-h9wr.log
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/.//root/.ssh/id_dsa
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/.//root/.ssh/id_ecdsa
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/.//root/.ssh/id_ed25519
++ ssh-keygen -q -p -P '' -N '' -f /tmp/rear.XXX/rootfs/.//root/.ssh/id_rsa

[Export of Github issue for rear/rear.]

#2422 PR merged: Also parse SSH Include files in 501_check_ssh_keys.sh¶