BF: fail2ban client can't handle multi word setcinfo or action[*] values #134

Merged
merged 5 commits into from Mar 11, 2013

Conversation

Projects
None yet
2 participants
Contributor

grooverdan commented Mar 10, 2013

BF: allow more than single word for command action[start,stop,ban,unban,check] and for setcinfo too in fail2ban-client. Previously a set

BF: general Exception catch was excessive. Only IOError and OSError are possible and have different meanings

@yarikoptic yarikoptic commented on the diff Mar 10, 2013

server/transmitter.py
@@ -189,7 +189,7 @@ def __commandSet(self, command):
elif command[1] == "setcinfo":
act = command[2]
key = command[3]
- value = command[4]
+ value = " ".join(command[4:])
@yarikoptic

yarikoptic Mar 10, 2013

Owner

why bother exactly if any given cmdline positional argument could be wrapped into " " or ' ' ?

I do see it as a possible improvement, but that then also requires documentation (--help output) clarification... but still wonder if it is worth it at all

@grooverdan

grooverdan Mar 10, 2013

Contributor

why bother exactly if any given cmdline positional argument could be wrapped into " " or ' ' ?

Because action* is almost always more that one word. Because this is the only case a wrapped argument is needed. Its still not the nicest but it gives consistent results. Because fail2ban currently discarding arguments and not reporting so much as an error.

The testcase:

./fail2ban-client -c config/ -s /tmp/f2b.sock -i start
add test pyinotify
set test addaction iptables
set test actionban iptables echo   >> /tmp/ban
set test actionunban iptables echo   >> /tmp/unban
get test actionban iptables
get test actionunban iptables

Currently this records only the echo as the action.

but that then also requires documentation (--help output) clarification..

So


sets the start command of the action for . Quote with "" or '' if more than a one word command is required.

I do see it as a possible improvement,

but still wonder if it is worth it at all

make up your mind. Unless you knew about quoting you've got a barely usable set action* client interface.

@yarikoptic yarikoptic commented on the diff Mar 10, 2013

server/filter.py
logSys.error("Unable to open %s" % filename)
logSys.exception(e)
return False
+ except OSError, e: # pragma: no cover - requires race condition to tigger this
+ logSys.error("Error opening %s" % filename)
+ logSys.exception(e)
+ return False
@yarikoptic

yarikoptic Mar 10, 2013

Owner

consider me a paranoid or anyone else, I would still prefer to get an additional swallowing 'except Exception, e' at the end with an additional descriptive logSys.exception(e) and possibly logSys.error("This must have never happened") -- just to guarantee that we are not forgetting anything what FileContainer might throw there

@grooverdan

grooverdan Mar 10, 2013

Contributor

ok. good. added commit for this.

yarikoptic merged commit 5e5eaaf into fail2ban:master Mar 11, 2013

1 check failed

default The Travis build failed
Details
Owner

yarikoptic commented Mar 11, 2013

ok -- looked all good... so I merged to not keep it away for too long

but still would be nice to signal this feature (of joining multiple positional args). may be via '<>' instead of '' in help output with a minimalistic description for that?

grooverdan deleted the grooverdan:misc-fixes branch Mar 13, 2013

Contributor

grooverdan commented Mar 13, 2013

Is there a need? Looking in common/protocol.py I see VALUE, CMD, REGEX and maybe ACTION and JAIL that are likely to contain spaces. If we redo the parsing to closely couple to the commands here we can grab the remainder of the line as the last positional argument and this will solve VALUE, CMD and REGEX. Is supporting spaces in ACTION and JAIL in the command line something you'd like to support?

<> is already used in the action commands for things like and I don't find it as something that I'd intuitively separate arguments with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment