From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-eopbgr130047.outbound.protection.outlook.com ([40.107.13.47] helo=EUR01-HE1-obe.outbound.protection.outlook.com) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1k7zuJ-0003l7-4i for ptxdist@pengutronix.de; Tue, 18 Aug 2020 13:36:00 +0200 Received: from nbmx01.hytera.de (unknown [172.21.102.22]) by ibmx32.hytera.de (Postfix) with ESMTP id F0903AD90 for ; Tue, 18 Aug 2020 13:35:49 +0200 (CEST) References: <20200817144502.11265-1-christian.hermann@hytera.de> <20200817144502.11265-2-christian.hermann@hytera.de> <20200818065735.GG8684@pengutronix.de> From: Christian Hermann Message-ID: <9d391b01-ee44-12f0-f17c-f97c597e0cc1@hytera.de> Date: Tue, 18 Aug 2020 13:35:54 +0200 MIME-Version: 1.0 In-Reply-To: <20200818065735.GG8684@pengutronix.de> Content-Language: en-US Subject: Re: [ptxdist] [PATCH 1/1] openssh/rc-once: build all supported hostkeys, simplify List-Id: PTXdist Development Mailing List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: ptxdist@pengutronix.de Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ptxdist-bounces@pengutronix.de Sender: "ptxdist" To: ptxdist@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 >> --- >> 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