Re: [ptxdist] [PATCH 1/1] openssh/rc-once: build all supported hostkeys, simplify

[Christian Hermann <christian.hermann@hytera.de>]

On 18.08.20 08:57, Michael Olbrich wrote:
> On Mon, Aug 17, 2020 at 04:45:02PM +0200, Christian Hermann wrote:
>> 1. Ensure all supported hostkeys are generated, even if not configured
>> explicitly.
>> Parsing /etc/sshd_config is insufficient for determining which types of
>> hostkeys are actually supported by the running instance of sshd.
>> Ask `sshd -T` instead, as openssh uses sensible defaults if no custom
>> 'HostKey' lines are present in /etc/ssh/sshd_config.
> 
> I like this part. But please split it into a separate patch to make it more
> readable.
will do, thanks.
> 
>> 2. Simplify key generation logic
> 
> This needs some changes.
> 
>> Signed-off-by: Christian Hermann <christian.hermann@hytera.de>
>> ---
>>  projectroot/etc/rc.once.d/openssh | 57 ++++++++++++++-----------------
>>  1 file changed, 25 insertions(+), 32 deletions(-)
>>
>> diff --git a/projectroot/etc/rc.once.d/openssh b/projectroot/etc/rc.once.d/openssh
>> index 66cfa06df..bc3634958 100644
>> --- a/projectroot/etc/rc.once.d/openssh
>> +++ b/projectroot/etc/rc.once.d/openssh
>> @@ -3,47 +3,40 @@
>>  PATH=/sbin:/bin:/usr/sbin:/usr/bin
>>  
>>  get_hostkeys() {
>> -	[ -f /etc/ssh/sshd_config ] || return
>> -	sed -n 's/^HostKey[ \t][ \t]*\(.*\)/\1/p' /etc/ssh/sshd_config
>> -}
>> -
>> -host_keys_required() {
>> -	hostkeys="$(get_hostkeys)"
>> -	if [ "$hostkeys" ]; then
>> -		echo "$hostkeys"
>> -	else
>> -		# No HostKey directives found, so we pick secure defaults
>> -		echo /etc/ssh/ssh_host_ed25519_key
>> -	fi
>> +	sshd -T | sed -E -n -e 's/^hostkey[[:space:]]+(.*)/\1/p'
>>  }
>>  
>>  create_key() {
>> -	keytype="$1"
>> -	prettykeytype="$(echo $_type | tr a-z A-Z)"
>> -	shift
>> -	hostkeys="$1"
>> -	shift
>> -
>> -	file="/etc/ssh/ssh_host_${keytype}_key"
>> -
>> -	if echo "$hostkeys" | grep -x -F "$file" >/dev/null; then
>> -		echo "Create $prettykeytype key; this may take some time ..."
>> -		rm -f $file &&
>> -		ssh-keygen -q -f "$file" -N '' -t "$keytype" "$@" || return
>> -		echo "Created $prettykeytype key."
>> -	fi
>> +	keyfile="$1"
>> +	keytype="$(echo "$keyfile" | sed -E -e 's/.*ssh_host_(.*)_key$/\1/')"
>> +	prettykeytype="$(echo "$keytype" | tr '[:lower:]' '[:upper:]')"
> 
> So, I have a patch queued for mainline that removed the $prettykeytype
> entirely. I came across a BSP that didn't have 'tr' at all. And ':lower:' /
> ':upper:' is worse, because that's a separate option in busybox...
Yeah, testing this with busybox  was on my list as well...
> 
>> +
>> +	keygen_args=
>> +	case "$keytype" in
>> +		rsa) keygen_args="-b 4096" ;;
>> +	esac
>> +
>> +	echo "Creating $prettykeytype key"
>> +	rm -f "$keyfile"
> 
> Keep the '&&'. If the file cannot be deleted, then calling ssh-keygen makes
> no sense.
> 
>> +	ssh-keygen -q -f "$keyfile" -N "" -t "$keytype" $keygen_args
>>  }
>>  
>>  create_keys() {
>> -	hostkeys="$(host_keys_required)"
>> +	hostkeys="$(get_hostkeys)"
>> +
>> +	# no hostkeys reported by sshd.  Try to provide a fallback
>> +	#FIXME: if `sshd -T` fails, we very likely have bigger problems. shout to stderr and exit instead?
> 
> Hmmm, I think, this requires some testing. What happens is if there are no
> Keys configured in the config? Or a syntax error?
> 
> But in the end, I guess that sshd won't work anyways, so I think we should
> just fail with an appropriate error message.
>>> +	if [ -z "$hostkeys" ]; then
>> +		fallback="/etc/ssh/ssh_host_ed25519_key"
>> +		echo "HostKey $fallback" >>/etc/ssh/sshd_config
> 
> Definitively not. Don't touch the config.
> 
>> +	fi
>>  
>> -	create_key "dsa" "$hostkeys" &&
>> -	create_key "ecdsa" "$hostkeys" &&
>> -	create_key "ed25519" "$hostkeys" &&
>> -	create_key "rsa" "$hostkeys" -b 4096
>> +	for keyfile in $hostkeys; do
>> +		create_key "$keyfile"
> 
> 		create_key "$keyfile" || return
> 
> We need to propagate the error here. Otherwise, only the last key will
> produce an error.
> If we cannot create a key, then something is really broken.
indeed. I'm inclined to just `set -e` on top then
> 
>> +	done
>>  }
>>  
>>  if ! create_keys; then
>> -	echo "Generating SSH keys failed!"
>> +	echo "Generating SSH keys failed!" >&2
> 
> I don't care either way, but does this make a difference? The message
> should be visible on the console either way.
It doesn't make a difference of course, i just considered it good style
to print errors to stderr.
it's your call in the end
> 
> Michael
> 
>>  	exit 1
>>  fi
>> -- 
>> 2.28.0
>>
>>
>> _______________________________________________
>> ptxdist mailing list
>> ptxdist@pengutronix.de
>> To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de
>>
> 

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de
To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de