Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1066615 - sshd.service shouldn't call /usr/sbin/sshd-keygen directly using ExecStartPre
Summary: sshd.service shouldn't call /usr/sbin/sshd-keygen directly using ExecStartPre
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: openssh
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
low
Target Milestone: ---
Assignee: Petr Lautrbach
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-02-18 17:42 UTC by Pavel Šimerda (pavlix)
Modified: 2014-07-17 11:48 UTC (History)
5 users (show)

Fixed In Version: openssh-6.6.1p1-1.fc21
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-07-17 11:48:38 UTC


Attachments (Terms of Use)

Description Pavel Šimerda (pavlix) 2014-02-18 17:42:00 UTC
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.

Comment 1 Pavel Šimerda (pavlix) 2014-02-18 19:03:29 UTC
Also, I'm not entirely sure but maybe the 'ssh-keygen.service' dependencies should go to 'sshd.socket' instead of 'sshd@.service'.

Comment 2 Petr Lautrbach 2014-02-19 13:18:07 UTC
There were some problems with it in past, see bug https://bugzilla.redhat.com/show_bug.cgi?id=810419#c4

Comment 3 Pavel Šimerda (pavlix) 2014-02-20 03:31:26 UTC
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

Comment 4 Pavel Šimerda (pavlix) 2014-02-20 07:49:22 UTC
Just found out that dnssec-trigger uses this feature:

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

Comment 5 Petr Lautrbach 2014-05-08 10:03:27 UTC
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/

Comment 6 Pavel Šimerda (pavlix) 2014-05-08 10:44:53 UTC
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).

Comment 7 Petr Lautrbach 2014-05-09 07:24:33 UTC
(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

Comment 8 Petr Lautrbach 2014-05-09 07:26:26 UTC
(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

Comment 9 Pavel Šimerda (pavlix) 2014-05-09 08:33:59 UTC
(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!

Comment 10 Petr Lautrbach 2014-05-13 17:51:40 UTC
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/

Comment 11 Pavel Šimerda (pavlix) 2014-05-13 20:22:27 UTC
(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


Note You need to log in before you can comment on or make changes to this bug.