From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on sa.local.altlinux.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00 autolearn=unavailable autolearn_force=no version=3.4.1 Date: Thu, 1 Oct 2020 21:43:04 +0200 From: Alexey Gladkov To: Arseny Maslennikov Message-ID: <20201001194304.oktcp7jqmdgg34pn@comp-core-i7-2640m-0182e6> References: <9bca7626b593f896de4283cba2d6290ec99eb4f2.1576183643.git.legion@altlinux.org> <20200917131013.GB286846@cello> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="p66kmonxbkgd76mw" Content-Disposition: inline In-Reply-To: <20200917131013.GB286846@cello> Cc: devel@lists.altlinux.org, ldv@altlinux.org Subject: Re: [devel] [PATCH hasher-priv v1 1/3] Make a daemon from the hasher-priv X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Oct 2020 19:43:13 -0000 Archived-At: List-Archive: List-Post: --p66kmonxbkgd76mw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > >=20 > > All privileged operations moved to the daemon. Commands to the server > > are sent through the unix domain socket. The credentials which the send= er > > specifies are checked by the kernel. The hasher-priv no longer SUID. >=20 > I'm going to suggest some English literacy/typo improvements in a separate > email. >=20 > >=20 > > For each user server creates a separate session process that executes > > commands only from the user who created it. The session process ends >=20 > 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. >=20 > 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. > >=20 >=20 >=20 >=20 > > Signed-off-by: Alexey Gladkov > > --- > > hasher-priv/.gitignore | 1 + > > hasher-priv/DESIGN | 281 ++++++++++++++++---------- >=20 > 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. >=20 > 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. >=20 > 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 ++++---- >=20 > After reading the following 2 new files... >=20 > > hasher-priv/caller_server.c | 373 ++++++++++++++++++++++++++++++++++ > > hasher-priv/caller_task.c | 214 ++++++++++++++++++++ >=20 > ..., I'm not sure why do they have to be separate. > They can even be merged with caller.c, perhaps. >=20 > > 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 >=20 > 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. >=20 > I'm going to post reviews in separate messages, one answer per file. >=20 --=20 Rgrds, legion --p66kmonxbkgd76mw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABECAB0WIQSuzIk+w2aWgaEZLHKOFEXcaOMeVAUCX3YxSAAKCRCOFEXcaOMe VHlaAKCtFoNZtR1WXpcbj2vq68dHCav5PwCcCzowrYutJoiY8e9q8YF6Z5IjfdI= =jhor -----END PGP SIGNATURE----- --p66kmonxbkgd76mw--