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

[Arseny Maslennikov <ar@cs.msu.ru>]

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

On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote:
> From: Alexey Gladkov <legion@altlinux.org>
> 
> All privileged operations moved to the daemon. Commands to the server
> are sent through the unix domain socket. The credentials which the sender
> specifies are checked by the kernel. The hasher-priv no longer SUID.

I'm going to suggest some English literacy/typo improvements in a separate
email.

> 
> For each user server creates a separate session process that executes
> commands only from the user who created it. The session process ends

Since that new process will still be privileged, why the additional fork
and the new listening socket inode? Is the strive for less complex
daemon source code the only driver for that, albeit a perfectly viable
one?

> after a certain period of inactivity.

No problem with that, but IMO we still should be careful
about resource leaks.

> 



> Signed-off-by: Alexey Gladkov <legion@altlinux.org>
> ---
>  hasher-priv/.gitignore      |   1 +
>  hasher-priv/DESIGN          | 281 ++++++++++++++++----------

I'd like to see a formal description of the client-server protocol in e.
g. hasher-priv/IPC, that shows the intended message exchanges and
meanings. The message contents can be rather easily inferred from the C
headers, but the semantics cannot. This ends up as a source of
programming errors, see the other emails.

The relevant information is probably already present in the DESIGN file,
but, as this is documentation for humans, we should wrap it into a form
that's easier to comprehend without wasting much time.

I'm also worried about the message format using plain C ABI structs.
We're not going to use it over the network on machines with different
architectures, and we're never going to support a client and server from
different package builds, sure. But is this enough of a justification?

>  hasher-priv/Makefile        |  28 ++-
>  hasher-priv/caller.c        |  81 ++++----

After reading the following 2 new files...

>  hasher-priv/caller_server.c | 373 ++++++++++++++++++++++++++++++++++
>  hasher-priv/caller_task.c   | 214 ++++++++++++++++++++

..., I'm not sure why do they have to be separate.
They can even be merged with caller.c, perhaps.

>  hasher-priv/cmdline.c       |  27 ++-
>  hasher-priv/communication.c | 392 ++++++++++++++++++++++++++++++++++++
>  hasher-priv/communication.h |  77 +++++++
>  hasher-priv/config.c        | 143 ++++++++++++-
>  hasher-priv/epoll.c         |  39 ++++
>  hasher-priv/epoll.h         |  18 ++
>  hasher-priv/hasher-priv.c   |  78 +++++++
>  hasher-priv/hasher-privd.c  | 375 ++++++++++++++++++++++++++++++++++
>  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          |  33 +--
>  hasher-priv/server.conf     |  13 ++
>  hasher-priv/sockets.c       | 183 +++++++++++++++++
>  hasher-priv/sockets.h       |  32 +++
>  hasher-priv/x11.c           |   1 +
>  28 files changed, 2632 insertions(+), 246 deletions(-)
>  create mode 100644 hasher-priv/caller_server.c
>  create mode 100644 hasher-priv/caller_task.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/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

Wow, this patch is rather big. That would be easier to review if there
was a bunch of logically self-contained commits, which could later be
squashed by the merging maintainer, perhaps, if there's a requirement
that every commit has to build successfully and run well.

I'm going to post reviews in separate messages, one answer per file.


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