[bug#56608] [PATCH v2 1/2] gnu: security: Add fail2ban-service-type.

[Maxim Cournoyer]

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