Package: guix-patches;
Reported by: muradm <mail <at> muradm.net>
Date: Sun, 17 Jul 2022 02:33:01 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #20 received at 56608 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: muradm <mail <at> muradm.net> Cc: 56608 <at> debbugs.gnu.org Subject: Re: bug#56608: [PATCH v2 1/2] gnu: security: Add fail2ban-service-type. Date: Mon, 22 Aug 2022 14:53:57 -0400
Hi,
Well done implementing the configuration records using
define-configuration! I believe it'll help its users avoiding mistakes.
Here are some comments for the guix.texi bits which are not
automatically generated:
muradm <mail <at> muradm.net> writes:
> * gnu/services/security.scm: New module.
> * gnu/local.mk: Add new security module.
> * doc/guix.text: Add fail2ban-service-type documentation.
> ---
> doc/guix.texi | 249 ++++++++++++++++++++++++
> gnu/local.mk | 2 +
> gnu/services/security.scm | 385 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 636 insertions(+)
> create mode 100644 gnu/services/security.scm
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 023b48ae35..5467f47412 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -36283,6 +36283,255 @@ Extra command line options for @code{nix-service-type}.
> @end table
> @end deftp
>
> +@cindex Fail2ban
> +@subsubheading Fail2ban service
> +
> +@uref{http://www.fail2ban.org/, @code{fail2ban}} scans log files
> +(e.g. @code{/var/log/apache/error_log}) and bans IPs that show the malicious
> +signs -- too many password failures, seeking for exploits, etc.
> +
> +@code{fail2ban} service is provided in @code{(gnu services security)} module.
> +
> +This is the type of the service that runs @code{fail2ban} daemon. It can be
^
two spaces
> +used in various ways, which are:
> +
> +@itemize
> +
> +@item Explicit configuration
> +users are free to enable @code{fail2ban} configuration without strong
> +dependency.
> +
> +@item On-demand extending configuration
> +convenience @code{fail2ban-jail-service} function is provided, in order
> +to extend existing services on-demand.
> +
> +@item Permanent extending configuration
> +service developers may not @code{fail2ban-service-type} in service-type's
^ missing verb
> +extensions.
> +
> +@end itemize
> +
> +@defvr {Scheme Variable} fail2ban-service-type
> +
> +This is the type of the service that runs @code{fail2ban} daemon. It can be
^
two spaces
> +configured explicitly as following:
> +
> +@lisp
> +(append
> + (list
> + ;; excplicit configuration, this way fail2ban daemon
> + ;; will start and do its thing for sshd jail
Typo (excplicit). Also, please use full sentences for stand-alone
comments (with proper punctuation).
> + (service fail2ban-service-type
> + (fail2ban-configuration
> + (extra-jails
> + (list
> + (fail2ban-jail-configuration
> + (name "sshd")
> + (enabled #t))))))
> + ;; there is no direct dependency with actual openssh
> + ;; server configuration, it could even be omitted here
Likewise.
[...]
> diff --git a/gnu/services/security.scm b/gnu/services/security.scm
> new file mode 100644
> index 0000000000..79e202565c
> --- /dev/null
> +++ b/gnu/services/security.scm
> @@ -0,0 +1,385 @@
[...]
> +(define-configuration/no-serialization 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 the
parenthesizes.
> +
> +(define serialize-fail2ban-ignorecache-configuration
> + (match-lambda
> + (($ <fail2ban-ignorecache-configuration> _ key max-count max-time)
> + (format #f "key=\"~a\", max-count=~d, max-time=~d"
> + key max-count max-time))))
> +
> +(define-maybe/no-serialization string)
> +
> +(define-configuration/no-serialization fail2ban-jail-filter-configuration
> + (name (string) "Filter to use.")
> + (mode maybe-string "Mode for filter."))
> +
> +(define serialize-fail2ban-jail-filter-configuration
> + (match-lambda
> + (($ <fail2ban-jail-filter-configuration> _ name mode)
> + (format #f "~a~a"
> + name (if (eq? 'unset mode) "" (format #f "[mode=~a]" mode))))))
You could use (ice-9 format) with a conditional formatting here. The
example from info '(guile) Formatted Output' is
--8<---------------cut here---------------start------------->8---
(format #f "~:[false~;not false~]" 'abc) ⇒ "not false"
--8<---------------cut here---------------end--------------->8---
> +(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 'list-of'
procedure from (gnu services configuration) on it.
[...]
> +(define (fail2ban-jail-configuration-serialize-fail2ban-ignorecache-configuration field-name value)
> + (fail2ban-jail-configuration-serialize-string
> + field-name (serialize-fail2ban-ignorecache-configuration value)))
> +
> +(define (fail2ban-jail-configuration-serialize-fail2ban-jail-filter-configuration field-name value)
> + (fail2ban-jail-configuration-serialize-string
> + field-name (serialize-fail2ban-jail-filter-configuration value)))
> +
> +(define (fail2ban-jail-configuration-serialize-logencoding field-name value)
> + (if (eq? 'unset value) ""
> + (fail2ban-jail-configuration-serialize-string
> + field-name (fail2ban-logencoding->string value))))
> +
> +(define (fail2ban-jail-configuration-serialize-list-of-strings field-name value)
> + (if (null? value) ""
> + (fail2ban-jail-configuration-serialize-string
> + field-name (string-join value " "))))
> +
> +(define (fail2ban-jail-configuration-serialize-list-of-fail2ban-jail-actions field-name value)
> + (if (null? value) ""
> + (fail2ban-jail-configuration-serialize-string
> + field-name (string-join
> + (map serialize-fail2ban-jail-action-configuration value) "\n"))))
> +
> +(define (fail2ban-jail-configuration-serialize-symbol field-name value)
> + (fail2ban-jail-configuration-serialize-string field-name (symbol->string value)))
> +
> +(define (fail2ban-jail-configuration-serialize-extra-content field-name value)
> + (if (eq? 'unset value) "" (string-append "\n" value "\n")))
> +
> +(define-maybe integer (prefix fail2ban-jail-configuration-))
> +(define-maybe string (prefix fail2ban-jail-configuration-))
> +(define-maybe boolean (prefix fail2ban-jail-configuration-))
> +(define-maybe symbol (prefix fail2ban-jail-configuration-))
> +(define-maybe fail2ban-ignorecache-configuration (prefix fail2ban-jail-configuration-))
> +(define-maybe fail2ban-jail-filter-configuration (prefix fail2ban-jail-configuration-))
Is using the prefix absolutely necessary? It makes things awkwardly
long. Since fail2ban-service-type it's the first citizen of (gnu
services security), it could enjoy the luxury of not using a prefix, in
my opinion. Later services may need to define their own prefix.
> +(define-configuration fail2ban-jail-configuration
> + (name
> + (string)
> + "Required name of this jail configuration.")
> + (enabled
I'd use enabled? and employ a name normalizer (e.g., stripping any
trailing '?') in the boolean serializer.
> + maybe-boolean
> + "Either @code{#t} or @code{#f} for @samp{true} and
> +@samp{false} respectively.")
> + (backend
> + maybe-symbol
> + "Backend to be used to detect changes in the @code{ogpath}."
> + fail2ban-jail-configuration-serialize-backend)
> + (maxretry
I think we could use hyphen in the field names, and use a normalizer to
strip them at serialization time (assuming hyphens are never allowed in
a key name).
> + maybe-integer
> + "Is the number of failures before a host get banned
> +(e.g. @code{(maxretry 5)}).")
> + (maxmatches
> + maybe-integer
> + "Is the number of matches stored in ticket (resolvable via
> +tag @code{<matches>}) in action.")
> + (findtime
> + maybe-string
> + "A host is banned if it has generated @code{maxretry} during the last
> +@code{findtime} seconds (e.g. @code{(findtime \"10m\")}).")
> + (bantime
> + maybe-string
> + "Is the number of seconds that a host is banned
> +(e.g. @code{(bantime \"10m\")}).")
> + (bantime.increment
> + maybe-boolean
> + "Allows to use database for searching of previously banned
> +ip's to increase a default ban time using special formula.")
> + (bantime.factor
> + maybe-string
> + "Is a coefficient to calculate exponent growing of the
> +formula or common multiplier.")
> + (bantime.formula
> + maybe-string
> + "Used by default to calculate next value of ban time.")
> + (bantime.multipliers
> + maybe-string
> + "Used to calculate next value of ban time instead of formula.")
> + (bantime.maxtime
> + maybe-string
> + "Is the max number of seconds using the ban time can reach
> +(doesn't grow further).")
> + (bantime.rndtime
> + maybe-string
> + "Is the max number of seconds using for mixing with random time
> +to prevent ``clever'' botnets calculate exact time IP can be unbanned again.")
> + (bantime.overalljails
We could have the normalization "bantime-" -> "bantime." done in the
serializers, to use more Schemey names.
> + maybe-boolean
> + "Either @code{#t} or @code{#f} for @samp{true} and @samp{false} respectively.
> +@itemize
> +@item @code{true} - specifies the search of IP in the database will be executed cross over all jails
> +@item @code{false} - only current jail of the ban IP will be searched
> +@end itemize")
> + (ignorecommand
> + maybe-string
> + "External command that will take an tagged arguments to ignore.
> +Note: while provided, currently unimplemented in the context of @code{guix}.")
Then I'd remove it, as I don't see in which other context it would be
useful.
> + (ignoreself
> + maybe-boolean
> + "Specifies whether the local resp. own IP addresses should be ignored.")
> + (ignoreip
> + (list-of-strings '())
> + "Can be a list of IP addresses, CIDR masks or DNS hosts. @code{fail2ban}
^
Please use two spaces after period.
> +will not ban a host which matches an address in this list.")
> + (ignorecache
> + maybe-fail2ban-ignorecache-configuration
> + "Provide cache parameters for ignore failure check.")
> + (filter
> + maybe-fail2ban-jail-filter-configuration
> + "Defines the filter to use by the jail, using
> +@code{<fail2ban-jail-filter-configuration>}.
> +By default jails have names matching their filter name.")
> + (logtimezone
> + maybe-string
> + "Force the time zone for log lines that don't have one.")
> + (logencoding
> + maybe-symbol
> + "Specifies the encoding of the log files handled by the jail.
> +Possible values: @code{'ascii}, @code{'utf-8}, @code{'auto}."
> + fail2ban-jail-configuration-serialize-logencoding)
> + (logpath
> + (list-of-strings '())
> + "Filename(s) of the log files to be monitored.")
The convention in GNU is to use "file name" rather than "filename".
> + (action
> + (list-of-fail2ban-jail-actions '())
> + "List of @code{<fail2ban-jail-action-configuration>}.")
> + (extra-content
> + maybe-string
> + "Extra content for the jail configuration."
> + fail2ban-jail-configuration-serialize-extra-content)
> + (prefix fail2ban-jail-configuration-))
> +
> +(define list-of-fail2ban-jail-configurations?
> + (list-of fail2ban-jail-configuration?))
> +
> +(define (serialize-fail2ban-jail-configuration config)
> + #~(string-append
> + #$(format #f "[~a]\n" (fail2ban-jail-configuration-name config))
> + #$(serialize-configuration
> + config fail2ban-jail-configuration-fields)))
> +
> +(define-configuration/no-serialization fail2ban-configuration
> + (fail2ban
> + (package fail2ban)
> + "The @code{fail2ban} package to use. It used for both binaries and
> +as base default configuration that will be extended with
> +@code{<fail2ban-jail-configuration>}s.")
> + (run-directory
> + (string "/var/run/fail2ban")
> + "State directory for @code{fail2ban} daemon.")
> + (jails
> + (list-of-fail2ban-jail-configurations '())
> + "Instances of @code{<fail2ban-jail-configuration>} collected from
> +extensions.")
> + (extra-jails
> + (list-of-fail2ban-jail-configurations '())
> + "Instances of @code{<fail2ban-jail-configuration>} provided by user
> +explicitly.")
> + (extra-content
> + maybe-string
> + "Extra raw content to add at the end of @file{jail.local}."))
^the @file{jail.local} file.
> +
> +(define (serialize-fail2ban-configuration config)
> + (let* ((jails (fail2ban-configuration-jails config))
> + (extra-jails (fail2ban-configuration-extra-jails config))
> + (extra-content (fail2ban-configuration-extra-content config)))
> + (interpose
> + (append (map serialize-fail2ban-jail-configuration
> + (append jails extra-jails))
> + (list (if (eq? 'unset extra-content) "" extra-content))))))
> +
> +(define (make-fail2ban-configuration-package config)
> + (let* ((fail2ban (fail2ban-configuration-fail2ban config))
> + (jail-local (apply mixed-text-file "jail.local"
> + (serialize-fail2ban-configuration config))))
> + (computed-file
> + "fail2ban-configuration"
> + (with-imported-modules '((guix build utils))
> + #~(begin
> + (use-modules (guix build utils))
> + (let* ((out (ungexp output)))
^ use just let here
> + (mkdir-p (string-append out "/etc/fail2ban"))
> + (copy-recursively
> + (string-append #$fail2ban "/etc/fail2ban")
> + (string-append out "/etc/fail2ban"))
> + (symlink
> + #$jail-local
> + (string-append out "/etc/fail2ban/jail.local"))))))))
> +
> +(define (fail2ban-shepherd-service config)
> + (match-record config <fail2ban-configuration>
> + (fail2ban run-directory)
> + (let* ((fail2ban-server (file-append fail2ban "/bin/fail2ban-server"))
> + (pid-file (in-vicinity run-directory "fail2ban.pid"))
> + (socket-file (in-vicinity run-directory "fail2ban.sock"))
> + (config-dir (make-fail2ban-configuration-package config))
> + (config-dir (file-append config-dir "/etc/fail2ban"))
> + (fail2ban-action
> + (lambda args
> + #~(lambda _
> + (invoke #$fail2ban-server
> + "-c" #$config-dir
> + "-p" #$pid-file
> + "-s" #$socket-file
> + "-b"
> + #$@args)))))
> +
> + ;; TODO: Add 'reload' action.
> + (list (shepherd-service
> + (provision '(fail2ban))
> + (documentation "Run the fail2ban daemon.")
> + (requirement '(user-processes))
> + (modules `((ice-9 match)
> + ,@%default-modules))
> + (start (fail2ban-action "start"))
> + (stop (fail2ban-action "stop")))))))
> +
> +(define fail2ban-service-type
> + (service-type (name 'fail2ban)
> + (extensions
> + (list (service-extension shepherd-root-service-type
> + fail2ban-shepherd-service)))
> + (compose concatenate)
> + (extend (lambda (config jails)
> + (fail2ban-configuration
> + (inherit config)
> + (jails
> + (append
> + (fail2ban-configuration-jails config)
> + jails)))))
> + (default-value (fail2ban-configuration))
> + (description "Run the fail2ban server.")))
> +
> +(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 jail) is
more common
That looks very good. I haven't checked the tests yet, but I think with
the extra polishing suggested above, it should be in a good place to be
merged soon!
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.