Bug 1325535

Why do you have the PartOf dependency there? It looks weird for the oneshot units.

(In reply to Lukáš Nykrýn from comment #1)
> Why do you have the PartOf dependency there? It looks weird for the oneshot
> units.

Yes, it does not have to be there. It was part of the old service (and actually still is) and I didn't study if we should remove that. Thanks for comment. Noted.

See https://fedoraproject.org/wiki/Packaging:Initial_Service_Setup. You should try to align your unit as much as possible with guidelines there: e.g. RemainAfterExit=no.

You must remove Before=sshd.socket.

How exactly is the ssh-keygen@.service installed? Does sshd.service has Wants=ssh-keygen or similar?

(In reply to Jakub Jelen from comment #0)

> ExecStartPost=-/usr/sbin/restorecon /etc/ssh/ssh_host_%i_key{,.pub}

IIRC, such shell expansions ({}) are not supported. Rewrite this as two ExecStartPost lines or wrap this by invocation of /bin/bash -c "...".

(In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> See https://fedoraproject.org/wiki/Packaging:Initial_Service_Setup. You
> should try to align your unit as much as possible with guidelines there:
> e.g. RemainAfterExit=no.

It is not from our heads (bug #1066615, comment #6):

> 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

But I am ok with removing that. We should probably remove this line. It does not have any reasonable effect in this case (as far as I understand that, or does it?).


> You must remove Before=sshd.socket.

Why? Wouldn't it happen if we would not generate the keys and create socket activation, that the sshd-keygen service would not start?


> How exactly is the ssh-keygen@.service installed? Does sshd.service has
> Wants=ssh-keygen or similar?

Yes, as mentioned in the description above.

Thank you for the comments and the link. But it is only one use case, to generate the keys during first boot. You can also use the service to regenerate them or change the subset of the keys that you want to auto-create. At least it historically was, configurable by AUTOCREATE_SERVER_KEYS variable and now I can think that dependency can be handled by Wants= somehow.

(In reply to Jakub Jelen from comment #5)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> > See https://fedoraproject.org/wiki/Packaging:Initial_Service_Setup. You
> > should try to align your unit as much as possible with guidelines there:
> > e.g. RemainAfterExit=no.
> 
> It is not from our heads (bug #1066615, comment #6):
>
> > 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
> 
> But I am ok with removing that. We should probably remove this line. It does
> not have any reasonable effect in this case (as far as I understand that, or
> does it?).

You have Condition...= which means that the unit will not be started again if it run successfully before. So RemainAfterExit=yes is redundant.
Also removing it allows you to remove the keys, and simply restart sshd.service, to have them regenerated. Maybe a bit of a corner case, but just makes things minimally more robust.

> > You must remove Before=sshd.socket.
> 
> Why? Wouldn't it happen if we would not generate the keys and create socket
> activation, that the sshd-keygen service would not start?

OK, "must" is too strong. I should have said "should".

As you say below, ssh-keygen@ services are pulled in the the service unit, and the service unit, not the socket unit, actually uses the keys. sockets.target is started early in boot. Things would work even with the dependency, but it's better to avoid pushing things into early boot. Especially that generating the key can be pretty costly, and uses entropy, and for many machines they keys might not be ever used.

> > How exactly is the ssh-keygen@.service installed? Does sshd.service has
> > Wants=ssh-keygen or similar?
> 
> Yes, as mentioned in the description above.
OK.

> Thank you for the comments and the link. But it is only one use case, to
> generate the keys during first boot. You can also use the service to
> regenerate them or change the subset of the keys that you want to
> auto-create. At least it historically was, configurable by
> AUTOCREATE_SERVER_KEYS variable and now I can think that dependency can be
> handled by Wants= somehow.

There's always the possibility of masking the ssh-keygen@.service as a way of disabling it. I think that's reasonable replacement for AUTOCREATE_SERVER_KEYS=no variable.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> As you say below, ssh-keygen@ services are pulled in the the service unit,
> and the service unit, not the socket unit, actually uses the keys.
> sockets.target is started early in boot. Things would work even with the
> dependency, but it's better to avoid pushing things into early boot.
> Especially that generating the key can be pretty costly, and uses entropy,
> and for many machines they keys might not be ever used.

Thanks for comments. It makes sense. Removed

About the renaming according to the guidelines, I am afraid sshd-keygen is somehow established term (it will probably live all the way through RHEL7 at least), so renaming to  sshd-init  would probably cause more trouble than help.

> If it needs to run multiple commands, it is recommended to create a script file in the package's /usr/libexec/<packagename> directory and execute that.

This is probably applicable to our case. Having 5 ExecStartPost commands is "multiple" and we should use bash script again (also to cover FIPS).

My basic testing went well so far. Since we iterated to some usable version, is it too late to push it to Fedora 24 Beta to make sure it is tested well and documented?

Current version looks like this:
 * we still need EnvirnmentFile for  SSH_USE_STRONG_RNG  (does systemd pass environment variables?)
 * RemainAfterExit=no  is default, so omitting should be ok

[Unit]
Description=OpenSSH %i Server Key Generation
ConditionFileNotEmpty=|!/etc/ssh/ssh_host_%i_key
Before=sshd.service

[Service]
Type=oneshot
EnvironmentFile=-/etc/sysconfig/sshd
ExecStart=/usr/libexec/openssh/sshd-keygen.sh %i


Bash script:
 * based on the proposed version by Yu Watanabe in bug #1317722
 * drops support for RSA1 keys
 * support for FIPS works
 * moved to appropriate folder

#!/bin/bash

# Create the host keys for the OpenSSH server.
KEYTYPE=$1
case $KEYTYPE in
	"dsa") ;& # disabled in FIPS
	"ed25519")
		FIPS=/proc/sys/crypto/fips_enabled
		if [[ -r "$FIPS" && $(cat $FIPS) == "1" ]]; then
			exit 0
		fi ;;
	"rsa") ;; # always ok
	"ecdsa") ;;
	*) # wrong argument
		exit 12 ;;
esac

KEYGEN=/usr/bin/ssh-keygen
KEY=/etc/ssh/ssh_host_${KEYTYPE}_key

if [[ ! -x $KEYGEN ]]; then
	exit 13
fi

# remove old keys
rm -f $RSA_KEY{,.pub}

# create new keys
if ! $KEYGEN -q -t $KEYTYPE -f $KEY -C '' -N '' >&/dev/null; then
	exit 1
fi

# sanitize permissions
/usr/bin/chgrp ssh_keys $KEY
/usr/bin/chmod 640 $KEY
/usr/bin/chmod 644 $KEY.pub
if [[ -x /usr/sbin/restorecon ]]; then
	/usr/sbin/restorecon $KEY{,.pub}
fi

exit 0

(In reply to Jakub Jelen from comment #7)
> About the renaming according to the guidelines, I am afraid sshd-keygen is
> somehow established term (it will probably live all the way through RHEL7 at
> least), so renaming to  sshd-init  would probably cause more trouble than
> help.
+1 on keeping the old name.

> My basic testing went well so far. Since we iterated to some usable version,
> is it too late to push it to Fedora 24 Beta to make sure it is tested well
> and documented?
The updated policies specify that "breaking changes" and incompatible upgrades
should be avoid. This change will be pretty much invisible to installed
systems so I think it's totally fine to push it.

> Current version looks like this:
>  * we still need EnvirnmentFile for  SSH_USE_STRONG_RNG  (does systemd pass
> environment variables?)
It does.

But shouldn't SSH_USE_STRONG_RNG be always used? Most people will not
know about the config file, and anyway those keys will be generated before
first login, so the default is what pretty much anyone will use.
I don't think you'd ever want to run that key generation with
SSH_USE_STRONG_RNG=0.

The warning in the man page is wrong for Linux, and a hardware random
generator is not necessary to use /dev/random, the kernel replenishes
the pool based on hardware events. Seeding the generation of ssh keys
is the type of activity that /dev/random is there for. For a majority
of machines the keys will be generated without issue. In case of poor
entropy-deprived systems the generation will be slowed until the
entropy is acquired, but it's a one-off event, so waiting a bit is OK.

(I tried to measure how much the entropy pool is depleted when
generating a rsa on my laptop by watching
/proc/sys/kernel/random/entropy_avail. It seems completely negligible,
and sometimes entropy goes *up* after generating a key. I suppose
this is an effect of me typing on the keyboard and disk access, etc.
This is on an old laptop with no rng.)

> [Unit]
...
> Bash script:
...

Looks good to me.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #8)
> (In reply to Jakub Jelen from comment #7)
> > About the renaming according to the guidelines, I am afraid sshd-keygen is
> > somehow established term (it will probably live all the way through RHEL7 at
> > least), so renaming to  sshd-init  would probably cause more trouble than
> > help.
> +1 on keeping the old name.
> 
> > My basic testing went well so far. Since we iterated to some usable version,
> > is it too late to push it to Fedora 24 Beta to make sure it is tested well
> > and documented?
> The updated policies specify that "breaking changes" and incompatible
> upgrades
> should be avoid. This change will be pretty much invisible to installed
> systems so I think it's totally fine to push it.
> 
> > Current version looks like this:
> >  * we still need EnvirnmentFile for  SSH_USE_STRONG_RNG  (does systemd pass
> > environment variables?)
> It does.
> 
> But shouldn't SSH_USE_STRONG_RNG be always used? Most people will not
> know about the config file, and anyway those keys will be generated before
> first login, so the default is what pretty much anyone will use.
> I don't think you'd ever want to run that key generation with
> SSH_USE_STRONG_RNG=0.
> 
> The warning in the man page is wrong for Linux, and a hardware random
> generator is not necessary to use /dev/random, the kernel replenishes
> the pool based on hardware events. Seeding the generation of ssh keys
> is the type of activity that /dev/random is there for. For a majority
> of machines the keys will be generated without issue. In case of poor
> entropy-deprived systems the generation will be slowed until the
> entropy is acquired, but it's a one-off event, so waiting a bit is OK.
> 
> (I tried to measure how much the entropy pool is depleted when
> generating a rsa on my laptop by watching
> /proc/sys/kernel/random/entropy_avail. It seems completely negligible,
> and sometimes entropy goes *up* after generating a key. I suppose
> this is an effect of me typing on the keyboard and disk access, etc.
> This is on an old laptop with no rng.)


Yeah, measuring that from an interactive session is basically impossible, since even the *lack* of input can affect entropy. (If a live session exists, even the amount of "dead-air" can create entropy).

However, if this is being generated on first boot in a VM, there may still be issues. The best solution for this is to boot VMs with the virtio-rng device enabled (which will feed the entropy pool from the hypervisor's /dev/random, which in turn is getting entropy from the activity of all the VMs running on it).

However, if you read through https://bugzilla.redhat.com/show_bug.cgi?id=1212082#c11 and onwards, it seems that it should be perfectly safe for us to read from /dev/urandom (not the 'u') here, since the initscripts package is writing the random seed file during installation (with entropy generated during that process).

One more note:

 * sshd@.service  should also  Wants=sshd-keygen@{keys}.service, which makes it less "state-of-art", because we again duplicate configuration for two use cases, which might be again confusing


Is there any way to make both services (sshd and sshd@) want sshd-keygen without duplication? I was thinking about adding another intermediate "dummy" dependency (sshd-keygen.service ?), which would depend on the instantiated ones:

    sshd.service      sshd@.service
              \       /
       Wants=  \     / Wants=
       Before=  \   /  Before=
                 \ /
            sshd-keygen.service (?)
            /        |        \
           /         |         \ Wants=
          /   Wants= |          \
  Wants= /           |          sshd-keygen
        /            |
       /            sshd-keygen
      /
    sshd-keygen
        
Would it work this way and is it advisable? Is there any convention to name such dummy service without Exec? Is it even service?

sshd-keygen.target would be the right unit type.

By default .services which are part of a target have an automatically added Before= dependency, so
# sshd-keygen@.service
...
[Install]
WantedBy=sshd-keygen.target

and

# sshd.service and sshd@.service
[Unit]
Wants=sshd-keygen.target
After=sshd-keygen.target
...

(In reply to Stephen Gallagher from comment #9)
> However, if you read through
> https://bugzilla.redhat.com/show_bug.cgi?id=1212082#c11 and onwards, it
> seems that it should be perfectly safe for us to read from /dev/urandom (not
> the 'u') here

OK. Thank you for the link. I change my comment about SSH_USE_STRONG_RNG: it seems that the opposite should be done: simply remove the configurability from the sshd-keygen scripts, and always use /dev/urandom. This removes the need for the sysconfig file and makes the user interface simpler. If somebody want to generate the key in a special way, they can always do that by calling the right commands manually.

> since the initscripts package is writing the random seed file
> during installation (with entropy generated during that process).

I don't think the initscripts package does that anymore, systemd-random-seed.service does.

Thanks for comments. I tested it briefly once more and it seems to be quite working, except the use case, when I remove the keys and restart sshd.service. In this case, sshd-keygen.target does not restart the "children" and server starts without keys, which is not cool.

This seems that was solved by the  PartOf=  option in the old  sshd-keygen, isn't it? Does it sound reasonable Lukas?

# sshd-keygen.target
PartOf=sshd.service

to make sure that restart of sshd will check for existence of keys and generate missing.


About the socket activation, it works somehow. Sequence
 * disable & stop sshd.service
 * enable & start sshd.socket
 * remove keys
 * connect
works even without  PartOf=sshd.socket  dependency, even though the target is left active.


(In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> (In reply to Stephen Gallagher from comment #9)
> > However, if you read through
> > https://bugzilla.redhat.com/show_bug.cgi?id=1212082#c11 and onwards, it
> > seems that it should be perfectly safe for us to read from /dev/urandom (not
> > the 'u') here
> 
> OK. Thank you for the link. I change my comment about SSH_USE_STRONG_RNG: it
> seems that the opposite should be done: simply remove the configurability
> from the sshd-keygen scripts, and always use /dev/urandom. This removes the
> need for the sysconfig file and makes the user interface simpler. If
> somebody want to generate the key in a special way, they can always do that
> by calling the right commands manually.

Actually, the  /etc/sysconfig/sshd  file is not used only for key generation (sshd-keygen), but also for service itself, which makes the  sshd  use strong random number generator for session keys and others. This is not common use case for personal laptop, but might be requirement in RHEL deployment.

Adding scratch build for Fedora 23 [1] (tested, based on the above comments) and sending Rawhide build [2].

[1] http://koji.fedoraproject.org/koji/taskinfo?taskID=13671328
[2] http://koji.fedoraproject.org/koji/taskinfo?taskID=13672102

openssh-7.2p2-4.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-965bd6926e

Just for reference, I will have to change
https://github.com/cgwalters/ansible-personal/blob/master/cloud-init/user-data.secure#L20
to *also* disable the relevant services, right?

It's okay, but so far no one seems to have linked any real world users of AUTOCREATE_SERVER_KEYS, so now there's one here.

Also worth noting the architecture which sets it via cloud-init's boothook.  Should still be fine as that happens before sshd.

openssh-7.2p2-4.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-965bd6926e

I don't think you need to disable the services. If can generate the key, and you can just not use it.

(In reply to Colin Walters from comment #16)
> Just for reference, I will have to change
> https://github.com/cgwalters/ansible-personal/blob/master/cloud-init/user-
> data.secure#L20
> to *also* disable the relevant services, right?

One (the most correct) way would be to disable autocreation of rsa and ecdsa keys:

  systemctl mask sshd-keygen@rsa
  systemctl mask sshd-keygen@ecsa

(disable does not work in this configuration, when we have Wants= in sshd-keygen.target. Any thoughts from systemd guys?)

The other would be to create all of them and use just ed25519 keys by modifying  sshd_config option

    HostKey /etc/ssh/ssh_host_ed25519_key


> It's okay, but so far no one seems to have linked any real world users of
> AUTOCREATE_SERVER_KEYS, so now there's one here.

Thanks for mentioning it.

(In reply to Jakub Jelen from comment #19)
> (In reply to Colin Walters from comment #16)
> > Just for reference, I will have to change
> > https://github.com/cgwalters/ansible-personal/blob/master/cloud-init/user-
> > data.secure#L20
> > to *also* disable the relevant services, right?
> 
> One (the most correct) way would be to disable autocreation of rsa and ecdsa
> keys:
> 
>   systemctl mask sshd-keygen@rsa
>   systemctl mask sshd-keygen@ecsa
> 
> (disable does not work in this configuration, when we have Wants= in
> sshd-keygen.target. Any thoughts from systemd guys?)
> 

That's exactly as designed. "Disabled" means that the default target (usually either multi-user.target or graphical.target) doesn't try to auto-start it directly, but it can be pulled in as a dependency of another unit. "Masked" means that it never gets started at all under any circumstances.


All that said, the simpler approach is to do what is specified in https://fedoraproject.org/wiki/Packaging:Initial_Service_Setup which is to just have the service use ConditionPathExists to skip running if a key already exists (which may have been added manually or via previous autogeneration).

(In reply to Stephen Gallagher from comment #20)
> That's exactly as designed. "Disabled" means that the default target
> (usually either multi-user.target or graphical.target) doesn't try to
> auto-start it directly, but it can be pulled in as a dependency of another
> unit. "Masked" means that it never gets started at all under any
> circumstances.

Thanks. It makes sense.

> All that said, the simpler approach is to do what is specified in
> https://fedoraproject.org/wiki/Packaging:Initial_Service_Setup which is to
> just have the service use ConditionPathExists to skip running if a key
> already exists (which may have been added manually or via previous
> autogeneration).

But it is not the problem here. Condition is in place and working fine. The discussed case is now  when user does not want to generate some keys even if they are not there. But the mask option sounds reasonable and is probably right way to go also for further documentation.

(In reply to Jakub Jelen from comment #21)
> But it is not the problem here. Condition is in place and working fine. The
> discussed case is now  when user does not want to generate some keys even if
> they are not there. But the mask option sounds reasonable and is probably
> right way to go also for further documentation.


Sorry, I misunderstood. I'm not sure if that's the right approach necessarily, though. If they don't want to use the keys, it seems to me like that should be a decision is best made by the user in the sshd_config file. Basing whether to use the key upon whether it exists is error-prone: if it suddenly got created it would cause an unexpected behavior change.

(In reply to Stephen Gallagher from comment #22)
> Sorry, I misunderstood. I'm not sure if that's the right approach
> necessarily, though. If they don't want to use the keys, it seems to me like
> that should be a decision is best made by the user in the sshd_config file.
> Basing whether to use the key upon whether it exists is error-prone: if it
> suddenly got created it would cause an unexpected behavior change.

No no no. The default  sshd_config  of course contains the list of keys that should be used:

  HostKey /etc/ssh/ssh_host_rsa_key
  #HostKey /etc/ssh/ssh_host_dsa_key
  HostKey /etc/ssh/ssh_host_ecdsa_key
  HostKey /etc/ssh/ssh_host_ed25519_key


By commenting out, removing or adding a new one, you define what keys are used. 

The sshd-keygen is different mechanism, which is completely unrelated to the list of keys actually used. I tried to explain in the mail on devel@[1], but maybe it was not clear. This bugzilla is only about sshd-keygen service and about key creation during first boot or when administrator decides to rotate the keys.

[1] https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/RAVATCRUEWV7FX56Z2BV32RWPTT2YGAO/#2AHH4AFYYDWPE6SUG3ZCQJKNAXNUWDT7

openssh-7.2p2-4.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.