mailarchive of the ptxdist mailing list
 help / color / mirror / Atom feed
From: Christian Hermann <christian.hermann@hytera.de>
To: ptxdist@pengutronix.de
Subject: Re: [ptxdist] [PATCH 1/1] openssh/rc-once: build all supported hostkeys, simplify
Date: Tue, 18 Aug 2020 13:35:54 +0200	[thread overview]
Message-ID: <9d391b01-ee44-12f0-f17c-f97c597e0cc1@hytera.de> (raw)
In-Reply-To: <20200818065735.GG8684@pengutronix.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

  reply	other threads:[~2020-08-18 11:36 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-17 14:45 [ptxdist] [RFC] rework openssh/rc-once Christian Hermann
2020-08-17 14:45 ` [ptxdist] [PATCH 1/1] openssh/rc-once: build all supported hostkeys, simplify Christian Hermann
2020-08-18  6:57   ` Michael Olbrich
2020-08-18 11:35     ` Christian Hermann [this message]
2020-08-18 14:02     ` Christian Hermann
2020-08-18 15:38       ` Michael Olbrich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9d391b01-ee44-12f0-f17c-f97c597e0cc1@hytera.de \
    --to=christian.hermann@hytera.de \
    --cc=ptxdist@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox