From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp12.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms5.migadu.com with LMTPS id EApIG6gjBWN7jQAAbAwnHQ (envelope-from ) for ; Tue, 23 Aug 2022 20:59:52 +0200 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp12.migadu.com with LMTPS id 2MAyG6gjBWNPnQAAauVa8A (envelope-from ) for ; Tue, 23 Aug 2022 20:59:52 +0200 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 287172B946 for ; Tue, 23 Aug 2022 20:59:52 +0200 (CEST) Received: from localhost ([::1]:48636 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oQZ7v-0007zC-D5 for larch@yhetil.org; Tue, 23 Aug 2022 14:59:51 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:34068) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oQZ0M-00046D-E4 for guix-patches@gnu.org; Tue, 23 Aug 2022 14:52:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:55313) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oQZ0M-0007bP-49 for guix-patches@gnu.org; Tue, 23 Aug 2022 14:52:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oQZ0L-0005K4-Qt for guix-patches@gnu.org; Tue, 23 Aug 2022 14:52:01 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#56608] [PATCH v2 1/2] gnu: security: Add fail2ban-service-type. Resent-From: muradm Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Tue, 23 Aug 2022 18:52:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 56608 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Maxim Cournoyer Cc: 56608@debbugs.gnu.org Received: via spool by 56608-submit@debbugs.gnu.org id=B56608.166128070020433 (code B ref 56608); Tue, 23 Aug 2022 18:52:01 +0000 Received: (at 56608) by debbugs.gnu.org; 23 Aug 2022 18:51:40 +0000 Received: from localhost ([127.0.0.1]:45062 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oQYzz-0005JU-Df for submit@debbugs.gnu.org; Tue, 23 Aug 2022 14:51:39 -0400 Received: from nomad-cl1.staging.muradm.net ([139.162.159.157]:48008 helo=nomad-cl1.muradm.net) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oQYzx-0005JH-MR for 56608@debbugs.gnu.org; Tue, 23 Aug 2022 14:51:38 -0400 Received: from localhost ([127.0.0.1]:49098) by nomad-cl1.muradm.net with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1oQYz1-0006nN-30; Tue, 23 Aug 2022 18:50:40 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=muradm.net; s=mail; h=Content-Type:MIME-Version:Message-ID:In-reply-to:Date:Subject:Cc:To :From:References: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=eM+bCvqGOqWe0SZ6QfUOdAEK30xx0ga0vJM7llnqb0A=; b=x654jYVHyy3zp9DDy1S77TwtjE DMbnJstTzo8QlRToH51dcxrUYNxp4iWA51djhRrzbsw87cRhLwRXwWNx9H/ldv2566r9Zg9KvdGSE v9Jc6bfHdKOXizrGq767MBzzCa7rowhx24owcP3ZwdGylZzXseZ2EiFADVHPzc+3glTbECrqBKos4 ibZAfdqVr7L8t5yXIHnZ+rsNaPUcr4OTMMmgKEESbZ16vbxJfoSui/SCLsH9WjKmlEctzL2La9U6r tWm6BVeDOeTlsTsAnw72u90zUdxXrluaCzZAtLQff/3lg36hM5VS0sD399evuswjc3LcWPT/jnLE5 YyU+0zsu+XVOVdAG2qs67c0yNHEPhIhhZ3uBwGv8Z9/NMu4yfz+MDtgqokb7KPvflrj/Ye0/Je5Ed SvUU2ChLWcPQst8Qtp8IjOopwb7m4sLZBPSAec5NZuD8zIBBnedZGmpOJ1aYWJRcYiN9qq2vodyc+ CtA3YMOWylt8RzQ7SzHLmhFn; Received: from muradm by localhost with local (Exim 4.96) (envelope-from ) id 1oQYzo-0004Ma-0l; Tue, 23 Aug 2022 21:51:28 +0300 References: <87edxxqpg3.fsf@gmail.com> <20220822172607.31515-1-mail@muradm.net> <20220822172607.31515-2-mail@muradm.net> <87o7wcgldm.fsf@gmail.com> User-agent: mu4e 1.8.7; emacs 29.0.50 From: muradm Date: Tue, 23 Aug 2022 21:22:28 +0300 In-reply-to: <87o7wcgldm.fsf@gmail.com> Message-ID: <87v8qiyes0.fsf@muradm.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Migadu-Flow: FLOW_IN X-Migadu-To: larch@yhetil.org X-Migadu-Country: US ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1661281192; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:resent-cc:resent-from:resent-sender: resent-message-id:in-reply-to:in-reply-to:references:references: list-id:list-help:list-unsubscribe:list-subscribe:list-post: dkim-signature; bh=eM+bCvqGOqWe0SZ6QfUOdAEK30xx0ga0vJM7llnqb0A=; b=Kyj5ce2bObk4QpbhCe5voBmjwl7QZiQnpcNZQiOl61IkDwoJZi0c7XN6xLdJ+fu1MEKtgF yJ/VNjAI9HNSa8fFI03QHr07EP0B4X0Q7bgoT7SvEGAyd3acceBESh/lM5mSMuFMsMKNO9 v5CpiPzJLuJidrGDHSseXsI9+TA0R4+j6qU1gO8ufelvvEjtBefnkGab63K5HCSMCo7VwS +tHDv51tHVUxmN7zgoAok7O2vOZQbOp228oltEZfqq/VlrDNKOBLH7byYf0sG1drOMzby6 S48oRH6QtxSB/P53U5RebzLN7Ch+hPOECw5uA7w5aLVNYXqlpSJooJzZzRjkmQ== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1661281192; a=rsa-sha256; cv=none; b=DRYL7SIG9uxYDWbuzW0+u9c+gy3CaNT3FSUYVgth0DJDPDaxs6pb6NUhCu2wWuNXXuWjd8 0nn2LyDaLuzooITToiUWH6qTzLbr9BMeJgTwc7aq9r6dDDQt4wAW0D68U5wXLZZcZkgppj PFuLKM17O1z1NqJq9/aJjhaF1qUtR9jfyND/wna1o27xdi0T4vYdqPdNBdRqtNUKFSetVq k/JpKffHUtFJSy6s+RY/X4S209OhBcP1kdSfktnq4Darrz8HCV+Rf3vKdzJD2mQ2oVhMEn WnGnrCDKf5KS1F9MS+54XWg/iWxJsFw9PsoBduZ3r7g+y22vCfIU5hUATUuZzg== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=muradm.net header.s=mail header.b=x654jYVH; dmarc=none; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org" X-Migadu-Spam-Score: -1.50 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=muradm.net header.s=mail header.b=x654jYVH; dmarc=none; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org" X-Migadu-Queue-Id: 287172B946 X-Spam-Score: -1.50 X-Migadu-Scanner: scn0.migadu.com X-TUID: BrPe1ksyDuda --=-=-= Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi, Here are comments only, changes will be with squashed patch later=20 on. Maxim Cournoyer writes: > Hi, > > Well done implementing the configuration records using > define-configuration! I believe it'll help its users avoiding=20 > mistakes. Honestly, it was not very pleasant experience... IMHO :) I see why you want it, but in my experience/opinion it is not=20 right way.. > Here are some comments for the guix.texi bits which are not > automatically generated: > > muradm writes: > [...] >> +This is the type of the service that runs @code{fail2ban}=20 >> daemon. It can be > ^ > two=20 > spaces Done. [...] >> +@item Permanent extending configuration >> +service developers may not @code{fail2ban-service-type} in=20 >> service-type's > ^ missing verb Actually type s/not/use/, done. [...] >> +This is the type of the service that runs @code{fail2ban}=20 >> daemon. It can be > ^ > two=20 > spaces Done. [...] >> + ;; excplicit configuration, this way fail2ban daemon >> + ;; will start and do its thing for sshd jail > > Typo (excplicit). Also, please use full sentences for=20 > stand-alone > comments (with proper punctuation). Done. [...] >> + ;; there is no direct dependency with actual openssh >> + ;; server configuration, it could even be omitted here > > Likewise. Done. [...] >> +(define-configuration/no-serialization=20 >> fail2ban-ignorecache-configuration >> + (key (string) "Cache key.") >> + (max-count (integer) "Cache size.") >> + (max-time (integer) "Cache time.")) > > Note that when you do not use a default value, you can leave out=20 > the > parenthesizes. > Done in all relevant places. [...] >> +(define serialize-fail2ban-jail-filter-configuration >> + (match-lambda >> + (($ _ name mode) >> + (format #f "~a~a" >> + name (if (eq? 'unset mode) "" (format #f=20 >> "[mode=3D~a]" mode)))))) > > You could use (ice-9 format) with a conditional formatting here.=20 > The > example from info '(guile) Formatted Output' is > > (format #f "~:[false~;not false~]" 'abc) =E2=87=92 "not false" > Done with ~@[] instead. >> +(define (list-of-arguments? lst) >> + (every >> + (lambda (e) (and (pair? e) >> + (string? (car e)) >> + (or (string? (cdr e)) >> + (list-of-strings? (cdr e))))) >> + lst)) > > It could be better to define a argument? predicate, use the=20 > 'list-of' > procedure from (gnu services configuration) on it. > Done. [...] >> +(define-maybe boolean (prefix fail2ban-jail-configuration-)) >> +(define-maybe symbol (prefix fail2ban-jail-configuration-)) >> +(define-maybe fail2ban-ignorecache-configuration (prefix=20 >> fail2ban-jail-configuration-)) >> +(define-maybe fail2ban-jail-filter-configuration (prefix=20 >> fail2ban-jail-configuration-)) > > Is using the prefix absolutely necessary? It makes things=20 > awkwardly > long. Since fail2ban-service-type it's the first citizen of (gnu > services security), it could enjoy the luxury of not using a=20 > prefix, in > my opinion. Later services may need to define their own prefix. > There are already four configuration records which are somewhat overlapping. Pay attention to *serialize-string functions. Also security may/will have more stuff probably, why do future contrubutors life harder. I feel bad/uncomfortable in removing prefix and poluting name space, so I left it long. [...] >> + (enabled > > I'd use enabled? and employ a name normalizer (e.g., stripping=20 > any > trailing '?') in the boolean serializer. > Done including few other places. [...] >> + (maxretry > > I think we could use hyphen in the field names, and use a=20 > normalizer to > strip them at serialization time (assuming hyphens are never=20 > allowed in > a key name). > Done. [...] >> + (bantime.overalljails > > We could have the normalization "bantime-" -> "bantime." done in=20 > the > serializers, to use more Schemey names. > Done, although I find it a bit fragile. [...] >> + (ignorecommand >> + maybe-string >> + "External command that will take an tagged arguments to=20 >> ignore. >> +Note: while provided, currently unimplemented in the context=20 >> of @code{guix}.") > > Then I'd remove it, as I don't see in which other context it=20 > would be > useful. > What I wanted to say here, is that field does not support gexp=20 like thing at the moment. Still if user really needs this, command can be used from global package or via publishing to easy location like /etc/scripts/some-tool. [...] >> + "Can be a list of IP addresses, CIDR masks or DNS hosts.=20 >> @code{fail2ban} > ^ > Please use two spaces after period. > Done. [...] >> + "Filename(s) of the log files to be monitored.") > > The convention in GNU is to use "file name" rather than=20 > "filename". > Done, however I just copied from fail2ban own documentation/code,=20 so I'm not very sure if such things should be fixed like this. [...] >> + "Extra raw content to add at the end of=20 >> @file{jail.local}.")) > ^the=20 > @file{jail.local}=20 > file. > Done. [...] >> + (let* ((out (ungexp output))) > ^ use just let here > Done. [...] >> +(define (fail2ban-jail-service svc-type jail) >> + (service-type >> + (inherit svc-type) >> + (extensions >> + (append >> + (service-type-extensions svc-type) >> + (list (service-extension fail2ban-service-type >> + (lambda _ (list jail)))))))) > ^ nitpick, but (compose list=20 > jail) is > more common > Done. > That looks very good. I haven't checked the tests yet, but I=20 > think with > the extra polishing suggested above, it should be in a good=20 > place to be > merged soon! > > Thanks, > > Maxim --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEESPY5lma9A9l5HGLP6M7O0mLOBeIFAmMFIa8ACgkQ6M7O0mLO BeLRERAAoyYSDmCJNjuUWs8JqRa0h78PQR+MjxYCTaoVPIghnHO16tRYi6GrtiFB vlrWNsEC0rHrD50oilzO/sTSUSpeSJMLf+QN6irUa+/DqDel8rzgCEhuFtYdNrR5 R80j7OzrtRLSec41oMTa/q7F17vNj3qg7RBY6Up6p6Gl9sQ9sSix8U5ZJwbjyspy wOLj7kTvDZKvB3MhMgXu7k/r2yJ+enf+h1qnjVIwGHHX0/Io0s1DScW1XBLfPAMa URrc40fglqfNfAbhAaTKTpLUqZNRZS6OEsisfIZPGdU9zJQ/q+Q3jCwU17Cc2Oxh AGeN4i2gnlImrSVzaYR74LUGUo3OvkVBwWLgOBUguOvU2emGYL0vb+4xS9ni6XIM KZF2ytMHvwIKs4REL70gYxsSVjb4OWCUOaVStzQjmuVuZAET3Cu2qhVRgeo2o/6a 5ewxIu7F4sfVOJA7EmeYiOXN++PCzFbQbyvZi2M8HwOTeFP6FzJdM00hbxMuz9sy tMxbxecOOmPF5qXMfpl2SM8IT+uEUxetbKta5Y5M2pPy/YrmUwoNqKdWLE0nMBjz HrSeCkWhgPjq1ss0Iph4Q4ZmC6g0YlvlFUjZ9Nv8QYFOt2BbJzAtstVZnsOOxxjJ YhopV+P4f4V8JbggUZ9fazjqy88Xi6SHdbxwtsCE1ndmtcC61wE= =qcAJ -----END PGP SIGNATURE----- --=-=-=--