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

Add fail2ban sensor #9975

Merged
merged 11 commits into from
Oct 23, 2017
Merged

Add fail2ban sensor #9975

merged 11 commits into from
Oct 23, 2017

Conversation

fronzbot
Copy link
Contributor

@fronzbot fronzbot commented Oct 19, 2017

Description:

I was looking for an easy way to display active and historical bans on my system within home-assistant, and decided to add this platform as a solution. Please let me know if you think there is a better way to implement this.

What it does

Each 'jail' set up within fail2ban can be added as a sensor within homeassistant. This component will access the fail2ban log (which you provide via the config) and scan it for any IPs that have been banned within each specified jail. Any IP that is found will be added to a list within sensor.state_attributes to keep a running tab on each IP currently banned. Each uniquely banned IP will also be added to a permanent list to keep track of all IPs banned for that jail, up to a total of 10 (any more and the oldest banned IP is removed, to prevent the list from becoming too long). The state displayed within home-assistant is the most recently banned IP for the given jail.

Tested platforms

I've tested this with the following setups:

  • Custom homeassistant filter (using this guide) running on Debian 9
  • Same filter running in a homeassistant docker on an unRAID server behind nginx

For both those platforms, fail2ban still needs to be manually configured to work correctly. For the docker in unRAID behind nginx, there were some extra steps needed but it's working great for me right now. I plan on adding the latter to the documentation when I get to it.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#3679

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: fail2ban
    name: fail2ban
    jails:
      - hass-iptables
      - ssh
    file_path: /var/log/fail2ban.log
    scan_interval: 120

Checklist:

If user exposed functionality or configuration variables are added/changed:

  • Documentation added/updated in home-assistant.github.io
  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

Please use regex, that will be faster than you complex parse.

@fronzbot
Copy link
Contributor Author

fronzbot commented Oct 19, 2017

@pvizeli I don't believe that's true Source. I'm also not entirely certain of which part of the parse you're referring to, though. The only parse I do is check if a string exists within a line. Could you elaborate on your request a bit?

I had thought about using regex initially, but given the linked thread and the hit to readability, I don't think it is the correct choice here.

EDIT - maybe I see where you're thinking there can be improvement: lines 97 to 114? Simplify that if/else junk to a regex?

@fronzbot
Copy link
Contributor Author

@pvizeli I removed the for loop from the BanLogParser.read_log function, as that seemed like the best place for optimization via regex. This simplified the update function's if/else logic a bit, as well. I'm not sure if this is what you meant, but it seemed like the only place a regex would make sense. Let me know your opinion.

@fronzbot
Copy link
Contributor Author

fronzbot commented Oct 20, 2017

Ok, now I'm not sure that regex was the right thing. I ran a benchmark for the regex in that latest commit vs the following type of code:

# read example
with open('somefile', 'r') as fh:
    for line in fh.readlines():
        # Do something

Here's the regex example:

# regex example
with open('somefile', 'r') as fh:
    x = re.findall(r'some_regex', fh.read())

And the read snippet is significantly faster than the regex. Maybe my regex isn't the best implementation, but I cannot see where a regex would be faster than the original implementation. Please let me know how you'd like me to proceed with this PR.

Here are the results (read_times label corresponds to the for loop snippet):
image

- also was missing return False in timer for default behavior
@pvizeli
Copy link
Member

pvizeli commented Oct 20, 2017

That had a simple reason, you don't optimize the regex with compile.
Do compile all regex on startup and you are 200% faster

vol.All(cv.ensure_list, vol.Length(min=1)),
vol.Optional(CONF_FILE_PATH, default=DEFAULT_LOG): cv.isfile,
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_SCAN_INTERVAL, default=DEFAULT_SCAN_INTERVAL):
Copy link
Member

Choose a reason for hiding this comment

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

That is already inside PLATFORM_SCHEMA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok. I figured I needed to overload it to set the default scan interval, so is there a different way to do that? Or should I just remove the default and force the user to decide how often to open the fail2ban log for reading?

Copy link
Member

Choose a reason for hiding this comment

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

Set SCAN_INTERVAL on top of you platform and you have set the default interval

@fronzbot
Copy link
Contributor Author

Oh, that makes total sense now. Thanks for that information 👍

- renamed DEFAULT_SCAN_INTERVAL to SCAN_INTERVAL
@@ -26,7 +26,7 @@

DEFAULT_NAME = 'fail2ban'
DEFAULT_LOG = '/var/log/fail2ban.log'
DEFAULT_SCAN_INTERVAL = 120
SCAN_INTERVAL = 120
Copy link
Member

Choose a reason for hiding this comment

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

timedelta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, whoops. Fixed.

try:
with open(self.log_file, 'r', encoding='utf-8') as file_data:
self.data = re.findall(
r'\[' + re.escape(jail) + r'\].(Ban|Unban) ([\w+\.]{3,})',
Copy link
Member

Choose a reason for hiding this comment

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

Compile the regex into a dict and use the jail as key. Use also this syntax: r"...{}...".format()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like the following?

self.data[jail] = re.findall(r"...{}...".format(jail), ...)

Copy link
Member

Choose a reason for hiding this comment

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

no, like self.xy[jail] = re.compile(r"...{}...".format(jail))

@fronzbot
Copy link
Contributor Author

@pvizeli OK, I think I implemented what you requested. Let me know if there are any more changes you'd like to see

@pvizeli pvizeli merged commit f9d89a0 into home-assistant:dev Oct 23, 2017
@balloob balloob mentioned this pull request Nov 2, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants