Allow unbanip of tuple IDs #3533

lade-odoo · 2023-06-22T10:13:30Z

As specified here, IDs in Fail2ban can be tuples. However, when a tuple ID is banned, there is no way to remove it via the unbanip command line. If tried, the tuple will be considered as a list with each element being an ID. Instead, we should try to parse the string to see if it represents an object.

sebres · 2023-06-22T12:12:59Z

Confirmed.
Just eval is evil, thus I'd rewrite it somehow, either with ast.literal_eval or with RE parsing...
Since literal_eval is safe against "injections" and other kind of malicious evaluations, I'd probably prefer that way:

+ from ast import literal_eval
...
		# IPs can be represented in string format (e.g.: tuples)
-		is_ip_parsed = True
-		if isinstance(ip, str):
+		if isinstance(ip, str) and str[:1] in ("(","["):
			try:
-				ip = eval(ip)
+				ip = literal_eval(ip) 
-			except Exception:
+			except SyntaxError:
-				is_ip_parsed = False
+				pass
...
-			if not is_ip_parsed and not isinstance(ip, IPAddr):
+			if not isinstance(ip, (IPAddr, tuple, list)):

However I don't think actions.py is the proper module to fix this. Rather it must be fixed somewhere in

fail2ban/fail2ban/server/server.py

Line 531 in 48c91df

def setUnbanIP(self, name=None, value=None, ifexists=True):

sebres

As already wrote... actions doesn't look like appropriate place to fix that.

sebres · 2023-06-22T12:42:19Z

Moreover, thinking about that a bit, it doesn't look as a good fix at all.
The problem here is the ambiguity by (123,345) supplied to unban command, since fail2ban allows to ban IDs (not IPs and tuples only), so fail2ban-client unban '(123,345)' can be:

either a tuple ID (123, 345)
or a string ID "(123,345)", for which this fix introduce a certain incompatibility (if supplied without quotes as before).

Concerning this matter nothing occurs to me excepting probably some new option (like --expr), that'd enable parsing of parameters as an expression:

- set <JAIL> unbanip [--report-absent] <IP> ... <IP>
+ set <JAIL> unbanip [--report-absent] [--expr] <IP> ... <IP>
- unban <IP> ... <IP>
+ unban [--expr] <IP> ... <IP>

Alternatively, the selector in banmanager may be extended to accept string comparison, so tuple (123, 345) becomes equal to string "(123, 345)" and therefore gets unbanned.

lade-odoo · 2023-06-22T13:00:18Z

I agree with you concerning the --expr flag, this sounds like the best idea. I don't like the idea of converting the tuple to a string. Mostly because we are already using tuple ID everywhere so let's keep coherency and introduce this flag instead of playing with conversion string <-> tuple!

As specified [here], IDs in Fail2ban can be tuples. However, when a tuple ID is banned, there is no way to remove it via the `unbanip` command line. If tried, the tuple will be considered as a list with each element being an ID. Instead, we should try to parse the string to see if it represents an object. [here]: https://github.com/fail2ban/fail2ban/blob/226a59445a8046b7f86c3bc072be1d78555ccdb0/fail2ban/server/failregex.py#L313

lade-odoo · 2023-06-22T14:26:41Z

Changed the modification with a small refactoring in order to avoid the recursive method. This would cause issues as it would recurse on the TUPLE_ID which must be considered as an ID and not a simple list/tuple.

sebres · 2023-06-22T16:38:15Z

Thx!

Changed the modification with a small refactoring in order to avoid the recursive method. This would cause issues as it would recurse on the TUPLE_ID which must be considered as an ID and not a simple list/tuple.

Understand.
I miss few tests for all that, so I'll add them later and make a review (for instance there is still unban command in transmitter which must be extended in similar way, also it'd consider both options in vice versa order, etc).

… for `unban` command, etc

sebres · 2023-06-22T17:38:53Z

Done.
However there is probably still one case to "solve" - banip, that must also accept tuple IDs now for consistent reasons.

lade-odoo · 2023-06-23T13:49:50Z

Solved the issue of banip and other command that also needed to be adapted.

sebres

Thx!
I can't approve the changes right now... see my comments.

sebres · 2023-06-23T15:49:55Z

fail2ban/helpers.py

+ except SyntaxError:
+ return v
+ if value:
+ return list(map(parseExpr, value)) if isinstance(value, (list, tuple)) else parseExpr(value)


By the way, why it needs an explicit conversion to list (dereference a map-iterator) here?
checked by isinstance somewhere or just a simple prevention (against double usage, etc)?

sebres · 2023-06-23T15:51:13Z

fail2ban/helpers.py

+ return v
+ if value:
+ return list(map(parseExpr, value)) if isinstance(value, (list, tuple)) else parseExpr(value)
+ return None


Hmm... return value maybe? otherwise it'd return None by empty string or empty list.

sebres · 2023-06-23T16:07:17Z

fail2ban/server/filter.py

@@ -468,7 +468,7 @@ def performSvc(self, force=False):

 def addAttempt(self, ip, *matches):
 """Generate a failed attempt for ip"""
- if not isinstance(ip, IPAddr):
+ if not isinstance(ip, (IPAddr, list, tuple)):


Does it really bother somewhere? If so then we'd need to fix that in IPAddr, because normally IPAddr can safe handle raw values (not IPs, inclusive tuple or lists) without any problem. But has additional facility to check whether it's IP, etc.
So we'd always prefer IPAddr as identifier/IP in internal filters/actions functionality if possible.

>>> from fail2ban.server.ipdns import IPAddr >>> id = ('A','B') >>> ip = IPAddr(id); ip ('A', 'B') >>> ip == id True >>> ip.family == IPAddr.CIDR_RAW True >>> ip.raw ('A', 'B')

sebres · 2023-06-23T16:11:25Z

fail2ban/server/server.py

@@ -523,27 +522,19 @@ def getAction(self, name, value):
 def setBanTime(self, name, value):
 self.__jails[name].actions.setBanTime(value)

- def addAttemptIP(self, name, *args):
- return self.__jails[name].filter.addAttempt(*args)
+ def addAttemptIP(self, name, ip, failures):


I'm not sure we should change the API here (*failures can be supplied similarly from outside).

at least this would be better:

- def addAttemptIP(self, name, ip, failures): + def addAttemptIP(self, name, ip, *args):

sebres requested changes Jun 22, 2023

View reviewed changes

lade-odoo added 2 commits June 22, 2023 16:23

fixup! Allow unbanip of tuple IDs

e03e9bd

lade-odoo force-pushed the master-unban_tuple_fid-lade branch from e04113c to e03e9bd Compare June 22, 2023 14:25

sebres added 2 commits June 22, 2023 19:20

code review: parse options properly (order independently), same logic…

6afdf23

… for `unban` command, etc

extended tests covering unban tuple IDs

34390b3

allow tuple IDs on all commands

98a4cee

lade-odoo force-pushed the master-unban_tuple_fid-lade branch from 8c60a21 to 98a4cee Compare June 23, 2023 13:46

sebres reviewed Jun 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow unbanip of tuple IDs #3533

Allow unbanip of tuple IDs #3533

lade-odoo commented Jun 22, 2023

sebres commented Jun 22, 2023

sebres left a comment

sebres commented Jun 22, 2023 •

edited

lade-odoo commented Jun 22, 2023

lade-odoo commented Jun 22, 2023

sebres commented Jun 22, 2023

sebres commented Jun 22, 2023

lade-odoo commented Jun 23, 2023

sebres left a comment

sebres Jun 23, 2023

sebres Jun 23, 2023

sebres Jun 23, 2023

sebres Jun 23, 2023

sebres Jun 23, 2023

Allow unbanip of tuple IDs #3533

Are you sure you want to change the base?

Allow unbanip of tuple IDs #3533

Conversation

lade-odoo commented Jun 22, 2023

sebres commented Jun 22, 2023

sebres left a comment

Choose a reason for hiding this comment

sebres commented Jun 22, 2023 • edited

lade-odoo commented Jun 22, 2023

lade-odoo commented Jun 22, 2023

sebres commented Jun 22, 2023

sebres commented Jun 22, 2023

lade-odoo commented Jun 23, 2023

sebres left a comment

Choose a reason for hiding this comment

sebres Jun 23, 2023

Choose a reason for hiding this comment

sebres Jun 23, 2023

Choose a reason for hiding this comment

sebres Jun 23, 2023

Choose a reason for hiding this comment

sebres Jun 23, 2023

Choose a reason for hiding this comment

sebres Jun 23, 2023

Choose a reason for hiding this comment

sebres commented Jun 22, 2023 •

edited