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

[Alexey Gladkov <legion@altlinux.ru>]

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

On Thu, Sep 17, 2020 at 04:10:13PM +0300, Arseny Maslennikov wrote:
> 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?

We have privileged server. When a user request comes in a session daemon
is forked for him that opens a socket for such user. A separate process
is created for each job. If the session daemon is inactive, then after a
session_timeout it will terminate. This is done to isolate one user from
another. You cannot DoS the main server.

> > after a certain period of inactivity.
> 
> No problem with that, but IMO we still should be careful
> about resource leaks.

The admin controls the number of users and hence the number of possible
session daemons.

> > 
> 
> 
> 
> > 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?

Don't be fooled that a socket is being used. The hasher-provd will not be
able to work over the network. The file descriptors are passed over
the socket and privileges are checked.

> >  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.
> 



-- 
Rgrds, legion


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