diff mbox series

[2/4] package/openssh: improve integration for systemd

Message ID 20200605225905.14082-2-nolange79@gmail.com
State Awaiting Upstream
Delegated to: Thomas Petazzoni
Headers show
Series [1/4] package/openssh: Depend on libaudit if available | expand

Commit Message

Norbert Lange June 5, 2020, 10:59 p.m. UTC 
the openssh daemon is not suited for systemd's simple
service type. dependend services should only start
when sshd is ready to accept connections.

A patch is added from debian to allow openssh
to communicate this state.

Restarts are prevented if the reason is a faulty
config file (errocode 255).

The "user confinement directory" is changed to
'/run/sshd' which is automatically managed by systemd.

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 package/openssh/00-systemd-readiness.patch | 84 ++++++++++++++++++++++
 package/openssh/openssh.mk                 | 14 +++-
 package/openssh/sshd-sysusers.conf         |  2 +-
 package/openssh/sshd.service               | 13 +++-
 4 files changed, 109 insertions(+), 4 deletions(-)
 create mode 100644 package/openssh/00-systemd-readiness.patch

Comments

Thomas Petazzoni June 6, 2020, 8:31 p.m. UTC | #1 
On Sat,  6 Jun 2020 00:59:02 +0200
Norbert Lange <nolange79@gmail.com> wrote:

> the openssh daemon is not suited for systemd's simple
> service type. dependend services should only start
> when sshd is ready to accept connections.
> 
> A patch is added from debian to allow openssh
> to communicate this state.
> 
> Restarts are prevented if the reason is a faulty
> config file (errocode 255).
> 
> The "user confinement directory" is changed to
> '/run/sshd' which is automatically managed by systemd.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/openssh/00-systemd-readiness.patch | 84 ++++++++++++++++++++++
>  package/openssh/openssh.mk                 | 14 +++-
>  package/openssh/sshd-sysusers.conf         |  2 +-
>  package/openssh/sshd.service               | 13 +++-
>  4 files changed, 109 insertions(+), 4 deletions(-)
>  create mode 100644 package/openssh/00-systemd-readiness.patch

On this patch, as well as patches 3/4 and 4/4 in this series, I would
really appreciate some review from people more knowledgeable than I am
in systemd.

Matt ? Jérémy ?

Best regards,

Thomas
Jérémy ROSEN June 7, 2020, 10:54 a.m. UTC | #2 
Le sam. 6 juin 2020 à 00:59, Norbert Lange <nolange79@gmail.com> a écrit :

> the openssh daemon is not suited for systemd's simple
> service type. dependend services should only start
> when sshd is ready to accept connections.
>
> A patch is added from debian to allow openssh
> to communicate this state.
>
> Restarts are prevented if the reason is a faulty
> config file (errocode 255).
>
> The "user confinement directory" is changed to
> '/run/sshd' which is automatically managed by systemd.
>
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/openssh/00-systemd-readiness.patch | 84 ++++++++++++++++++++++
>  package/openssh/openssh.mk                 | 14 +++-
>  package/openssh/sshd-sysusers.conf         |  2 +-
>  package/openssh/sshd.service               | 13 +++-
>  4 files changed, 109 insertions(+), 4 deletions(-)
>  create mode 100644 package/openssh/00-systemd-readiness.patch
>
> diff --git a/package/openssh/00-systemd-readiness.patch
> b/package/openssh/00-systemd-readiness.patch
> new file mode 100644
> index 0000000000..be3b6b0074
> --- /dev/null
> +++ b/package/openssh/00-systemd-readiness.patch
> @@ -0,0 +1,84 @@
> +From ab765b2bd55062a704f09da8f8c1c4ad1d6630a7 Mon Sep 17 00:00:00 2001
> +From: Michael Biebl <biebl@debian.org>
> +Date: Mon, 21 Dec 2015 16:08:47 +0000
> +Subject: Add systemd readiness notification support
> +
> +Bug-Debian: https://bugs.debian.org/778913
> +Forwarded: no
> +Last-Update: 2017-08-22
> +
> +Patch-Name: systemd-readiness.patch
> +---
> + configure.ac | 24 ++++++++++++++++++++++++
> + sshd.c       |  9 +++++++++
> + 2 files changed, 33 insertions(+)
> +
> +diff --git a/configure.ac b/configure.ac
> +index e894db9fc..c119d6fd1 100644
> +--- a/configure.ac
> ++++ b/configure.ac
> +@@ -4499,6 +4499,29 @@ AC_ARG_WITH([kerberos5],
> + AC_SUBST([GSSLIBS])
> + AC_SUBST([K5LIBS])
> +
> ++# Check whether user wants systemd support
> ++SYSTEMD_MSG="no"
> ++AC_ARG_WITH(systemd,
> ++      [  --with-systemd          Enable systemd support],
> ++      [ if test "x$withval" != "xno" ; then
> ++              AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no])
> ++              if test "$PKGCONFIG" != "no"; then
> ++                      AC_MSG_CHECKING([for libsystemd])
> ++                      if $PKGCONFIG --exists libsystemd; then
> ++                              SYSTEMD_CFLAGS=`$PKGCONFIG --cflags
> libsystemd`
> ++                              SYSTEMD_LIBS=`$PKGCONFIG --libs libsystemd`
> ++                              CPPFLAGS="$CPPFLAGS $SYSTEMD_CFLAGS"
> ++                              SSHDLIBS="$SSHDLIBS $SYSTEMD_LIBS"
> ++                              AC_MSG_RESULT([yes])
> ++                              AC_DEFINE(HAVE_SYSTEMD, 1, [Define if you
> want systemd support.])
> ++                              SYSTEMD_MSG="yes"
> ++                      else
> ++                              AC_MSG_RESULT([no])
> ++                      fi
> ++              fi
> ++      fi ]
> ++)
> ++
> + # Looking for programs, paths and files
> +
> + PRIVSEP_PATH=/var/empty
> +@@ -5305,6 +5328,7 @@ echo "                   libldns support: $LDNS_MSG"
> + echo "  Solaris process contract support: $SPC_MSG"
> + echo "           Solaris project support: $SP_MSG"
> + echo "         Solaris privilege support: $SPP_MSG"
> ++echo "                   systemd support: $SYSTEMD_MSG"
> + echo "       IP address in \$DISPLAY hack: $DISPLAY_HACK_MSG"
> + echo "           Translate v4 in v6 hack: $IPV4_IN6_HACK_MSG"
> + echo "                  BSD Auth support: $BSD_AUTH_MSG"
> +diff --git a/sshd.c b/sshd.c
> +index 4e8ff0662..5e7679a33 100644
> +--- a/sshd.c
> ++++ b/sshd.c
> +@@ -85,6 +85,10 @@
> + #include <prot.h>
> + #endif
> +
> ++#ifdef HAVE_SYSTEMD
> ++#include <systemd/sd-daemon.h>
> ++#endif
> ++
> + #include "xmalloc.h"
> + #include "ssh.h"
> + #include "ssh2.h"
> +@@ -1951,6 +1955,11 @@ main(int ac, char **av)
> +                       }
> +               }
> +
> ++#ifdef HAVE_SYSTEMD
> ++              /* Signal systemd that we are ready to accept connections
> */
> ++              sd_notify(0, "READY=1");
> ++#endif
> ++
> +               /* Accept a connection and return in a forked child */
> +               server_accept_loop(&sock_in, &sock_out,
> +                   &newsock, config_s);
> diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
> index 55b917e20a..d425db1428 100644
> --- a/package/openssh/openssh.mk
> +++ b/package/openssh/openssh.mk
> @@ -12,6 +12,7 @@ OPENSSH_CONF_ENV = \
>         LD="$(TARGET_CC)" \
>         LDFLAGS="$(TARGET_CFLAGS)" \
>         LIBS=`$(PKG_CONFIG_HOST_BINARY) --libs openssl`
> +OPENSSH_AUTORECONF = YES
>  OPENSSH_CONF_OPTS = \
>         --sysconfdir=/etc/ssh \
>         --with-default-path=$(BR2_SYSTEM_DEFAULT_PATH) \
> @@ -22,9 +23,20 @@ OPENSSH_CONF_OPTS = \
>         --disable-wtmpx \
>         --disable-strip
>
> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
> +OPENSSH_DEPENDENCIES = systemd
> +
> +OPENSSH_CONF_OPTS += \
> +       --with-privsep-path=/run/sshd \
> +       --with-pid-dir=/run \
> +       --with-systemd
> +
> +else
> +
>  define OPENSSH_PERMISSIONS
>         /var/empty d 755 root root - - - - -
>  endef
>

Do we still need this when using systemd, or can it be commented out ?


> +endif
>
>  ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
>  OPENSSH_CONF_OPTS += --without-pie
> @@ -72,7 +84,7 @@ define OPENSSH_INSTALL_SYSTEMD_SYSUSERS
>  endef
>  else
>  define OPENSSH_USERS
> -       sshd -1 sshd -1 * /var/empty - - SSH drop priv user
> +       sshd -1 sshd -1 * $(if
> $(BR2_PACKAGE_SYSTEMD),/run/sshd,/var/empty) - - SSH drop priv user
>  endef
>  endif
>
> diff --git a/package/openssh/sshd-sysusers.conf
> b/package/openssh/sshd-sysusers.conf
> index ac77aec065..303d0dbb63 100644
> --- a/package/openssh/sshd-sysusers.conf
> +++ b/package/openssh/sshd-sysusers.conf
> @@ -1 +1 @@
> -u sshd - "SSH drop priv user" /var/empty
> +u sshd - "SSH drop priv user" /run/sshd
> diff --git a/package/openssh/sshd.service b/package/openssh/sshd.service
> index b5e96b3a25..715bd3f7eb 100644
> --- a/package/openssh/sshd.service
> +++ b/package/openssh/sshd.service
> @@ -1,11 +1,20 @@
>  [Unit]
>  Description=OpenSSH server daemon
> -After=syslog.target network.target auditd.service
> +Documentation=man:sshd(8) man:sshd_config(5)
> +After=network.target auditd.service


>  [Service]
>  ExecStartPre=/usr/bin/ssh-keygen -A
> -ExecStart=/usr/sbin/sshd -D -e
> +ExecStartPre=/usr/sbin/sshd -t
> +ExecStart=/usr/sbin/sshd -D
>
You droped the -e, so  you are logging to syslog
However you droped the dependency on syslog.target earlier...
(maybe it should be syslog.socket instead of .target, btw)

how exactly do you want to log  ? (I think logging to stdout is better, it
will be
redirected to the journal.


> +ExecReload=/usr/sbin/sshd -t
>  ExecReload=/bin/kill -HUP $MAINPID
> +KillMode=process
>

Wouldn't mixed be better here ?
I'm not really sure what the use-case for procss is anyway...


> +Restart=on-failure
> +RestartPreventExitStatus=255
> +Type=notify
> +RuntimeDirectory=sshd
> +RuntimeDirectoryMode=0755
>
>  [Install]
>  WantedBy=multi-user.target
> --
> 2.26.2
>
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>
Norbert Lange June 7, 2020, 7:03 p.m. UTC | #3 
Am So., 7. Juni 2020 um 12:54 Uhr schrieb Jérémy ROSEN <jeremy.rosen@smile.fr>:
>
>
>
> Le sam. 6 juin 2020 à 00:59, Norbert Lange <nolange79@gmail.com> a écrit :
>>
>> the openssh daemon is not suited for systemd's simple
>> service type. dependend services should only start
>> when sshd is ready to accept connections.
>>
>> A patch is added from debian to allow openssh
>> to communicate this state.
>>
>> Restarts are prevented if the reason is a faulty
>> config file (errocode 255).
>>
>> The "user confinement directory" is changed to
>> '/run/sshd' which is automatically managed by systemd.
>>
>> Signed-off-by: Norbert Lange <nolange79@gmail.com>
>> ---
>>  package/openssh/00-systemd-readiness.patch | 84 ++++++++++++++++++++++
>>  package/openssh/openssh.mk                 | 14 +++-
>>  package/openssh/sshd-sysusers.conf         |  2 +-
>>  package/openssh/sshd.service               | 13 +++-
>>  4 files changed, 109 insertions(+), 4 deletions(-)
>>  create mode 100644 package/openssh/00-systemd-readiness.patch
>>
>> diff --git a/package/openssh/00-systemd-readiness.patch b/package/openssh/00-systemd-readiness.patch
>> new file mode 100644
>> index 0000000000..be3b6b0074
>> --- /dev/null
>> +++ b/package/openssh/00-systemd-readiness.patch
>> @@ -0,0 +1,84 @@
>> +From ab765b2bd55062a704f09da8f8c1c4ad1d6630a7 Mon Sep 17 00:00:00 2001
>> +From: Michael Biebl <biebl@debian.org>
>> +Date: Mon, 21 Dec 2015 16:08:47 +0000
>> +Subject: Add systemd readiness notification support
>> +
>> +Bug-Debian: https://bugs.debian.org/778913
>> +Forwarded: no
>> +Last-Update: 2017-08-22
>> +
>> +Patch-Name: systemd-readiness.patch
>> +---
>> + configure.ac | 24 ++++++++++++++++++++++++
>> + sshd.c       |  9 +++++++++
>> + 2 files changed, 33 insertions(+)
>> +
>> +diff --git a/configure.ac b/configure.ac
>> +index e894db9fc..c119d6fd1 100644
>> +--- a/configure.ac
>> ++++ b/configure.ac
>> +@@ -4499,6 +4499,29 @@ AC_ARG_WITH([kerberos5],
>> + AC_SUBST([GSSLIBS])
>> + AC_SUBST([K5LIBS])
>> +
>> ++# Check whether user wants systemd support
>> ++SYSTEMD_MSG="no"
>> ++AC_ARG_WITH(systemd,
>> ++      [  --with-systemd          Enable systemd support],
>> ++      [ if test "x$withval" != "xno" ; then
>> ++              AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no])
>> ++              if test "$PKGCONFIG" != "no"; then
>> ++                      AC_MSG_CHECKING([for libsystemd])
>> ++                      if $PKGCONFIG --exists libsystemd; then
>> ++                              SYSTEMD_CFLAGS=`$PKGCONFIG --cflags libsystemd`
>> ++                              SYSTEMD_LIBS=`$PKGCONFIG --libs libsystemd`
>> ++                              CPPFLAGS="$CPPFLAGS $SYSTEMD_CFLAGS"
>> ++                              SSHDLIBS="$SSHDLIBS $SYSTEMD_LIBS"
>> ++                              AC_MSG_RESULT([yes])
>> ++                              AC_DEFINE(HAVE_SYSTEMD, 1, [Define if you want systemd support.])
>> ++                              SYSTEMD_MSG="yes"
>> ++                      else
>> ++                              AC_MSG_RESULT([no])
>> ++                      fi
>> ++              fi
>> ++      fi ]
>> ++)
>> ++
>> + # Looking for programs, paths and files
>> +
>> + PRIVSEP_PATH=/var/empty
>> +@@ -5305,6 +5328,7 @@ echo "                   libldns support: $LDNS_MSG"
>> + echo "  Solaris process contract support: $SPC_MSG"
>> + echo "           Solaris project support: $SP_MSG"
>> + echo "         Solaris privilege support: $SPP_MSG"
>> ++echo "                   systemd support: $SYSTEMD_MSG"
>> + echo "       IP address in \$DISPLAY hack: $DISPLAY_HACK_MSG"
>> + echo "           Translate v4 in v6 hack: $IPV4_IN6_HACK_MSG"
>> + echo "                  BSD Auth support: $BSD_AUTH_MSG"
>> +diff --git a/sshd.c b/sshd.c
>> +index 4e8ff0662..5e7679a33 100644
>> +--- a/sshd.c
>> ++++ b/sshd.c
>> +@@ -85,6 +85,10 @@
>> + #include <prot.h>
>> + #endif
>> +
>> ++#ifdef HAVE_SYSTEMD
>> ++#include <systemd/sd-daemon.h>
>> ++#endif
>> ++
>> + #include "xmalloc.h"
>> + #include "ssh.h"
>> + #include "ssh2.h"
>> +@@ -1951,6 +1955,11 @@ main(int ac, char **av)
>> +                       }
>> +               }
>> +
>> ++#ifdef HAVE_SYSTEMD
>> ++              /* Signal systemd that we are ready to accept connections */
>> ++              sd_notify(0, "READY=1");
>> ++#endif
>> ++
>> +               /* Accept a connection and return in a forked child */
>> +               server_accept_loop(&sock_in, &sock_out,
>> +                   &newsock, config_s);
>> diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
>> index 55b917e20a..d425db1428 100644
>> --- a/package/openssh/openssh.mk
>> +++ b/package/openssh/openssh.mk
>> @@ -12,6 +12,7 @@ OPENSSH_CONF_ENV = \
>>         LD="$(TARGET_CC)" \
>>         LDFLAGS="$(TARGET_CFLAGS)" \
>>         LIBS=`$(PKG_CONFIG_HOST_BINARY) --libs openssl`
>> +OPENSSH_AUTORECONF = YES
>>  OPENSSH_CONF_OPTS = \
>>         --sysconfdir=/etc/ssh \
>>         --with-default-path=$(BR2_SYSTEM_DEFAULT_PATH) \
>> @@ -22,9 +23,20 @@ OPENSSH_CONF_OPTS = \
>>         --disable-wtmpx \
>>         --disable-strip
>>
>> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
>> +OPENSSH_DEPENDENCIES = systemd
>> +
>> +OPENSSH_CONF_OPTS += \
>> +       --with-privsep-path=/run/sshd \
>> +       --with-pid-dir=/run \
>> +       --with-systemd
>> +
>> +else
>> +
>>  define OPENSSH_PERMISSIONS
>>         /var/empty d 755 root root - - - - -
>>  endef
>
>
> Do we still need this when using systemd, or can it be commented out ?

Not sure what you mean with "this"?
The OPENSSH_PERMISSIONS block is needed when not using systemd and it
is only active then.

>
>
>>
>> +endif
>>
>>  ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
>>  OPENSSH_CONF_OPTS += --without-pie
>> @@ -72,7 +84,7 @@ define OPENSSH_INSTALL_SYSTEMD_SYSUSERS
>>  endef
>>  else
>>  define OPENSSH_USERS
>> -       sshd -1 sshd -1 * /var/empty - - SSH drop priv user
>> +       sshd -1 sshd -1 * $(if $(BR2_PACKAGE_SYSTEMD),/run/sshd,/var/empty) - - SSH drop priv user
>>  endef
>>  endif
>>
>> diff --git a/package/openssh/sshd-sysusers.conf b/package/openssh/sshd-sysusers.conf
>> index ac77aec065..303d0dbb63 100644
>> --- a/package/openssh/sshd-sysusers.conf
>> +++ b/package/openssh/sshd-sysusers.conf
>> @@ -1 +1 @@
>> -u sshd - "SSH drop priv user" /var/empty
>> +u sshd - "SSH drop priv user" /run/sshd
>> diff --git a/package/openssh/sshd.service b/package/openssh/sshd.service
>> index b5e96b3a25..715bd3f7eb 100644
>> --- a/package/openssh/sshd.service
>> +++ b/package/openssh/sshd.service
>> @@ -1,11 +1,20 @@
>>  [Unit]
>>  Description=OpenSSH server daemon
>> -After=syslog.target network.target auditd.service
>> +Documentation=man:sshd(8) man:sshd_config(5)
>> +After=network.target auditd.service
>>
>>
>>  [Service]
>>  ExecStartPre=/usr/bin/ssh-keygen -A
>> -ExecStart=/usr/sbin/sshd -D -e
>> +ExecStartPre=/usr/sbin/sshd -t
>> +ExecStart=/usr/sbin/sshd -D
>
> You droped the -e, so  you are logging to syslog
> However you droped the dependency on syslog.target earlier...
> (maybe it should be syslog.socket instead of .target, btw)


syslog.target is long long gone, and the syslog will be
unconditionally available
https://www.freedesktop.org/wiki/Software/systemd/syslog/


>
>
> how exactly do you want to log  ? (I think logging to stdout is better, it will be
> redirected to the journal.


stdout is not really useful if syslog is supported.

>
>
>>
>> +ExecReload=/usr/sbin/sshd -t
>>  ExecReload=/bin/kill -HUP $MAINPID
>> +KillMode=process
>
>
> Wouldn't mixed be better here ?
> I'm not really sure what the use-case for procss is anyway...


I taken that from debian, I could not argue against it (there is a
long discussion which I linked above).
Can you argue *for* mixed?

>
>
>>
>> +Restart=on-failure
>> +RestartPreventExitStatus=255
>> +Type=notify
>> +RuntimeDirectory=sshd
>> +RuntimeDirectoryMode=0755
>>
>>  [Install]
>>  WantedBy=multi-user.target
>> --
>> 2.26.2
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>
>
>
> --
>
>
> 20 rue des Jardins
> 92600 Asnières-sur-Seine
>
> Jérémy ROSEN
> Architecte technique
>
>  jeremy.rosen@smile.fr
>   +33 6 88 25 87 42
>  http://www.smile.eu
>
>
>

Regards, Norbert
Jérémy ROSEN June 7, 2020, 7:16 p.m. UTC | #4 
Le dim. 7 juin 2020 à 21:03, Norbert Lange <nolange79@gmail.com> a écrit :

> Am So., 7. Juni 2020 um 12:54 Uhr schrieb Jérémy ROSEN <
> jeremy.rosen@smile.fr>:
> >
> >
> >
> > Le sam. 6 juin 2020 à 00:59, Norbert Lange <nolange79@gmail.com> a
> écrit :
> >>
> >> the openssh daemon is not suited for systemd's simple
> >> service type. dependend services should only start
> >> when sshd is ready to accept connections.
> >>
> >> A patch is added from debian to allow openssh
> >> to communicate this state.
> >>
> >> Restarts are prevented if the reason is a faulty
> >> config file (errocode 255).
> >>
> >> The "user confinement directory" is changed to
> >> '/run/sshd' which is automatically managed by systemd.
> >>
> >> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> >> ---
> >>  package/openssh/00-systemd-readiness.patch | 84 ++++++++++++++++++++++
> >>  package/openssh/openssh.mk                 | 14 +++-
> >>  package/openssh/sshd-sysusers.conf         |  2 +-
> >>  package/openssh/sshd.service               | 13 +++-
> >>  4 files changed, 109 insertions(+), 4 deletions(-)
> >>  create mode 100644 package/openssh/00-systemd-readiness.patch
> >>
> >> diff --git a/package/openssh/00-systemd-readiness.patch
> b/package/openssh/00-systemd-readiness.patch
> >> new file mode 100644
> >> index 0000000000..be3b6b0074
> >> --- /dev/null
> >> +++ b/package/openssh/00-systemd-readiness.patch
> >> @@ -0,0 +1,84 @@
> >> +From ab765b2bd55062a704f09da8f8c1c4ad1d6630a7 Mon Sep 17 00:00:00 2001
> >> +From: Michael Biebl <biebl@debian.org>
> >> +Date: Mon, 21 Dec 2015 16:08:47 +0000
> >> +Subject: Add systemd readiness notification support
> >> +
> >> +Bug-Debian: https://bugs.debian.org/778913
> >> +Forwarded: no
> >> +Last-Update: 2017-08-22
> >> +
> >> +Patch-Name: systemd-readiness.patch
> >> +---
> >> + configure.ac | 24 ++++++++++++++++++++++++
> >> + sshd.c       |  9 +++++++++
> >> + 2 files changed, 33 insertions(+)
> >> +
> >> +diff --git a/configure.ac b/configure.ac
> >> +index e894db9fc..c119d6fd1 100644
> >> +--- a/configure.ac
> >> ++++ b/configure.ac
> >> +@@ -4499,6 +4499,29 @@ AC_ARG_WITH([kerberos5],
> >> + AC_SUBST([GSSLIBS])
> >> + AC_SUBST([K5LIBS])
> >> +
> >> ++# Check whether user wants systemd support
> >> ++SYSTEMD_MSG="no"
> >> ++AC_ARG_WITH(systemd,
> >> ++      [  --with-systemd          Enable systemd support],
> >> ++      [ if test "x$withval" != "xno" ; then
> >> ++              AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no])
> >> ++              if test "$PKGCONFIG" != "no"; then
> >> ++                      AC_MSG_CHECKING([for libsystemd])
> >> ++                      if $PKGCONFIG --exists libsystemd; then
> >> ++                              SYSTEMD_CFLAGS=`$PKGCONFIG --cflags
> libsystemd`
> >> ++                              SYSTEMD_LIBS=`$PKGCONFIG --libs
> libsystemd`
> >> ++                              CPPFLAGS="$CPPFLAGS $SYSTEMD_CFLAGS"
> >> ++                              SSHDLIBS="$SSHDLIBS $SYSTEMD_LIBS"
> >> ++                              AC_MSG_RESULT([yes])
> >> ++                              AC_DEFINE(HAVE_SYSTEMD, 1, [Define if
> you want systemd support.])
> >> ++                              SYSTEMD_MSG="yes"
> >> ++                      else
> >> ++                              AC_MSG_RESULT([no])
> >> ++                      fi
> >> ++              fi
> >> ++      fi ]
> >> ++)
> >> ++
> >> + # Looking for programs, paths and files
> >> +
> >> + PRIVSEP_PATH=/var/empty
> >> +@@ -5305,6 +5328,7 @@ echo "                   libldns support:
> $LDNS_MSG"
> >> + echo "  Solaris process contract support: $SPC_MSG"
> >> + echo "           Solaris project support: $SP_MSG"
> >> + echo "         Solaris privilege support: $SPP_MSG"
> >> ++echo "                   systemd support: $SYSTEMD_MSG"
> >> + echo "       IP address in \$DISPLAY hack: $DISPLAY_HACK_MSG"
> >> + echo "           Translate v4 in v6 hack: $IPV4_IN6_HACK_MSG"
> >> + echo "                  BSD Auth support: $BSD_AUTH_MSG"
> >> +diff --git a/sshd.c b/sshd.c
> >> +index 4e8ff0662..5e7679a33 100644
> >> +--- a/sshd.c
> >> ++++ b/sshd.c
> >> +@@ -85,6 +85,10 @@
> >> + #include <prot.h>
> >> + #endif
> >> +
> >> ++#ifdef HAVE_SYSTEMD
> >> ++#include <systemd/sd-daemon.h>
> >> ++#endif
> >> ++
> >> + #include "xmalloc.h"
> >> + #include "ssh.h"
> >> + #include "ssh2.h"
> >> +@@ -1951,6 +1955,11 @@ main(int ac, char **av)
> >> +                       }
> >> +               }
> >> +
> >> ++#ifdef HAVE_SYSTEMD
> >> ++              /* Signal systemd that we are ready to accept
> connections */
> >> ++              sd_notify(0, "READY=1");
> >> ++#endif
> >> ++
> >> +               /* Accept a connection and return in a forked child */
> >> +               server_accept_loop(&sock_in, &sock_out,
> >> +                   &newsock, config_s);
> >> diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
> >> index 55b917e20a..d425db1428 100644
> >> --- a/package/openssh/openssh.mk
> >> +++ b/package/openssh/openssh.mk
> >> @@ -12,6 +12,7 @@ OPENSSH_CONF_ENV = \
> >>         LD="$(TARGET_CC)" \
> >>         LDFLAGS="$(TARGET_CFLAGS)" \
> >>         LIBS=`$(PKG_CONFIG_HOST_BINARY) --libs openssl`
> >> +OPENSSH_AUTORECONF = YES
> >>  OPENSSH_CONF_OPTS = \
> >>         --sysconfdir=/etc/ssh \
> >>         --with-default-path=$(BR2_SYSTEM_DEFAULT_PATH) \
> >> @@ -22,9 +23,20 @@ OPENSSH_CONF_OPTS = \
> >>         --disable-wtmpx \
> >>         --disable-strip
> >>
> >> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
> >> +OPENSSH_DEPENDENCIES = systemd
> >> +
> >> +OPENSSH_CONF_OPTS += \
> >> +       --with-privsep-path=/run/sshd \
> >> +       --with-pid-dir=/run \
> >> +       --with-systemd
> >> +
> >> +else
> >> +
> >>  define OPENSSH_PERMISSIONS
> >>         /var/empty d 755 root root - - - - -
> >>  endef
> >
> >
> > Do we still need this when using systemd, or can it be commented out ?
>
> Not sure what you mean with "this"?
> The OPENSSH_PERMISSIONS block is needed when not using systemd and it
> is only active then.
>
>
my bad, I missed the enclosing ifeq()


> >
> >
> >>
> >> +endif
> >>
> >>  ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
> >>  OPENSSH_CONF_OPTS += --without-pie
> >> @@ -72,7 +84,7 @@ define OPENSSH_INSTALL_SYSTEMD_SYSUSERS
> >>  endef
> >>  else
> >>  define OPENSSH_USERS
> >> -       sshd -1 sshd -1 * /var/empty - - SSH drop priv user
> >> +       sshd -1 sshd -1 * $(if
> $(BR2_PACKAGE_SYSTEMD),/run/sshd,/var/empty) - - SSH drop priv user
> >>  endef
> >>  endif
> >>
> >> diff --git a/package/openssh/sshd-sysusers.conf
> b/package/openssh/sshd-sysusers.conf
> >> index ac77aec065..303d0dbb63 100644
> >> --- a/package/openssh/sshd-sysusers.conf
> >> +++ b/package/openssh/sshd-sysusers.conf
> >> @@ -1 +1 @@
> >> -u sshd - "SSH drop priv user" /var/empty
> >> +u sshd - "SSH drop priv user" /run/sshd
> >> diff --git a/package/openssh/sshd.service b/package/openssh/sshd.service
> >> index b5e96b3a25..715bd3f7eb 100644
> >> --- a/package/openssh/sshd.service
> >> +++ b/package/openssh/sshd.service
> >> @@ -1,11 +1,20 @@
> >>  [Unit]
> >>  Description=OpenSSH server daemon
> >> -After=syslog.target network.target auditd.service
> >> +Documentation=man:sshd(8) man:sshd_config(5)
> >> +After=network.target auditd.service
> >>
> >>
> >>  [Service]
> >>  ExecStartPre=/usr/bin/ssh-keygen -A
> >> -ExecStart=/usr/sbin/sshd -D -e
> >> +ExecStartPre=/usr/sbin/sshd -t
> >> +ExecStart=/usr/sbin/sshd -D
> >
> > You droped the -e, so  you are logging to syslog
> > However you droped the dependency on syslog.target earlier...
> > (maybe it should be syslog.socket instead of .target, btw)
>
>
> syslog.target is long long gone, and the syslog will be
> unconditionally available
> https://www.freedesktop.org/wiki/Software/systemd/syslog/
>
>
> >
> >
> > how exactly do you want to log  ? (I think logging to stdout is better,
> it will be
> > redirected to the journal.
>
>
> stdout is not really useful if syslog is supported.
>
> i'd go the other way round

syslog is not really necessary if stdout is available,
but it's a matter of taste :P so let's go your way.


> >
> >
> >>
> >> +ExecReload=/usr/sbin/sshd -t
> >>  ExecReload=/bin/kill -HUP $MAINPID
> >> +KillMode=process
> >
> >
> > Wouldn't mixed be better here ?
> > I'm not really sure what the use-case for procss is anyway...
>
>
> I taken that from debian, I could not argue against it (there is a
> long discussion which I linked above).
> Can you argue *for* mixed?
>
>

I didn't see any link
* process : SIGTERM and SIGKILL is sent only to MainPID
* mixed : SIGTERM is sent to MainPID, SIGKILL is sent to every process in
the service cgroup.

This means that if all works well, they do the same thing

in case the MainPID fails to properly terminate its children, process would
leave children alive
but mixed woul kill everybody

Since we are trying to terminate the service, it makes sense to me to make
sur all child process
are killed.

but I don't see your link so I may be missing something

> >
> >
> >>
> >> +Restart=on-failure
> >> +RestartPreventExitStatus=255
> >> +Type=notify
> >> +RuntimeDirectory=sshd
> >> +RuntimeDirectoryMode=0755
> >>
> >>  [Install]
> >>  WantedBy=multi-user.target
> >> --
> >> 2.26.2
> >>
> >> _______________________________________________
> >> buildroot mailing list
> >> buildroot@busybox.net
> >> http://lists.busybox.net/mailman/listinfo/buildroot
> >
> >
> >
> > --
> >
> >
> > 20 rue des Jardins
> > 92600 Asnières-sur-Seine
> >
> > Jérémy ROSEN
> > Architecte technique
> >
> >  jeremy.rosen@smile.fr
> >   +33 6 88 25 87 42
> >  http://www.smile.eu
> >
> >
> >
>
> Regards, Norbert
>
Norbert Lange June 7, 2020, 7:24 p.m. UTC | #5 
Am So., 7. Juni 2020 um 21:16 Uhr schrieb Jérémy ROSEN <jeremy.rosen@smile.fr>:
>
>
>
> Le dim. 7 juin 2020 à 21:03, Norbert Lange <nolange79@gmail.com> a écrit :
>>
>> Am So., 7. Juni 2020 um 12:54 Uhr schrieb Jérémy ROSEN <jeremy.rosen@smile.fr>:
>> >
>> >
>> >
>> > Le sam. 6 juin 2020 à 00:59, Norbert Lange <nolange79@gmail.com> a écrit :
>> >>
>> >> the openssh daemon is not suited for systemd's simple
>> >> service type. dependend services should only start
>> >> when sshd is ready to accept connections.
>> >>
>> >> A patch is added from debian to allow openssh
>> >> to communicate this state.
>> >>
>> >> Restarts are prevented if the reason is a faulty
>> >> config file (errocode 255).
>> >>
>> >> The "user confinement directory" is changed to
>> >> '/run/sshd' which is automatically managed by systemd.
>> >>
>> >> Signed-off-by: Norbert Lange <nolange79@gmail.com>
>> >> ---
>> >>  package/openssh/00-systemd-readiness.patch | 84 ++++++++++++++++++++++
>> >>  package/openssh/openssh.mk                 | 14 +++-
>> >>  package/openssh/sshd-sysusers.conf         |  2 +-
>> >>  package/openssh/sshd.service               | 13 +++-
>> >>  4 files changed, 109 insertions(+), 4 deletions(-)
>> >>  create mode 100644 package/openssh/00-systemd-readiness.patch
>> >>
>> >> diff --git a/package/openssh/00-systemd-readiness.patch b/package/openssh/00-systemd-readiness.patch
>> >> new file mode 100644
>> >> index 0000000000..be3b6b0074
>> >> --- /dev/null
>> >> +++ b/package/openssh/00-systemd-readiness.patch
>> >> @@ -0,0 +1,84 @@
>> >> +From ab765b2bd55062a704f09da8f8c1c4ad1d6630a7 Mon Sep 17 00:00:00 2001
>> >> +From: Michael Biebl <biebl@debian.org>
>> >> +Date: Mon, 21 Dec 2015 16:08:47 +0000
>> >> +Subject: Add systemd readiness notification support
>> >> +
>> >> +Bug-Debian: https://bugs.debian.org/778913
>> >> +Forwarded: no
>> >> +Last-Update: 2017-08-22
>> >> +
>> >> +Patch-Name: systemd-readiness.patch
>> >> +---
>> >> + configure.ac | 24 ++++++++++++++++++++++++
>> >> + sshd.c       |  9 +++++++++
>> >> + 2 files changed, 33 insertions(+)
>> >> +
>> >> +diff --git a/configure.ac b/configure.ac
>> >> +index e894db9fc..c119d6fd1 100644
>> >> +--- a/configure.ac
>> >> ++++ b/configure.ac
>> >> +@@ -4499,6 +4499,29 @@ AC_ARG_WITH([kerberos5],
>> >> + AC_SUBST([GSSLIBS])
>> >> + AC_SUBST([K5LIBS])
>> >> +
>> >> ++# Check whether user wants systemd support
>> >> ++SYSTEMD_MSG="no"
>> >> ++AC_ARG_WITH(systemd,
>> >> ++      [  --with-systemd          Enable systemd support],
>> >> ++      [ if test "x$withval" != "xno" ; then
>> >> ++              AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no])
>> >> ++              if test "$PKGCONFIG" != "no"; then
>> >> ++                      AC_MSG_CHECKING([for libsystemd])
>> >> ++                      if $PKGCONFIG --exists libsystemd; then
>> >> ++                              SYSTEMD_CFLAGS=`$PKGCONFIG --cflags libsystemd`
>> >> ++                              SYSTEMD_LIBS=`$PKGCONFIG --libs libsystemd`
>> >> ++                              CPPFLAGS="$CPPFLAGS $SYSTEMD_CFLAGS"
>> >> ++                              SSHDLIBS="$SSHDLIBS $SYSTEMD_LIBS"
>> >> ++                              AC_MSG_RESULT([yes])
>> >> ++                              AC_DEFINE(HAVE_SYSTEMD, 1, [Define if you want systemd support.])
>> >> ++                              SYSTEMD_MSG="yes"
>> >> ++                      else
>> >> ++                              AC_MSG_RESULT([no])
>> >> ++                      fi
>> >> ++              fi
>> >> ++      fi ]
>> >> ++)
>> >> ++
>> >> + # Looking for programs, paths and files
>> >> +
>> >> + PRIVSEP_PATH=/var/empty
>> >> +@@ -5305,6 +5328,7 @@ echo "                   libldns support: $LDNS_MSG"
>> >> + echo "  Solaris process contract support: $SPC_MSG"
>> >> + echo "           Solaris project support: $SP_MSG"
>> >> + echo "         Solaris privilege support: $SPP_MSG"
>> >> ++echo "                   systemd support: $SYSTEMD_MSG"
>> >> + echo "       IP address in \$DISPLAY hack: $DISPLAY_HACK_MSG"
>> >> + echo "           Translate v4 in v6 hack: $IPV4_IN6_HACK_MSG"
>> >> + echo "                  BSD Auth support: $BSD_AUTH_MSG"
>> >> +diff --git a/sshd.c b/sshd.c
>> >> +index 4e8ff0662..5e7679a33 100644
>> >> +--- a/sshd.c
>> >> ++++ b/sshd.c
>> >> +@@ -85,6 +85,10 @@
>> >> + #include <prot.h>
>> >> + #endif
>> >> +
>> >> ++#ifdef HAVE_SYSTEMD
>> >> ++#include <systemd/sd-daemon.h>
>> >> ++#endif
>> >> ++
>> >> + #include "xmalloc.h"
>> >> + #include "ssh.h"
>> >> + #include "ssh2.h"
>> >> +@@ -1951,6 +1955,11 @@ main(int ac, char **av)
>> >> +                       }
>> >> +               }
>> >> +
>> >> ++#ifdef HAVE_SYSTEMD
>> >> ++              /* Signal systemd that we are ready to accept connections */
>> >> ++              sd_notify(0, "READY=1");
>> >> ++#endif
>> >> ++
>> >> +               /* Accept a connection and return in a forked child */
>> >> +               server_accept_loop(&sock_in, &sock_out,
>> >> +                   &newsock, config_s);
>> >> diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
>> >> index 55b917e20a..d425db1428 100644
>> >> --- a/package/openssh/openssh.mk
>> >> +++ b/package/openssh/openssh.mk
>> >> @@ -12,6 +12,7 @@ OPENSSH_CONF_ENV = \
>> >>         LD="$(TARGET_CC)" \
>> >>         LDFLAGS="$(TARGET_CFLAGS)" \
>> >>         LIBS=`$(PKG_CONFIG_HOST_BINARY) --libs openssl`
>> >> +OPENSSH_AUTORECONF = YES
>> >>  OPENSSH_CONF_OPTS = \
>> >>         --sysconfdir=/etc/ssh \
>> >>         --with-default-path=$(BR2_SYSTEM_DEFAULT_PATH) \
>> >> @@ -22,9 +23,20 @@ OPENSSH_CONF_OPTS = \
>> >>         --disable-wtmpx \
>> >>         --disable-strip
>> >>
>> >> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
>> >> +OPENSSH_DEPENDENCIES = systemd
>> >> +
>> >> +OPENSSH_CONF_OPTS += \
>> >> +       --with-privsep-path=/run/sshd \
>> >> +       --with-pid-dir=/run \
>> >> +       --with-systemd
>> >> +
>> >> +else
>> >> +
>> >>  define OPENSSH_PERMISSIONS
>> >>         /var/empty d 755 root root - - - - -
>> >>  endef
>> >
>> >
>> > Do we still need this when using systemd, or can it be commented out ?
>>
>> Not sure what you mean with "this"?
>> The OPENSSH_PERMISSIONS block is needed when not using systemd and it
>> is only active then.
>>
>
> my bad, I missed the enclosing ifeq()
>
>>
>> >
>> >
>> >>
>> >> +endif
>> >>
>> >>  ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
>> >>  OPENSSH_CONF_OPTS += --without-pie
>> >> @@ -72,7 +84,7 @@ define OPENSSH_INSTALL_SYSTEMD_SYSUSERS
>> >>  endef
>> >>  else
>> >>  define OPENSSH_USERS
>> >> -       sshd -1 sshd -1 * /var/empty - - SSH drop priv user
>> >> +       sshd -1 sshd -1 * $(if $(BR2_PACKAGE_SYSTEMD),/run/sshd,/var/empty) - - SSH drop priv user
>> >>  endef
>> >>  endif
>> >>
>> >> diff --git a/package/openssh/sshd-sysusers.conf b/package/openssh/sshd-sysusers.conf
>> >> index ac77aec065..303d0dbb63 100644
>> >> --- a/package/openssh/sshd-sysusers.conf
>> >> +++ b/package/openssh/sshd-sysusers.conf
>> >> @@ -1 +1 @@
>> >> -u sshd - "SSH drop priv user" /var/empty
>> >> +u sshd - "SSH drop priv user" /run/sshd
>> >> diff --git a/package/openssh/sshd.service b/package/openssh/sshd.service
>> >> index b5e96b3a25..715bd3f7eb 100644
>> >> --- a/package/openssh/sshd.service
>> >> +++ b/package/openssh/sshd.service
>> >> @@ -1,11 +1,20 @@
>> >>  [Unit]
>> >>  Description=OpenSSH server daemon
>> >> -After=syslog.target network.target auditd.service
>> >> +Documentation=man:sshd(8) man:sshd_config(5)
>> >> +After=network.target auditd.service
>> >>
>> >>
>> >>  [Service]
>> >>  ExecStartPre=/usr/bin/ssh-keygen -A
>> >> -ExecStart=/usr/sbin/sshd -D -e
>> >> +ExecStartPre=/usr/sbin/sshd -t
>> >> +ExecStart=/usr/sbin/sshd -D
>> >
>> > You droped the -e, so  you are logging to syslog
>> > However you droped the dependency on syslog.target earlier...
>> > (maybe it should be syslog.socket instead of .target, btw)
>>
>>
>> syslog.target is long long gone, and the syslog will be
>> unconditionally available
>> https://www.freedesktop.org/wiki/Software/systemd/syslog/
>>
>>
>> >
>> >
>> > how exactly do you want to log  ? (I think logging to stdout is better, it will be
>> > redirected to the journal.
>>
>>
>> stdout is not really useful if syslog is supported.
>>
> i'd go the other way round
>
> syslog is not really necessary if stdout is available,
> but it's a matter of taste :P so let's go your way.

Its more the point, that Openssh already implemented syslog, and thats
a clear functional superset of listening to stdout.

>
>>
>> >
>> >
>> >>
>> >> +ExecReload=/usr/sbin/sshd -t
>> >>  ExecReload=/bin/kill -HUP $MAINPID
>> >> +KillMode=process
>> >
>> >
>> > Wouldn't mixed be better here ?
>> > I'm not really sure what the use-case for procss is anyway...
>>
>>
>> I taken that from debian, I could not argue against it (there is a
>> long discussion which I linked above).
>> Can you argue *for* mixed?
>>
>
>
> I didn't see any link
> * process : SIGTERM and SIGKILL is sent only to MainPID
> * mixed : SIGTERM is sent to MainPID, SIGKILL is sent to every process in the service cgroup.
>
> This means that if all works well, they do the same thing
>
> in case the MainPID fails to properly terminate its children, process would leave children alive
> but mixed woul kill everybody
>
> Since we are trying to terminate the service, it makes sense to me to make sur all child process
> are killed.
>
> but I don't see your link so I may be missing something

The link is in the added patch: https://bugs.debian.org/778913

As said, I could not argue either way, but I got some respect for the
debian guys ;)

Norbert
Jérémy ROSEN June 7, 2020, 7:42 p.m. UTC | #6 
Le dim. 7 juin 2020 à 21:24, Norbert Lange <nolange79@gmail.com> a écrit :

> Am So., 7. Juni 2020 um 21:16 Uhr schrieb Jérémy ROSEN <
> jeremy.rosen@smile.fr>:
> >
> >
> >
> > Le dim. 7 juin 2020 à 21:03, Norbert Lange <nolange79@gmail.com> a
> écrit :
> >>
> >> Am So., 7. Juni 2020 um 12:54 Uhr schrieb Jérémy ROSEN <
> jeremy.rosen@smile.fr>:
> >> >
> >> >
> >> >
> >> > Le sam. 6 juin 2020 à 00:59, Norbert Lange <nolange79@gmail.com> a
> écrit :
> >> >>
> >> >> the openssh daemon is not suited for systemd's simple
> >> >> service type. dependend services should only start
> >> >> when sshd is ready to accept connections.
> >> >>
> >> >> A patch is added from debian to allow openssh
> >> >> to communicate this state.
> >> >>
> >> >> Restarts are prevented if the reason is a faulty
> >> >> config file (errocode 255).
> >> >>
> >> >> The "user confinement directory" is changed to
> >> >> '/run/sshd' which is automatically managed by systemd.
> >> >>
> >> >> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> >> >> ---
> >> >>  package/openssh/00-systemd-readiness.patch | 84
> ++++++++++++++++++++++
> >> >>  package/openssh/openssh.mk                 | 14 +++-
> >> >>  package/openssh/sshd-sysusers.conf         |  2 +-
> >> >>  package/openssh/sshd.service               | 13 +++-
> >> >>  4 files changed, 109 insertions(+), 4 deletions(-)
> >> >>  create mode 100644 package/openssh/00-systemd-readiness.patch
> >> >>
> >> >> diff --git a/package/openssh/00-systemd-readiness.patch
> b/package/openssh/00-systemd-readiness.patch
> >> >> new file mode 100644
> >> >> index 0000000000..be3b6b0074
> >> >> --- /dev/null
> >> >> +++ b/package/openssh/00-systemd-readiness.patch
> >> >> @@ -0,0 +1,84 @@
> >> >> +From ab765b2bd55062a704f09da8f8c1c4ad1d6630a7 Mon Sep 17 00:00:00
> 2001
> >> >> +From: Michael Biebl <biebl@debian.org>
> >> >> +Date: Mon, 21 Dec 2015 16:08:47 +0000
> >> >> +Subject: Add systemd readiness notification support
> >> >> +
> >> >> +Bug-Debian: https://bugs.debian.org/778913
> >> >> +Forwarded: no
> >> >> +Last-Update: 2017-08-22
> >> >> +
> >> >> +Patch-Name: systemd-readiness.patch
> >> >> +---
> >> >> + configure.ac | 24 ++++++++++++++++++++++++
> >> >> + sshd.c       |  9 +++++++++
> >> >> + 2 files changed, 33 insertions(+)
> >> >> +
> >> >> +diff --git a/configure.ac b/configure.ac
> >> >> +index e894db9fc..c119d6fd1 100644
> >> >> +--- a/configure.ac
> >> >> ++++ b/configure.ac
> >> >> +@@ -4499,6 +4499,29 @@ AC_ARG_WITH([kerberos5],
> >> >> + AC_SUBST([GSSLIBS])
> >> >> + AC_SUBST([K5LIBS])
> >> >> +
> >> >> ++# Check whether user wants systemd support
> >> >> ++SYSTEMD_MSG="no"
> >> >> ++AC_ARG_WITH(systemd,
> >> >> ++      [  --with-systemd          Enable systemd support],
> >> >> ++      [ if test "x$withval" != "xno" ; then
> >> >> ++              AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no])
> >> >> ++              if test "$PKGCONFIG" != "no"; then
> >> >> ++                      AC_MSG_CHECKING([for libsystemd])
> >> >> ++                      if $PKGCONFIG --exists libsystemd; then
> >> >> ++                              SYSTEMD_CFLAGS=`$PKGCONFIG --cflags
> libsystemd`
> >> >> ++                              SYSTEMD_LIBS=`$PKGCONFIG --libs
> libsystemd`
> >> >> ++                              CPPFLAGS="$CPPFLAGS $SYSTEMD_CFLAGS"
> >> >> ++                              SSHDLIBS="$SSHDLIBS $SYSTEMD_LIBS"
> >> >> ++                              AC_MSG_RESULT([yes])
> >> >> ++                              AC_DEFINE(HAVE_SYSTEMD, 1, [Define
> if you want systemd support.])
> >> >> ++                              SYSTEMD_MSG="yes"
> >> >> ++                      else
> >> >> ++                              AC_MSG_RESULT([no])
> >> >> ++                      fi
> >> >> ++              fi
> >> >> ++      fi ]
> >> >> ++)
> >> >> ++
> >> >> + # Looking for programs, paths and files
> >> >> +
> >> >> + PRIVSEP_PATH=/var/empty
> >> >> +@@ -5305,6 +5328,7 @@ echo "                   libldns support:
> $LDNS_MSG"
> >> >> + echo "  Solaris process contract support: $SPC_MSG"
> >> >> + echo "           Solaris project support: $SP_MSG"
> >> >> + echo "         Solaris privilege support: $SPP_MSG"
> >> >> ++echo "                   systemd support: $SYSTEMD_MSG"
> >> >> + echo "       IP address in \$DISPLAY hack: $DISPLAY_HACK_MSG"
> >> >> + echo "           Translate v4 in v6 hack: $IPV4_IN6_HACK_MSG"
> >> >> + echo "                  BSD Auth support: $BSD_AUTH_MSG"
> >> >> +diff --git a/sshd.c b/sshd.c
> >> >> +index 4e8ff0662..5e7679a33 100644
> >> >> +--- a/sshd.c
> >> >> ++++ b/sshd.c
> >> >> +@@ -85,6 +85,10 @@
> >> >> + #include <prot.h>
> >> >> + #endif
> >> >> +
> >> >> ++#ifdef HAVE_SYSTEMD
> >> >> ++#include <systemd/sd-daemon.h>
> >> >> ++#endif
> >> >> ++
> >> >> + #include "xmalloc.h"
> >> >> + #include "ssh.h"
> >> >> + #include "ssh2.h"
> >> >> +@@ -1951,6 +1955,11 @@ main(int ac, char **av)
> >> >> +                       }
> >> >> +               }
> >> >> +
> >> >> ++#ifdef HAVE_SYSTEMD
> >> >> ++              /* Signal systemd that we are ready to accept
> connections */
> >> >> ++              sd_notify(0, "READY=1");
> >> >> ++#endif
> >> >> ++
> >> >> +               /* Accept a connection and return in a forked child
> */
> >> >> +               server_accept_loop(&sock_in, &sock_out,
> >> >> +                   &newsock, config_s);
> >> >> diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
> >> >> index 55b917e20a..d425db1428 100644
> >> >> --- a/package/openssh/openssh.mk
> >> >> +++ b/package/openssh/openssh.mk
> >> >> @@ -12,6 +12,7 @@ OPENSSH_CONF_ENV = \
> >> >>         LD="$(TARGET_CC)" \
> >> >>         LDFLAGS="$(TARGET_CFLAGS)" \
> >> >>         LIBS=`$(PKG_CONFIG_HOST_BINARY) --libs openssl`
> >> >> +OPENSSH_AUTORECONF = YES
> >> >>  OPENSSH_CONF_OPTS = \
> >> >>         --sysconfdir=/etc/ssh \
> >> >>         --with-default-path=$(BR2_SYSTEM_DEFAULT_PATH) \
> >> >> @@ -22,9 +23,20 @@ OPENSSH_CONF_OPTS = \
> >> >>         --disable-wtmpx \
> >> >>         --disable-strip
> >> >>
> >> >> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
> >> >> +OPENSSH_DEPENDENCIES = systemd
> >> >> +
> >> >> +OPENSSH_CONF_OPTS += \
> >> >> +       --with-privsep-path=/run/sshd \
> >> >> +       --with-pid-dir=/run \
> >> >> +       --with-systemd
> >> >> +
> >> >> +else
> >> >> +
> >> >>  define OPENSSH_PERMISSIONS
> >> >>         /var/empty d 755 root root - - - - -
> >> >>  endef
> >> >
> >> >
> >> > Do we still need this when using systemd, or can it be commented out ?
> >>
> >> Not sure what you mean with "this"?
> >> The OPENSSH_PERMISSIONS block is needed when not using systemd and it
> >> is only active then.
> >>
> >
> > my bad, I missed the enclosing ifeq()
> >
> >>
> >> >
> >> >
> >> >>
> >> >> +endif
> >> >>
> >> >>  ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
> >> >>  OPENSSH_CONF_OPTS += --without-pie
> >> >> @@ -72,7 +84,7 @@ define OPENSSH_INSTALL_SYSTEMD_SYSUSERS
> >> >>  endef
> >> >>  else
> >> >>  define OPENSSH_USERS
> >> >> -       sshd -1 sshd -1 * /var/empty - - SSH drop priv user
> >> >> +       sshd -1 sshd -1 * $(if
> $(BR2_PACKAGE_SYSTEMD),/run/sshd,/var/empty) - - SSH drop priv user
> >> >>  endef
> >> >>  endif
> >> >>
> >> >> diff --git a/package/openssh/sshd-sysusers.conf
> b/package/openssh/sshd-sysusers.conf
> >> >> index ac77aec065..303d0dbb63 100644
> >> >> --- a/package/openssh/sshd-sysusers.conf
> >> >> +++ b/package/openssh/sshd-sysusers.conf
> >> >> @@ -1 +1 @@
> >> >> -u sshd - "SSH drop priv user" /var/empty
> >> >> +u sshd - "SSH drop priv user" /run/sshd
> >> >> diff --git a/package/openssh/sshd.service
> b/package/openssh/sshd.service
> >> >> index b5e96b3a25..715bd3f7eb 100644
> >> >> --- a/package/openssh/sshd.service
> >> >> +++ b/package/openssh/sshd.service
> >> >> @@ -1,11 +1,20 @@
> >> >>  [Unit]
> >> >>  Description=OpenSSH server daemon
> >> >> -After=syslog.target network.target auditd.service
> >> >> +Documentation=man:sshd(8) man:sshd_config(5)
> >> >> +After=network.target auditd.service
> >> >>
> >> >>
> >> >>  [Service]
> >> >>  ExecStartPre=/usr/bin/ssh-keygen -A
> >> >> -ExecStart=/usr/sbin/sshd -D -e
> >> >> +ExecStartPre=/usr/sbin/sshd -t
> >> >> +ExecStart=/usr/sbin/sshd -D
> >> >
> >> > You droped the -e, so  you are logging to syslog
> >> > However you droped the dependency on syslog.target earlier...
> >> > (maybe it should be syslog.socket instead of .target, btw)
> >>
> >>
> >> syslog.target is long long gone, and the syslog will be
> >> unconditionally available
> >> https://www.freedesktop.org/wiki/Software/systemd/syslog/
> >>
> >>
> >> >
> >> >
> >> > how exactly do you want to log  ? (I think logging to stdout is
> better, it will be
> >> > redirected to the journal.
> >>
> >>
> >> stdout is not really useful if syslog is supported.
> >>
> > i'd go the other way round
> >
> > syslog is not really necessary if stdout is available,
> > but it's a matter of taste :P so let's go your way.
>
> Its more the point, that Openssh already implemented syslog, and thats
> a clear functional superset of listening to stdout.
>
> >
> >>
> >> >
> >> >
> >> >>
> >> >> +ExecReload=/usr/sbin/sshd -t
> >> >>  ExecReload=/bin/kill -HUP $MAINPID
> >> >> +KillMode=process
> >> >
> >> >
> >> > Wouldn't mixed be better here ?
> >> > I'm not really sure what the use-case for procss is anyway...
> >>
> >>
> >> I taken that from debian, I could not argue against it (there is a
> >> long discussion which I linked above).
> >> Can you argue *for* mixed?
> >>
> >
> >
> > I didn't see any link
> > * process : SIGTERM and SIGKILL is sent only to MainPID
> > * mixed : SIGTERM is sent to MainPID, SIGKILL is sent to every process
> in the service cgroup.
> >
> > This means that if all works well, they do the same thing
> >
> > in case the MainPID fails to properly terminate its children, process
> would leave children alive
> > but mixed woul kill everybody
> >
> > Since we are trying to terminate the service, it makes sense to me to
> make sur all child process
> > are killed.
> >
> > but I don't see your link so I may be missing something
>
> The link is in the added patch: https://bugs.debian.org/778913
>
> As said, I could not argue either way, but I got some respect for the
> debian guys ;)
>
>
The thread does not actually discuss process vs mixed...

so doesn't really help here.
OTOH, the debian version has been vetted by mbiel which is a systemc
core-maintainer.

so i would go with mixed if I were to write the service from scratch, but
since I don't have an
explanation for the choice of process, I'm not entirely sure...

A possibility is that ssh creates a process per connection. in that case
* process would not kill all ongoing connections
* mixed would

maybe it was chosen to protect existing connection. that would make some
sense.




> Norbert
>
Norbert Lange June 11, 2020, 12:04 a.m. UTC | #7 
Am So., 7. Juni 2020 um 21:42 Uhr schrieb Jérémy ROSEN <jeremy.rosen@smile.fr>:
>
>
>
> Le dim. 7 juin 2020 à 21:24, Norbert Lange <nolange79@gmail.com> a écrit :
>>
>> Am So., 7. Juni 2020 um 21:16 Uhr schrieb Jérémy ROSEN <jeremy.rosen@smile.fr>:
>> >
>> >
>> >
>> > Le dim. 7 juin 2020 à 21:03, Norbert Lange <nolange79@gmail.com> a écrit :
>> >>
>> >> Am So., 7. Juni 2020 um 12:54 Uhr schrieb Jérémy ROSEN <jeremy.rosen@smile.fr>:
>> >> >
>> >> >
>> >> >
>> >> > Le sam. 6 juin 2020 à 00:59, Norbert Lange <nolange79@gmail.com> a écrit :
>> >> >>
>> >> >> the openssh daemon is not suited for systemd's simple
>> >> >> service type. dependend services should only start
>> >> >> when sshd is ready to accept connections.
>> >> >>
>> >> >> A patch is added from debian to allow openssh
>> >> >> to communicate this state.
>> >> >>
>> >> >> Restarts are prevented if the reason is a faulty
>> >> >> config file (errocode 255).
>> >> >>
>> >> >> The "user confinement directory" is changed to
>> >> >> '/run/sshd' which is automatically managed by systemd.
>> >> >>
>> >> >> Signed-off-by: Norbert Lange <nolange79@gmail.com>
>> >> >> ---
>> >> >>  package/openssh/00-systemd-readiness.patch | 84 ++++++++++++++++++++++
>> >> >>  package/openssh/openssh.mk                 | 14 +++-
>> >> >>  package/openssh/sshd-sysusers.conf         |  2 +-
>> >> >>  package/openssh/sshd.service               | 13 +++-
>> >> >>  4 files changed, 109 insertions(+), 4 deletions(-)
>> >> >>  create mode 100644 package/openssh/00-systemd-readiness.patch
>> >> >>
>> >> >> diff --git a/package/openssh/00-systemd-readiness.patch b/package/openssh/00-systemd-readiness.patch
>> >> >> new file mode 100644
>> >> >> index 0000000000..be3b6b0074
>> >> >> --- /dev/null
>> >> >> +++ b/package/openssh/00-systemd-readiness.patch
>> >> >> @@ -0,0 +1,84 @@
>> >> >> +From ab765b2bd55062a704f09da8f8c1c4ad1d6630a7 Mon Sep 17 00:00:00 2001
>> >> >> +From: Michael Biebl <biebl@debian.org>
>> >> >> +Date: Mon, 21 Dec 2015 16:08:47 +0000
>> >> >> +Subject: Add systemd readiness notification support
>> >> >> +
>> >> >> +Bug-Debian: https://bugs.debian.org/778913
>> >> >> +Forwarded: no
>> >> >> +Last-Update: 2017-08-22
>> >> >> +
>> >> >> +Patch-Name: systemd-readiness.patch
>> >> >> +---
>> >> >> + configure.ac | 24 ++++++++++++++++++++++++
>> >> >> + sshd.c       |  9 +++++++++
>> >> >> + 2 files changed, 33 insertions(+)
>> >> >> +
>> >> >> +diff --git a/configure.ac b/configure.ac
>> >> >> +index e894db9fc..c119d6fd1 100644
>> >> >> +--- a/configure.ac
>> >> >> ++++ b/configure.ac
>> >> >> +@@ -4499,6 +4499,29 @@ AC_ARG_WITH([kerberos5],
>> >> >> + AC_SUBST([GSSLIBS])
>> >> >> + AC_SUBST([K5LIBS])
>> >> >> +
>> >> >> ++# Check whether user wants systemd support
>> >> >> ++SYSTEMD_MSG="no"
>> >> >> ++AC_ARG_WITH(systemd,
>> >> >> ++      [  --with-systemd          Enable systemd support],
>> >> >> ++      [ if test "x$withval" != "xno" ; then
>> >> >> ++              AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no])
>> >> >> ++              if test "$PKGCONFIG" != "no"; then
>> >> >> ++                      AC_MSG_CHECKING([for libsystemd])
>> >> >> ++                      if $PKGCONFIG --exists libsystemd; then
>> >> >> ++                              SYSTEMD_CFLAGS=`$PKGCONFIG --cflags libsystemd`
>> >> >> ++                              SYSTEMD_LIBS=`$PKGCONFIG --libs libsystemd`
>> >> >> ++                              CPPFLAGS="$CPPFLAGS $SYSTEMD_CFLAGS"
>> >> >> ++                              SSHDLIBS="$SSHDLIBS $SYSTEMD_LIBS"
>> >> >> ++                              AC_MSG_RESULT([yes])
>> >> >> ++                              AC_DEFINE(HAVE_SYSTEMD, 1, [Define if you want systemd support.])
>> >> >> ++                              SYSTEMD_MSG="yes"
>> >> >> ++                      else
>> >> >> ++                              AC_MSG_RESULT([no])
>> >> >> ++                      fi
>> >> >> ++              fi
>> >> >> ++      fi ]
>> >> >> ++)
>> >> >> ++
>> >> >> + # Looking for programs, paths and files
>> >> >> +
>> >> >> + PRIVSEP_PATH=/var/empty
>> >> >> +@@ -5305,6 +5328,7 @@ echo "                   libldns support: $LDNS_MSG"
>> >> >> + echo "  Solaris process contract support: $SPC_MSG"
>> >> >> + echo "           Solaris project support: $SP_MSG"
>> >> >> + echo "         Solaris privilege support: $SPP_MSG"
>> >> >> ++echo "                   systemd support: $SYSTEMD_MSG"
>> >> >> + echo "       IP address in \$DISPLAY hack: $DISPLAY_HACK_MSG"
>> >> >> + echo "           Translate v4 in v6 hack: $IPV4_IN6_HACK_MSG"
>> >> >> + echo "                  BSD Auth support: $BSD_AUTH_MSG"
>> >> >> +diff --git a/sshd.c b/sshd.c
>> >> >> +index 4e8ff0662..5e7679a33 100644
>> >> >> +--- a/sshd.c
>> >> >> ++++ b/sshd.c
>> >> >> +@@ -85,6 +85,10 @@
>> >> >> + #include <prot.h>
>> >> >> + #endif
>> >> >> +
>> >> >> ++#ifdef HAVE_SYSTEMD
>> >> >> ++#include <systemd/sd-daemon.h>
>> >> >> ++#endif
>> >> >> ++
>> >> >> + #include "xmalloc.h"
>> >> >> + #include "ssh.h"
>> >> >> + #include "ssh2.h"
>> >> >> +@@ -1951,6 +1955,11 @@ main(int ac, char **av)
>> >> >> +                       }
>> >> >> +               }
>> >> >> +
>> >> >> ++#ifdef HAVE_SYSTEMD
>> >> >> ++              /* Signal systemd that we are ready to accept connections */
>> >> >> ++              sd_notify(0, "READY=1");
>> >> >> ++#endif
>> >> >> ++
>> >> >> +               /* Accept a connection and return in a forked child */
>> >> >> +               server_accept_loop(&sock_in, &sock_out,
>> >> >> +                   &newsock, config_s);
>> >> >> diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
>> >> >> index 55b917e20a..d425db1428 100644
>> >> >> --- a/package/openssh/openssh.mk
>> >> >> +++ b/package/openssh/openssh.mk
>> >> >> @@ -12,6 +12,7 @@ OPENSSH_CONF_ENV = \
>> >> >>         LD="$(TARGET_CC)" \
>> >> >>         LDFLAGS="$(TARGET_CFLAGS)" \
>> >> >>         LIBS=`$(PKG_CONFIG_HOST_BINARY) --libs openssl`
>> >> >> +OPENSSH_AUTORECONF = YES
>> >> >>  OPENSSH_CONF_OPTS = \
>> >> >>         --sysconfdir=/etc/ssh \
>> >> >>         --with-default-path=$(BR2_SYSTEM_DEFAULT_PATH) \
>> >> >> @@ -22,9 +23,20 @@ OPENSSH_CONF_OPTS = \
>> >> >>         --disable-wtmpx \
>> >> >>         --disable-strip
>> >> >>
>> >> >> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
>> >> >> +OPENSSH_DEPENDENCIES = systemd
>> >> >> +
>> >> >> +OPENSSH_CONF_OPTS += \
>> >> >> +       --with-privsep-path=/run/sshd \
>> >> >> +       --with-pid-dir=/run \
>> >> >> +       --with-systemd
>> >> >> +
>> >> >> +else
>> >> >> +
>> >> >>  define OPENSSH_PERMISSIONS
>> >> >>         /var/empty d 755 root root - - - - -
>> >> >>  endef
>> >> >
>> >> >
>> >> > Do we still need this when using systemd, or can it be commented out ?
>> >>
>> >> Not sure what you mean with "this"?
>> >> The OPENSSH_PERMISSIONS block is needed when not using systemd and it
>> >> is only active then.
>> >>
>> >
>> > my bad, I missed the enclosing ifeq()
>> >
>> >>
>> >> >
>> >> >
>> >> >>
>> >> >> +endif
>> >> >>
>> >> >>  ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
>> >> >>  OPENSSH_CONF_OPTS += --without-pie
>> >> >> @@ -72,7 +84,7 @@ define OPENSSH_INSTALL_SYSTEMD_SYSUSERS
>> >> >>  endef
>> >> >>  else
>> >> >>  define OPENSSH_USERS
>> >> >> -       sshd -1 sshd -1 * /var/empty - - SSH drop priv user
>> >> >> +       sshd -1 sshd -1 * $(if $(BR2_PACKAGE_SYSTEMD),/run/sshd,/var/empty) - - SSH drop priv user
>> >> >>  endef
>> >> >>  endif
>> >> >>
>> >> >> diff --git a/package/openssh/sshd-sysusers.conf b/package/openssh/sshd-sysusers.conf
>> >> >> index ac77aec065..303d0dbb63 100644
>> >> >> --- a/package/openssh/sshd-sysusers.conf
>> >> >> +++ b/package/openssh/sshd-sysusers.conf
>> >> >> @@ -1 +1 @@
>> >> >> -u sshd - "SSH drop priv user" /var/empty
>> >> >> +u sshd - "SSH drop priv user" /run/sshd
>> >> >> diff --git a/package/openssh/sshd.service b/package/openssh/sshd.service
>> >> >> index b5e96b3a25..715bd3f7eb 100644
>> >> >> --- a/package/openssh/sshd.service
>> >> >> +++ b/package/openssh/sshd.service
>> >> >> @@ -1,11 +1,20 @@
>> >> >>  [Unit]
>> >> >>  Description=OpenSSH server daemon
>> >> >> -After=syslog.target network.target auditd.service
>> >> >> +Documentation=man:sshd(8) man:sshd_config(5)
>> >> >> +After=network.target auditd.service
>> >> >>
>> >> >>
>> >> >>  [Service]
>> >> >>  ExecStartPre=/usr/bin/ssh-keygen -A
>> >> >> -ExecStart=/usr/sbin/sshd -D -e
>> >> >> +ExecStartPre=/usr/sbin/sshd -t
>> >> >> +ExecStart=/usr/sbin/sshd -D
>> >> >
>> >> > You droped the -e, so  you are logging to syslog
>> >> > However you droped the dependency on syslog.target earlier...
>> >> > (maybe it should be syslog.socket instead of .target, btw)
>> >>
>> >>
>> >> syslog.target is long long gone, and the syslog will be
>> >> unconditionally available
>> >> https://www.freedesktop.org/wiki/Software/systemd/syslog/
>> >>
>> >>
>> >> >
>> >> >
>> >> > how exactly do you want to log  ? (I think logging to stdout is better, it will be
>> >> > redirected to the journal.
>> >>
>> >>
>> >> stdout is not really useful if syslog is supported.
>> >>
>> > i'd go the other way round
>> >
>> > syslog is not really necessary if stdout is available,
>> > but it's a matter of taste :P so let's go your way.
>>
>> Its more the point, that Openssh already implemented syslog, and thats
>> a clear functional superset of listening to stdout.
>>
>> >
>> >>
>> >> >
>> >> >
>> >> >>
>> >> >> +ExecReload=/usr/sbin/sshd -t
>> >> >>  ExecReload=/bin/kill -HUP $MAINPID
>> >> >> +KillMode=process
>> >> >
>> >> >
>> >> > Wouldn't mixed be better here ?
>> >> > I'm not really sure what the use-case for procss is anyway...
>> >>
>> >>
>> >> I taken that from debian, I could not argue against it (there is a
>> >> long discussion which I linked above).
>> >> Can you argue *for* mixed?
>> >>
>> >
>> >
>> > I didn't see any link
>> > * process : SIGTERM and SIGKILL is sent only to MainPID
>> > * mixed : SIGTERM is sent to MainPID, SIGKILL is sent to every process in the service cgroup.
>> >
>> > This means that if all works well, they do the same thing
>> >
>> > in case the MainPID fails to properly terminate its children, process would leave children alive
>> > but mixed woul kill everybody
>> >
>> > Since we are trying to terminate the service, it makes sense to me to make sur all child process
>> > are killed.
>> >
>> > but I don't see your link so I may be missing something
>>
>> The link is in the added patch: https://bugs.debian.org/778913
>>
>> As said, I could not argue either way, but I got some respect for the
>> debian guys ;)
>>
>
> The thread does not actually discuss process vs mixed...
>
> so doesn't really help here.
> OTOH, the debian version has been vetted by mbiel which is a systemc core-maintainer.
>
> so i would go with mixed if I were to write the service from scratch, but since I don't have an
> explanation for the choice of process, I'm not entirely sure...
>
> A possibility is that ssh creates a process per connection. in that case
> * process would not kill all ongoing connections
> * mixed would
>
> maybe it was chosen to protect existing connection. that would make some sense.

Well, debian and arch seem to agree on using "process", I guess it
means to just prevent
new connections and not kill existing ones?

Can I get a "reviewed-by" for this patch, so this and #3 can be merged?

Norbert
Jérémy ROSEN June 11, 2020, 6:14 a.m. UTC | #8 
sure

Reviewed-by Jérémy Rosen <jeremy.rosen@smile.fr>

Sorry for noticing that everything was adressed

Le jeu. 11 juin 2020 à 02:04, Norbert Lange <nolange79@gmail.com> a écrit :

> Am So., 7. Juni 2020 um 21:42 Uhr schrieb Jérémy ROSEN <
> jeremy.rosen@smile.fr>:
> >
> >
> >
> > Le dim. 7 juin 2020 à 21:24, Norbert Lange <nolange79@gmail.com> a
> écrit :
> >>
> >> Am So., 7. Juni 2020 um 21:16 Uhr schrieb Jérémy ROSEN <
> jeremy.rosen@smile.fr>:
> >> >
> >> >
> >> >
> >> > Le dim. 7 juin 2020 à 21:03, Norbert Lange <nolange79@gmail.com> a
> écrit :
> >> >>
> >> >> Am So., 7. Juni 2020 um 12:54 Uhr schrieb Jérémy ROSEN <
> jeremy.rosen@smile.fr>:
> >> >> >
> >> >> >
> >> >> >
> >> >> > Le sam. 6 juin 2020 à 00:59, Norbert Lange <nolange79@gmail.com>
> a écrit :
> >> >> >>
> >> >> >> the openssh daemon is not suited for systemd's simple
> >> >> >> service type. dependend services should only start
> >> >> >> when sshd is ready to accept connections.
> >> >> >>
> >> >> >> A patch is added from debian to allow openssh
> >> >> >> to communicate this state.
> >> >> >>
> >> >> >> Restarts are prevented if the reason is a faulty
> >> >> >> config file (errocode 255).
> >> >> >>
> >> >> >> The "user confinement directory" is changed to
> >> >> >> '/run/sshd' which is automatically managed by systemd.
> >> >> >>
> >> >> >> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> >> >> >> ---
> >> >> >>  package/openssh/00-systemd-readiness.patch | 84
> ++++++++++++++++++++++
> >> >> >>  package/openssh/openssh.mk                 | 14 +++-
> >> >> >>  package/openssh/sshd-sysusers.conf         |  2 +-
> >> >> >>  package/openssh/sshd.service               | 13 +++-
> >> >> >>  4 files changed, 109 insertions(+), 4 deletions(-)
> >> >> >>  create mode 100644 package/openssh/00-systemd-readiness.patch
> >> >> >>
> >> >> >> diff --git a/package/openssh/00-systemd-readiness.patch
> b/package/openssh/00-systemd-readiness.patch
> >> >> >> new file mode 100644
> >> >> >> index 0000000000..be3b6b0074
> >> >> >> --- /dev/null
> >> >> >> +++ b/package/openssh/00-systemd-readiness.patch
> >> >> >> @@ -0,0 +1,84 @@
> >> >> >> +From ab765b2bd55062a704f09da8f8c1c4ad1d6630a7 Mon Sep 17
> 00:00:00 2001
> >> >> >> +From: Michael Biebl <biebl@debian.org>
> >> >> >> +Date: Mon, 21 Dec 2015 16:08:47 +0000
> >> >> >> +Subject: Add systemd readiness notification support
> >> >> >> +
> >> >> >> +Bug-Debian: https://bugs.debian.org/778913
> >> >> >> +Forwarded: no
> >> >> >> +Last-Update: 2017-08-22
> >> >> >> +
> >> >> >> +Patch-Name: systemd-readiness.patch
> >> >> >> +---
> >> >> >> + configure.ac | 24 ++++++++++++++++++++++++
> >> >> >> + sshd.c       |  9 +++++++++
> >> >> >> + 2 files changed, 33 insertions(+)
> >> >> >> +
> >> >> >> +diff --git a/configure.ac b/configure.ac
> >> >> >> +index e894db9fc..c119d6fd1 100644
> >> >> >> +--- a/configure.ac
> >> >> >> ++++ b/configure.ac
> >> >> >> +@@ -4499,6 +4499,29 @@ AC_ARG_WITH([kerberos5],
> >> >> >> + AC_SUBST([GSSLIBS])
> >> >> >> + AC_SUBST([K5LIBS])
> >> >> >> +
> >> >> >> ++# Check whether user wants systemd support
> >> >> >> ++SYSTEMD_MSG="no"
> >> >> >> ++AC_ARG_WITH(systemd,
> >> >> >> ++      [  --with-systemd          Enable systemd support],
> >> >> >> ++      [ if test "x$withval" != "xno" ; then
> >> >> >> ++              AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no])
> >> >> >> ++              if test "$PKGCONFIG" != "no"; then
> >> >> >> ++                      AC_MSG_CHECKING([for libsystemd])
> >> >> >> ++                      if $PKGCONFIG --exists libsystemd; then
> >> >> >> ++                              SYSTEMD_CFLAGS=`$PKGCONFIG
> --cflags libsystemd`
> >> >> >> ++                              SYSTEMD_LIBS=`$PKGCONFIG --libs
> libsystemd`
> >> >> >> ++                              CPPFLAGS="$CPPFLAGS
> $SYSTEMD_CFLAGS"
> >> >> >> ++                              SSHDLIBS="$SSHDLIBS $SYSTEMD_LIBS"
> >> >> >> ++                              AC_MSG_RESULT([yes])
> >> >> >> ++                              AC_DEFINE(HAVE_SYSTEMD, 1,
> [Define if you want systemd support.])
> >> >> >> ++                              SYSTEMD_MSG="yes"
> >> >> >> ++                      else
> >> >> >> ++                              AC_MSG_RESULT([no])
> >> >> >> ++                      fi
> >> >> >> ++              fi
> >> >> >> ++      fi ]
> >> >> >> ++)
> >> >> >> ++
> >> >> >> + # Looking for programs, paths and files
> >> >> >> +
> >> >> >> + PRIVSEP_PATH=/var/empty
> >> >> >> +@@ -5305,6 +5328,7 @@ echo "                   libldns support:
> $LDNS_MSG"
> >> >> >> + echo "  Solaris process contract support: $SPC_MSG"
> >> >> >> + echo "           Solaris project support: $SP_MSG"
> >> >> >> + echo "         Solaris privilege support: $SPP_MSG"
> >> >> >> ++echo "                   systemd support: $SYSTEMD_MSG"
> >> >> >> + echo "       IP address in \$DISPLAY hack: $DISPLAY_HACK_MSG"
> >> >> >> + echo "           Translate v4 in v6 hack: $IPV4_IN6_HACK_MSG"
> >> >> >> + echo "                  BSD Auth support: $BSD_AUTH_MSG"
> >> >> >> +diff --git a/sshd.c b/sshd.c
> >> >> >> +index 4e8ff0662..5e7679a33 100644
> >> >> >> +--- a/sshd.c
> >> >> >> ++++ b/sshd.c
> >> >> >> +@@ -85,6 +85,10 @@
> >> >> >> + #include <prot.h>
> >> >> >> + #endif
> >> >> >> +
> >> >> >> ++#ifdef HAVE_SYSTEMD
> >> >> >> ++#include <systemd/sd-daemon.h>
> >> >> >> ++#endif
> >> >> >> ++
> >> >> >> + #include "xmalloc.h"
> >> >> >> + #include "ssh.h"
> >> >> >> + #include "ssh2.h"
> >> >> >> +@@ -1951,6 +1955,11 @@ main(int ac, char **av)
> >> >> >> +                       }
> >> >> >> +               }
> >> >> >> +
> >> >> >> ++#ifdef HAVE_SYSTEMD
> >> >> >> ++              /* Signal systemd that we are ready to accept
> connections */
> >> >> >> ++              sd_notify(0, "READY=1");
> >> >> >> ++#endif
> >> >> >> ++
> >> >> >> +               /* Accept a connection and return in a forked
> child */
> >> >> >> +               server_accept_loop(&sock_in, &sock_out,
> >> >> >> +                   &newsock, config_s);
> >> >> >> diff --git a/package/openssh/openssh.mk b/package/openssh/
> openssh.mk
> >> >> >> index 55b917e20a..d425db1428 100644
> >> >> >> --- a/package/openssh/openssh.mk
> >> >> >> +++ b/package/openssh/openssh.mk
> >> >> >> @@ -12,6 +12,7 @@ OPENSSH_CONF_ENV = \
> >> >> >>         LD="$(TARGET_CC)" \
> >> >> >>         LDFLAGS="$(TARGET_CFLAGS)" \
> >> >> >>         LIBS=`$(PKG_CONFIG_HOST_BINARY) --libs openssl`
> >> >> >> +OPENSSH_AUTORECONF = YES
> >> >> >>  OPENSSH_CONF_OPTS = \
> >> >> >>         --sysconfdir=/etc/ssh \
> >> >> >>         --with-default-path=$(BR2_SYSTEM_DEFAULT_PATH) \
> >> >> >> @@ -22,9 +23,20 @@ OPENSSH_CONF_OPTS = \
> >> >> >>         --disable-wtmpx \
> >> >> >>         --disable-strip
> >> >> >>
> >> >> >> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
> >> >> >> +OPENSSH_DEPENDENCIES = systemd
> >> >> >> +
> >> >> >> +OPENSSH_CONF_OPTS += \
> >> >> >> +       --with-privsep-path=/run/sshd \
> >> >> >> +       --with-pid-dir=/run \
> >> >> >> +       --with-systemd
> >> >> >> +
> >> >> >> +else
> >> >> >> +
> >> >> >>  define OPENSSH_PERMISSIONS
> >> >> >>         /var/empty d 755 root root - - - - -
> >> >> >>  endef
> >> >> >
> >> >> >
> >> >> > Do we still need this when using systemd, or can it be commented
> out ?
> >> >>
> >> >> Not sure what you mean with "this"?
> >> >> The OPENSSH_PERMISSIONS block is needed when not using systemd and it
> >> >> is only active then.
> >> >>
> >> >
> >> > my bad, I missed the enclosing ifeq()
> >> >
> >> >>
> >> >> >
> >> >> >
> >> >> >>
> >> >> >> +endif
> >> >> >>
> >> >> >>  ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
> >> >> >>  OPENSSH_CONF_OPTS += --without-pie
> >> >> >> @@ -72,7 +84,7 @@ define OPENSSH_INSTALL_SYSTEMD_SYSUSERS
> >> >> >>  endef
> >> >> >>  else
> >> >> >>  define OPENSSH_USERS
> >> >> >> -       sshd -1 sshd -1 * /var/empty - - SSH drop priv user
> >> >> >> +       sshd -1 sshd -1 * $(if
> $(BR2_PACKAGE_SYSTEMD),/run/sshd,/var/empty) - - SSH drop priv user
> >> >> >>  endef
> >> >> >>  endif
> >> >> >>
> >> >> >> diff --git a/package/openssh/sshd-sysusers.conf
> b/package/openssh/sshd-sysusers.conf
> >> >> >> index ac77aec065..303d0dbb63 100644
> >> >> >> --- a/package/openssh/sshd-sysusers.conf
> >> >> >> +++ b/package/openssh/sshd-sysusers.conf
> >> >> >> @@ -1 +1 @@
> >> >> >> -u sshd - "SSH drop priv user" /var/empty
> >> >> >> +u sshd - "SSH drop priv user" /run/sshd
> >> >> >> diff --git a/package/openssh/sshd.service
> b/package/openssh/sshd.service
> >> >> >> index b5e96b3a25..715bd3f7eb 100644
> >> >> >> --- a/package/openssh/sshd.service
> >> >> >> +++ b/package/openssh/sshd.service
> >> >> >> @@ -1,11 +1,20 @@
> >> >> >>  [Unit]
> >> >> >>  Description=OpenSSH server daemon
> >> >> >> -After=syslog.target network.target auditd.service
> >> >> >> +Documentation=man:sshd(8) man:sshd_config(5)
> >> >> >> +After=network.target auditd.service
> >> >> >>
> >> >> >>
> >> >> >>  [Service]
> >> >> >>  ExecStartPre=/usr/bin/ssh-keygen -A
> >> >> >> -ExecStart=/usr/sbin/sshd -D -e
> >> >> >> +ExecStartPre=/usr/sbin/sshd -t
> >> >> >> +ExecStart=/usr/sbin/sshd -D
> >> >> >
> >> >> > You droped the -e, so  you are logging to syslog
> >> >> > However you droped the dependency on syslog.target earlier...
> >> >> > (maybe it should be syslog.socket instead of .target, btw)
> >> >>
> >> >>
> >> >> syslog.target is long long gone, and the syslog will be
> >> >> unconditionally available
> >> >> https://www.freedesktop.org/wiki/Software/systemd/syslog/
> >> >>
> >> >>
> >> >> >
> >> >> >
> >> >> > how exactly do you want to log  ? (I think logging to stdout is
> better, it will be
> >> >> > redirected to the journal.
> >> >>
> >> >>
> >> >> stdout is not really useful if syslog is supported.
> >> >>
> >> > i'd go the other way round
> >> >
> >> > syslog is not really necessary if stdout is available,
> >> > but it's a matter of taste :P so let's go your way.
> >>
> >> Its more the point, that Openssh already implemented syslog, and thats
> >> a clear functional superset of listening to stdout.
> >>
> >> >
> >> >>
> >> >> >
> >> >> >
> >> >> >>
> >> >> >> +ExecReload=/usr/sbin/sshd -t
> >> >> >>  ExecReload=/bin/kill -HUP $MAINPID
> >> >> >> +KillMode=process
> >> >> >
> >> >> >
> >> >> > Wouldn't mixed be better here ?
> >> >> > I'm not really sure what the use-case for procss is anyway...
> >> >>
> >> >>
> >> >> I taken that from debian, I could not argue against it (there is a
> >> >> long discussion which I linked above).
> >> >> Can you argue *for* mixed?
> >> >>
> >> >
> >> >
> >> > I didn't see any link
> >> > * process : SIGTERM and SIGKILL is sent only to MainPID
> >> > * mixed : SIGTERM is sent to MainPID, SIGKILL is sent to every
> process in the service cgroup.
> >> >
> >> > This means that if all works well, they do the same thing
> >> >
> >> > in case the MainPID fails to properly terminate its children, process
> would leave children alive
> >> > but mixed woul kill everybody
> >> >
> >> > Since we are trying to terminate the service, it makes sense to me to
> make sur all child process
> >> > are killed.
> >> >
> >> > but I don't see your link so I may be missing something
> >>
> >> The link is in the added patch: https://bugs.debian.org/778913
> >>
> >> As said, I could not argue either way, but I got some respect for the
> >> debian guys ;)
> >>
> >
> > The thread does not actually discuss process vs mixed...
> >
> > so doesn't really help here.
> > OTOH, the debian version has been vetted by mbiel which is a systemc
> core-maintainer.
> >
> > so i would go with mixed if I were to write the service from scratch,
> but since I don't have an
> > explanation for the choice of process, I'm not entirely sure...
> >
> > A possibility is that ssh creates a process per connection. in that case
> > * process would not kill all ongoing connections
> > * mixed would
> >
> > maybe it was chosen to protect existing connection. that would make some
> sense.
>
> Well, debian and arch seem to agree on using "process", I guess it
> means to just prevent
> new connections and not kill existing ones?
>
> Can I get a "reviewed-by" for this patch, so this and #3 can be merged?
>
> Norbert
>
diff mbox series

Patch

diff --git a/package/openssh/00-systemd-readiness.patch b/package/openssh/00-systemd-readiness.patch
new file mode 100644
index 0000000000..be3b6b0074
--- /dev/null
+++ b/package/openssh/00-systemd-readiness.patch
@@ -0,0 +1,84 @@ 
+From ab765b2bd55062a704f09da8f8c1c4ad1d6630a7 Mon Sep 17 00:00:00 2001
+From: Michael Biebl <biebl@debian.org>
+Date: Mon, 21 Dec 2015 16:08:47 +0000
+Subject: Add systemd readiness notification support
+
+Bug-Debian: https://bugs.debian.org/778913
+Forwarded: no
+Last-Update: 2017-08-22
+
+Patch-Name: systemd-readiness.patch
+---
+ configure.ac | 24 ++++++++++++++++++++++++
+ sshd.c       |  9 +++++++++
+ 2 files changed, 33 insertions(+)
+
+diff --git a/configure.ac b/configure.ac
+index e894db9fc..c119d6fd1 100644
+--- a/configure.ac
++++ b/configure.ac
+@@ -4499,6 +4499,29 @@ AC_ARG_WITH([kerberos5],
+ AC_SUBST([GSSLIBS])
+ AC_SUBST([K5LIBS])
+
++# Check whether user wants systemd support
++SYSTEMD_MSG="no"
++AC_ARG_WITH(systemd,
++	[  --with-systemd          Enable systemd support],
++	[ if test "x$withval" != "xno" ; then
++		AC_PATH_TOOL([PKGCONFIG], [pkg-config], [no])
++		if test "$PKGCONFIG" != "no"; then
++			AC_MSG_CHECKING([for libsystemd])
++			if $PKGCONFIG --exists libsystemd; then
++				SYSTEMD_CFLAGS=`$PKGCONFIG --cflags libsystemd`
++				SYSTEMD_LIBS=`$PKGCONFIG --libs libsystemd`
++				CPPFLAGS="$CPPFLAGS $SYSTEMD_CFLAGS"
++				SSHDLIBS="$SSHDLIBS $SYSTEMD_LIBS"
++				AC_MSG_RESULT([yes])
++				AC_DEFINE(HAVE_SYSTEMD, 1, [Define if you want systemd support.])
++				SYSTEMD_MSG="yes"
++			else
++				AC_MSG_RESULT([no])
++			fi
++		fi
++	fi ]
++)
++
+ # Looking for programs, paths and files
+
+ PRIVSEP_PATH=/var/empty
+@@ -5305,6 +5328,7 @@ echo "                   libldns support: $LDNS_MSG"
+ echo "  Solaris process contract support: $SPC_MSG"
+ echo "           Solaris project support: $SP_MSG"
+ echo "         Solaris privilege support: $SPP_MSG"
++echo "                   systemd support: $SYSTEMD_MSG"
+ echo "       IP address in \$DISPLAY hack: $DISPLAY_HACK_MSG"
+ echo "           Translate v4 in v6 hack: $IPV4_IN6_HACK_MSG"
+ echo "                  BSD Auth support: $BSD_AUTH_MSG"
+diff --git a/sshd.c b/sshd.c
+index 4e8ff0662..5e7679a33 100644
+--- a/sshd.c
++++ b/sshd.c
+@@ -85,6 +85,10 @@
+ #include <prot.h>
+ #endif
+
++#ifdef HAVE_SYSTEMD
++#include <systemd/sd-daemon.h>
++#endif
++
+ #include "xmalloc.h"
+ #include "ssh.h"
+ #include "ssh2.h"
+@@ -1951,6 +1955,11 @@ main(int ac, char **av)
+ 			}
+ 		}
+
++#ifdef HAVE_SYSTEMD
++		/* Signal systemd that we are ready to accept connections */
++		sd_notify(0, "READY=1");
++#endif
++
+ 		/* Accept a connection and return in a forked child */
+ 		server_accept_loop(&sock_in, &sock_out,
+ 		    &newsock, config_s);
diff --git a/package/openssh/openssh.mk b/package/openssh/openssh.mk
index 55b917e20a..d425db1428 100644
--- a/package/openssh/openssh.mk
+++ b/package/openssh/openssh.mk
@@ -12,6 +12,7 @@  OPENSSH_CONF_ENV = \
 	LD="$(TARGET_CC)" \
 	LDFLAGS="$(TARGET_CFLAGS)" \
 	LIBS=`$(PKG_CONFIG_HOST_BINARY) --libs openssl`
+OPENSSH_AUTORECONF = YES
 OPENSSH_CONF_OPTS = \
 	--sysconfdir=/etc/ssh \
 	--with-default-path=$(BR2_SYSTEM_DEFAULT_PATH) \
@@ -22,9 +23,20 @@  OPENSSH_CONF_OPTS = \
 	--disable-wtmpx \
 	--disable-strip
 
+ifeq ($(BR2_PACKAGE_SYSTEMD),y)
+OPENSSH_DEPENDENCIES = systemd
+
+OPENSSH_CONF_OPTS += \
+	--with-privsep-path=/run/sshd \
+	--with-pid-dir=/run \
+	--with-systemd
+
+else
+
 define OPENSSH_PERMISSIONS
 	/var/empty d 755 root root - - - - -
 endef
+endif
 
 ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
 OPENSSH_CONF_OPTS += --without-pie
@@ -72,7 +84,7 @@  define OPENSSH_INSTALL_SYSTEMD_SYSUSERS
 endef
 else
 define OPENSSH_USERS
-	sshd -1 sshd -1 * /var/empty - - SSH drop priv user
+	sshd -1 sshd -1 * $(if $(BR2_PACKAGE_SYSTEMD),/run/sshd,/var/empty) - - SSH drop priv user
 endef
 endif
 
diff --git a/package/openssh/sshd-sysusers.conf b/package/openssh/sshd-sysusers.conf
index ac77aec065..303d0dbb63 100644
--- a/package/openssh/sshd-sysusers.conf
+++ b/package/openssh/sshd-sysusers.conf
@@ -1 +1 @@ 
-u sshd - "SSH drop priv user" /var/empty
+u sshd - "SSH drop priv user" /run/sshd
diff --git a/package/openssh/sshd.service b/package/openssh/sshd.service
index b5e96b3a25..715bd3f7eb 100644
--- a/package/openssh/sshd.service
+++ b/package/openssh/sshd.service
@@ -1,11 +1,20 @@ 
 [Unit]
 Description=OpenSSH server daemon
-After=syslog.target network.target auditd.service
+Documentation=man:sshd(8) man:sshd_config(5)
+After=network.target auditd.service
 
 [Service]
 ExecStartPre=/usr/bin/ssh-keygen -A
-ExecStart=/usr/sbin/sshd -D -e
+ExecStartPre=/usr/sbin/sshd -t
+ExecStart=/usr/sbin/sshd -D
+ExecReload=/usr/sbin/sshd -t
 ExecReload=/bin/kill -HUP $MAINPID
+KillMode=process
+Restart=on-failure
+RestartPreventExitStatus=255
+Type=notify
+RuntimeDirectory=sshd
+RuntimeDirectoryMode=0755
 
 [Install]
 WantedBy=multi-user.target