Re: [PATCH] sshd-regen-keys: Improve service, make more robust

[Henning Schild <henning.schild@siemens.com>]

Am Fri, 26 Mar 2021 08:35:51 +0100
schrieb "[ext] Henning Schild" <henning.schild@siemens.com>:

> Am Thu, 25 Mar 2021 19:53:46 +0100
> schrieb Jan Kiszka <jan.kiszka@siemens.com>:
> 
> > On 25.03.21 15:30, Henning Schild wrote:  
> > > I am beginning to think we should fix that upstream. If the
> > > upstream service file would generate the keys if missing ... all
> > > isar would need to do is remove the files. Either with a package
> > > hook or with a image-postprocess
> > > 
> > > Am Thu, 25 Mar 2021 13:54:02 +0100
> > > schrieb Jan Kiszka <jan.kiszka@siemens.com>:
> > >     
> > >> From: Jan Kiszka <jan.kiszka@siemens.com>
> > >>
> > >> This improves a number of things:
> > >>
> > >>  - stop the service while regenerating keys, rather than
> > >> disabling its auto-start    
> > > 
> > > Not sure this is going to work. There is this "Before=ssh.service"
> > > which i would expect makes sure it should never end up being
> > > "is-active". And that dpkg-reconfigure also plays with is-active
> > > ... /var/lib/dpkg/info/openssh-server.postinst
> > > 
> > > The idea was to reuse the key generation code from that postinst,
> > > but the construct we need to build to get that to work seems to be
> > > getting out of hand and too complicated. In fact it is
> > > systemd-only, which could be an issue for some.
> > > 
> > > Maybe running after ssh
> > > - remove
> > > - "create with own code"
> > >   - "copy those few ssh-keygen lines"
> > >   - or "source openssh-server.postinst && create_keys"
> > > - killall -HUP sshd (systemctl reload ssh)
> > > might turn out to be the simpler and easier to maintain version.
> > > 
> > > For sure Harald should be involved, did add him to Cc.
> > >     
> > 
> > I don't mind any simpler solution. It need to be robust as well,
> > that's all. The one we have so far once again fell apart today and
> > costed me hours to understand and resolve (because it was slow to
> > reproduce).  
> 
> What i proposed should hopefully be more robust and simpler, but i
> have no time to implement and test it.
> 
> What could be even simpler
> 
> /etc/systemd/system/sshd.service.d/generate-missing-keys.conf
>  [Service]
>  ExecStartPre=
>  ExecStartPre=/usr/bin/ssh-keygen -A
>  ExecStartPre=/usr/sbin/sshd -t
> 
> DEBIAN_DEPENDS="openssh-server"
> 
> postinst
>  rm -v /etc/ssh/ssh_host_*_key*
> 
> That ExecStartPre is what seems to be missing in the service file from
> debian because they seem to assume they fully deal with keys at
> installation time and never at runtime.
> Unfortunately we need 3 lines because we need to prepend before the
> "sshd -t". First to "overwrite", second "our content", third "content
> from original"

Because of that prepend and having to copy existing "ExecStartPre" into
the snippet, a Before-service is probably better. Because that simply
does not care what the original service might look like.
Did send a patch.

regards,
Henning


> Tried that manually on a system, with the systemd snippet you get new
> keys every time the exisiting ones go missing. 
> 
> regards,
> Henning
> 
> > 
> > Jan
> >   
> > > Henning
> > >     
> > >>  - fix restart test condition
> > >>  - also check that /tmp is writable (better safe than sorry)
> > >>  - do not disabling the regen service if it was not successful
> > >>
> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > >> ---
> > >>
> > >> This obsoletes Quirin's patch "sshd-regen-keys: do not enable ssh
> > >> server if previously disabled".
> > >>
> > >>  .../sshd-regen-keys/files/sshd-regen-keys.service  |  2 +-
> > >>  .../sshd-regen-keys/files/sshd-regen-keys.sh       | 14
> > >> ++++++++------ ...hd-regen-keys_0.3.bb => sshd-regen-keys_0.4.bb}
> > >> |  0 3 files changed, 9 insertions(+), 7 deletions(-)
> > >>  rename
> > >> meta/recipes-support/sshd-regen-keys/{sshd-regen-keys_0.3.bb =>
> > >> sshd-regen-keys_0.4.bb} (100%)  
> > >>
> > >> diff --git
> > >> a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
> > >> b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
> > >> index f50d34c8..e7142e69 100644 ---
> > >> a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
> > >> +++
> > >> b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.service
> > >> @@ -5,13 +5,13 @@ Conflicts=shutdown.target
> > >> After=systemd-remount-fs.service Before=shutdown.target
> > >> ssh.service ConditionPathIsReadWrite=/etc
> > >> +ConditionPathIsReadWrite=/tmp [Service]
> > >>  Type=oneshot
> > >>  RemainAfterExit=yes
> > >>  Environment=DEBIAN_FRONTEND=noninteractive
> > >>  ExecStart=/usr/sbin/sshd-regen-keys.sh
> > >> -ExecStartPost=-/bin/systemctl disable sshd-regen-keys.service
> > >>  StandardOutput=syslog
> > >>  StandardError=syslog
> > >>  
> > >> diff --git
> > >> a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
> > >> b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
> > >> index 910d879b..9b19f9d3 100644 ---
> > >> a/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
> > >> +++
> > >> b/meta/recipes-support/sshd-regen-keys/files/sshd-regen-keys.sh
> > >> @@ -1,9 +1,9 @@ #!/usr/bin/env sh echo -n "SSH server is "
> > >> -if systemctl is-enabled ssh; then
> > >> -    SSHD_ENABLED="true"
> > >> -    systemctl disable --no-reload ssh
> > >> +if systemctl is-active ssh; then
> > >> +    SSHD_ACTIVE="true"
> > >> +    systemctl stop ssh
> > >>  fi
> > >>  
> > >>  echo "Removing keys ..."
> > >> @@ -12,9 +12,11 @@ rm -v /etc/ssh/ssh_host_*_key*
> > >>  echo "Regenerating keys ..."
> > >>  dpkg-reconfigure openssh-server
> > >>  
> > >> -if test -n $SSHD_ENABLED; then
> > >> -    echo "Reenabling ssh server ..."
> > >> -    systemctl enable --no-reload ssh
> > >> +if test -n "$SSHD_ACTIVE"; then
> > >> +    echo "Restarting ssh server ..."
> > >> +    systemctl start ssh
> > >>  fi
> > >>  
> > >> +systemctl disable sshd-regen-keys.service
> > >> +
> > >>  sync
> > >> diff --git
> > >> a/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb
> > >> b/meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb
> > >> similarity index 100% rename from
> > >> meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.3.bb
> > >> rename to
> > >> meta/recipes-support/sshd-regen-keys/sshd-regen-keys_0.4.bb    
> > >     
> >   
>