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=-3.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,RP_MATCHES_RCVD autolearn=unavailable autolearn_force=no version=3.4.1 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=cs.msu.ru; s=dkim; h=Subject:In-Reply-To:Content-Type:MIME-Version:References:Message-ID :Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=hFa3akQBUC32SzTzpSPrzy47j1ZDLEFCipW7NApjxKQ=; b=ekML7xih87T/apeynH5CD3khNz 6146E0ifd75Nq0X31mm8otUn+VjJODZ9nLzMQe7ZLTR9Bl/lQJ+YCdh8G1mJRAgejBlvsg4durgzk oVjfdmbL1NQGhc9y03pxBNkYi2oeCJ5OwuWQGkx/ezyPiL4cUUrHkBqR5ISKCHjM7wJHV7olqM6Zp Xhe7znzZotePZ1g855SQMefZu9DQyjJnYBkIZ7zdggeVRuizPdzVLI8DQmhn84EyuQwBBB0h42KP6 qG42plqL5/KUsdnWHEoZhcUvIBeZZrc14eQCQWgMLCDHrDRJEU2DrJBQGQdOcKKzEe74QYfvP6AKM yOpAbdnw==; Date: Thu, 17 Sep 2020 16:10:13 +0300 From: Arseny Maslennikov To: Alex Gladkov , devel@lists.altlinux.org Message-ID: <20200917131013.GB286846@cello> References: <9bca7626b593f896de4283cba2d6290ec99eb4f2.1576183643.git.legion@altlinux.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="uQr8t48UFsdbeI+V" Content-Disposition: inline In-Reply-To: <9bca7626b593f896de4283cba2d6290ec99eb4f2.1576183643.git.legion@altlinux.org> OpenPGP: url=http://grep.cs.msu.ru/~ar/pgp-key.asc X-SA-Exim-Connect-IP: 10.7.5.179 X-SA-Exim-Mail-From: ar@cs.msu.ru X-SA-Exim-Version: 4.2.1 X-SA-Exim-Scanned: Yes (on mail.cs.msu.ru) Cc: 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, 17 Sep 2020 13:10:21 -0000 Archived-At: List-Archive: List-Post: --uQr8t48UFsdbeI+V Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 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. >=20 > 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. >=20 > 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? > 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 ++++++++++++++++++++ =2E.., 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. --uQr8t48UFsdbeI+V Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE56JD3UKTLEu/ddrm9dQjyAYL01AFAl9jYDQACgkQ9dQjyAYL 01B2FBAAg8EbEIB9PHHW6lmIFujIt9SvuzwFepCjkVo9X1uhiYnN/mHmtquOgFT4 fG5ZfopfNCouqBLi6uUy7SNcR3ZNWBywzPYL5YCppKluheWYVMrMJr4PYewiKEzo NrzfiQvjE7RH757elM/ZRb6bmylM7ocbpirOmDA037Qk3USvcGyc6+G9/Jdrbze3 ElnfrbUompypdi9Ab249C6uWquCU3Y5NG6nzonGDDe+6XEV9ETtcfTCSqEdVIaA7 gftIR/0kmHG6F3zm+QS48N0BlESYpMDPtquqzhkmbBJxWM1eHpwMbwAqUPCFHBkE rqRqroGiAZHsouZkc+k+D3GOJmJPWowb5oOXfR0wj9JiymGTYCDSZd4anlJ3eYH0 0dJzkls/HCKk/08+z0V3WMwVU2INomFaqI8EfIO6dXXFpkz/PTJ36yb9bS8TO4JG DzPndfnF9RePxTrh8vODv3Dvzfow7LIDfzMlsgRgvSB5rWNAIar8TjBvSzLuprh2 TlX6V7xlPaG1Gw8hUQgOdxMwU/lDhorFqDLNoN9dDcrptg9sN/WPgVgOTWikLSPz kXstvUiOUYUibBuZVS43OxFjLi73y9MzJRdt1tDjNnXcaNgCnXUhpT+1ARe4wHqo zxr1Q9+oFxK5m87OMw70BW2hnNcrCfVC6fU1VfNujum9FMxSQP0= =asYT -----END PGP SIGNATURE----- --uQr8t48UFsdbeI+V--