Bug 1134997

+++ This bug was initially created as a clone of Bug #1066615 +++

There is a sshd-keygen.service that handles the generation of missing SSH keys on startup. The inetd-style sshd@.service triggered by the sshd.socket uses it via dependencies. The classic sshd.service, on the other hand, mimics the sshd-keygen.service by calling the /usr/sbin/sshd-keygen directly.

I'm filing this ticket because I believe the classic sshd.service should use sshd-keygen.service as well. In other words, I think sshd-keygen.service should provide a uniform way to handle key generation for both sshd.service and sshd.socket/sshd@.service.

diff --git a/sshd.service b/sshd.service
index 6348bc1..e8c88d4 100644
--- a/sshd.service
+++ b/sshd.service
@@ -1,10 +1,10 @@
 [Unit]
 Description=OpenSSH server daemon
-After=syslog.target network.target auditd.service
+Wants=sshd-keygen.service
+After=syslog.target network.target auditd.service sshd-keygen.service
 
 [Service]
 EnvironmentFile=/etc/sysconfig/sshd
-ExecStartPre=/usr/sbin/sshd-keygen
 ExecStart=/usr/sbin/sshd -D $OPTIONS
 ExecReload=/bin/kill -HUP $MAINPID
 KillMode=process

This solution is not only cleaner but also makes use of systemd's ConditionPathExists to avoid spawning the external script when it's not necessary.

--- Additional comment from Pavel Šimerda (pavlix) on 2014-02-18 20:03:29 CET ---

Also, I'm not entirely sure but maybe the 'ssh-keygen.service' dependencies should go to 'sshd.socket' instead of 'sshd@.service'.

--- Additional comment from Petr Lautrbach on 2014-02-19 14:18:07 CET ---

There were some problems with it in past, see bug https://bugzilla.redhat.com/show_bug.cgi?id=810419#c4

--- Additional comment from Pavel Šimerda (pavlix) on 2014-02-20 04:31:26 CET ---

From the other bug:

> 'BindTo=sshd.service' is needed. ssh-keygen,service is oneshot and stays after
> exit in running state. And as we need to start ssh-keygen.service every time
> sshd.service starts, so we need to stop ssh-keygen.service together with
> sshd.service. Unfortunately there is side effect that sshd.service is started
> even if sshd-keygen.service is started manually.

This seems to be a use case for PartOf rather than Requires/BindTo.

from systemd.unit(5):

PartOf=

Configures dependencies similar to Requires=, but limited to stopping and restarting of units. When systemd stops or restarts the units listed here, the action is propagated to this unit. Note that this is a one way dependency - changes to this unit do not affect the listed units.

sshd-keygen.service:

[Unit]
PartOf=sshd.service

--- Additional comment from Pavel Šimerda (pavlix) on 2014-02-20 08:49:22 CET ---

Just found out that dnssec-trigger uses this feature:

http://pkgs.fedoraproject.org/cgit/dnssec-trigger.git/tree/dnssec-triggerd-resolvconf-handle.service

--- Additional comment from Petr Lautrbach on 2014-05-08 12:03:27 CEST ---

Could you please try openssh-6.6p1-0.4.fc21 build [1] if it works for you?

[1] http://copr-be.cloud.fedoraproject.org/results/plautrba/openssh_testing/epel-7-x86_64/openssh-6.6p1-0.4.fc21/

--- Additional comment from Pavel Šimerda (pavlix) on 2014-05-08 12:44:53 CEST ---

Thanks! Just downloaded and extracted openssh-server RPM and are checking its systemd files...

** sshd.service **

1) missing After=sshd-keygen.service (Wants is correct but not enough)

2) "we do no longer recommend people to order their units after syslog.target"

http://www.freedesktop.org/wiki/Software/systemd/syslog/

3) Why depend on network.target? Does SSH need to have networking configured? Is it capable of using IP_FREEBIND for addresses not yet configured?

Note that After=network.target is almost always useless as network.target nowadays doesn't guarantee configured network like it did at the time of /etc/init.d/network.

4) After=auditd.service is redundant, see:

auditd.service: Before=sysinit.target
basic.target: After=sysinit.target

"systemd automatically adds dependencies of the types Requires= and After= for this target unit to all services (except for those with DefaultDependencies=no)."

http://www.freedesktop.org/software/systemd/man/systemd.special.html

The automatic ordering of those is as follows

auditd < sysinit < basic < sshd

** sshd@.service **

5) I talked to lnykryn about sshd@.service dependency on sshd-keygen.service. It depends on whether you want the key generation to happen at boot time, in which case it belongs instead to sshd.socket, or you actually want it to happen for the first client connected, in which case the dependency is correct and RemainAfterExit ensures it's only called once

6) What should happen when ssh-keygen.service fails? If it should block the sshd.service or sshd.socket, they should use Requires instead of Wants.

7) What about sshd.service or sshd.socket restarts? Should it trigger new key generation? If yes, then this can IMO be solved by putting 'PartOf=sshd.service sshd.socket' in sshd-keygen.service.

8) auditd.service is not needed (see #4).

--- Additional comment from Petr Lautrbach on 2014-05-09 09:24:33 CEST ---

(In reply to Pavel Šimerda (pavlix) from comment #6)
> 1) missing After=sshd-keygen.service (Wants is correct but not enough)

added
 
> 2) "we do no longer recommend people to order their units after
> syslog.target"
> 

removed

> 
> 3) Why depend on network.target? Does SSH need to have networking
> configured? Is it capable of using IP_FREEBIND for addresses not yet
> configured?

It's not.

> Note that After=network.target is almost always useless as network.target
> nowadays doesn't guarantee configured network like it did at the time of
> /etc/init.d/network.
> 

In my testing virtual machine with NetworkManager.service enabled and static ip configuration, After=network.target makes a difference so I will keep it.

> 4) After=auditd.service is redundant, see:
> 
> auditd.service: Before=sysinit.target
> basic.target: After=sysinit.target
> 
> "systemd automatically adds dependencies of the types Requires= and After=
> for this target unit to all services (except for those with
> DefaultDependencies=no)."
> 
> http://www.freedesktop.org/software/systemd/man/systemd.special.html
> 
> The automatic ordering of those is as follows
> 
> auditd < sysinit < basic < sshd

Although I'd rather not rely on the default automatic behaviour of systemd which tends to change, I remove that.

> 
> ** sshd@.service **
> 

> 6) What should happen when ssh-keygen.service fails? If it should block the
> sshd.service or sshd.socket, they should use Requires instead of Wants.

ssh-keygen.service fail shouldn't be fatal or block sshd.service. E.g when there's a new key type added in update and you have your previous keys generated but sshd-keygen.service will fail on the new one. Then you can still run sshd using the old keys.

> 5) I talked to lnykryn about sshd@.service dependency on
> sshd-keygen.service. It depends on whether you want the key generation to
> happen at boot time, in which case it belongs instead to sshd.socket, or you
> actually want it to happen for the first client connected, in which case the
> dependency is correct and RemainAfterExit ensures it's only called once

The latter. 
 
> 7) What about sshd.service or sshd.socket restarts? Should it trigger new
> key generation? If yes, then this can IMO be solved by putting
> 'PartOf=sshd.service sshd.socket' in sshd-keygen.service.

sshd-keygen.service is run only if defined keys doesn't exists. It could be 


> 8) auditd.service is not needed (see #4).

removed

Proposal units:

sshd.service:
[Unit]
Description=OpenSSH server daemon
After=network.target sshd-keygen.service
Wants=sshd-keygen.service

[Service]
EnvironmentFile=/etc/sysconfig/sshd
ExecStart=/usr/sbin/sshd -D $OPTIONS
ExecReload=/bin/kill -HUP $MAINPID
KillMode=process
Restart=on-failure
RestartSec=42s

[Install]
WantedBy=multi-user.target


sshd@.service:
[Unit]
Description=OpenSSH per-connection server daemon
Wants=sshd-keygen.service
After=sshd-keygen.service

[Service]
EnvironmentFile=-/etc/sysconfig/sshd
ExecStart=-/usr/sbin/sshd -i $OPTIONS
StandardInput=socket


sshd-keygen.service:
[Unit]
Description=OpenSSH Server Key Generation
ConditionPathExists=|!/etc/ssh/ssh_host_rsa_key
ConditionPathExists=|!/etc/ssh/ssh_host_ecdsa_key
ConditionPathExists=|!/etc/ssh/ssh_host_ed25519_key
PartOf=sshd.service sshd.socket

[Service]
ExecStart=/usr/sbin/sshd-keygen
Type=oneshot
RemainAfterExit=yes

--- Additional comment from Petr Lautrbach on 2014-05-09 09:26:26 CEST ---

(In reply to Petr Lautrbach from comment #7)
> > 5) I talked to lnykryn about sshd@.service dependency on
> > sshd-keygen.service. It depends on whether you want the key generation to
> > happen at boot time, in which case it belongs instead to sshd.socket, or you
> > actually want it to happen for the first client connected, in which case the
> > dependency is correct and RemainAfterExit ensures it's only called once
> 
> The latter. 
>  
> > 7) What about sshd.service or sshd.socket restarts? Should it trigger new
> > key generation? If yes, then this can IMO be solved by putting
> > 'PartOf=sshd.service sshd.socket' in sshd-keygen.service.
> 
> sshd-keygen.service is run only if defined keys doesn't exists. It could be 
> 

Drop that lines as noise. I've connected sshd-keygen.service to sshd.socket

--- Additional comment from Pavel Šimerda (pavlix) on 2014-05-09 10:33:59 CEST ---

(In reply to Petr Lautrbach from comment #7)
> > 3) Why depend on network.target? Does SSH need to have networking
> > configured? Is it capable of using IP_FREEBIND for addresses not yet
> > configured?
> 
> It's not.
> 
> > Note that After=network.target is almost always useless as network.target
> > nowadays doesn't guarantee configured network like it did at the time of
> > /etc/init.d/network.
> > 
> 
> In my testing virtual machine with NetworkManager.service enabled and static
> ip configuration, After=network.target makes a difference so I will keep it.

Good then. It's an issue of it's own and I have just started bug #1096081 to give it more care and time.

> > 7) What about sshd.service or sshd.socket restarts? Should it trigger new
> > key generation? If yes, then this can IMO be solved by putting
> > 'PartOf=sshd.service sshd.socket' in sshd-keygen.service.
> 
> sshd-keygen.service is run only if defined keys doesn't exists. It could be

'PartOf=sshd.service sshd.socket' will basically make 'restart' or 'stop+start' after manually deleting the keys regenerate them, that's all.

> Proposal units:
> 
> sshd.service:
> [Unit]
> Description=OpenSSH server daemon
> After=network.target sshd-keygen.service
> Wants=sshd-keygen.service

+1

> sshd@.service:
> [Unit]
> Description=OpenSSH per-connection server daemon
> Wants=sshd-keygen.service
> After=sshd-keygen.service

+1

> sshd-keygen.service:
> [Unit]
> Description=OpenSSH Server Key Generation
> ConditionPathExists=|!/etc/ssh/ssh_host_rsa_key
> ConditionPathExists=|!/etc/ssh/ssh_host_ecdsa_key
> ConditionPathExists=|!/etc/ssh/ssh_host_ed25519_key
> PartOf=sshd.service sshd.socket

+1

> [Service]
> ...
> Type=oneshot
> RemainAfterExit=yes

+1

Thanks a lot!

--- Additional comment from Petr Lautrbach on 2014-05-13 19:51:40 CEST ---

Thanks for the review, I'll push the changes with the rebase for #1059618. In the meantime you can use the openssh-6.6.1p1-0.1.fc21 from copr [1]

[1] http://copr.fedoraproject.org/coprs/plautrba/openssh_testing/builds/

--- Additional comment from Pavel Šimerda (pavlix) on 2014-05-13 22:22:27 CEST ---

(In reply to Petr Lautrbach from comment #10)
> Thanks for the review, I'll push the changes with the rebase for #1059618.
> In the meantime you can use the openssh-6.6.1p1-0.1.fc21 from copr [1]
> 
> [1] http://copr.fedoraproject.org/coprs/plautrba/openssh_testing/builds/

Thanks. I don't mean to actually use it before it gets to rawhide. But I'm curious whether copr provides web-based access to the files. In this case it doesn't matter whether source or binary package. In case of a regular build, I would be using web git at pkgs.fedoraproject.org.

Pavel