[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#56608] [PATCH v2 1/2] gnu: security: Add fail2ban-service-type.
|
From: |
muradm |
|
Subject: |
[bug#56608] [PATCH v2 1/2] gnu: security: Add fail2ban-service-type. |
|
Date: |
Tue, 23 Aug 2022 21:22:28 +0300 |
|
User-agent: |
mu4e 1.8.7; emacs 29.0.50 |
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
signature.asc
Description: PGP signature
[bug#56608] [PATCH v2 1/2] gnu: security: Add fail2ban-service-type., muradm, 2022/08/22