[Buildroot] [PATCH 1/1] package/openssh: improve init script

[Buildroot] [PATCH 1/1] package/openssh: improve init script

[Ignacy Gawędzki]

The current init script for sshd is too simplistic and its use of
killall to terminate the daemon has the annoying downside of killing
every instance of sshd, possibly including the one spawned for the
interactive session in which the script itself is started.  If the
intention was to simply restart the daemon, killing the current
session ultimately kills the script and the daemon is not properly
started again.

Improve the init script in the following ways:

  Use start-stop-daemon to avoid running the daemon more than once and
  to safely send termination signals to the right process.

  Add a proper reload action, by sending the daemon the SIGHUP signal.

  During restart, check that the daemon has properly terminated and if
  not, wait one second before sending it the SIGKILL signal.

Signed-off-by: Ignacy Gaw?dzki <ignacy.gawedzki@green-communications.fr>
---
 package/openssh/S50sshd | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
index 22da41d1ca..66cdd5e291 100644
--- a/package/openssh/S50sshd
+++ b/package/openssh/S50sshd
@@ -13,20 +13,29 @@ start() {
 	/usr/bin/ssh-keygen -A
 
 	printf "Starting sshd: "
-	/usr/sbin/sshd
-	touch /var/lock/sshd
-	echo "OK"
+	start-stop-daemon -S -q -x /usr/sbin/sshd -p /run/sshd.pid &&
+	{ touch /var/lock/sshd; echo "OK"; } || echo "FAIL"
 }
 stop() {
 	printf "Stopping sshd: "
-	killall sshd
-	rm -f /var/lock/sshd
-	echo "OK"
+	start-stop-daemon -K -q -x /usr/sbin/sshd -p /run/sshd.pid &&
+	{ rm -f /var/lock/sshd; echo "OK"; } || echo "FAIL"
 }
 restart() {
 	stop
+	# Ensure the daemon has terminated before calling start again.
+	if start-stop-daemon -K -q -x /usr/sbin/sshd -p /run/sshd.pid -t; then
+		sleep 1
+		start-stop-daemon -K -s KILL -q -x /usr/sbin/sshd \
+				  -p /run/sshd.pid
+	fi
 	start
 }
+reload() {
+	printf "Reloading sshd: "
+	start-stop-daemon -K -s HUP -q -x /usr/sbin/sshd -p /run/sshd.pid &&
+	echo "OK" || echo "FAIL"
+}
 
 case "$1" in
   start)
@@ -35,13 +44,15 @@ case "$1" in
   stop)
 	stop
 	;;
-  restart|reload)
+  reload)
+	reload
+	;;
+  restart)
 	restart
 	;;
   *)
-	echo "Usage: $0 {start|stop|restart}"
+	echo "Usage: $0 {start|stop|reload|restart}"
 	exit 1
 esac
 
 exit $?
-
-- 
2.20.1

[Buildroot] [PATCH 1/1] package/openssh: improve init script

[Thomas Petazzoni]

Hello Ignacy,

Thanks a lot for working on the openssh init script. I'm adding in Cc:
Carlos Santos, who worked on a template for our init scripts, and is
generally trying to make our init script more consistent.

See some comments below.

On Thu, 23 Jan 2020 13:17:55 +0100
Ignacy Gaw?dzki <ignacy.gawedzki@green-communications.fr> wrote:

> The current init script for sshd is too simplistic and its use of
> killall to terminate the daemon has the annoying downside of killing
> every instance of sshd, possibly including the one spawned for the
> interactive session in which the script itself is started.  If the
> intention was to simply restart the daemon, killing the current
> session ultimately kills the script and the daemon is not properly
> started again.
> 
> Improve the init script in the following ways:
> 
>   Use start-stop-daemon to avoid running the daemon more than once and
>   to safely send termination signals to the right process.
> 
>   Add a proper reload action, by sending the daemon the SIGHUP signal.

This should ideally be done in a separate patch.

>   During restart, check that the daemon has properly terminated and if
>   not, wait one second before sending it the SIGKILL signal.

We never do that in any of our other init scripts, I'm not sure we want
to do that here. Is there any reason to do that?

> Signed-off-by: Ignacy Gaw?dzki <ignacy.gawedzki@green-communications.fr>
> ---
>  package/openssh/S50sshd | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)

Overall, could you look at package/busybox/S01syslogd, and use it as a
template for S50openssh ?

> diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
> index 22da41d1ca..66cdd5e291 100644
> --- a/package/openssh/S50sshd
> +++ b/package/openssh/S50sshd
> @@ -13,20 +13,29 @@ start() {
>  	/usr/bin/ssh-keygen -A
>  
>  	printf "Starting sshd: "
> -	/usr/sbin/sshd
> -	touch /var/lock/sshd
> -	echo "OK"
> +	start-stop-daemon -S -q -x /usr/sbin/sshd -p /run/sshd.pid &&
> +	{ touch /var/lock/sshd; echo "OK"; } || echo "FAIL"

I am a bit confused by this touch /var/lock/sshd. Is it really
necessary? The git history was not very helpful on this, as it has been
in Buildroot since the init script was introduced years ago. The fact
that it is created *after* the sshd daemon is started makes it really
weird.

Do you have any comment/insight on this ?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] package/openssh: improve init script

[Ignacy Gawędzki]

On Mon, Feb 03, 2020 at 02:27:21PM +0100, thus spake Thomas Petazzoni:
> Hello Ignacy,

Hi,

> Thanks a lot for working on the openssh init script. I'm adding in Cc:
> Carlos Santos, who worked on a template for our init scripts, and is
> generally trying to make our init script more consistent.
> 
> See some comments below.

See my replies below them.

> On Thu, 23 Jan 2020 13:17:55 +0100
> Ignacy Gaw?dzki <ignacy.gawedzki@green-communications.fr> wrote:
> 
> > The current init script for sshd is too simplistic and its use of
> > killall to terminate the daemon has the annoying downside of killing
> > every instance of sshd, possibly including the one spawned for the
> > interactive session in which the script itself is started.  If the
> > intention was to simply restart the daemon, killing the current
> > session ultimately kills the script and the daemon is not properly
> > started again.
> > 
> > Improve the init script in the following ways:
> > 
> >   Use start-stop-daemon to avoid running the daemon more than once and
> >   to safely send termination signals to the right process.
> > 
> >   Add a proper reload action, by sending the daemon the SIGHUP signal.
> 
> This should ideally be done in a separate patch.
> 
> >   During restart, check that the daemon has properly terminated and if
> >   not, wait one second before sending it the SIGKILL signal.
> 
> We never do that in any of our other init scripts, I'm not sure we want
> to do that here. Is there any reason to do that?

Of course, if start-stop-daemon -S finds a matching running process,
it returns an error.  Sending a SIGTERM to the running process during
the stop phase doesn't guarantee that the process will have already terminated
during the start phase, even after sleeping.  If I could use
the --retry option of standalone start-stop-daemon, I would.  But with
the assumption of Busybox, I need to make sure that the daemon has
terminated by the time I attempt to start it again.

> > Signed-off-by: Ignacy Gaw?dzki <ignacy.gawedzki@green-communications.fr>
> > ---
> >  package/openssh/S50sshd | 29 ++++++++++++++++++++---------
> >  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> Overall, could you look at package/busybox/S01syslogd, and use it as a
> template for S50openssh ?

If you mean that I should be testing for start-stop-daemon's return
code explicitly by setting status=$? and using if-then-else-fi
constructs, then sure, yes.

> 
> > diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
> > index 22da41d1ca..66cdd5e291 100644
> > --- a/package/openssh/S50sshd
> > +++ b/package/openssh/S50sshd
> > @@ -13,20 +13,29 @@ start() {
> >  	/usr/bin/ssh-keygen -A
> >  
> >  	printf "Starting sshd: "
> > -	/usr/sbin/sshd
> > -	touch /var/lock/sshd
> > -	echo "OK"
> > +	start-stop-daemon -S -q -x /usr/sbin/sshd -p /run/sshd.pid &&
> > +	{ touch /var/lock/sshd; echo "OK"; } || echo "FAIL"
> 
> I am a bit confused by this touch /var/lock/sshd. Is it really
> necessary? The git history was not very helpful on this, as it has been
> in Buildroot since the init script was introduced years ago. The fact
> that it is created *after* the sshd daemon is started makes it really
> weird.
> 
> Do you have any comment/insight on this ?

Quite frankly, I have no idea why it's there and by keeping it, I just
wanted stay compatible with whatever is possibly using it.

Cheers,

Ignacy

-- 
Ignacy Gaw?dzki
R&D Engineer
Green Communications

[Buildroot] [PATCH 1/1] package/openssh: improve init script

[Thomas Petazzoni]

Hello,

On Mon, 3 Feb 2020 15:12:53 +0100
Ignacy Gaw?dzki <ignacy.gawedzki@green-communications.fr> wrote:

> > > Improve the init script in the following ways:
> > > 
> > >   Use start-stop-daemon to avoid running the daemon more than once and
> > >   to safely send termination signals to the right process.
> > > 
> > >   Add a proper reload action, by sending the daemon the SIGHUP signal.  
> > 
> > This should ideally be done in a separate patch.
> >   
> > >   During restart, check that the daemon has properly terminated and if
> > >   not, wait one second before sending it the SIGKILL signal.  
> > 
> > We never do that in any of our other init scripts, I'm not sure we want
> > to do that here. Is there any reason to do that?  
> 
> Of course, if start-stop-daemon -S finds a matching running process,
> it returns an error.  Sending a SIGTERM to the running process during
> the stop phase doesn't guarantee that the process will have already terminated
> during the start phase, even after sleeping.  If I could use
> the --retry option of standalone start-stop-daemon, I would.  But with
> the assumption of Busybox, I need to make sure that the daemon has
> terminated by the time I attempt to start it again.

The problem that you're exposing here doesn't seem to be specific to
openssh, so I think we don't want to solve it specifically for openssh.

> > Overall, could you look at package/busybox/S01syslogd, and use it as a
> > template for S50openssh ?  
> 
> If you mean that I should be testing for start-stop-daemon's return
> code explicitly by setting status=$? and using if-then-else-fi
> constructs, then sure, yes.

I meant to make sure the init script looks as much as possible like
S01syslogd, i.e same indentation, same organization, same logic. As I
said, Carlos started an effort to try to make our init scripts more
consistent between each other, so let's try to go in that direction :-)

> > I am a bit confused by this touch /var/lock/sshd. Is it really
> > necessary? The git history was not very helpful on this, as it has been
> > in Buildroot since the init script was introduced years ago. The fact
> > that it is created *after* the sshd daemon is started makes it really
> > weird.
> > 
> > Do you have any comment/insight on this ?  
> 
> Quite frankly, I have no idea why it's there and by keeping it, I just
> wanted stay compatible with whatever is possibly using it.

We had a brief look at it with Peter Korsgaard, we don't see why it
would be needed. Could you include a preparation patch in your series
that drops this touch+rm of /var/lock/sshd ?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/1] package/openssh: improve init script

[Ignacy Gawędzki]

Hi,

On Mon, Feb 03, 2020 at 03:26:03PM +0100, thus spake Thomas Petazzoni:
> > Of course, if start-stop-daemon -S finds a matching running process,
> > it returns an error.  Sending a SIGTERM to the running process during
> > the stop phase doesn't guarantee that the process will have already terminated
> > during the start phase, even after sleeping.  If I could use
> > the --retry option of standalone start-stop-daemon, I would.  But with
> > the assumption of Busybox, I need to make sure that the daemon has
> > terminated by the time I attempt to start it again.
> 
> The problem that you're exposing here doesn't seem to be specific to
> openssh, so I think we don't want to solve it specifically for openssh.

Could you please clarify your point?  Do you mean that I should give
up trying to harden openssh in that regard or that we should harden
every init script in Buildroot in a separate patch?

> > > Overall, could you look at package/busybox/S01syslogd, and use it as a
> > > template for S50openssh ?  
> > 
> > If you mean that I should be testing for start-stop-daemon's return
> > code explicitly by setting status=$? and using if-then-else-fi
> > constructs, then sure, yes.
> 
> I meant to make sure the init script looks as much as possible like
> S01syslogd, i.e same indentation, same organization, same logic. As I
> said, Carlos started an effort to try to make our init scripts more
> consistent between each other, so let's try to go in that direction :-)

Fine.

> > > I am a bit confused by this touch /var/lock/sshd. Is it really
> > > necessary? The git history was not very helpful on this, as it has been
> > > in Buildroot since the init script was introduced years ago. The fact
> > > that it is created *after* the sshd daemon is started makes it really
> > > weird.
> > > 
> > > Do you have any comment/insight on this ?  
> > 
> > Quite frankly, I have no idea why it's there and by keeping it, I just
> > wanted stay compatible with whatever is possibly using it.
> 
> We had a brief look at it with Peter Korsgaard, we don't see why it
> would be needed. Could you include a preparation patch in your series
> that drops this touch+rm of /var/lock/sshd ?

Okay, I'll prepare that, but please tell me what should I do regarding
the restart of sshd.

Cheers,

Ignacy

-- 
Ignacy Gaw?dzki
R&D Engineer
Green Communications