[PATCH] sshd-regen-keys: fix race condition

[PATCH] sshd-regen-keys: fix race condition

[Q. Gylstorff]

From: Quirin Gylstorff <quirin.gylstorff@siemens.com>

Systemd waits with starting service until a oneshot is finished this leads
to a race condition if you try to restart a service in a oneshot.

"Behavior of oneshot is similar to simple; however, the service manager will consider
the unit started after the main process exits. It will then start follow-up units.
RemainAfterExit= is particularly useful for this type of service.  Type=oneshot is the
implied default if neither Type= nor ExecStart= are specified."[1]

[1]: man systemd.service

Signed-off-by: Quirin Gylstorff <quirin.gylstorff@siemens.com>
---
 .../sshd-regen-keys/files/sshd-regen-keys.service               | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 a05e1a9..4c4dc0e 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
@@ -7,7 +7,7 @@ Before=shutdown.target sshd.service
 ConditionPathIsReadWrite=/etc
 
 [Service]
-Type=oneshot
+Type=simple
 RemainAfterExit=yes
 Environment=DEBIAN_FRONTEND=noninteractive
 ExecStart=/usr/sbin/sshd-regen-keys.sh
-- 
2.20.1

Re: [PATCH] sshd-regen-keys: fix race condition

[cedric_hombourger]

[-- Attachment #1.1: Type: text/plain, Size: 1512 bytes --]



On Thursday, March 12, 2020 at 5:48:39 PM UTC+1, Q. Gylstorff wrote:
>
> From: Quirin Gylstorff <quirin....@siemens.com <javascript:>> 
>
> Systemd waits with starting service until a oneshot is finished this leads 
> to a race condition if you try to restart a service in a oneshot. 
>
>
very good catch!
 

> "Behavior of oneshot is similar to simple; however, the service manager 
> will consider 
> the unit started after the main process exits. It will then start 
> follow-up units. 
> RemainAfterExit= is particularly useful for this type of service. 
>  Type=oneshot is the 
> implied default if neither Type= nor ExecStart= are specified."[1] 
>
> [1]: man systemd.service 
>
> Signed-off-by: Quirin Gylstorff <quirin....@siemens.com <javascript:>> 
> --- 
>  .../sshd-regen-keys/files/sshd-regen-keys.service               | 2 +- 
>  1 file changed, 1 insertion(+), 1 deletion(-) 
>
> 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 a05e1a9..4c4dc0e 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 
> @@ -7,7 +7,7 @@ Before=shutdown.target sshd.service 
>  ConditionPathIsReadWrite=/etc 
>   
>  [Service] 
> -Type=oneshot 
> +Type=simple 
>  RemainAfterExit=yes 
>  Environment=DEBIAN_FRONTEND=noninteractive 
>  ExecStart=/usr/sbin/sshd-regen-keys.sh 
> -- 
> 2.20.1 
>
>

[-- Attachment #1.2: Type: text/html, Size: 2375 bytes --]

Re: [PATCH] sshd-regen-keys: fix race condition

[Baurzhan Ismagulov]

Hello Quirin,

On Thu, Mar 12, 2020 at 05:48:37PM +0100, Q. Gylstorff wrote:
> Systemd waits with starting service until a oneshot is finished this leads
> to a race condition if you try to restart a service in a oneshot.
> 
> "Behavior of oneshot is similar to simple; however, the service manager will consider
> the unit started after the main process exits. It will then start follow-up units.
> RemainAfterExit= is particularly useful for this type of service.  Type=oneshot is the
> implied default if neither Type= nor ExecStart= are specified."[1]
> 
> [1]: man systemd.service

Could you please help me understand the race you are facing? I've gone through
a couple of scenarios and couldn't identify one.


Apart from that, systemctl(1) says for enable:

"Note that this does not have the effect of also starting any of the units
being enabled. If this is desired, combine this command with the --now switch,
or invoke start with appropriate arguments later."

Similarly, for disable:

"Note that this command does not implicitly stop the units that are being
disabled. If this is desired, either combine this command with the --now
switch, or invoke the stop command with appropriate arguments later."

Considering the following scenario:

1. systemd starts ssh. It reads e.g. one key file but not others.

2. systemd starts sshd-regen-keys.sh. It disables ssh but doesn't stop it, then
   removes the keys.

3. sshd continues reading the other keys.

Is it possible that sshd finds inconsistent set of keys or doesn't find the
other keys? Shouldn't we specify --now for both enable and disable?


With kind regards,
Baurzhan.

Re: [PATCH] sshd-regen-keys: fix race condition

[Jan Kiszka]

On 13.04.20 18:22, Baurzhan Ismagulov wrote:
> Hello Quirin,
> 
> On Thu, Mar 12, 2020 at 05:48:37PM +0100, Q. Gylstorff wrote:
>> Systemd waits with starting service until a oneshot is finished this leads
>> to a race condition if you try to restart a service in a oneshot.
>>
>> "Behavior of oneshot is similar to simple; however, the service manager will consider
>> the unit started after the main process exits. It will then start follow-up units.
>> RemainAfterExit= is particularly useful for this type of service.  Type=oneshot is the
>> implied default if neither Type= nor ExecStart= are specified."[1]
>>
>> [1]: man systemd.service
> 
> Could you please help me understand the race you are facing? I've gone through
> a couple of scenarios and couldn't identify one.
> 
> 
> Apart from that, systemctl(1) says for enable:
> 
> "Note that this does not have the effect of also starting any of the units
> being enabled. If this is desired, combine this command with the --now switch,
> or invoke start with appropriate arguments later."
> 
> Similarly, for disable:
> 
> "Note that this command does not implicitly stop the units that are being
> disabled. If this is desired, either combine this command with the --now
> switch, or invoke the stop command with appropriate arguments later."
> 
> Considering the following scenario:
> 
> 1. systemd starts ssh. It reads e.g. one key file but not others.
> 
> 2. systemd starts sshd-regen-keys.sh. It disables ssh but doesn't stop it, then
>     removes the keys.
> 
> 3. sshd continues reading the other keys.
> 
> Is it possible that sshd finds inconsistent set of keys or doesn't find the
> other keys? Shouldn't we specify --now for both enable and disable?
> 
> 
> With kind regards,
> Baurzhan.
> 

Quirin, I think this is still open, and - being about to create another 
one-shot service - I was wondering whether we need to fix more services.

Baurzhan, please fix your client settings so that you always preserve CC 
lists when replying.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] sshd-regen-keys: fix race condition

[Gylstorff Quirin]

The problem as shown at my system was that in sshd-regen-keys calls
dpkg-reconfigure which calls systemd restart ssh. The restart of ssh
is blocked by the oneshot. If you use an onshot without dpkg-reconfigure
it is no problem.

So it is
oneshot.service starts ->  systemd restart some.service -> error
simple.service starts -> systemd restart some.service -> ok

This occures on Debian 10.

Quirin

On 4/28/20 8:22 AM, Jan Kiszka wrote:
> On 13.04.20 18:22, Baurzhan Ismagulov wrote:
>> Hello Quirin,
>>
>> On Thu, Mar 12, 2020 at 05:48:37PM +0100, Q. Gylstorff wrote:
>>> Systemd waits with starting service until a oneshot is finished this 
>>> leads
>>> to a race condition if you try to restart a service in a oneshot.
>>>
>>> "Behavior of oneshot is similar to simple; however, the service 
>>> manager will consider
>>> the unit started after the main process exits. It will then start 
>>> follow-up units.
>>> RemainAfterExit= is particularly useful for this type of service.  
>>> Type=oneshot is the
>>> implied default if neither Type= nor ExecStart= are specified."[1]
>>>
>>> [1]: man systemd.service
>>
>> Could you please help me understand the race you are facing? I've gone 
>> through
>> a couple of scenarios and couldn't identify one.
>>
>>
>> Apart from that, systemctl(1) says for enable:
>>
>> "Note that this does not have the effect of also starting any of the 
>> units
>> being enabled. If this is desired, combine this command with the --now 
>> switch,
>> or invoke start with appropriate arguments later."
>>
>> Similarly, for disable:
>>
>> "Note that this command does not implicitly stop the units that are being
>> disabled. If this is desired, either combine this command with the --now
>> switch, or invoke the stop command with appropriate arguments later."
>>
>> Considering the following scenario:
>>
>> 1. systemd starts ssh. It reads e.g. one key file but not others.
>>
>> 2. systemd starts sshd-regen-keys.sh. It disables ssh but doesn't stop 
>> it, then
>>     removes the keys.
>>
>> 3. sshd continues reading the other keys.
>>
>> Is it possible that sshd finds inconsistent set of keys or doesn't 
>> find the
>> other keys? Shouldn't we specify --now for both enable and disable?
>>
>>
>> With kind regards,
>> Baurzhan.
>>
> 
> Quirin, I think this is still open, and - being about to create another 
> one-shot service - I was wondering whether we need to fix more services.
> 
> Baurzhan, please fix your client settings so that you always preserve CC 
> lists when replying.
> 
> Thanks,
> Jan
> 

-- 
Quirin Gylstorff

Siemens AG
Corporate Technology
Research in Digitalization and Automation
Smart Embedded Systems
CT RDA IOT SES-DE
Otto-Hahn-Ring 6
81739 Muenchen, Germany
Mobile: +49 173 3746683
mailto:quirin.gylstorff@siemens.com
www.siemens.com/ingenuityforlife

Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Jim 
Hagemann Snabe; Managing Board: Joe Kaeser, Chairman, President and 
Chief Executive Officer; Roland Busch, Lisa Davis, Klaus Helmrich, 
Cedrik Neike, Michael Sen, Ralf P. Thomas; Registered offices: Berlin 
and Munich, Germany; Commercial registries: Berlin Charlottenburg, HRB 
12300, Munich, HRB 6684; WEEE-Reg.-No. DE 23691322

Important notice: This e-mail and any attachment thereof contain 
corporate proprietary information. If you have received it by mistake, 
please notify us immediately by reply e-mail and delete this e-mail and 
its attachments from your system. Thank you.