Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow creation of multiple of the same action for a filter #189

Merged
merged 8 commits into from Apr 30, 2013

Conversation

kwirk
Copy link
Contributor

@kwirk kwirk commented Apr 27, 2013

Ref #37: A fix to allow multi actions of the same name, by setting a name value as part of the action extra options which can be set in jail.conf.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 45c9c45 on kwirk:multiaction into bddbf1e on fail2ban:0.9.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling bec70cb on kwirk:multiaction into bddbf1e on fail2ban:0.9.

Primary examples is `name` is used in iptables actions for the chain.
Also changed pop->get so actname can be used as keyword
@kwirk
Copy link
Contributor Author

kwirk commented Apr 27, 2013

Oops, noticed that using name, sort of clashed with nearly every action. Change to actname in a3e216b

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a3e216b on kwirk:multiaction into bddbf1e on fail2ban:0.9.

@yarikoptic
Copy link
Member

could you please digest for me what do you mean "clashed with nearly every action"?
wouldn't the last commit require changing many existing on users' systems jails definitions (and our jail.conf) to use actname= instead of name= in the actions? I still do not understand what is the problem here... sorry

@kwirk
Copy link
Contributor Author

kwirk commented Apr 28, 2013

@yarikoptic Sorry, should have explained more earlier :)
The issue I was concerned about was many action configs already use the <name> tag for things like the chain name in iptables, the jail name and subject field in sendmail, etc. This now remains the same, and doesn't effect the current config.
For those who want multiple of the same action, (e.g. two sendmails, each to different addresses), they can use the same name field in both (as per current config, e.g. jail name in email), and then use the extra actname field to distinguish them. The value of actname is what can then be used with fail2ban-client.

(I just realised that there is no way to get a list of actions via fail2ban-client...)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 219860e on kwirk:multiaction into bddbf1e on fail2ban:0.9.

@yarikoptic
Copy link
Member

Thanks @kwirk for the explanation. Things become complicated ;)

But ok -- I think it is indeed logical to have it separate -- we just would need to document it now since otherwise noone would have an idea about its existence ;-)

  • man/jail.conf.5
  • config/jail.conf -- some example jail ... probably the same asterisk could be a good one to adjust (since that is what "caused" it all)

@kwirk
Copy link
Contributor Author

kwirk commented Apr 29, 2013

@yarikoptic Commits added. Hopefully my man page syntax is correct and I've put in in the correct location...

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling aab9df9 on kwirk:multiaction into bddbf1e on fail2ban:0.9.

logpath = /var/log/asterisk/messages
maxretry = 10
# Astrix requires both tcp and udp
action = %(banaction)s[name=%(__name__)s-tcp, port="%(port)s", protocol="tcp", chain="%(chain)s", actname=%(banaction)s-tcp]
%(banaction)s[name=%(__name__)s-udp, port="%(port)s", protocol="udp", chain="%(chain)s", actname=%(banaction)s-udp]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which shows that in most (all?) cases we still need different name's and specification of identical to them actname is somewhat redundant... isn't it? may be if actname is not specified it could take the value of name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... In the case of iptables I think you're right that name would do, but if anything, this is a less typical case. I would say the most common use case would be the sending of multiple emails. On both iptables and sendmail, generally the name tag is being used to represent the jail name.
Also, when querying with fail2ban-client to get a list of actions, I think the action being named with the "bandaction" (e.g. iptables-tcp and iptables-udp) is clearer.

yarikoptic added a commit that referenced this pull request Apr 30, 2013
Allow creation of multiple of the same action for a filter -- use actname option. Close #37
@yarikoptic yarikoptic merged commit e019ab7 into fail2ban:0.9 Apr 30, 2013
@@ -65,6 +65,9 @@ def __init__(self, jail):
# @param name The action name

def addAction(self, name):
# Check is action name already exists
if name in [action.getName() for action in self.__actions]:
raise ValueError("Action %s already exists" % name)
action = Action(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have merged but then (while adding to the changelog) thought -- so actname seems to be just an internal beastie, not really of interest to user... and there is even this check.... having an explicit actname probably a good thing, but I still think it should the client which takes care about disambiguating actions with the same name (e.g. adding "-number" suffix) while reading the jail definition.
what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Makes things also easier.
I've had a shot at it, but wonder if there is a neater approach?
https://gist.github.com/kwirk/5491782

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

Successfully merging this pull request may close these issues.

None yet

4 participants