| Message ID | 20200123121755.vp2n7mx3xxpbrbyz@zenon.in.qult.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] package/openssh: improve init script | expand |
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
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
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
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
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 $? -
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(-)