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

[muradm <mail@muradm.net>]

[-- Attachment #1: Type: text/plain, Size: 6764 bytes --]


Hi,

Here are comments only, changes will be with squashed patch later 
on.

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi,
>
> Well done implementing the configuration records using
> define-configuration!  I believe it'll help its users avoiding 
> mistakes.

Honestly, it was not very pleasant experience... IMHO :)
I see why you want it, but in my experience/opinion it is not 
right way..

> Here are some comments for the guix.texi bits which are not
> automatically generated:
>
> muradm <mail@muradm.net> writes:
>

[...]

>> +This is the type of the service that runs @code{fail2ban} 
>> daemon. It can be
>                                                                     ^
>                                                                     two 
>                                                                     spaces

Done.

[...]

>> +@item Permanent extending configuration
>> +service developers may not @code{fail2ban-service-type} in 
>> service-type's
>                              ^ missing verb

Actually type s/not/use/, done.

[...]

>> +This is the type of the service that runs @code{fail2ban} 
>> daemon. It can be
>                                                                     ^
>                                                                     two 
>                                                                     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 
> 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 
>> 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.
>

Done in all relevant places.

[...]

>> +(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
>
> (format #f "~:[false~;not false~]" 'abc) ⇒ "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 
> '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 
>> 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.
>

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 
> 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 
> normalizer to
> strip them at serialization time (assuming hyphens are never 
> allowed in
> a key name).
>

Done.

[...]

>> +  (bantime.overalljails
>
> We could have the normalization "bantime-" -> "bantime." done in 
> 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 
>> 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.
>

What I wanted to say here, is that field does not support gexp 
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. 
>> @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 
> "filename".
>

Done, however I just copied from fail2ban own documentation/code, 
so
I'm not very sure if such things should be fixed like this.

[...]

>> +   "Extra raw content to add at the end of 
>> @file{jail.local}."))
>                                              ^the 
>                                              @file{jail.local} 
>                                              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 
>                                  jail)  is
>                                  more common
>

Done.

> 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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]