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