[devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[@ 2019-12-13 11:42 UTC (permalink / raw)]

[devel] [PATCH hasher-priv v1 1/3] Make a daemon from the hasher-priv

[@ 2019-12-13 11:42 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] Make a daemon from the hasher-priv

[@ 2020-09-17 13:10 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] Make a daemon from the hasher-priv

[@ 2020-10-01 19:43 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] Make a daemon from the hasher-priv

[@ 2020-10-01 21:24 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] Make a daemon from the hasher-priv

[@ 2020-10-01 23:38 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] *literacy*

[@ 2020-09-17 13:10 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] caller.c

[@ 2020-09-17 13:11 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] caller.c

[@ 2020-09-17 13:55 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] caller_server.c, caller_task.c

[@ 2020-09-17 13:11 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] caller_server.c, caller_task.c

[@ 2020-10-01 19:47 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] config.c

[@ 2020-09-17 13:11 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] config.c

[@ 2020-09-18 10:42 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] hasher-privd.c

[@ 2020-09-17 13:12 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] logging.c

[@ 2020-09-17 13:12 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] Makefile

[@ 2020-09-17 13:12 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] Makefile

[@ 2020-09-17 15:09 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] Makefile

[@ 2020-09-18 10:48 UTC (permalink / raw)]

Re: [devel] [PATCH hasher-priv v1 1/3] Makefile

[Andrey Savchenko]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On Fri, 18 Sep 2020 13:48:19 +0300 Dmitry V. Levin wrote:
> On Thu, Sep 17, 2020 at 04:12:36PM +0300, Arseny Maslennikov wrote:
> > On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote:
> > > diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
> > > index a815e9e..82aa385 100644
> > > --- a/hasher-priv/Makefile
> > > +++ b/hasher-priv/Makefile
> > > @@ -11,7 +11,7 @@ VERSION = $(shell sed '/^Version: */!d;s///;q' hasher-priv.spec)
> > >  HELPERS = getconf.sh getugid1.sh chrootuid1.sh getugid2.sh chrootuid2.sh
> > >  MAN5PAGES = $(PROJECT).conf.5
> > >  MAN8PAGES = $(PROJECT).8 hasher-useradd.8
> > > -TARGETS = $(PROJECT) hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
> > > +TARGETS = $(PROJECT) hasher-privd hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
> > 
> > To everyone: While the name "hasher-privd" minimises the amount of
> > renaming we have to do, it is too long a name, given that /proc/%d/comm
> > for each task is up to 16 bytes long on Linux,
> 
> Why should we care about /proc/%d/comm limitations?  Is this really an issue?

I agree. hasher-privd is readable and understandable name also
featuring less renames.

Best regards,
Andrew Savchenko

[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 1/3] Makefile

[Dmitry V. Levin]

On Thu, Sep 17, 2020 at 04:12:36PM +0300, Arseny Maslennikov wrote:
> On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote:
[...]
> > @@ -21,6 +21,7 @@ man5dir = $(mandir)/man5
> >  man8dir = $(mandir)/man8
> >  configdir = $(sysconfdir)/$(PROJECT)
> >  helperdir = $(libexecdir)/$(PROJECT)
> > +socketdir = /var/run
> 
> Why /var/run and not /run, especially in a new project?

It's the same thing nowadays, isn't it?

> Even further, I would suggest that we store the socket in
> /run/hasher-priv or something, setgid hashman, with 0710 rights. The
> major service managers can create the directory on startup for us:
> there's mkdir(1), there's RuntimeDirectory= and RuntimeDirectoryMode=.

I distinctly remember we discussed this the last autumn or winter.
Yes, unix domain socket access restrictions should be implemented
using directory permissions.


-- 
ldv

Re: [devel] [PATCH hasher-priv v1 1/3] Makefile

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

On Fri, Sep 18, 2020 at 02:33:12PM +0300, Dmitry V. Levin wrote:
> On Thu, Sep 17, 2020 at 04:12:36PM +0300, Arseny Maslennikov wrote:
> > On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote:
> [...]
> > > @@ -21,6 +21,7 @@ man5dir = $(mandir)/man5
> > >  man8dir = $(mandir)/man8
> > >  configdir = $(sysconfdir)/$(PROJECT)
> > >  helperdir = $(libexecdir)/$(PROJECT)
> > > +socketdir = /var/run
> > 
> > Why /var/run and not /run, especially in a new project?
> 
> It's the same thing nowadays, isn't it?

Short answer: That's why I asked the question.

Long answer: Depends on what you mean by "thing".

It is true that these two paths point to the same place now, but
/var/run is only kept for compatibility with software that expects this
path to be available, so we, the packagers, don't have to patch all of
it now.

The mountpoints, however, have an obvious but important difference:
/var/run requires /var to be mounted first to work correctly, and /run
does not — so the latter path is more simple and thus preferable, and
the former path was a mistake.

> 
> > Even further, I would suggest that we store the socket in
> > /run/hasher-priv or something, setgid hashman, with 0710 rights. The
> > major service managers can create the directory on startup for us:
> > there's mkdir(1), there's RuntimeDirectory= and RuntimeDirectoryMode=.
> 
> I distinctly remember we discussed this the last autumn or winter.

The discussion probably went over my head then, or was private.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 1/3] server.conf

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote:
> diff --git a/hasher-priv/server.conf b/hasher-priv/server.conf
> new file mode 100644
> index 0000000..53ea5c3
> --- /dev/null
> +++ b/hasher-priv/server.conf
> @@ -0,0 +1,13 @@
> +# Server configuration
> +
> +# Set the default logging priority. (can override with command line arguments)
> +priority=info
> +
> +# Write a pid file. (can override with command line arguments)
> +pidfile=/var/run/hasher-privd.pid
> +
> +# Stop user's session server after {session_timeout} seconds of inactivity.
> +session_timeout=3600
> +
> +# Allow users of this group to interact with hasher-privd via the control socket.
> +control_group=hashman

As noted earlier, in a different file: this unlucky name too strongly
associates with cgroups.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 1/3] server.conf

[Dmitry V. Levin]

On Thu, Sep 17, 2020 at 04:12:48PM +0300, Arseny Maslennikov wrote:
> On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote:
[...]
> > +# Allow users of this group to interact with hasher-privd via the control socket.
> > +control_group=hashman
> 
> As noted earlier, in a different file: this unlucky name too strongly
> associates with cgroups.

I suggest renaming it to access_group then.


-- 
ldv

Re: [devel] [PATCH hasher-priv v1 1/3] server.conf

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On Fri, Sep 18, 2020 at 01:50:20PM +0300, Dmitry V. Levin wrote:
> On Thu, Sep 17, 2020 at 04:12:48PM +0300, Arseny Maslennikov wrote:
> > On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote:
> [...]
> > > +# Allow users of this group to interact with hasher-privd via the control socket.
> > > +control_group=hashman
> > 
> > As noted earlier, in a different file: this unlucky name too strongly
> > associates with cgroups.
> 
> I suggest renaming it to access_group then.
> 

That name looks fine to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Alex Gladkov]

From: Alexey Gladkov <legion@altlinux.org>

Signed-off-by: Alexey Gladkov <legion@altlinux.org>
---
 hasher-priv/Makefile              |  4 ++
 hasher-priv/hasher-privd.service  | 11 ++++
 hasher-priv/hasher-privd.sysvinit | 86 +++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100644 hasher-priv/hasher-privd.service
 create mode 100755 hasher-priv/hasher-privd.sysvinit

diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
index 82aa385..c73216f 100644
--- a/hasher-priv/Makefile
+++ b/hasher-priv/Makefile
@@ -14,6 +14,8 @@ MAN8PAGES = $(PROJECT).8 hasher-useradd.8
 TARGETS = $(PROJECT) hasher-privd hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
 
 sysconfdir = /etc
+initdir=$(sysconfdir)/rc.d/init.d
+systemd_unitdir=/lib/systemd/system
 libexecdir = /usr/lib
 sbindir = /usr/sbin
 mandir = /usr/share/man
@@ -72,6 +74,8 @@ install: all
 	$(MKDIR_P) -m750 $(DESTDIR)$(helperdir)
 	$(INSTALL) -p -m700 $(PROJECT) $(DESTDIR)$(helperdir)/
 	$(INSTALL) -p -m755 $(HELPERS) $(DESTDIR)$(helperdir)/
+	$(MKDIR_P) -m755 $(DESTDIR)$(initdir)
+	$(INSTALL) -p -m755 hasher-privd.sysvinit $(DESTDIR)$(initdir)/hasher-privd
 	$(MKDIR_P) -m755 $(DESTDIR)$(sbindir)
 	$(INSTALL) -p -m755 hasher-privd $(DESTDIR)$(sbindir)/
 	$(INSTALL) -p -m755 hasher-useradd $(DESTDIR)$(sbindir)/
diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
new file mode 100644
index 0000000..e5ed9ac
--- /dev/null
+++ b/hasher-priv/hasher-privd.service
@@ -0,0 +1,11 @@
+[Unit]
+Description=A privileged helper for the hasher project
+ConditionVirtualization=!container
+Documentation=man:hasher-priv(8)
+
+[Service]
+ExecStart=/usr/sbin/hasher-privd
+Restart=on-failure
+
+[Install]
+WantedBy=multi-user.target
diff --git a/hasher-priv/hasher-privd.sysvinit b/hasher-priv/hasher-privd.sysvinit
new file mode 100755
index 0000000..914fb53
--- /dev/null
+++ b/hasher-priv/hasher-privd.sysvinit
@@ -0,0 +1,86 @@
+#! /bin/sh
+
+### BEGIN INIT INFO
+# Short-Description:    A privileged helper for the hasher project
+# Description:          A privileged helper for the hasher project
+# Provides:             hasher-priv
+# Required-Start:       $remote_fs
+# Required-Stop:        $remote_fs
+# Default-Start:        2 3 4 5
+# Default-Stop:         0 1 6
+### END INIT INFO
+
+WITHOUT_RC_COMPAT=1
+
+# Source function library.
+. /etc/init.d/functions
+
+NAME=hasher-privd
+PIDFILE="/var/run/$NAME.pid"
+LOCKFILE="/var/lock/subsys/$NAME"
+RETVAL=0
+
+start()
+{
+	start_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" -- "$NAME"
+	RETVAL=$?
+	return $RETVAL
+}
+
+stop()
+{
+	stop_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" "$NAME"
+	RETVAL=$?
+	return $RETVAL
+}
+
+restart()
+{
+	stop
+	start
+}
+
+# See how we were called.
+case "$1" in
+	start)
+		start
+		;;
+	stop)
+		stop
+		;;
+	status)
+		status --pidfile "$PIDFILE" "$NAME"
+		RETVAL=$?
+		;;
+	restart)
+		restart
+		;;
+	reload)
+		restart
+		;;
+	condstart)
+		if [ ! -e "$LOCKFILE" ]; then
+			start
+		fi
+		;;
+	condstop)
+		if [ -e "$LOCKFILE" ]; then
+			stop
+		fi
+		;;
+	condrestart)
+		if [ -e "$LOCKFILE" ]; then
+			restart
+		fi
+		;;
+	condreload)
+		if [ -e "$LOCKFILE" ]; then
+			reload
+		fi
+		;;
+	*)
+		msg_usage "${0##*/} {start|stop|status|restart|reload|condstart|condstop|condrestart|condreload}"
+		RETVAL=1
+esac
+
+exit $RETVAL
-- 
2.24.0

Re: [devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Mikhail Novosyolov]

13.12.2019 14:42, Alex Gladkov пишет:
> From: Alexey Gladkov <legion@altlinux.org>
>
> Signed-off-by: Alexey Gladkov <legion@altlinux.org>
> ---
>  hasher-priv/Makefile              |  4 ++
>  hasher-priv/hasher-privd.service  | 11 ++++
>  hasher-priv/hasher-privd.sysvinit | 86 +++++++++++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
>  create mode 100644 hasher-priv/hasher-privd.service
>  create mode 100755 hasher-priv/hasher-privd.sysvinit
>
> diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
> index 82aa385..c73216f 100644
> --- a/hasher-priv/Makefile
> +++ b/hasher-priv/Makefile
> @@ -14,6 +14,8 @@ MAN8PAGES = $(PROJECT).8 hasher-useradd.8
>  TARGETS = $(PROJECT) hasher-privd hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
>  
>  sysconfdir = /etc
> +initdir=$(sysconfdir)/rc.d/init.d
> +systemd_unitdir=/lib/systemd/system
>  libexecdir = /usr/lib
>  sbindir = /usr/sbin
>  mandir = /usr/share/man
> @@ -72,6 +74,8 @@ install: all
>  	$(MKDIR_P) -m750 $(DESTDIR)$(helperdir)
>  	$(INSTALL) -p -m700 $(PROJECT) $(DESTDIR)$(helperdir)/
>  	$(INSTALL) -p -m755 $(HELPERS) $(DESTDIR)$(helperdir)/
> +	$(MKDIR_P) -m755 $(DESTDIR)$(initdir)
> +	$(INSTALL) -p -m755 hasher-privd.sysvinit $(DESTDIR)$(initdir)/hasher-privd
>  	$(MKDIR_P) -m755 $(DESTDIR)$(sbindir)
>  	$(INSTALL) -p -m755 hasher-privd $(DESTDIR)$(sbindir)/
>  	$(INSTALL) -p -m755 hasher-useradd $(DESTDIR)$(sbindir)/
> diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
> new file mode 100644
> index 0000000..e5ed9ac
> --- /dev/null
> +++ b/hasher-priv/hasher-privd.service
> @@ -0,0 +1,11 @@
> +[Unit]
> +Description=A privileged helper for the hasher project
> +ConditionVirtualization=!container
А если контейнеру выданы нужные привелегии/capabilities, то почему нельзя? Может, лучше ConditionCapability=?
> +Documentation=man:hasher-priv(8)
> +
> +[Service]
> +ExecStart=/usr/sbin/hasher-privd
> +Restart=on-failure
> +
> +[Install]
> +WantedBy=multi-user.target
> diff --git a/hasher-priv/hasher-privd.sysvinit b/hasher-priv/hasher-privd.sysvinit
> new file mode 100755
> index 0000000..914fb53
> --- /dev/null
> +++ b/hasher-priv/hasher-privd.sysvinit
> @@ -0,0 +1,86 @@
> +#! /bin/sh
> +
> +### BEGIN INIT INFO
> +# Short-Description:    A privileged helper for the hasher project
> +# Description:          A privileged helper for the hasher project
> +# Provides:             hasher-priv
> +# Required-Start:       $remote_fs
> +# Required-Stop:        $remote_fs
> +# Default-Start:        2 3 4 5
> +# Default-Stop:         0 1 6
> +### END INIT INFO
> +
> +WITHOUT_RC_COMPAT=1
> +
> +# Source function library.
> +. /etc/init.d/functions
> +
> +NAME=hasher-privd
> +PIDFILE="/var/run/$NAME.pid"
> +LOCKFILE="/var/lock/subsys/$NAME"
> +RETVAL=0
> +
> +start()
> +{
> +	start_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" -- "$NAME"
> +	RETVAL=$?
> +	return $RETVAL
> +}
> +
> +stop()
> +{
> +	stop_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" "$NAME"
> +	RETVAL=$?
> +	return $RETVAL
> +}
> +
> +restart()
> +{
> +	stop
> +	start
> +}
> +
> +# See how we were called.
> +case "$1" in
> +	start)
> +		start
> +		;;
> +	stop)
> +		stop
> +		;;
> +	status)
> +		status --pidfile "$PIDFILE" "$NAME"
> +		RETVAL=$?
> +		;;
> +	restart)
> +		restart
> +		;;
> +	reload)
> +		restart
> +		;;
> +	condstart)
> +		if [ ! -e "$LOCKFILE" ]; then
> +			start
> +		fi
> +		;;
> +	condstop)
> +		if [ -e "$LOCKFILE" ]; then
> +			stop
> +		fi
> +		;;
> +	condrestart)
> +		if [ -e "$LOCKFILE" ]; then
> +			restart
> +		fi
> +		;;
> +	condreload)
> +		if [ -e "$LOCKFILE" ]; then
> +			reload
> +		fi
> +		;;
> +	*)
> +		msg_usage "${0##*/} {start|stop|status|restart|reload|condstart|condstop|condrestart|condreload}"
> +		RETVAL=1
> +esac
> +
> +exit $RETVAL

Re: [devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Mikhail Novosyolov]

18.06.2020 01:31, Mikhail Novosyolov пишет:
> 13.12.2019 14:42, Alex Gladkov пишет:
>> diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
>> new file mode 100644
>> index 0000000..e5ed9ac
>> --- /dev/null
>> +++ b/hasher-priv/hasher-privd.service
>> @@ -0,0 +1,11 @@
>> +[Unit]
>> +Description=A privileged helper for the hasher project
>> +ConditionVirtualization=!container
> А если контейнеру выданы нужные привелегии/capabilities, то почему нельзя? Может, лучше ConditionCapability=?

...или вообще без таких проверок условий, не смог запуститься - написал ошибку в явном виде

P.S. Какие capabilities нужны, чтобы hsh работал в systemd-nspawn?

Re: [devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Alexey Gladkov]

On Thu, Jun 18, 2020 at 01:38:36AM +0300, Mikhail Novosyolov wrote:
> 18.06.2020 01:31, Mikhail Novosyolov пишет:
> > 13.12.2019 14:42, Alex Gladkov пишет:
> >> diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
> >> new file mode 100644
> >> index 0000000..e5ed9ac
> >> --- /dev/null
> >> +++ b/hasher-priv/hasher-privd.service
> >> @@ -0,0 +1,11 @@
> >> +[Unit]
> >> +Description=A privileged helper for the hasher project
> >> +ConditionVirtualization=!container
> > А если контейнеру выданы нужные привелегии/capabilities, то почему нельзя? Может, лучше ConditionCapability=?
> 
> ...или вообще без таких проверок условий, не смог запуститься - написал ошибку в явном виде
> 
> P.S. Какие capabilities нужны, чтобы hsh работал в systemd-nspawn?

Так как hasher может создавать namespace, то нужен CAP_SYS_ADMIN.

-- 
Rgrds, legion

Re: [devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Alexey Gladkov]

On Thu, Jun 18, 2020 at 01:31:38AM +0300, Mikhail Novosyolov wrote:
> 13.12.2019 14:42, Alex Gladkov пишет:
> > From: Alexey Gladkov <legion@altlinux.org>
> >
> > Signed-off-by: Alexey Gladkov <legion@altlinux.org>
> > ---
> >  hasher-priv/Makefile              |  4 ++
> >  hasher-priv/hasher-privd.service  | 11 ++++
> >  hasher-priv/hasher-privd.sysvinit | 86 +++++++++++++++++++++++++++++++
> >  3 files changed, 101 insertions(+)
> >  create mode 100644 hasher-priv/hasher-privd.service
> >  create mode 100755 hasher-priv/hasher-privd.sysvinit
> >
> > diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
> > index 82aa385..c73216f 100644
> > --- a/hasher-priv/Makefile
> > +++ b/hasher-priv/Makefile
> > @@ -14,6 +14,8 @@ MAN8PAGES = $(PROJECT).8 hasher-useradd.8
> >  TARGETS = $(PROJECT) hasher-privd hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
> >  
> >  sysconfdir = /etc
> > +initdir=$(sysconfdir)/rc.d/init.d
> > +systemd_unitdir=/lib/systemd/system
> >  libexecdir = /usr/lib
> >  sbindir = /usr/sbin
> >  mandir = /usr/share/man
> > @@ -72,6 +74,8 @@ install: all
> >  	$(MKDIR_P) -m750 $(DESTDIR)$(helperdir)
> >  	$(INSTALL) -p -m700 $(PROJECT) $(DESTDIR)$(helperdir)/
> >  	$(INSTALL) -p -m755 $(HELPERS) $(DESTDIR)$(helperdir)/
> > +	$(MKDIR_P) -m755 $(DESTDIR)$(initdir)
> > +	$(INSTALL) -p -m755 hasher-privd.sysvinit $(DESTDIR)$(initdir)/hasher-privd
> >  	$(MKDIR_P) -m755 $(DESTDIR)$(sbindir)
> >  	$(INSTALL) -p -m755 hasher-privd $(DESTDIR)$(sbindir)/
> >  	$(INSTALL) -p -m755 hasher-useradd $(DESTDIR)$(sbindir)/
> > diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
> > new file mode 100644
> > index 0000000..e5ed9ac
> > --- /dev/null
> > +++ b/hasher-priv/hasher-privd.service
> > @@ -0,0 +1,11 @@
> > +[Unit]
> > +Description=A privileged helper for the hasher project
> > +ConditionVirtualization=!container
> А если контейнеру выданы нужные привелегии/capabilities, то почему нельзя? Может, лучше ConditionCapability=?

Я не специалист в systemd. Я не смогу написать ConditionCapability
правильно. Я брал за основу какой-то другой сервис. Если вы считаете, что
это критично, то могу убрать этот сервис вообще.

> > +Documentation=man:hasher-priv(8)
> > +
> > +[Service]
> > +ExecStart=/usr/sbin/hasher-privd
> > +Restart=on-failure
> > +
> > +[Install]
> > +WantedBy=multi-user.target
> > diff --git a/hasher-priv/hasher-privd.sysvinit b/hasher-priv/hasher-privd.sysvinit
> > new file mode 100755
> > index 0000000..914fb53
> > --- /dev/null
> > +++ b/hasher-priv/hasher-privd.sysvinit
> > @@ -0,0 +1,86 @@
> > +#! /bin/sh
> > +
> > +### BEGIN INIT INFO
> > +# Short-Description:    A privileged helper for the hasher project
> > +# Description:          A privileged helper for the hasher project
> > +# Provides:             hasher-priv
> > +# Required-Start:       $remote_fs
> > +# Required-Stop:        $remote_fs
> > +# Default-Start:        2 3 4 5
> > +# Default-Stop:         0 1 6
> > +### END INIT INFO
> > +
> > +WITHOUT_RC_COMPAT=1
> > +
> > +# Source function library.
> > +. /etc/init.d/functions
> > +
> > +NAME=hasher-privd
> > +PIDFILE="/var/run/$NAME.pid"
> > +LOCKFILE="/var/lock/subsys/$NAME"
> > +RETVAL=0
> > +
> > +start()
> > +{
> > +	start_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" -- "$NAME"
> > +	RETVAL=$?
> > +	return $RETVAL
> > +}
> > +
> > +stop()
> > +{
> > +	stop_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" "$NAME"
> > +	RETVAL=$?
> > +	return $RETVAL
> > +}
> > +
> > +restart()
> > +{
> > +	stop
> > +	start
> > +}
> > +
> > +# See how we were called.
> > +case "$1" in
> > +	start)
> > +		start
> > +		;;
> > +	stop)
> > +		stop
> > +		;;
> > +	status)
> > +		status --pidfile "$PIDFILE" "$NAME"
> > +		RETVAL=$?
> > +		;;
> > +	restart)
> > +		restart
> > +		;;
> > +	reload)
> > +		restart
> > +		;;
> > +	condstart)
> > +		if [ ! -e "$LOCKFILE" ]; then
> > +			start
> > +		fi
> > +		;;
> > +	condstop)
> > +		if [ -e "$LOCKFILE" ]; then
> > +			stop
> > +		fi
> > +		;;
> > +	condrestart)
> > +		if [ -e "$LOCKFILE" ]; then
> > +			restart
> > +		fi
> > +		;;
> > +	condreload)
> > +		if [ -e "$LOCKFILE" ]; then
> > +			reload
> > +		fi
> > +		;;
> > +	*)
> > +		msg_usage "${0##*/} {start|stop|status|restart|reload|condstart|condstop|condrestart|condreload}"
> > +		RETVAL=1
> > +esac
> > +
> > +exit $RETVAL
> 

-- 
Rgrds, legion

Re: [devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Mikhail Novosyolov]

18.06.2020 01:43, Alexey Gladkov пишет:
> On Thu, Jun 18, 2020 at 01:31:38AM +0300, Mikhail Novosyolov wrote:
>> 13.12.2019 14:42, Alex Gladkov пишет:
>>> From: Alexey Gladkov <legion@altlinux.org>
>>>
>>> Signed-off-by: Alexey Gladkov <legion@altlinux.org>
>>> ---
>>>  hasher-priv/Makefile              |  4 ++
>>>  hasher-priv/hasher-privd.service  | 11 ++++
>>>  hasher-priv/hasher-privd.sysvinit | 86 +++++++++++++++++++++++++++++++
>>>  3 files changed, 101 insertions(+)
>>>  create mode 100644 hasher-priv/hasher-privd.service
>>>  create mode 100755 hasher-priv/hasher-privd.sysvinit
>>>
>>> diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
>>> index 82aa385..c73216f 100644
>>> --- a/hasher-priv/Makefile
>>> +++ b/hasher-priv/Makefile
>>> @@ -14,6 +14,8 @@ MAN8PAGES = $(PROJECT).8 hasher-useradd.8
>>>  TARGETS = $(PROJECT) hasher-privd hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
>>>  
>>>  sysconfdir = /etc
>>> +initdir=$(sysconfdir)/rc.d/init.d
>>> +systemd_unitdir=/lib/systemd/system
>>>  libexecdir = /usr/lib
>>>  sbindir = /usr/sbin
>>>  mandir = /usr/share/man
>>> @@ -72,6 +74,8 @@ install: all
>>>  	$(MKDIR_P) -m750 $(DESTDIR)$(helperdir)
>>>  	$(INSTALL) -p -m700 $(PROJECT) $(DESTDIR)$(helperdir)/
>>>  	$(INSTALL) -p -m755 $(HELPERS) $(DESTDIR)$(helperdir)/
>>> +	$(MKDIR_P) -m755 $(DESTDIR)$(initdir)
>>> +	$(INSTALL) -p -m755 hasher-privd.sysvinit $(DESTDIR)$(initdir)/hasher-privd
>>>  	$(MKDIR_P) -m755 $(DESTDIR)$(sbindir)
>>>  	$(INSTALL) -p -m755 hasher-privd $(DESTDIR)$(sbindir)/
>>>  	$(INSTALL) -p -m755 hasher-useradd $(DESTDIR)$(sbindir)/
>>> diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
>>> new file mode 100644
>>> index 0000000..e5ed9ac
>>> --- /dev/null
>>> +++ b/hasher-priv/hasher-privd.service
>>> @@ -0,0 +1,11 @@
>>> +[Unit]
>>> +Description=A privileged helper for the hasher project
>>> +ConditionVirtualization=!container
>> А если контейнеру выданы нужные привелегии/capabilities, то почему нельзя? Может, лучше ConditionCapability=?
> Я не специалист в systemd. Я не смогу написать ConditionCapability
> правильно. Я брал за основу какой-то другой сервис. Если вы считаете, что
> это критично, то могу убрать этот сервис вообще.
Критичным не считаю, я вообще не очень в курсе, как работает хешер, мимо проходил, но эту строку бы убрал из сервиса systemd.

Re: [devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 5338 bytes --]

On Fri, Dec 13, 2019 at 12:42:04PM +0100, Alex Gladkov wrote:
> From: Alexey Gladkov <legion@altlinux.org>
> 
> Signed-off-by: Alexey Gladkov <legion@altlinux.org>
> ---
>  hasher-priv/Makefile              |  4 ++
>  hasher-priv/hasher-privd.service  | 11 ++++
>  hasher-priv/hasher-privd.sysvinit | 86 +++++++++++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
>  create mode 100644 hasher-priv/hasher-privd.service
>  create mode 100755 hasher-priv/hasher-privd.sysvinit
> 
> diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
> index 82aa385..c73216f 100644
> --- a/hasher-priv/Makefile
> +++ b/hasher-priv/Makefile
> @@ -14,6 +14,8 @@ MAN8PAGES = $(PROJECT).8 hasher-useradd.8
>  TARGETS = $(PROJECT) hasher-privd hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
>  
>  sysconfdir = /etc
> +initdir=$(sysconfdir)/rc.d/init.d
> +systemd_unitdir=/lib/systemd/system
>  libexecdir = /usr/lib
>  sbindir = /usr/sbin
>  mandir = /usr/share/man
> @@ -72,6 +74,8 @@ install: all
>  	$(MKDIR_P) -m750 $(DESTDIR)$(helperdir)
>  	$(INSTALL) -p -m700 $(PROJECT) $(DESTDIR)$(helperdir)/
>  	$(INSTALL) -p -m755 $(HELPERS) $(DESTDIR)$(helperdir)/
> +	$(MKDIR_P) -m755 $(DESTDIR)$(initdir)
> +	$(INSTALL) -p -m755 hasher-privd.sysvinit $(DESTDIR)$(initdir)/hasher-privd

The systemd service is not installed.

>  	$(MKDIR_P) -m755 $(DESTDIR)$(sbindir)
>  	$(INSTALL) -p -m755 hasher-privd $(DESTDIR)$(sbindir)/
>  	$(INSTALL) -p -m755 hasher-useradd $(DESTDIR)$(sbindir)/
> diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
> new file mode 100644
> index 0000000..e5ed9ac
> --- /dev/null
> +++ b/hasher-priv/hasher-privd.service
> @@ -0,0 +1,11 @@
> +[Unit]
> +Description=A privileged helper for the hasher project
> +ConditionVirtualization=!container

In response to earlier reviewers: hasher-priv as of today does not work
inside a userns-unprivileged container and does not produce clear
diagnostics (and, from my own experience when I was joining ALT, the
developers did not as well). Thus, for now this condition is justified.
Perhaps in the future, when (and if) we introduce the ability to reuse a
mainstream container runtime as the hasher environment for users R and
B, it would make sense for us to lift this condition.

> +Documentation=man:hasher-priv(8)

Ah yes, I forgot. The patchset contains no changes to the man pages, so
the effort and behaviour change is not reflected. I agree it's best to
revisit them once we're done with the code, though.

> +
> +[Service]
> +ExecStart=/usr/sbin/hasher-privd

Suggested replacement:
"ExecStart=/usr/sbin/hasher-privd -f"

The service implicitly, by default, has Type=simple, which means the
following:
- the main process(-es) is defined by the ExecStart= command line(-s)
  and is intended to persist while the service is launched and active;
- its pid/tgid is tracked by the service manager and can be queried;
- the service manager puts it into its own cgroup;
- its standard output and standard error are redirected to system log;
- (follows from the above) the main process never has a controlling
  terminal or standard file descriptors pointing to any terminal, its
  sid is equal to its tgid — and so it does not have to perform
  manual steps to daemonize.

> +Restart=on-failure
> +
> +[Install]
> +WantedBy=multi-user.target
> diff --git a/hasher-priv/hasher-privd.sysvinit b/hasher-priv/hasher-privd.sysvinit
> new file mode 100755
> index 0000000..914fb53
> --- /dev/null
> +++ b/hasher-priv/hasher-privd.sysvinit
> @@ -0,0 +1,86 @@
> +#! /bin/sh
> +
> +### BEGIN INIT INFO
> +# Short-Description:    A privileged helper for the hasher project
> +# Description:          A privileged helper for the hasher project
> +# Provides:             hasher-priv
> +# Required-Start:       $remote_fs
> +# Required-Stop:        $remote_fs
> +# Default-Start:        2 3 4 5
> +# Default-Stop:         0 1 6
> +### END INIT INFO
> +
> +WITHOUT_RC_COMPAT=1
> +
> +# Source function library.
> +. /etc/init.d/functions
> +
> +NAME=hasher-privd
> +PIDFILE="/var/run/$NAME.pid"
> +LOCKFILE="/var/lock/subsys/$NAME"
> +RETVAL=0
> +
> +start()
> +{
> +	start_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" -- "$NAME"
> +	RETVAL=$?
> +	return $RETVAL
> +}
> +
> +stop()
> +{
> +	stop_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" "$NAME"
> +	RETVAL=$?
> +	return $RETVAL
> +}
> +
> +restart()
> +{
> +	stop
> +	start
> +}
> +
> +# See how we were called.
> +case "$1" in
> +	start)
> +		start
> +		;;
> +	stop)
> +		stop
> +		;;
> +	status)
> +		status --pidfile "$PIDFILE" "$NAME"
> +		RETVAL=$?
> +		;;
> +	restart)
> +		restart
> +		;;
> +	reload)
> +		restart
> +		;;
> +	condstart)
> +		if [ ! -e "$LOCKFILE" ]; then
> +			start
> +		fi
> +		;;
> +	condstop)
> +		if [ -e "$LOCKFILE" ]; then
> +			stop
> +		fi
> +		;;
> +	condrestart)
> +		if [ -e "$LOCKFILE" ]; then
> +			restart
> +		fi
> +		;;
> +	condreload)
> +		if [ -e "$LOCKFILE" ]; then
> +			reload
> +		fi
> +		;;
> +	*)
> +		msg_usage "${0##*/} {start|stop|status|restart|reload|condstart|condstop|condrestart|condreload}"
> +		RETVAL=1
> +esac
> +
> +exit $RETVAL
> -- 
> 2.24.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Alexey Gladkov]

[-- Attachment #1: Type: text/plain, Size: 5952 bytes --]

On Thu, Sep 17, 2020 at 04:10:52PM +0300, Arseny Maslennikov wrote:
> On Fri, Dec 13, 2019 at 12:42:04PM +0100, Alex Gladkov wrote:
> > From: Alexey Gladkov <legion@altlinux.org>
> > 
> > Signed-off-by: Alexey Gladkov <legion@altlinux.org>
> > ---
> >  hasher-priv/Makefile              |  4 ++
> >  hasher-priv/hasher-privd.service  | 11 ++++
> >  hasher-priv/hasher-privd.sysvinit | 86 +++++++++++++++++++++++++++++++
> >  3 files changed, 101 insertions(+)
> >  create mode 100644 hasher-priv/hasher-privd.service
> >  create mode 100755 hasher-priv/hasher-privd.sysvinit
> > 
> > diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
> > index 82aa385..c73216f 100644
> > --- a/hasher-priv/Makefile
> > +++ b/hasher-priv/Makefile
> > @@ -14,6 +14,8 @@ MAN8PAGES = $(PROJECT).8 hasher-useradd.8
> >  TARGETS = $(PROJECT) hasher-privd hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
> >  
> >  sysconfdir = /etc
> > +initdir=$(sysconfdir)/rc.d/init.d
> > +systemd_unitdir=/lib/systemd/system
> >  libexecdir = /usr/lib
> >  sbindir = /usr/sbin
> >  mandir = /usr/share/man
> > @@ -72,6 +74,8 @@ install: all
> >  	$(MKDIR_P) -m750 $(DESTDIR)$(helperdir)
> >  	$(INSTALL) -p -m700 $(PROJECT) $(DESTDIR)$(helperdir)/
> >  	$(INSTALL) -p -m755 $(HELPERS) $(DESTDIR)$(helperdir)/
> > +	$(MKDIR_P) -m755 $(DESTDIR)$(initdir)
> > +	$(INSTALL) -p -m755 hasher-privd.sysvinit $(DESTDIR)$(initdir)/hasher-privd
> 
> The systemd service is not installed.

I don't really care about systemd. I'm not an expert in creating services
for it. I hope that someone who can create and test the service. It may be
you :)

> >  	$(MKDIR_P) -m755 $(DESTDIR)$(sbindir)
> >  	$(INSTALL) -p -m755 hasher-privd $(DESTDIR)$(sbindir)/
> >  	$(INSTALL) -p -m755 hasher-useradd $(DESTDIR)$(sbindir)/
> > diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
> > new file mode 100644
> > index 0000000..e5ed9ac
> > --- /dev/null
> > +++ b/hasher-priv/hasher-privd.service
> > @@ -0,0 +1,11 @@
> > +[Unit]
> > +Description=A privileged helper for the hasher project
> > +ConditionVirtualization=!container
> 
> In response to earlier reviewers: hasher-priv as of today does not work
> inside a userns-unprivileged container and does not produce clear
> diagnostics (and, from my own experience when I was joining ALT, the
> developers did not as well). Thus, for now this condition is justified.
> Perhaps in the future, when (and if) we introduce the ability to reuse a
> mainstream container runtime as the hasher environment for users R and
> B, it would make sense for us to lift this condition.
> 
> > +Documentation=man:hasher-priv(8)
> 
> Ah yes, I forgot. The patchset contains no changes to the man pages, so
> the effort and behaviour change is not reflected. I agree it's best to
> revisit them once we're done with the code, though.
> 
> > +
> > +[Service]
> > +ExecStart=/usr/sbin/hasher-privd
> 
> Suggested replacement:
> "ExecStart=/usr/sbin/hasher-privd -f"
> 
> The service implicitly, by default, has Type=simple, which means the
> following:
> - the main process(-es) is defined by the ExecStart= command line(-s)
>   and is intended to persist while the service is launched and active;
> - its pid/tgid is tracked by the service manager and can be queried;
> - the service manager puts it into its own cgroup;
> - its standard output and standard error are redirected to system log;
> - (follows from the above) the main process never has a controlling
>   terminal or standard file descriptors pointing to any terminal, its
>   sid is equal to its tgid — and so it does not have to perform
>   manual steps to daemonize.
> 
> > +Restart=on-failure
> > +
> > +[Install]
> > +WantedBy=multi-user.target
> > diff --git a/hasher-priv/hasher-privd.sysvinit b/hasher-priv/hasher-privd.sysvinit
> > new file mode 100755
> > index 0000000..914fb53
> > --- /dev/null
> > +++ b/hasher-priv/hasher-privd.sysvinit
> > @@ -0,0 +1,86 @@
> > +#! /bin/sh
> > +
> > +### BEGIN INIT INFO
> > +# Short-Description:    A privileged helper for the hasher project
> > +# Description:          A privileged helper for the hasher project
> > +# Provides:             hasher-priv
> > +# Required-Start:       $remote_fs
> > +# Required-Stop:        $remote_fs
> > +# Default-Start:        2 3 4 5
> > +# Default-Stop:         0 1 6
> > +### END INIT INFO
> > +
> > +WITHOUT_RC_COMPAT=1
> > +
> > +# Source function library.
> > +. /etc/init.d/functions
> > +
> > +NAME=hasher-privd
> > +PIDFILE="/var/run/$NAME.pid"
> > +LOCKFILE="/var/lock/subsys/$NAME"
> > +RETVAL=0
> > +
> > +start()
> > +{
> > +	start_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" -- "$NAME"
> > +	RETVAL=$?
> > +	return $RETVAL
> > +}
> > +
> > +stop()
> > +{
> > +	stop_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" "$NAME"
> > +	RETVAL=$?
> > +	return $RETVAL
> > +}
> > +
> > +restart()
> > +{
> > +	stop
> > +	start
> > +}
> > +
> > +# See how we were called.
> > +case "$1" in
> > +	start)
> > +		start
> > +		;;
> > +	stop)
> > +		stop
> > +		;;
> > +	status)
> > +		status --pidfile "$PIDFILE" "$NAME"
> > +		RETVAL=$?
> > +		;;
> > +	restart)
> > +		restart
> > +		;;
> > +	reload)
> > +		restart
> > +		;;
> > +	condstart)
> > +		if [ ! -e "$LOCKFILE" ]; then
> > +			start
> > +		fi
> > +		;;
> > +	condstop)
> > +		if [ -e "$LOCKFILE" ]; then
> > +			stop
> > +		fi
> > +		;;
> > +	condrestart)
> > +		if [ -e "$LOCKFILE" ]; then
> > +			restart
> > +		fi
> > +		;;
> > +	condreload)
> > +		if [ -e "$LOCKFILE" ]; then
> > +			reload
> > +		fi
> > +		;;
> > +	*)
> > +		msg_usage "${0##*/} {start|stop|status|restart|reload|condstart|condstop|condrestart|condreload}"
> > +		RETVAL=1
> > +esac
> > +
> > +exit $RETVAL
> > -- 
> > 2.24.0
> > 



-- 
Rgrds, legion


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [devel] [PATCH hasher-priv v1 2/3] Add systemd and sysvinit service files

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 6685 bytes --]

On Thu, Oct 01, 2020 at 07:25:34PM +0200, Alexey Gladkov wrote:
> On Thu, Sep 17, 2020 at 04:10:52PM +0300, Arseny Maslennikov wrote:
> > On Fri, Dec 13, 2019 at 12:42:04PM +0100, Alex Gladkov wrote:
> > > From: Alexey Gladkov <legion@altlinux.org>
> > > 
> > > Signed-off-by: Alexey Gladkov <legion@altlinux.org>
> > > ---
> > >  hasher-priv/Makefile              |  4 ++
> > >  hasher-priv/hasher-privd.service  | 11 ++++
> > >  hasher-priv/hasher-privd.sysvinit | 86 +++++++++++++++++++++++++++++++
> > >  3 files changed, 101 insertions(+)
> > >  create mode 100644 hasher-priv/hasher-privd.service
> > >  create mode 100755 hasher-priv/hasher-privd.sysvinit
> > > 
> > > diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
> > > index 82aa385..c73216f 100644
> > > --- a/hasher-priv/Makefile
> > > +++ b/hasher-priv/Makefile
> > > @@ -14,6 +14,8 @@ MAN8PAGES = $(PROJECT).8 hasher-useradd.8
> > >  TARGETS = $(PROJECT) hasher-privd hasher-useradd $(HELPERS) $(MAN5PAGES) $(MAN8PAGES)
> > >  
> > >  sysconfdir = /etc
> > > +initdir=$(sysconfdir)/rc.d/init.d
> > > +systemd_unitdir=/lib/systemd/system
> > >  libexecdir = /usr/lib
> > >  sbindir = /usr/sbin
> > >  mandir = /usr/share/man
> > > @@ -72,6 +74,8 @@ install: all
> > >  	$(MKDIR_P) -m750 $(DESTDIR)$(helperdir)
> > >  	$(INSTALL) -p -m700 $(PROJECT) $(DESTDIR)$(helperdir)/
> > >  	$(INSTALL) -p -m755 $(HELPERS) $(DESTDIR)$(helperdir)/
> > > +	$(MKDIR_P) -m755 $(DESTDIR)$(initdir)
> > > +	$(INSTALL) -p -m755 hasher-privd.sysvinit $(DESTDIR)$(initdir)/hasher-privd
> > 
> > The systemd service is not installed.
> 
> I don't really care about systemd. I'm not an expert in creating services
> for it. I hope that someone who can create and test the service. It may be
> you :)
> 

I understand. That note was declaring a statement, not blaming you in
any way; I can help take care of systemd support.

> > >  	$(MKDIR_P) -m755 $(DESTDIR)$(sbindir)
> > >  	$(INSTALL) -p -m755 hasher-privd $(DESTDIR)$(sbindir)/
> > >  	$(INSTALL) -p -m755 hasher-useradd $(DESTDIR)$(sbindir)/
> > > diff --git a/hasher-priv/hasher-privd.service b/hasher-priv/hasher-privd.service
> > > new file mode 100644
> > > index 0000000..e5ed9ac
> > > --- /dev/null
> > > +++ b/hasher-priv/hasher-privd.service
> > > @@ -0,0 +1,11 @@
> > > +[Unit]
> > > +Description=A privileged helper for the hasher project
> > > +ConditionVirtualization=!container
> > 
> > In response to earlier reviewers: hasher-priv as of today does not work
> > inside a userns-unprivileged container and does not produce clear
> > diagnostics (and, from my own experience when I was joining ALT, the
> > developers did not as well). Thus, for now this condition is justified.
> > Perhaps in the future, when (and if) we introduce the ability to reuse a
> > mainstream container runtime as the hasher environment for users R and
> > B, it would make sense for us to lift this condition.
> > 
> > > +Documentation=man:hasher-priv(8)
> > 
> > Ah yes, I forgot. The patchset contains no changes to the man pages, so
> > the effort and behaviour change is not reflected. I agree it's best to
> > revisit them once we're done with the code, though.
> > 
> > > +
> > > +[Service]
> > > +ExecStart=/usr/sbin/hasher-privd
> > 
> > Suggested replacement:
> > "ExecStart=/usr/sbin/hasher-privd -f"
> > 
> > The service implicitly, by default, has Type=simple, which means the
> > following:
> > - the main process(-es) is defined by the ExecStart= command line(-s)
> >   and is intended to persist while the service is launched and active;
> > - its pid/tgid is tracked by the service manager and can be queried;
> > - the service manager puts it into its own cgroup;
> > - its standard output and standard error are redirected to system log;
> > - (follows from the above) the main process never has a controlling
> >   terminal or standard file descriptors pointing to any terminal, its
> >   sid is equal to its tgid — and so it does not have to perform
> >   manual steps to daemonize.
> > 
> > > +Restart=on-failure
> > > +
> > > +[Install]
> > > +WantedBy=multi-user.target
> > > diff --git a/hasher-priv/hasher-privd.sysvinit b/hasher-priv/hasher-privd.sysvinit
> > > new file mode 100755
> > > index 0000000..914fb53
> > > --- /dev/null
> > > +++ b/hasher-priv/hasher-privd.sysvinit
> > > @@ -0,0 +1,86 @@
> > > +#! /bin/sh
> > > +
> > > +### BEGIN INIT INFO
> > > +# Short-Description:    A privileged helper for the hasher project
> > > +# Description:          A privileged helper for the hasher project
> > > +# Provides:             hasher-priv
> > > +# Required-Start:       $remote_fs
> > > +# Required-Stop:        $remote_fs
> > > +# Default-Start:        2 3 4 5
> > > +# Default-Stop:         0 1 6
> > > +### END INIT INFO
> > > +
> > > +WITHOUT_RC_COMPAT=1
> > > +
> > > +# Source function library.
> > > +. /etc/init.d/functions
> > > +
> > > +NAME=hasher-privd
> > > +PIDFILE="/var/run/$NAME.pid"
> > > +LOCKFILE="/var/lock/subsys/$NAME"
> > > +RETVAL=0
> > > +
> > > +start()
> > > +{
> > > +	start_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" -- "$NAME"
> > > +	RETVAL=$?
> > > +	return $RETVAL
> > > +}
> > > +
> > > +stop()
> > > +{
> > > +	stop_daemon --pidfile "$PIDFILE" --lockfile "$LOCKFILE" "$NAME"
> > > +	RETVAL=$?
> > > +	return $RETVAL
> > > +}
> > > +
> > > +restart()
> > > +{
> > > +	stop
> > > +	start
> > > +}
> > > +
> > > +# See how we were called.
> > > +case "$1" in
> > > +	start)
> > > +		start
> > > +		;;
> > > +	stop)
> > > +		stop
> > > +		;;
> > > +	status)
> > > +		status --pidfile "$PIDFILE" "$NAME"
> > > +		RETVAL=$?
> > > +		;;
> > > +	restart)
> > > +		restart
> > > +		;;
> > > +	reload)
> > > +		restart
> > > +		;;
> > > +	condstart)
> > > +		if [ ! -e "$LOCKFILE" ]; then
> > > +			start
> > > +		fi
> > > +		;;
> > > +	condstop)
> > > +		if [ -e "$LOCKFILE" ]; then
> > > +			stop
> > > +		fi
> > > +		;;
> > > +	condrestart)
> > > +		if [ -e "$LOCKFILE" ]; then
> > > +			restart
> > > +		fi
> > > +		;;
> > > +	condreload)
> > > +		if [ -e "$LOCKFILE" ]; then
> > > +			reload
> > > +		fi
> > > +		;;
> > > +	*)
> > > +		msg_usage "${0##*/} {start|stop|status|restart|reload|condstart|condstop|condrestart|condreload}"
> > > +		RETVAL=1
> > > +esac
> > > +
> > > +exit $RETVAL
> > > -- 
> > > 2.24.0
> > > 
> 
> 
> 
> -- 
> Rgrds, legion
> 



> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[devel] [PATCH hasher-priv v1 3/3] Add cgroup support

[Alex Gladkov]

From: Alexey Gladkov <legion@altlinux.org>

Signed-off-by: Alexey Gladkov <legion@altlinux.org>
---
 hasher-priv/Makefile      |   2 +-
 hasher-priv/caller_task.c |   3 +
 hasher-priv/cgroup.c      | 119 ++++++++++++++++++++++++++++++++++++++
 hasher-priv/config.c      |   5 ++
 hasher-priv/priv.h        |   2 +
 hasher-priv/server.conf   |   9 +++
 6 files changed, 139 insertions(+), 1 deletion(-)
 create mode 100644 hasher-priv/cgroup.c

diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
index c73216f..e999972 100644
--- a/hasher-priv/Makefile
+++ b/hasher-priv/Makefile
@@ -51,7 +51,7 @@ server_SRC = hasher-privd.c \
 	chdir.c chdiruid.c chid.c child.c chrootuid.c cmdline.c \
 	config.c fds.c getconf.c getugid.c ipc.c killuid.c io_log.c io_x11.c \
 	makedev.c mount.c net.c parent.c pass.c pty.c signal.c tty.c \
-	unshare.c xmalloc.c x11.c
+	unshare.c xmalloc.c x11.c cgroup.c
 server_OBJ = $(server_SRC:.c=.o)
 
 DEP = $(SRC:.c=.d) $(server_SRC:.c=.d)
diff --git a/hasher-priv/caller_task.c b/hasher-priv/caller_task.c
index d8f2dd5..722e0a6 100644
--- a/hasher-priv/caller_task.c
+++ b/hasher-priv/caller_task.c
@@ -95,6 +95,9 @@ caller_task(struct task *task)
 		return pid;
 	}
 
+	if (join_cgroup() < 0)
+		exit(rc);
+
 	if ((rc = reopen_iostreams(task->stdin, task->stdout, task->stderr)) < 0)
 		exit(rc);
 
diff --git a/hasher-priv/cgroup.c b/hasher-priv/cgroup.c
new file mode 100644
index 0000000..ac14938
--- /dev/null
+++ b/hasher-priv/cgroup.c
@@ -0,0 +1,119 @@
+
+/*
+  Copyright (C) 2019  Alexey Gladkov <legion@altlinux.org>
+
+  The cgroup helper for hasher-privd program.
+
+  SPDX-License-Identifier: GPL-2.0-or-later
+*/
+
+#include <sys/param.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <unistd.h>
+#include <stdio.h>
+#include <stdarg.h>
+#include <string.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "logging.h"
+#include "priv.h"
+
+int
+join_cgroup(void)
+{
+	int ret = 0;
+
+	if (!server_cgroup_template)
+		return ret;
+
+	char cgroup_path[MAXPATHLEN];
+
+	size_t i, j, escape;
+	size_t len = strlen(server_cgroup_template);
+	int fd = -1;
+
+	i = j = escape = 0;
+
+	for (; i < len; i++) {
+		if (j > sizeof(cgroup_path)) {
+			err("path too long");
+			ret = -1;
+			goto fail;
+		}
+
+		if (escape) {
+			ssize_t n = 0;
+			char *p = cgroup_path + j;
+			size_t sz = (size_t) (p - cgroup_path);
+
+			switch (server_cgroup_template[i]) {
+				case 'u':
+					n = snprintf(p, sz, "%s", caller_user);
+					break;
+				case 'U':
+					n = snprintf(p, sz, "%u", caller_uid);
+					break;
+				case 'G':
+					n = snprintf(p, sz, "%u", caller_gid);
+					break;
+				case 'N':
+					n = snprintf(p, sz, "%u", caller_num);
+					break;
+				case '%':
+					n = snprintf(p, sz, "%%");
+					break;
+			}
+
+			if (n <= 0) {
+				err("unable to expand escape sequence: %%%c",
+				    server_cgroup_template[i]);
+				ret = -1;
+				goto fail;
+			}
+
+			j += (size_t) n;
+
+			escape = 0;
+			continue;
+
+		} else if (server_cgroup_template[i] == '%') {
+			escape = 1;
+			continue;
+
+		} else if (server_cgroup_template[i] == '/' && j > 0) {
+			cgroup_path[j] = '\0';
+
+			errno = 0;
+			if (mkdir(cgroup_path, 0755) < 0 && errno != EEXIST) {
+				err("mkdir: %s: errno=%d: %m", cgroup_path, errno);
+				ret = -1;
+				goto fail;
+			}
+		}
+
+		cgroup_path[j++] = server_cgroup_template[i];
+	}
+
+	cgroup_path[j] = '\0';
+
+	if ((fd = open(cgroup_path, O_CREAT | O_WRONLY | O_CLOEXEC, 0644)) < 0) {
+		err("open: %s: %m", cgroup_path);
+		ret = -1;
+		goto fail;
+	}
+
+	if (dprintf(fd, "%d\n", getpid()) < 0) {
+		err("dprintf: %s: unable to write pid", cgroup_path);
+		ret = -1;
+	}
+fail:
+	if (fd >= 0 && close(fd) < 0) {
+		err("close: %s: %m", cgroup_path);
+		ret = -1;
+	}
+
+	return ret;
+}
diff --git a/hasher-priv/config.c b/hasher-priv/config.c
index 6b6bdb1..3faf936 100644
--- a/hasher-priv/config.c
+++ b/hasher-priv/config.c
@@ -30,6 +30,7 @@ const char *const *chroot_prefix_list;
 const char *chroot_prefix_path;
 const char *change_user1, *change_user2;
 char *server_control_group = NULL;
+char *server_cgroup_template = NULL;
 char *server_pidfile = NULL;
 const char *term;
 const char *x11_display, *x11_key;
@@ -671,6 +672,9 @@ set_server_config(const char *name, const char *value, const char *filename)
 	} else if (!strcasecmp("control_group", name)) {
 		free(server_control_group);
 		server_control_group = xstrdup(value);
+	} else if (!strcasecmp("cgroup_template", name)) {
+		free(server_cgroup_template);
+		server_cgroup_template = xstrdup(value);
 	} else {
 		bad_option_name(name, filename);
 	}
@@ -771,4 +775,5 @@ free_server_configuration(void)
 {
 	free(server_pidfile);
 	free(server_control_group);
+	free(server_cgroup_template);
 }
diff --git a/hasher-priv/priv.h b/hasher-priv/priv.h
index f0eb9f9..f29603a 100644
--- a/hasher-priv/priv.h
+++ b/hasher-priv/priv.h
@@ -120,6 +120,7 @@ int     do_chrootuid2(void);
 
 int process_caller_task(int, struct task *);
 pid_t fork_server(int, uid_t, gid_t, unsigned);
+int join_cgroup(void);
 
 extern const char *chroot_path;
 extern const char **chroot_argv;
@@ -162,6 +163,7 @@ extern work_limit_t wlimit;
 extern int server_log_priority;
 extern unsigned long server_session_timeout;
 extern char *server_control_group;
+extern char *server_cgroup_template;
 extern char *server_pidfile;
 extern gid_t server_gid;
 
diff --git a/hasher-priv/server.conf b/hasher-priv/server.conf
index 53ea5c3..9e70487 100644
--- a/hasher-priv/server.conf
+++ b/hasher-priv/server.conf
@@ -11,3 +11,12 @@ session_timeout=3600
 
 # Allow users of this group to interact with hasher-privd via the control socket.
 control_group=hashman
+
+# Template for cgroup path to which task handler should be added.
+#
+# %u -- Session's user name.
+# %U -- Session's user numeric ID.
+# %G -- Session's group numeric ID.
+# %N -- Session's user number.
+#
+#cgroup_template=/sys/fs/cgroup2/hasher-priv/%u/cgroup.procs
-- 
2.24.0

Re: [devel] [PATCH hasher-priv v1 3/3] Add cgroup support

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 7807 bytes --]

On Fri, Dec 13, 2019 at 12:42:05PM +0100, Alex Gladkov wrote:
> From: Alexey Gladkov <legion@altlinux.org>
> 

Could you please explain what you're trying to do with this patch?
Even if it's obvious from the source itself, we still must have an
opportunity to discuss, and a decent explanation should stay in the
project history.

Most likely, it'll turn out we _at least_ have to pass Delegate=yes to
the systemd service:

       Delegate=
           Turns on delegation of further resource control
           partitioning to processes of the unit. Units where
           this is enabled may create and manage their own
           private subhierarchy of control groups below the
           control group of the unit itself.
Manual page systemd.resource-control(5): lines 786-791

Do we only support cgroup2 and ignore cgroup1? If yes, great, but
perhaps then we might want to have a setting to not fiddle with cgroup
trees, to support the unfortunate users that have to run Docker and
other garbage.

> Signed-off-by: Alexey Gladkov <legion@altlinux.org>
> ---
>  hasher-priv/Makefile      |   2 +-
>  hasher-priv/caller_task.c |   3 +
>  hasher-priv/cgroup.c      | 119 ++++++++++++++++++++++++++++++++++++++
>  hasher-priv/config.c      |   5 ++
>  hasher-priv/priv.h        |   2 +
>  hasher-priv/server.conf   |   9 +++
>  6 files changed, 139 insertions(+), 1 deletion(-)
>  create mode 100644 hasher-priv/cgroup.c
> 
> diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
> index c73216f..e999972 100644
> --- a/hasher-priv/Makefile
> +++ b/hasher-priv/Makefile
> @@ -51,7 +51,7 @@ server_SRC = hasher-privd.c \
>  	chdir.c chdiruid.c chid.c child.c chrootuid.c cmdline.c \
>  	config.c fds.c getconf.c getugid.c ipc.c killuid.c io_log.c io_x11.c \
>  	makedev.c mount.c net.c parent.c pass.c pty.c signal.c tty.c \
> -	unshare.c xmalloc.c x11.c
> +	unshare.c xmalloc.c x11.c cgroup.c
>  server_OBJ = $(server_SRC:.c=.o)
>  
>  DEP = $(SRC:.c=.d) $(server_SRC:.c=.d)
> diff --git a/hasher-priv/caller_task.c b/hasher-priv/caller_task.c
> index d8f2dd5..722e0a6 100644
> --- a/hasher-priv/caller_task.c
> +++ b/hasher-priv/caller_task.c
> @@ -95,6 +95,9 @@ caller_task(struct task *task)
>  		return pid;
>  	}
>  
> +	if (join_cgroup() < 0)
> +		exit(rc);
> +
>  	if ((rc = reopen_iostreams(task->stdin, task->stdout, task->stderr)) < 0)
>  		exit(rc);
>  
> diff --git a/hasher-priv/cgroup.c b/hasher-priv/cgroup.c
> new file mode 100644
> index 0000000..ac14938
> --- /dev/null
> +++ b/hasher-priv/cgroup.c
> @@ -0,0 +1,119 @@
> +
> +/*
> +  Copyright (C) 2019  Alexey Gladkov <legion@altlinux.org>
> +
> +  The cgroup helper for hasher-privd program.
> +
> +  SPDX-License-Identifier: GPL-2.0-or-later
> +*/
> +
> +#include <sys/param.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include "logging.h"
> +#include "priv.h"
> +
> +int
> +join_cgroup(void)
> +{
> +	int ret = 0;
> +
> +	if (!server_cgroup_template)
> +		return ret;
> +
> +	char cgroup_path[MAXPATHLEN];
> +
> +	size_t i, j, escape;
> +	size_t len = strlen(server_cgroup_template);
> +	int fd = -1;
> +
> +	i = j = escape = 0;
> +
> +	for (; i < len; i++) {
> +		if (j > sizeof(cgroup_path)) {
> +			err("path too long");
> +			ret = -1;
> +			goto fail;
> +		}
> +
> +		if (escape) {
> +			ssize_t n = 0;
> +			char *p = cgroup_path + j;
> +			size_t sz = (size_t) (p - cgroup_path);
> +
> +			switch (server_cgroup_template[i]) {
> +				case 'u':
> +					n = snprintf(p, sz, "%s", caller_user);
> +					break;
> +				case 'U':
> +					n = snprintf(p, sz, "%u", caller_uid);
> +					break;
> +				case 'G':
> +					n = snprintf(p, sz, "%u", caller_gid);
> +					break;
> +				case 'N':
> +					n = snprintf(p, sz, "%u", caller_num);
> +					break;
> +				case '%':
> +					n = snprintf(p, sz, "%%");
> +					break;
> +			}
> +
> +			if (n <= 0) {
> +				err("unable to expand escape sequence: %%%c",
> +				    server_cgroup_template[i]);
> +				ret = -1;
> +				goto fail;
> +			}
> +
> +			j += (size_t) n;
> +
> +			escape = 0;
> +			continue;
> +
> +		} else if (server_cgroup_template[i] == '%') {
> +			escape = 1;
> +			continue;
> +
> +		} else if (server_cgroup_template[i] == '/' && j > 0) {
> +			cgroup_path[j] = '\0';
> +
> +			errno = 0;
> +			if (mkdir(cgroup_path, 0755) < 0 && errno != EEXIST) {
> +				err("mkdir: %s: errno=%d: %m", cgroup_path, errno);
> +				ret = -1;
> +				goto fail;
> +			}
> +		}
> +
> +		cgroup_path[j++] = server_cgroup_template[i];
> +	}
> +
> +	cgroup_path[j] = '\0';
> +
> +	if ((fd = open(cgroup_path, O_CREAT | O_WRONLY | O_CLOEXEC, 0644)) < 0) {
> +		err("open: %s: %m", cgroup_path);
> +		ret = -1;
> +		goto fail;
> +	}
> +
> +	if (dprintf(fd, "%d\n", getpid()) < 0) {
> +		err("dprintf: %s: unable to write pid", cgroup_path);
> +		ret = -1;
> +	}
> +fail:
> +	if (fd >= 0 && close(fd) < 0) {
> +		err("close: %s: %m", cgroup_path);
> +		ret = -1;
> +	}
> +
> +	return ret;
> +}
> diff --git a/hasher-priv/config.c b/hasher-priv/config.c
> index 6b6bdb1..3faf936 100644
> --- a/hasher-priv/config.c
> +++ b/hasher-priv/config.c
> @@ -30,6 +30,7 @@ const char *const *chroot_prefix_list;
>  const char *chroot_prefix_path;
>  const char *change_user1, *change_user2;
>  char *server_control_group = NULL;
> +char *server_cgroup_template = NULL;
>  char *server_pidfile = NULL;
>  const char *term;
>  const char *x11_display, *x11_key;
> @@ -671,6 +672,9 @@ set_server_config(const char *name, const char *value, const char *filename)
>  	} else if (!strcasecmp("control_group", name)) {
>  		free(server_control_group);
>  		server_control_group = xstrdup(value);
> +	} else if (!strcasecmp("cgroup_template", name)) {
> +		free(server_cgroup_template);
> +		server_cgroup_template = xstrdup(value);
>  	} else {
>  		bad_option_name(name, filename);
>  	}
> @@ -771,4 +775,5 @@ free_server_configuration(void)
>  {
>  	free(server_pidfile);
>  	free(server_control_group);
> +	free(server_cgroup_template);
>  }
> diff --git a/hasher-priv/priv.h b/hasher-priv/priv.h
> index f0eb9f9..f29603a 100644
> --- a/hasher-priv/priv.h
> +++ b/hasher-priv/priv.h
> @@ -120,6 +120,7 @@ int     do_chrootuid2(void);
>  
>  int process_caller_task(int, struct task *);
>  pid_t fork_server(int, uid_t, gid_t, unsigned);
> +int join_cgroup(void);
>  
>  extern const char *chroot_path;
>  extern const char **chroot_argv;
> @@ -162,6 +163,7 @@ extern work_limit_t wlimit;
>  extern int server_log_priority;
>  extern unsigned long server_session_timeout;
>  extern char *server_control_group;
> +extern char *server_cgroup_template;
>  extern char *server_pidfile;
>  extern gid_t server_gid;
>  
> diff --git a/hasher-priv/server.conf b/hasher-priv/server.conf
> index 53ea5c3..9e70487 100644
> --- a/hasher-priv/server.conf
> +++ b/hasher-priv/server.conf
> @@ -11,3 +11,12 @@ session_timeout=3600
>  
>  # Allow users of this group to interact with hasher-privd via the control socket.
>  control_group=hashman
> +
> +# Template for cgroup path to which task handler should be added.
> +#
> +# %u -- Session's user name.
> +# %U -- Session's user numeric ID.
> +# %G -- Session's group numeric ID.
> +# %N -- Session's user number.
> +#
> +#cgroup_template=/sys/fs/cgroup2/hasher-priv/%u/cgroup.procs
> -- 
> 2.24.0
> 
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 3/3] Add cgroup support

[Alexey Gladkov]

[-- Attachment #1: Type: text/plain, Size: 8752 bytes --]

On Thu, Sep 17, 2020 at 04:11:07PM +0300, Arseny Maslennikov wrote:
> On Fri, Dec 13, 2019 at 12:42:05PM +0100, Alex Gladkov wrote:
> > From: Alexey Gladkov <legion@altlinux.org>
> > 
> 
> Could you please explain what you're trying to do with this patch?
> Even if it's obvious from the source itself, we still must have an
> opportunity to discuss, and a decent explanation should stay in the
> project history.

I think this patch is simple enough.

> Most likely, it'll turn out we _at least_ have to pass Delegate=yes to
> the systemd service:
> 
>        Delegate=
>            Turns on delegation of further resource control
>            partitioning to processes of the unit. Units where
>            this is enabled may create and manage their own
>            private subhierarchy of control groups below the
>            control group of the unit itself.
> Manual page systemd.resource-control(5): lines 786-791

I'm pretty sure the hasher-priv shouldn't be tied to systemd. I'm also
convinced that the server will not be tied. The hasher-privd must be able
to run on systems without systemd.

> Do we only support cgroup2 and ignore cgroup1? If yes, great, but
> perhaps then we might want to have a setting to not fiddle with cgroup
> trees, to support the unfortunate users that have to run Docker and
> other garbage.

Yeah, I didn't plan on supporting legacy version of cgroups. Docker
already can work with cgroupsv2.

> 
> > Signed-off-by: Alexey Gladkov <legion@altlinux.org>
> > ---
> >  hasher-priv/Makefile      |   2 +-
> >  hasher-priv/caller_task.c |   3 +
> >  hasher-priv/cgroup.c      | 119 ++++++++++++++++++++++++++++++++++++++
> >  hasher-priv/config.c      |   5 ++
> >  hasher-priv/priv.h        |   2 +
> >  hasher-priv/server.conf   |   9 +++
> >  6 files changed, 139 insertions(+), 1 deletion(-)
> >  create mode 100644 hasher-priv/cgroup.c
> > 
> > diff --git a/hasher-priv/Makefile b/hasher-priv/Makefile
> > index c73216f..e999972 100644
> > --- a/hasher-priv/Makefile
> > +++ b/hasher-priv/Makefile
> > @@ -51,7 +51,7 @@ server_SRC = hasher-privd.c \
> >  	chdir.c chdiruid.c chid.c child.c chrootuid.c cmdline.c \
> >  	config.c fds.c getconf.c getugid.c ipc.c killuid.c io_log.c io_x11.c \
> >  	makedev.c mount.c net.c parent.c pass.c pty.c signal.c tty.c \
> > -	unshare.c xmalloc.c x11.c
> > +	unshare.c xmalloc.c x11.c cgroup.c
> >  server_OBJ = $(server_SRC:.c=.o)
> >  
> >  DEP = $(SRC:.c=.d) $(server_SRC:.c=.d)
> > diff --git a/hasher-priv/caller_task.c b/hasher-priv/caller_task.c
> > index d8f2dd5..722e0a6 100644
> > --- a/hasher-priv/caller_task.c
> > +++ b/hasher-priv/caller_task.c
> > @@ -95,6 +95,9 @@ caller_task(struct task *task)
> >  		return pid;
> >  	}
> >  
> > +	if (join_cgroup() < 0)
> > +		exit(rc);
> > +
> >  	if ((rc = reopen_iostreams(task->stdin, task->stdout, task->stderr)) < 0)
> >  		exit(rc);
> >  
> > diff --git a/hasher-priv/cgroup.c b/hasher-priv/cgroup.c
> > new file mode 100644
> > index 0000000..ac14938
> > --- /dev/null
> > +++ b/hasher-priv/cgroup.c
> > @@ -0,0 +1,119 @@
> > +
> > +/*
> > +  Copyright (C) 2019  Alexey Gladkov <legion@altlinux.org>
> > +
> > +  The cgroup helper for hasher-privd program.
> > +
> > +  SPDX-License-Identifier: GPL-2.0-or-later
> > +*/
> > +
> > +#include <sys/param.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +
> > +#include <unistd.h>
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +
> > +#include "logging.h"
> > +#include "priv.h"
> > +
> > +int
> > +join_cgroup(void)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!server_cgroup_template)
> > +		return ret;
> > +
> > +	char cgroup_path[MAXPATHLEN];
> > +
> > +	size_t i, j, escape;
> > +	size_t len = strlen(server_cgroup_template);
> > +	int fd = -1;
> > +
> > +	i = j = escape = 0;
> > +
> > +	for (; i < len; i++) {
> > +		if (j > sizeof(cgroup_path)) {
> > +			err("path too long");
> > +			ret = -1;
> > +			goto fail;
> > +		}
> > +
> > +		if (escape) {
> > +			ssize_t n = 0;
> > +			char *p = cgroup_path + j;
> > +			size_t sz = (size_t) (p - cgroup_path);
> > +
> > +			switch (server_cgroup_template[i]) {
> > +				case 'u':
> > +					n = snprintf(p, sz, "%s", caller_user);
> > +					break;
> > +				case 'U':
> > +					n = snprintf(p, sz, "%u", caller_uid);
> > +					break;
> > +				case 'G':
> > +					n = snprintf(p, sz, "%u", caller_gid);
> > +					break;
> > +				case 'N':
> > +					n = snprintf(p, sz, "%u", caller_num);
> > +					break;
> > +				case '%':
> > +					n = snprintf(p, sz, "%%");
> > +					break;
> > +			}
> > +
> > +			if (n <= 0) {
> > +				err("unable to expand escape sequence: %%%c",
> > +				    server_cgroup_template[i]);
> > +				ret = -1;
> > +				goto fail;
> > +			}
> > +
> > +			j += (size_t) n;
> > +
> > +			escape = 0;
> > +			continue;
> > +
> > +		} else if (server_cgroup_template[i] == '%') {
> > +			escape = 1;
> > +			continue;
> > +
> > +		} else if (server_cgroup_template[i] == '/' && j > 0) {
> > +			cgroup_path[j] = '\0';
> > +
> > +			errno = 0;
> > +			if (mkdir(cgroup_path, 0755) < 0 && errno != EEXIST) {
> > +				err("mkdir: %s: errno=%d: %m", cgroup_path, errno);
> > +				ret = -1;
> > +				goto fail;
> > +			}
> > +		}
> > +
> > +		cgroup_path[j++] = server_cgroup_template[i];
> > +	}
> > +
> > +	cgroup_path[j] = '\0';
> > +
> > +	if ((fd = open(cgroup_path, O_CREAT | O_WRONLY | O_CLOEXEC, 0644)) < 0) {
> > +		err("open: %s: %m", cgroup_path);
> > +		ret = -1;
> > +		goto fail;
> > +	}
> > +
> > +	if (dprintf(fd, "%d\n", getpid()) < 0) {
> > +		err("dprintf: %s: unable to write pid", cgroup_path);
> > +		ret = -1;
> > +	}
> > +fail:
> > +	if (fd >= 0 && close(fd) < 0) {
> > +		err("close: %s: %m", cgroup_path);
> > +		ret = -1;
> > +	}
> > +
> > +	return ret;
> > +}
> > diff --git a/hasher-priv/config.c b/hasher-priv/config.c
> > index 6b6bdb1..3faf936 100644
> > --- a/hasher-priv/config.c
> > +++ b/hasher-priv/config.c
> > @@ -30,6 +30,7 @@ const char *const *chroot_prefix_list;
> >  const char *chroot_prefix_path;
> >  const char *change_user1, *change_user2;
> >  char *server_control_group = NULL;
> > +char *server_cgroup_template = NULL;
> >  char *server_pidfile = NULL;
> >  const char *term;
> >  const char *x11_display, *x11_key;
> > @@ -671,6 +672,9 @@ set_server_config(const char *name, const char *value, const char *filename)
> >  	} else if (!strcasecmp("control_group", name)) {
> >  		free(server_control_group);
> >  		server_control_group = xstrdup(value);
> > +	} else if (!strcasecmp("cgroup_template", name)) {
> > +		free(server_cgroup_template);
> > +		server_cgroup_template = xstrdup(value);
> >  	} else {
> >  		bad_option_name(name, filename);
> >  	}
> > @@ -771,4 +775,5 @@ free_server_configuration(void)
> >  {
> >  	free(server_pidfile);
> >  	free(server_control_group);
> > +	free(server_cgroup_template);
> >  }
> > diff --git a/hasher-priv/priv.h b/hasher-priv/priv.h
> > index f0eb9f9..f29603a 100644
> > --- a/hasher-priv/priv.h
> > +++ b/hasher-priv/priv.h
> > @@ -120,6 +120,7 @@ int     do_chrootuid2(void);
> >  
> >  int process_caller_task(int, struct task *);
> >  pid_t fork_server(int, uid_t, gid_t, unsigned);
> > +int join_cgroup(void);
> >  
> >  extern const char *chroot_path;
> >  extern const char **chroot_argv;
> > @@ -162,6 +163,7 @@ extern work_limit_t wlimit;
> >  extern int server_log_priority;
> >  extern unsigned long server_session_timeout;
> >  extern char *server_control_group;
> > +extern char *server_cgroup_template;
> >  extern char *server_pidfile;
> >  extern gid_t server_gid;
> >  
> > diff --git a/hasher-priv/server.conf b/hasher-priv/server.conf
> > index 53ea5c3..9e70487 100644
> > --- a/hasher-priv/server.conf
> > +++ b/hasher-priv/server.conf
> > @@ -11,3 +11,12 @@ session_timeout=3600
> >  
> >  # Allow users of this group to interact with hasher-privd via the control socket.
> >  control_group=hashman
> > +
> > +# Template for cgroup path to which task handler should be added.
> > +#
> > +# %u -- Session's user name.
> > +# %U -- Session's user numeric ID.
> > +# %G -- Session's group numeric ID.
> > +# %N -- Session's user number.
> > +#
> > +#cgroup_template=/sys/fs/cgroup2/hasher-priv/%u/cgroup.procs
> > -- 
> > 2.24.0
> > 
> > _______________________________________________
> > Devel mailing list
> > Devel@lists.altlinux.org
> > https://lists.altlinux.org/mailman/listinfo/devel



-- 
Rgrds, legion


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [devel] [PATCH hasher-priv v1 3/3] Add cgroup support

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 3183 bytes --]

On Thu, Oct 01, 2020 at 09:17:33PM +0200, Alexey Gladkov wrote:
> On Thu, Sep 17, 2020 at 04:11:07PM +0300, Arseny Maslennikov wrote:
> > On Fri, Dec 13, 2019 at 12:42:05PM +0100, Alex Gladkov wrote:
> > > From: Alexey Gladkov <legion@altlinux.org>
> > > 
> > 
> > Could you please explain what you're trying to do with this patch?
> > Even if it's obvious from the source itself, we still must have an
> > opportunity to discuss, and a decent explanation should stay in the
> > project history.
> 
> I think this patch is simple enough.

There's a misunderstanding here. I'm not asking to explain the
semantics (what this patch does) — I repeat, it's rather obvious from
the source itself, the patch is indeed simple. I'm trying to get how the
patch's author would describe the pragmatic value of this patch. IOW:
we see this patch does XXX. What, in Alexey's view, are we trying to
achieve by implementing XXX?

Descriptive commit messages are done (and are enforced in successful
communities, e. g. LKML) for a reason.

The above essentially is my previous comment here, reworded and clarified.

If for some reason you believe it's shameful or rude to the community to
"waste time" on textual explanations, fair enough — I'll maybe write a commit
message myself (with my take on why this might be useful) and then most
likely ACK the same patch, with authorship reattributed to you via From:
in the patch body and the new commit message. Or else NAK this
particular revision with an empty commit message and leave it up to
ldv@.
If it were up to me, I would not approve of empty commit messages in a
lasting, crucial project like hasher-privd. People are forgetful, and
commit messages exist to help.

> 
> > Most likely, it'll turn out we _at least_ have to pass Delegate=yes to
> > the systemd service:
> > 
> >        Delegate=
> >            Turns on delegation of further resource control
> >            partitioning to processes of the unit. Units where
> >            this is enabled may create and manage their own
> >            private subhierarchy of control groups below the
> >            control group of the unit itself.
> > Manual page systemd.resource-control(5): lines 786-791
> 
> I'm pretty sure the hasher-priv shouldn't be tied to systemd.

I agree.

> I'm also
> convinced that the server will not be tied. The hasher-privd must be able
> to run on systems without systemd.

Sure it must. No one is trying to drop support for anything-but-systemd
from hasher-privd.

Here I was talking about the operational details of the daemon running
_under_ systemd, not without systemd. The insight about Delegate=yes
does not interfere with non-systemd installations.

> 
> > Do we only support cgroup2 and ignore cgroup1? If yes, great, but
> > perhaps then we might want to have a setting to not fiddle with cgroup
> > trees, to support the unfortunate users that have to run Docker and
> > other garbage.
> 
> Yeah, I didn't plan on supporting legacy version of cgroups. Docker
> already can work with cgroupsv2.

Oh, I heard they were just recently working on cgroup2 support.
Okay.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 3/3] Add cgroup support

[Alexey Gladkov]

[-- Attachment #1: Type: text/plain, Size: 2692 bytes --]

On Thu, Oct 01, 2020 at 11:23:53PM +0300, Arseny Maslennikov wrote:
> > > Could you please explain what you're trying to do with this patch?
> > > Even if it's obvious from the source itself, we still must have an
> > > opportunity to discuss, and a decent explanation should stay in the
> > > project history.
> > 
> > I think this patch is simple enough.
> 
> There's a misunderstanding here. I'm not asking to explain the
> semantics (what this patch does) — I repeat, it's rather obvious from
> the source itself, the patch is indeed simple. I'm trying to get how the
> patch's author would describe the pragmatic value of this patch. IOW:
> we see this patch does XXX. What, in Alexey's view, are we trying to
> achieve by implementing XXX?

I remember that this patch was the result of a discussion with ldv. I
didn't want to add complex support for different versions of cgroups. The
idea was that the admin would prepare the system for use of cgroups by the
hasher-privd daemon.

I'm not considering the hasher-privd as an end user server. This is a
low-level server on which you can build different solutions. I don't mean
just hasher. With this in mind, I don't think that this server should do
everything out of the box without configuration.

Does this make sense to you?

> Descriptive commit messages are done (and are enforced in successful
> communities, e. g. LKML) for a reason.
> 
> The above essentially is my previous comment here, reworded and clarified.
> 
> If for some reason you believe it's shameful or rude to the community to
> "waste time" on textual explanations, fair enough — I'll maybe write a commit
> message myself (with my take on why this might be useful) and then most
> likely ACK the same patch, with authorship reattributed to you via From:
> in the patch body and the new commit message. Or else NAK this
> particular revision with an empty commit message and leave it up to
> ldv@.
> If it were up to me, I would not approve of empty commit messages in a
> lasting, crucial project like hasher-privd. People are forgetful, and
> commit messages exist to help.

Ok.

> > > Do we only support cgroup2 and ignore cgroup1? If yes, great, but
> > > perhaps then we might want to have a setting to not fiddle with cgroup
> > > trees, to support the unfortunate users that have to run Docker and
> > > other garbage.
> > 
> > Yeah, I didn't plan on supporting legacy version of cgroups. Docker
> > already can work with cgroupsv2.
> 
> Oh, I heard they were just recently working on cgroup2 support.

https://github.com/opencontainers/runc/blob/master/docs/cgroup-v2.md

-- 
Rgrds, legion


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [devel] [PATCH hasher-priv v1 3/3] Add cgroup support

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 6153 bytes --]

On Fri, Oct 02, 2020 at 02:42:55AM +0200, Alexey Gladkov wrote:
> On Thu, Oct 01, 2020 at 11:23:53PM +0300, Arseny Maslennikov wrote:
> > > > Could you please explain what you're trying to do with this patch?
> > > > Even if it's obvious from the source itself, we still must have an
> > > > opportunity to discuss, and a decent explanation should stay in the
> > > > project history.
> > > 
> > > I think this patch is simple enough.
> > 
> > There's a misunderstanding here. I'm not asking to explain the
> > semantics (what this patch does) — I repeat, it's rather obvious from
> > the source itself, the patch is indeed simple. I'm trying to get how the
> > patch's author would describe the pragmatic value of this patch. IOW:
> > we see this patch does XXX. What, in Alexey's view, are we trying to
> > achieve by implementing XXX?
> 
> I remember that this patch was the result of a discussion with ldv.

That discussion then likely was not public; in part, that's why I'm asking.

> I didn't want to add complex support for different versions of cgroups.

I also believe that complexity would be unnecessary today.

> The
> idea was that the admin would prepare the system for use of cgroups by the
> hasher-privd daemon.

If I understood correctly ^U
To put it another way, we're doing this because the machine admin might
want hasher-privd to put the processes it spawns in cgroups
_at_an_arbitrary_path_, at the administrator's discretion.

Ok, this is a valid explanation and a valid feature. Thank you.

Might not be fully implementable on systemd-based installations,
though[1], but we'll look into it in time and work out something that
fits everyone's varying needs and circumstances. I'm not yet sure from
systemd's documentation if that program is OK with us making arbitrary
cgroup trees _in_the_root_cgroup_. But we definitely can get a cgroup
subtree to work in; this works.

[1] https://systemd.io/CGROUP_DELEGATION/

> 
> I'm not considering the hasher-privd as an end user server. This is a
> low-level server on which you can build different solutions. I don't mean
> just hasher.

Subject: the future of hasher-privd

I'm not particularly opposed to the expansion of hasher-privd's utility
scope; there are quite a lot of potential use cases: hasher-privd as a
general-purpose cgroup manager, hasher-privd as a daemon-based
NO_NEW_PRIVS-ready policy-enforcing "su -", ...

While this sounds interesting, I believe there are currently some
obstacles. Would those solutions on top of hasher-privd be
co-installable and co-existing on a single machine? E.g. two hasher-privd
init scripts with different configuration files for different things,
spawning different processes.
Or they wouldn't? Or a single hasher-privd instance — aka node, aka main
process if you will — would do both services? I don't yet have a picture
of this in my head; this will have to be thought out.

Will the decoupled, generic hasher-privd have to expand its IPC API?

If we decouple hasher-privd from hasher, this would also mean we support
arbitrary clients, so we'll have to formally define the IPC interface,
see my concerns on it in a previous mail.

As it stands now, the hasher project currently sees hasher-privd as its
vital component, a specialized tool for a special purpose, configured at
/etc/hasher-priv/. You're proposing something different.

In short, it's gonna be a long road.


> With this in mind, I don't think that this server should do
> everything out of the box without configuration.

I believe the hasher project _would_ want some sane out-of-the-box
configuration. The generic privd you describe above might not, much like
runc/crun do not, but the hasher project definitely would. Furthermore, in
my personal (but shared by many) opinion, this hasher OOTB experience
would have to be catered to the common case of an ALT Team developer,
not to public builder services (which are already expected to take care
to tune and harden their non-trivial configuration, and we can even ship
recommendations for their use case in /usr/share/doc).

The approach you suggest here could work, if e. g. the decoupled privd
is shipped with no defaults, and the hasher project ships its own
defaults for the desired operation of privd.

> 
> Does this make sense to you?

Barring the questions raised above, it does, thank you.
I'm just being thorough =) We still need to support every use case, and
not forget anything.

> 
> > Descriptive commit messages are done (and are enforced in successful
> > communities, e. g. LKML) for a reason.
> > 
> > The above essentially is my previous comment here, reworded and clarified.
> > 
> > If for some reason you believe it's shameful or rude to the community to
> > "waste time" on textual explanations, fair enough — I'll maybe write a commit
> > message myself (with my take on why this might be useful) and then most
> > likely ACK the same patch, with authorship reattributed to you via From:
> > in the patch body and the new commit message. Or else NAK this
> > particular revision with an empty commit message and leave it up to
> > ldv@.
> > If it were up to me, I would not approve of empty commit messages in a
> > lasting, crucial project like hasher-privd. People are forgetful, and
> > commit messages exist to help.
> 
> Ok.
> 
> > > > Do we only support cgroup2 and ignore cgroup1? If yes, great, but
> > > > perhaps then we might want to have a setting to not fiddle with cgroup
> > > > trees, to support the unfortunate users that have to run Docker and
> > > > other garbage.
> > > 
> > > Yeah, I didn't plan on supporting legacy version of cgroups. Docker
> > > already can work with cgroupsv2.
> > 
> > Oh, I heard they were just recently working on cgroup2 support.
> 
> https://github.com/opencontainers/runc/blob/master/docs/cgroup-v2.md
> 
> -- 
> Rgrds, legion
> 



> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 3/3] Add cgroup support

[Alexey Gladkov]

[-- Attachment #1: Type: text/plain, Size: 4690 bytes --]

On Fri, Oct 02, 2020 at 02:46:45PM +0300, Arseny Maslennikov wrote:
> > > There's a misunderstanding here. I'm not asking to explain the
> > > semantics (what this patch does) — I repeat, it's rather obvious from
> > > the source itself, the patch is indeed simple. I'm trying to get how the
> > > patch's author would describe the pragmatic value of this patch. IOW:
> > > we see this patch does XXX. What, in Alexey's view, are we trying to
> > > achieve by implementing XXX?
> > 
> > I remember that this patch was the result of a discussion with ldv.
> 
> That discussion then likely was not public; in part, that's why I'm asking.

Yeah. Sometimes it happens.

> > The
> > idea was that the admin would prepare the system for use of cgroups by the
> > hasher-privd daemon.
> 
> If I understood correctly ^U
> To put it another way, we're doing this because the machine admin might
> want hasher-privd to put the processes it spawns in cgroups
> _at_an_arbitrary_path_, at the administrator's discretion.
> 
> Ok, this is a valid explanation and a valid feature. Thank you.

Yes. This is an optional feature for the administrator.

> > I'm not considering the hasher-privd as an end user server. This is a
> > low-level server on which you can build different solutions. I don't mean
> > just hasher.
> 
> Subject: the future of hasher-privd
> 
> I'm not particularly opposed to the expansion of hasher-privd's utility
> scope; there are quite a lot of potential use cases: hasher-privd as a
> general-purpose cgroup manager, hasher-privd as a daemon-based
> NO_NEW_PRIVS-ready policy-enforcing "su -", ...

At the time when I made this patch and thought in the future to try to
make the hasher-privd more general-purpose.

I had thoughts to add support for seccomp via the libkafel library, extend
the use of namespaces (user, pid, time, etc).

Another thought was not directly related to hasher-privd. I was thinking
about trying to implement the creation of a chroot from docker images.

> While this sounds interesting, I believe there are currently some
> obstacles. Would those solutions on top of hasher-privd be
> co-installable and co-existing on a single machine? E.g. two hasher-privd
> init scripts with different configuration files for different things,
> spawning different processes.

The hasher-priv/hasher-privd has a global configuration. As long as
different solutions are able to use it together, they can coexist. But
this reuse of the server seems a little strange to me.

> Or they wouldn't? Or a single hasher-privd instance — aka node, aka main
> process if you will — would do both services? I don't yet have a picture
> of this in my head; this will have to be thought out.
> 
> Will the decoupled, generic hasher-privd have to expand its IPC API?

I didn't expect the API to be public. I mean it will be used by someone
other than the hasher-priv. I didn't think that far.

I propose to postpone this question. We don't even have a server yet.

> If we decouple hasher-privd from hasher, this would also mean we support
> arbitrary clients, so we'll have to formally define the IPC interface,
> see my concerns on it in a previous mail.

Yep. When this happens, we will need to make a thoughtful public API.

> As it stands now, the hasher project currently sees hasher-privd as its
> vital component, a specialized tool for a special purpose, configured at
> /etc/hasher-priv/. You're proposing something different.
> 
> In short, it's gonna be a long road.

True.
 
> > With this in mind, I don't think that this server should do
> > everything out of the box without configuration.
> 
> I believe the hasher project _would_ want some sane out-of-the-box
> configuration. The generic privd you describe above might not, much like
> runc/crun do not, but the hasher project definitely would. Furthermore, in
> my personal (but shared by many) opinion, this hasher OOTB experience
> would have to be catered to the common case of an ALT Team developer,
> not to public builder services (which are already expected to take care
> to tune and harden their non-trivial configuration, and we can even ship
> recommendations for their use case in /usr/share/doc).

I agree with you but let's not do it all at once. I have not been able to
upstream the basic server implementation. I'm afraid more global changes
will be accepted even slower.

> The approach you suggest here could work, if e. g. the decoupled privd
> is shipped with no defaults, and the hasher project ships its own
> defaults for the desired operation of privd.

-- 
Rgrds, legion


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Alexey Tourbin]

On Fri, Dec 13, 2019 at 2:42 PM Alex Gladkov <legion@altlinux.ru> wrote:
> The hasher-priv is a SUID utility. This is not good. Separation of the
> server and client parts will allow us to remove SUID flag.

Removing the SUID flag shouldn't be an end in itself. You're still
running a process with root privileges which serves user requests.
It's the same, except that instead of the SUID flag, the process just
starts as root.  So you are not improving privilege separation or
something, you are only limiting the ability of the user to tamper
with the SUID binary. And tampering with the binary should be
pointless anyway (unless glibc is faulty and permits arbitrary code
injection, etc.).

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Andrey Savchenko]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

On Sun, 15 Dec 2019 11:50:13 +0300 Alexey Tourbin wrote:
> On Fri, Dec 13, 2019 at 2:42 PM Alex Gladkov <legion@altlinux.ru> wrote:
> > The hasher-priv is a SUID utility. This is not good. Separation of the
> > server and client parts will allow us to remove SUID flag.
> 
> Removing the SUID flag shouldn't be an end in itself. You're still
> running a process with root privileges which serves user requests.
> It's the same, except that instead of the SUID flag, the process just
> starts as root.  So you are not improving privilege separation or
> something, you are only limiting the ability of the user to tamper
> with the SUID binary. And tampering with the binary should be
> pointless anyway (unless glibc is faulty and permits arbitrary code
> injection, etc.).

The code separation for the privileged and the unprivileged
processes allows to reduce the attack surface when implemented
properly. Furthermore it should be possible to replace the SUID by
the Linux capabilities in future — so the code/process separation
makes even more sense here as it will lead to a smaller number of
capabilities required.

I have not reviewed this code yet, but I like the idea.

Best regards,
Andrew Savchenko

[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Sun, Dec 15, 2019 at 11:50:13AM +0300, Alexey Tourbin wrote:
> On Fri, Dec 13, 2019 at 2:42 PM Alex Gladkov <legion@altlinux.ru> wrote:
> > The hasher-priv is a SUID utility. This is not good. Separation of the
> > server and client parts will allow us to remove SUID flag.
> 
> Removing the SUID flag shouldn't be an end in itself. You're still
> running a process with root privileges which serves user requests.
> It's the same, except that instead of the SUID flag, the process just
> starts as root.  So you are not improving privilege separation or
> something, you are only limiting the ability of the user to tamper
> with the SUID binary. And tampering with the binary should be
> pointless anyway (unless glibc is faulty and permits arbitrary code
> injection, etc.).

While turning a suid root executable into a daemon doesn't automagically
make everything more secure, it's an important move in the right direction.

Firstly, the attack surface of a suid root executable is larger than
of the equivalent root daemon on the other side of a unix domain socket,
so this change narrows the attack surface.

Secondly, this change opens the way for more elaborate privilege separation.

Thirdly, it makes hasher available for PR_SET_NO_NEW_PRIVS'ed
processes (e.g. self-seccomp'ed) that cannot make use of suid executables.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Alexey Tourbin]

On Mon, Dec 16, 2019 at 12:35 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Sun, Dec 15, 2019 at 11:50:13AM +0300, Alexey Tourbin wrote:
> > On Fri, Dec 13, 2019 at 2:42 PM Alex Gladkov <legion@altlinux.ru> wrote:
> > > The hasher-priv is a SUID utility. This is not good. Separation of the
> > > server and client parts will allow us to remove SUID flag.
> >
> > Removing the SUID flag shouldn't be an end in itself. You're still
> > running a process with root privileges which serves user requests.
> > It's the same, except that instead of the SUID flag, the process just
> > starts as root.  So you are not improving privilege separation or
> > something, you are only limiting the ability of the user to tamper
> > with the SUID binary. And tampering with the binary should be
> > pointless anyway (unless glibc is faulty and permits arbitrary code
> > injection, etc.).
>
> While turning a suid root executable into a daemon doesn't automagically
> make everything more secure, it's an important move in the right direction.

Not necessarily. Conversion into a daemon takes more code, which can
have its own faults. Instead of relying on the set-uid mechanism,
you're very likely to up end up with a more complex DIY construction.

> Firstly, the attack surface of a suid root executable is larger than
> of the equivalent root daemon on the other side of a unix domain socket,
> so this change narrows the attack surface.

You are casting doubt on the venerable set-uid mechanism. What if it's
faulty? What if the user can tamper with the binary and somehow inject
arbitrary code? Well, you can do nothing about it, and moreover it's
not your problem. (Likewise, if the kernel is faulty and permits
privilege escalation, you can do nothing about it, and the only way
round is to fix the kernel.) Your basic mechanisms must be secure, and
it's doable. The "attack surface" is just a highbrow way of saying
that the dynamic loader should be insensitive to LD_PRELOAD. :)

> Secondly, this change opens the way for more elaborate privilege separation.
>
> Thirdly, it makes hasher available for PR_SET_NO_NEW_PRIVS'ed
> processes (e.g. self-seccomp'ed) that cannot make use of suid executables.

These might be valid arguments. Still, I find it hard to believe it's
really about security. hasher-priv is minimalistic, and its use is
limited to those few machines that need it, some of them booted over
the network. There is no good reason to believe that we might face any
security risks.

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Alexey Gladkov]

On Fri, Dec 13, 2019 at 12:42:02PM +0100, Alex Gladkov wrote:
> From: Alexey Gladkov <legion@altlinux.org>
> 
> The hasher-priv is a SUID utility. This is not good. Separation of the
> server and client parts will allow us to remove SUID flag.
> 
> The separation of server and client is not intended to give clients
> access over the network. This separation is only necessary to distinguish
> privileges. Only UNIX domain socket is used.
> 
> A separate session process is created for each connected user. Each such
> process ends after a certain period of inactivity.

Gently remind about patches.

> Alexey Gladkov (3):
>   Make a daemon from the hasher-priv
>   Add systemd and sysvinit service files
>   Add cgroup support
> 
>  hasher-priv/.gitignore            |   1 +
>  hasher-priv/DESIGN                | 281 +++++++++++++--------
>  hasher-priv/Makefile              |  34 ++-
>  hasher-priv/caller.c              |  81 +++---
>  hasher-priv/caller_server.c       | 373 ++++++++++++++++++++++++++++
>  hasher-priv/caller_task.c         | 217 +++++++++++++++++
>  hasher-priv/cgroup.c              | 119 +++++++++
>  hasher-priv/cmdline.c             |  27 +-
>  hasher-priv/communication.c       | 392 ++++++++++++++++++++++++++++++
>  hasher-priv/communication.h       |  77 ++++++
>  hasher-priv/config.c              | 148 ++++++++++-
>  hasher-priv/epoll.c               |  39 +++
>  hasher-priv/epoll.h               |  18 ++
>  hasher-priv/hasher-priv.c         |  78 ++++++
>  hasher-priv/hasher-privd.c        | 375 ++++++++++++++++++++++++++++
>  hasher-priv/hasher-privd.service  |  11 +
>  hasher-priv/hasher-privd.sysvinit |  86 +++++++
>  hasher-priv/io_log.c              |   2 +-
>  hasher-priv/io_x11.c              |   2 +-
>  hasher-priv/killuid.c             |   2 +-
>  hasher-priv/logging.c             |  64 +++++
>  hasher-priv/logging.h             |  55 +++++
>  hasher-priv/main.c                |  75 ------
>  hasher-priv/pass.c                | 117 ++++++++-
>  hasher-priv/pidfile.c             | 128 ++++++++++
>  hasher-priv/pidfile.h             |  44 ++++
>  hasher-priv/priv.h                |  35 ++-
>  hasher-priv/server.conf           |  22 ++
>  hasher-priv/sockets.c             | 183 ++++++++++++++
>  hasher-priv/sockets.h             |  32 +++
>  hasher-priv/x11.c                 |   1 +
>  31 files changed, 2872 insertions(+), 247 deletions(-)
>  create mode 100644 hasher-priv/caller_server.c
>  create mode 100644 hasher-priv/caller_task.c
>  create mode 100644 hasher-priv/cgroup.c
>  create mode 100644 hasher-priv/communication.c
>  create mode 100644 hasher-priv/communication.h
>  create mode 100644 hasher-priv/epoll.c
>  create mode 100644 hasher-priv/epoll.h
>  create mode 100644 hasher-priv/hasher-priv.c
>  create mode 100644 hasher-priv/hasher-privd.c
>  create mode 100644 hasher-priv/hasher-privd.service
>  create mode 100755 hasher-priv/hasher-privd.sysvinit
>  create mode 100644 hasher-priv/logging.c
>  create mode 100644 hasher-priv/logging.h
>  delete mode 100644 hasher-priv/main.c
>  create mode 100644 hasher-priv/pidfile.c
>  create mode 100644 hasher-priv/pidfile.h
>  create mode 100644 hasher-priv/server.conf
>  create mode 100644 hasher-priv/sockets.c
>  create mode 100644 hasher-priv/sockets.h
> 
> -- 
> 2.24.0

-- 
Rgrds, legion

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Alexey Gladkov]

On Fri, Dec 13, 2019 at 12:42:02PM +0100, Alex Gladkov wrote:
> From: Alexey Gladkov <legion@altlinux.org>

ping

> The hasher-priv is a SUID utility. This is not good. Separation of the
> server and client parts will allow us to remove SUID flag.
> 
> The separation of server and client is not intended to give clients
> access over the network. This separation is only necessary to distinguish
> privileges. Only UNIX domain socket is used.
> 
> A separate session process is created for each connected user. Each such
> process ends after a certain period of inactivity.
> 
> Alexey Gladkov (3):
>   Make a daemon from the hasher-priv
>   Add systemd and sysvinit service files
>   Add cgroup support
> 
>  hasher-priv/.gitignore            |   1 +
>  hasher-priv/DESIGN                | 281 +++++++++++++--------
>  hasher-priv/Makefile              |  34 ++-
>  hasher-priv/caller.c              |  81 +++---
>  hasher-priv/caller_server.c       | 373 ++++++++++++++++++++++++++++
>  hasher-priv/caller_task.c         | 217 +++++++++++++++++
>  hasher-priv/cgroup.c              | 119 +++++++++
>  hasher-priv/cmdline.c             |  27 +-
>  hasher-priv/communication.c       | 392 ++++++++++++++++++++++++++++++
>  hasher-priv/communication.h       |  77 ++++++
>  hasher-priv/config.c              | 148 ++++++++++-
>  hasher-priv/epoll.c               |  39 +++
>  hasher-priv/epoll.h               |  18 ++
>  hasher-priv/hasher-priv.c         |  78 ++++++
>  hasher-priv/hasher-privd.c        | 375 ++++++++++++++++++++++++++++
>  hasher-priv/hasher-privd.service  |  11 +
>  hasher-priv/hasher-privd.sysvinit |  86 +++++++
>  hasher-priv/io_log.c              |   2 +-
>  hasher-priv/io_x11.c              |   2 +-
>  hasher-priv/killuid.c             |   2 +-
>  hasher-priv/logging.c             |  64 +++++
>  hasher-priv/logging.h             |  55 +++++
>  hasher-priv/main.c                |  75 ------
>  hasher-priv/pass.c                | 117 ++++++++-
>  hasher-priv/pidfile.c             | 128 ++++++++++
>  hasher-priv/pidfile.h             |  44 ++++
>  hasher-priv/priv.h                |  35 ++-
>  hasher-priv/server.conf           |  22 ++
>  hasher-priv/sockets.c             | 183 ++++++++++++++
>  hasher-priv/sockets.h             |  32 +++
>  hasher-priv/x11.c                 |   1 +
>  31 files changed, 2872 insertions(+), 247 deletions(-)
>  create mode 100644 hasher-priv/caller_server.c
>  create mode 100644 hasher-priv/caller_task.c
>  create mode 100644 hasher-priv/cgroup.c
>  create mode 100644 hasher-priv/communication.c
>  create mode 100644 hasher-priv/communication.h
>  create mode 100644 hasher-priv/epoll.c
>  create mode 100644 hasher-priv/epoll.h
>  create mode 100644 hasher-priv/hasher-priv.c
>  create mode 100644 hasher-priv/hasher-privd.c
>  create mode 100644 hasher-priv/hasher-privd.service
>  create mode 100755 hasher-priv/hasher-privd.sysvinit
>  create mode 100644 hasher-priv/logging.c
>  create mode 100644 hasher-priv/logging.h
>  delete mode 100644 hasher-priv/main.c
>  create mode 100644 hasher-priv/pidfile.c
>  create mode 100644 hasher-priv/pidfile.h
>  create mode 100644 hasher-priv/server.conf
>  create mode 100644 hasher-priv/sockets.c
>  create mode 100644 hasher-priv/sockets.h
> 
> -- 
> 2.24.0
> 
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel

-- 
Rgrds, legion

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 4527 bytes --]

On Fri, Dec 13, 2019 at 12:42:02PM +0100, Alex Gladkov wrote:
> From: Alexey Gladkov <legion@altlinux.org>
> 
> The hasher-priv is a SUID utility. This is not good. Separation of the
> server and client parts will allow us to remove SUID flag.
> 
> The separation of server and client is not intended to give clients
> access over the network. This separation is only necessary to distinguish
> privileges. Only UNIX domain socket is used.
> 
> A separate session process is created for each connected user. Each such
> process ends after a certain period of inactivity.

Thank you for trying this idea out; despite the trolling attempts, this
effort is long welcome.

There are some issues with the patchset, which I intend to cover in
subsequent emails. I have published[1] some fix-up commits on top of
these patches in an attempt to ensure that, barring the issues with a
known fix, this works; however, some bugs are definitely still unsolved
by now, so I decided to discuss the more apparent points first.

[1] http://git.altlinux.org/people/arseny/packages/hasher-priv.git?a=summary

There's an issue when hasher-privd tries to fulfill a chrootuid{1,2}
request: the (eventually) unprivileged task executor process
successfully invokes waitpid() or the likes on a child process,
select()s on I/O descriptors, but gets CHLD later — and it looks like
the inherited signal handler causes it to wait again.
I've not yet found a decent reproducer — the following command:
`hsh-shell $workdir'
reproduces the issue reliably for me, but hsh-mkchroot, hsh-rmchroot,
hsh-install are all OK. The root cause nevertheless is not yet
established. It looks like this has to be patched somewhere in
chrootuid(), but I might be wrong on this one.

> 
> Alexey Gladkov (3):
>   Make a daemon from the hasher-priv
>   Add systemd and sysvinit service files
>   Add cgroup support
> 
>  hasher-priv/.gitignore            |   1 +
>  hasher-priv/DESIGN                | 281 +++++++++++++--------
>  hasher-priv/Makefile              |  34 ++-
>  hasher-priv/caller.c              |  81 +++---
>  hasher-priv/caller_server.c       | 373 ++++++++++++++++++++++++++++
>  hasher-priv/caller_task.c         | 217 +++++++++++++++++
>  hasher-priv/cgroup.c              | 119 +++++++++
>  hasher-priv/cmdline.c             |  27 +-
>  hasher-priv/communication.c       | 392 ++++++++++++++++++++++++++++++
>  hasher-priv/communication.h       |  77 ++++++
>  hasher-priv/config.c              | 148 ++++++++++-
>  hasher-priv/epoll.c               |  39 +++
>  hasher-priv/epoll.h               |  18 ++
>  hasher-priv/hasher-priv.c         |  78 ++++++
>  hasher-priv/hasher-privd.c        | 375 ++++++++++++++++++++++++++++
>  hasher-priv/hasher-privd.service  |  11 +
>  hasher-priv/hasher-privd.sysvinit |  86 +++++++
>  hasher-priv/io_log.c              |   2 +-
>  hasher-priv/io_x11.c              |   2 +-
>  hasher-priv/killuid.c             |   2 +-
>  hasher-priv/logging.c             |  64 +++++
>  hasher-priv/logging.h             |  55 +++++
>  hasher-priv/main.c                |  75 ------
>  hasher-priv/pass.c                | 117 ++++++++-
>  hasher-priv/pidfile.c             | 128 ++++++++++
>  hasher-priv/pidfile.h             |  44 ++++
>  hasher-priv/priv.h                |  35 ++-
>  hasher-priv/server.conf           |  22 ++
>  hasher-priv/sockets.c             | 183 ++++++++++++++
>  hasher-priv/sockets.h             |  32 +++
>  hasher-priv/x11.c                 |   1 +
>  31 files changed, 2872 insertions(+), 247 deletions(-)
>  create mode 100644 hasher-priv/caller_server.c
>  create mode 100644 hasher-priv/caller_task.c
>  create mode 100644 hasher-priv/cgroup.c
>  create mode 100644 hasher-priv/communication.c
>  create mode 100644 hasher-priv/communication.h
>  create mode 100644 hasher-priv/epoll.c
>  create mode 100644 hasher-priv/epoll.h
>  create mode 100644 hasher-priv/hasher-priv.c
>  create mode 100644 hasher-priv/hasher-privd.c
>  create mode 100644 hasher-priv/hasher-privd.service
>  create mode 100755 hasher-priv/hasher-privd.sysvinit
>  create mode 100644 hasher-priv/logging.c
>  create mode 100644 hasher-priv/logging.h
>  delete mode 100644 hasher-priv/main.c
>  create mode 100644 hasher-priv/pidfile.c
>  create mode 100644 hasher-priv/pidfile.h
>  create mode 100644 hasher-priv/server.conf
>  create mode 100644 hasher-priv/sockets.c
>  create mode 100644 hasher-priv/sockets.h
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Alexey Gladkov]

[-- Attachment #1: Type: text/plain, Size: 5096 bytes --]

On Thu, Sep 17, 2020 at 04:09:35PM +0300, Arseny Maslennikov wrote:
> On Fri, Dec 13, 2019 at 12:42:02PM +0100, Alex Gladkov wrote:
> > From: Alexey Gladkov <legion@altlinux.org>
> > 
> > The hasher-priv is a SUID utility. This is not good. Separation of the
> > server and client parts will allow us to remove SUID flag.
> > 
> > The separation of server and client is not intended to give clients
> > access over the network. This separation is only necessary to distinguish
> > privileges. Only UNIX domain socket is used.
> > 
> > A separate session process is created for each connected user. Each such
> > process ends after a certain period of inactivity.
> 
> Thank you for trying this idea out; despite the trolling attempts, this
> effort is long welcome.

I created this patchset a long time ago. I've already lost my context. It
might be better if you keep working on this patch.

> There are some issues with the patchset, which I intend to cover in
> subsequent emails. I have published[1] some fix-up commits on top of
> these patches in an attempt to ensure that, barring the issues with a
> known fix, this works; however, some bugs are definitely still unsolved
> by now, so I decided to discuss the more apparent points first.
> 
> [1] http://git.altlinux.org/people/arseny/packages/hasher-priv.git?a=summary

It looks like you've already started working on finalizing this patch :)

> There's an issue when hasher-privd tries to fulfill a chrootuid{1,2}
> request: the (eventually) unprivileged task executor process
> successfully invokes waitpid() or the likes on a child process,
> select()s on I/O descriptors, but gets CHLD later — and it looks like
> the inherited signal handler causes it to wait again.

Hm...

> I've not yet found a decent reproducer — the following command:
> `hsh-shell $workdir'

There is no such command. You need to send command to run /bin/sh.

> reproduces the issue reliably for me, but hsh-mkchroot, hsh-rmchroot,
> hsh-install are all OK. The root cause nevertheless is not yet
> established. It looks like this has to be patched somewhere in
> chrootuid(), but I might be wrong on this one.
> 
> > 
> > Alexey Gladkov (3):
> >   Make a daemon from the hasher-priv
> >   Add systemd and sysvinit service files
> >   Add cgroup support
> > 
> >  hasher-priv/.gitignore            |   1 +
> >  hasher-priv/DESIGN                | 281 +++++++++++++--------
> >  hasher-priv/Makefile              |  34 ++-
> >  hasher-priv/caller.c              |  81 +++---
> >  hasher-priv/caller_server.c       | 373 ++++++++++++++++++++++++++++
> >  hasher-priv/caller_task.c         | 217 +++++++++++++++++
> >  hasher-priv/cgroup.c              | 119 +++++++++
> >  hasher-priv/cmdline.c             |  27 +-
> >  hasher-priv/communication.c       | 392 ++++++++++++++++++++++++++++++
> >  hasher-priv/communication.h       |  77 ++++++
> >  hasher-priv/config.c              | 148 ++++++++++-
> >  hasher-priv/epoll.c               |  39 +++
> >  hasher-priv/epoll.h               |  18 ++
> >  hasher-priv/hasher-priv.c         |  78 ++++++
> >  hasher-priv/hasher-privd.c        | 375 ++++++++++++++++++++++++++++
> >  hasher-priv/hasher-privd.service  |  11 +
> >  hasher-priv/hasher-privd.sysvinit |  86 +++++++
> >  hasher-priv/io_log.c              |   2 +-
> >  hasher-priv/io_x11.c              |   2 +-
> >  hasher-priv/killuid.c             |   2 +-
> >  hasher-priv/logging.c             |  64 +++++
> >  hasher-priv/logging.h             |  55 +++++
> >  hasher-priv/main.c                |  75 ------
> >  hasher-priv/pass.c                | 117 ++++++++-
> >  hasher-priv/pidfile.c             | 128 ++++++++++
> >  hasher-priv/pidfile.h             |  44 ++++
> >  hasher-priv/priv.h                |  35 ++-
> >  hasher-priv/server.conf           |  22 ++
> >  hasher-priv/sockets.c             | 183 ++++++++++++++
> >  hasher-priv/sockets.h             |  32 +++
> >  hasher-priv/x11.c                 |   1 +
> >  31 files changed, 2872 insertions(+), 247 deletions(-)
> >  create mode 100644 hasher-priv/caller_server.c
> >  create mode 100644 hasher-priv/caller_task.c
> >  create mode 100644 hasher-priv/cgroup.c
> >  create mode 100644 hasher-priv/communication.c
> >  create mode 100644 hasher-priv/communication.h
> >  create mode 100644 hasher-priv/epoll.c
> >  create mode 100644 hasher-priv/epoll.h
> >  create mode 100644 hasher-priv/hasher-priv.c
> >  create mode 100644 hasher-priv/hasher-privd.c
> >  create mode 100644 hasher-priv/hasher-privd.service
> >  create mode 100755 hasher-priv/hasher-privd.sysvinit
> >  create mode 100644 hasher-priv/logging.c
> >  create mode 100644 hasher-priv/logging.h
> >  delete mode 100644 hasher-priv/main.c
> >  create mode 100644 hasher-priv/pidfile.c
> >  create mode 100644 hasher-priv/pidfile.h
> >  create mode 100644 hasher-priv/server.conf
> >  create mode 100644 hasher-priv/sockets.c
> >  create mode 100644 hasher-priv/sockets.h
> > 



-- 
Rgrds, legion


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]

On Thu, Oct 01, 2020 at 07:21:11PM +0200, Alexey Gladkov wrote:
> On Thu, Sep 17, 2020 at 04:09:35PM +0300, Arseny Maslennikov wrote:
> > On Fri, Dec 13, 2019 at 12:42:02PM +0100, Alex Gladkov wrote:
> > > From: Alexey Gladkov <legion@altlinux.org>
> > > 
> > > The hasher-priv is a SUID utility. This is not good. Separation of the
> > > server and client parts will allow us to remove SUID flag.
> > > 
> > > The separation of server and client is not intended to give clients
> > > access over the network. This separation is only necessary to distinguish
> > > privileges. Only UNIX domain socket is used.
> > > 
> > > A separate session process is created for each connected user. Each such
> > > process ends after a certain period of inactivity.
> > 
> > Thank you for trying this idea out; despite the trolling attempts, this
> > effort is long welcome.
> 
> I created this patchset a long time ago. I've already lost my context. It
> might be better if you keep working on this patch.
> 

Great! I'd like to work on this further.

> > There are some issues with the patchset, which I intend to cover in
> > subsequent emails. I have published[1] some fix-up commits on top of
> > these patches in an attempt to ensure that, barring the issues with a
> > known fix, this works; however, some bugs are definitely still unsolved
> > by now, so I decided to discuss the more apparent points first.
> > 
> > [1] http://git.altlinux.org/people/arseny/packages/hasher-priv.git?a=summary
> 
> It looks like you've already started working on finalizing this patch :)
> 
> > There's an issue when hasher-privd tries to fulfill a chrootuid{1,2}
> > request: the (eventually) unprivileged task executor process
> > successfully invokes waitpid() or the likes on a child process,
> > select()s on I/O descriptors, but gets CHLD later — and it looks like
> > the inherited signal handler causes it to wait again.
> 
> Hm...
> 
> > I've not yet found a decent reproducer — the following command:
> > `hsh-shell $workdir'
> 
> There is no such command. You need to send command to run /bin/sh.

Yes, there's no such IPC command, I was referring to a shell command run
in the host system by the caller user.

> 
> > reproduces the issue reliably for me, but hsh-mkchroot, hsh-rmchroot,
> > hsh-install are all OK. The root cause nevertheless is not yet
> > established. It looks like this has to be patched somewhere in
> > chrootuid(), but I might be wrong on this one.
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Alexey Gladkov]

[-- Attachment #1: Type: text/plain, Size: 3249 bytes --]

On Thu, Oct 01, 2020 at 08:44:00PM +0300, Arseny Maslennikov wrote:
> On Thu, Oct 01, 2020 at 07:21:11PM +0200, Alexey Gladkov wrote:
> > On Thu, Sep 17, 2020 at 04:09:35PM +0300, Arseny Maslennikov wrote:
> > > On Fri, Dec 13, 2019 at 12:42:02PM +0100, Alex Gladkov wrote:
> > > > From: Alexey Gladkov <legion@altlinux.org>
> > > > 
> > > > The hasher-priv is a SUID utility. This is not good. Separation of the
> > > > server and client parts will allow us to remove SUID flag.
> > > > 
> > > > The separation of server and client is not intended to give clients
> > > > access over the network. This separation is only necessary to distinguish
> > > > privileges. Only UNIX domain socket is used.
> > > > 
> > > > A separate session process is created for each connected user. Each such
> > > > process ends after a certain period of inactivity.
> > > 
> > > Thank you for trying this idea out; despite the trolling attempts, this
> > > effort is long welcome.
> > 
> > I created this patchset a long time ago. I've already lost my context. It
> > might be better if you keep working on this patch.
> > 
> 
> Great! I'd like to work on this further.

You have asked many questions. I didn’t answer everything because these
patches are already 5 years old and I can hardly remember what I had in my
head when I did them. Submitting patches to the mailing list was the
second attempt to upstream them. Actually, I was afraid of losing them
altogether, so I merged some of the patches. Originally I had about 10
patches in a patchset.

I'm not sure if I have time for this rework. But we can try. We can
discuss the hasher-privd in russian if you like :)

> > > There are some issues with the patchset, which I intend to cover in
> > > subsequent emails. I have published[1] some fix-up commits on top of
> > > these patches in an attempt to ensure that, barring the issues with a
> > > known fix, this works; however, some bugs are definitely still unsolved
> > > by now, so I decided to discuss the more apparent points first.
> > > 
> > > [1] http://git.altlinux.org/people/arseny/packages/hasher-priv.git?a=summary
> > 
> > It looks like you've already started working on finalizing this patch :)
> > 
> > > There's an issue when hasher-privd tries to fulfill a chrootuid{1,2}
> > > request: the (eventually) unprivileged task executor process
> > > successfully invokes waitpid() or the likes on a child process,
> > > select()s on I/O descriptors, but gets CHLD later — and it looks like
> > > the inherited signal handler causes it to wait again.
> > 
> > Hm...
> > 
> > > I've not yet found a decent reproducer — the following command:
> > > `hsh-shell $workdir'
> > 
> > There is no such command. You need to send command to run /bin/sh.
> 
> Yes, there's no such IPC command, I was referring to a shell command run
> in the host system by the caller user.
> 
> > 
> > > reproduces the issue reliably for me, but hsh-mkchroot, hsh-rmchroot,
> > > hsh-install are all OK. The root cause nevertheless is not yet
> > > established. It looks like this has to be patched somewhere in
> > > chrootuid(), but I might be wrong on this one.
> > > 



-- 
Rgrds, legion


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Arseny Maslennikov]

[-- Attachment #1: Type: text/plain, Size: 4901 bytes --]

On Thu, Oct 01, 2020 at 10:01:29PM +0200, Alexey Gladkov wrote:
> On Thu, Oct 01, 2020 at 08:44:00PM +0300, Arseny Maslennikov wrote:
> > On Thu, Oct 01, 2020 at 07:21:11PM +0200, Alexey Gladkov wrote:
> > > On Thu, Sep 17, 2020 at 04:09:35PM +0300, Arseny Maslennikov wrote:
> > > > On Fri, Dec 13, 2019 at 12:42:02PM +0100, Alex Gladkov wrote:
> > > > > From: Alexey Gladkov <legion@altlinux.org>
> > > > > 
> > > > > The hasher-priv is a SUID utility. This is not good. Separation of the
> > > > > server and client parts will allow us to remove SUID flag.
> > > > > 
> > > > > The separation of server and client is not intended to give clients
> > > > > access over the network. This separation is only necessary to distinguish
> > > > > privileges. Only UNIX domain socket is used.
> > > > > 
> > > > > A separate session process is created for each connected user. Each such
> > > > > process ends after a certain period of inactivity.
> > > > 
> > > > Thank you for trying this idea out; despite the trolling attempts, this
> > > > effort is long welcome.
> > > 
> > > I created this patchset a long time ago. I've already lost my context. It
> > > might be better if you keep working on this patch.
> > > 
> > 
> > Great! I'd like to work on this further.
> 
> You have asked many questions. I didn’t answer everything because these
> patches are already 5 years old and I can hardly remember what I had in my
> head when I did them. Submitting patches to the mailing list was the
> second attempt to upstream them. Actually, I was afraid of losing them
> altogether, so I merged some of the patches. Originally I had about 10
> patches in a patchset.
> 
> I'm not sure if I have time for this rework. But we can try.

So, I guess you won't mind if I would prepare a v2 which fixes some of
the issues discussed, based on my repo. We're in no hurry, since Dmitry
is currently away for the next couple of weeks.

> We can
> discuss the hasher-privd in russian if you like :)

I'm personally fine with both english and russian; looks like you're too.
The remaining concerns are:
* if everyone else interested can respond and continue the conversation
* if the community around hasher ever goes international.

I responded in english, since the patch messages were in english, and in
that case I usually take the (nowadays rare with covid) opportunity to
practice. Если же то, на что я отвечаю, пишут по-русски, то и отвечать,
наверное, следует тоже по-русски.

Если вдруг чувствуете, что лучше по-русски, можете на русский переключаться.

Ну и иногда пишешь что-то по-русски в некоторый
профессионально-технический разговор, а в реплике столько оказывается
непереводных терминов и собственных имён, что уж лучше по-английски бы писал. :)

> 
> > > > There are some issues with the patchset, which I intend to cover in
> > > > subsequent emails. I have published[1] some fix-up commits on top of
> > > > these patches in an attempt to ensure that, barring the issues with a
> > > > known fix, this works; however, some bugs are definitely still unsolved
> > > > by now, so I decided to discuss the more apparent points first.
> > > > 
> > > > [1] http://git.altlinux.org/people/arseny/packages/hasher-priv.git?a=summary
> > > 
> > > It looks like you've already started working on finalizing this patch :)
> > > 
> > > > There's an issue when hasher-privd tries to fulfill a chrootuid{1,2}
> > > > request: the (eventually) unprivileged task executor process
> > > > successfully invokes waitpid() or the likes on a child process,
> > > > select()s on I/O descriptors, but gets CHLD later — and it looks like
> > > > the inherited signal handler causes it to wait again.
> > > 
> > > Hm...
> > > 
> > > > I've not yet found a decent reproducer — the following command:
> > > > `hsh-shell $workdir'
> > > 
> > > There is no such command. You need to send command to run /bin/sh.
> > 
> > Yes, there's no such IPC command, I was referring to a shell command run
> > in the host system by the caller user.
> > 
> > > 
> > > > reproduces the issue reliably for me, but hsh-mkchroot, hsh-rmchroot,
> > > > hsh-install are all OK. The root cause nevertheless is not yet
> > > > established. It looks like this has to be patched somewhere in
> > > > chrootuid(), but I might be wrong on this one.
> > > > 
> 
> 
> 
> -- 
> Rgrds, legion
> 



> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [devel] [PATCH hasher-priv v1 0/3] Make a daemon from the hasher-priv

[Alexey Gladkov]

[-- Attachment #1: Type: text/plain, Size: 3954 bytes --]

On Fri, Oct 02, 2020 at 12:53:45AM +0300, Arseny Maslennikov wrote:
> On Thu, Oct 01, 2020 at 10:01:29PM +0200, Alexey Gladkov wrote:
> > On Thu, Oct 01, 2020 at 08:44:00PM +0300, Arseny Maslennikov wrote:
> > > On Thu, Oct 01, 2020 at 07:21:11PM +0200, Alexey Gladkov wrote:
> > > > On Thu, Sep 17, 2020 at 04:09:35PM +0300, Arseny Maslennikov wrote:
> > > > > On Fri, Dec 13, 2019 at 12:42:02PM +0100, Alex Gladkov wrote:
> > > > > > From: Alexey Gladkov <legion@altlinux.org>
> > > > > > 
> > > > > > The hasher-priv is a SUID utility. This is not good. Separation of the
> > > > > > server and client parts will allow us to remove SUID flag.
> > > > > > 
> > > > > > The separation of server and client is not intended to give clients
> > > > > > access over the network. This separation is only necessary to distinguish
> > > > > > privileges. Only UNIX domain socket is used.
> > > > > > 
> > > > > > A separate session process is created for each connected user. Each such
> > > > > > process ends after a certain period of inactivity.
> > > > > 
> > > > > Thank you for trying this idea out; despite the trolling attempts, this
> > > > > effort is long welcome.
> > > > 
> > > > I created this patchset a long time ago. I've already lost my context. It
> > > > might be better if you keep working on this patch.
> > > > 
> > > 
> > > Great! I'd like to work on this further.
> > 
> > You have asked many questions. I didn’t answer everything because these
> > patches are already 5 years old and I can hardly remember what I had in my
> > head when I did them. Submitting patches to the mailing list was the
> > second attempt to upstream them. Actually, I was afraid of losing them
> > altogether, so I merged some of the patches. Originally I had about 10
> > patches in a patchset.
> > 
> > I'm not sure if I have time for this rework. But we can try.
> 
> So, I guess you won't mind if I would prepare a v2 which fixes some of
> the issues discussed, based on my repo. We're in no hurry, since Dmitry
> is currently away for the next couple of weeks.

Sure! I have been waiting for a reaction for 5 years. We are definitely in
no hurry :)

> > We can
> > discuss the hasher-privd in russian if you like :)
> 
> I'm personally fine with both english and russian; looks like you're too.
> The remaining concerns are:
> * if everyone else interested can respond and continue the conversation
> * if the community around hasher ever goes international.

I can hardly imagine a situation that someone who is not russian speaking
would want to discuss these patches in this mailing list. If that happens
then I'll probably eat my red hat :)

> I responded in english, since the patch messages were in english, and in
> that case I usually take the (nowadays rare with covid) opportunity to
> practice. Если же то, на что я отвечаю, пишут по-русски, то и отвечать,
> наверное, следует тоже по-русски.

Я тоже стараюсь придерживаться такого подхода.

> Если вдруг чувствуете, что лучше по-русски, можете на русский переключаться.

Я пишу по-английски хуже и медленнее. Просто Дима меня совсем бы не понял,
если бы я коммиты по-русски написал :)

> Ну и иногда пишешь что-то по-русски в некоторый
> профессионально-технический разговор, а в реплике столько оказывается
> непереводных терминов и собственных имён, что уж лучше по-английски бы писал. :) 

Зато это мне лишняя практика русского :)

-- 
Rgrds, legion


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]