Advanced Ip filtering #4424

vkorn · 2016-11-16T21:28:24Z

Description:
Implementation of automatic IP ban in case of multiple failed login attempts. Partially related to this request.

File ip_bans.yaml automatically created in config folder.

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

Example entry for configuration.yaml (if applicable):
Two new params in http component:

http:
  ip_ban_enabled: True
  login_attempts_threshold: 2

Checklist:

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

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

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.

mention-bot · 2016-11-16T21:28:25Z

@vkorn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @mweinelt and @JshWright to be potential reviewers.

SEJeff · 2016-11-19T04:01:21Z

homeassistant/components/emulated_hue.py

+        trusted_networks=[],
+        ip_bans=[],
+        login_threshold=0,
+        is_ban_enabled=False


I realize that there are already mutable types in the kwargs in this function, but that in general is bad python and causes some gnarly bugs. Here is an example of why doing this is a very bad idea:

[jeff@omniscience ~]$ python3 Python 3.5.2 (default, Oct 14 2016, 12:54:53) [GCC 6.2.1 20160916 (Red Hat 6.2.1-2)] on linux Type "help", "copyright", "credits" or "license" for more information. >>> def append_one(data=[]): ... data.append(1) ... return data ... >>> append_one() [1] >>> append_one() [1, 1] >>> append_one() [1, 1, 1] >>> append_one(data=[2]) [2, 1] >>> append_one() [1, 1, 1, 1] >>> append_one() [1, 1, 1, 1, 1] >>>

Since those variables such as trusted_networks or ip_bans are initialized (at module load time) with mutable types, the values stay as though they're globals along the life of the process. This means it stays the life of the entire hass daemon unless those kwargs are passed at function call time.

In theory setup() should only run once, but even still if possible can you not do this? I think setting them to None and then checking in the body of the function, while more verbose, will prevent an entire class of bugs related to object mutability in the future.

@SEJeff, fixed in recent commit, please review.

SEJeff · 2016-11-21T04:02:40Z

👍 from me. @balloob ?

Danielhiversen · 2016-11-21T12:29:44Z

Nice feature.
Is the ip_ban_enabled parameter necessary?
login_attempts_threshold < 1 -> disable ip ban functionality?

vkorn · 2016-11-22T05:18:15Z

@Danielhiversen, ip_ban enabled responsible for rejecting all request from ip_bans.yaml file.
If login_attempts_threshold >= 1 it will automatically ban any new failed IPs.

balloob · 2016-11-23T07:10:16Z

homeassistant/components/http.py

@@ -386,6 +411,38 @@ def is_trusted_ip(self, remote_addr):
        return any(ip_address(remote_addr) in trusted_network
                   for trusted_network in self.hass.http.trusted_networks)

+    def wrong_login_attempt(self, remote_addr):
+        """Registering wrong login attempt."""
+        if not self.is_trusted_ip(remote_addr) \


Consider rewriting this with guard clauses. Every time you know you will never do anything besides the if statement, including not using an else statement, you can just reverse the if and return.

if self.is_trusted_ip(remote_addr): return etc…

Would a trusted ip ever be able to get into the wrong_login_attempt method ?

Yes, you're correct, trusted IP get authenticated = True -- removing this check

balloob · 2016-11-23T07:11:58Z

homeassistant/components/http.py

+
+    try:
+        list_ = load_yaml_config_file(path)
+        for ip_ban in list_:


You can validate a list like this:

vol.Schema([vol.Schema({…})])

balloob · 2016-11-23T07:12:24Z

homeassistant/components/http.py

+                ban = ip_schema(list_[ip_ban])
+                ban['ip_ban'] = ip_address(ip_ban)
+                ip_list.append(IpBan(**ban))
+            except vol.Invalid:


You should report if something is invalid or no one will know

balloob · 2016-11-23T07:12:43Z

homeassistant/components/http.py

+                continue
+
+    except(HomeAssistantError, FileNotFoundError):
+        return []


You should report when a HomeAssistantError occurs

Ok, adding check prior to this. For FileNotFount it doesn't make sense to report -- it just means that no bans have being applied

balloob · 2016-11-23T07:14:16Z

homeassistant/helpers/config_validation.py

+    """Validate timestamp."""
+    if isinstance(value, date) or isinstance(value, datetime):
+        return value
+    return datetime.strptime(value, "%Y-%m-%dT%H:%M:%S")


We should only support isoformat and the inverse dt_util.parse_datetime(…)

balloob · 2016-11-23T07:14:30Z

homeassistant/helpers/config_validation.py

@@ -297,6 +297,13 @@ def time(value):
    return time_val


+def exact_time(value):
+    """Validate timestamp."""
+    if isinstance(value, date) or isinstance(value, datetime):


When would this happen?

For example if there's no quotes on value:

banned_at: 2016-11-23T18:50:33

Could be reason of manual adding. Probably I should remove check for date instance and leave only datetime, otherwise validation is misleading

balloob · 2016-11-23T07:14:53Z

homeassistant/helpers/config_validation.py

@@ -297,6 +297,13 @@ def time(value):
    return time_val


+def exact_time(value):


datetime is the correct description

balloob · 2016-11-23T07:16:14Z

homeassistant/components/http.py

+                                remote_addr)
+                persistent_notification.async_create(
+                    self.hass,
+                    'To many login attempts from {}'.format(remote_addr),


bagobones · 2016-11-24T20:50:22Z

I just setup external access to my home assistant and exempted my home network.. I was thinking of setting up fail2ban but a native solution sounds great.

Two question:

Is this change compatible with the X-forwarded for option?
I think there should be a parameter / default expiry of banned IPs.. From experience a lot of late nite scans happen from many random cloud hosted IPs. I am afraid that without an automatic clean up the IP ban file could get very very large.

vkorn · 2016-11-24T21:22:18Z

@bagobones,

Yes
Didn't think about it, basically haven't notice such problems yet. Can implement something after this PR will be merged as another PR.

bagobones · 2016-11-24T21:31:22Z

@vkorn

Sounds great... Home assistant uses a web form for authentication but it probably wont be too long before a bot starts filling it in and failing.. working with fail2ban and other IP ban scripts the cleanup eventually becomes important.

Another reason as well is to not permanently lock your self out, some systems have a soft ban and a hard ban... IE such as X consecutive attempts results in a non-permanent 5-10min ban, however if there are additional attempts it moves into a permanent / long term ban.

You may also want an IP ban whitelist for users that have the API password enabled at all times, as they may locally lock them self out otherwise.

vkorn added 3 commits November 16, 2016 12:45

Added IP Bans configuration

dcf1fb9

Fixing warnings

579b15d

Added ban enabled option and unit tests

47715be

vkorn mentioned this pull request Nov 16, 2016

IP Bans configuration home-assistant/home-assistant.io#1450

Merged

Fixed py34 tox

4465ef9

SEJeff suggested changes Nov 19, 2016

View reviewed changes

http: requested changes fix

fa6f24d

SEJeff approved these changes Nov 21, 2016

View reviewed changes

balloob reviewed Nov 23, 2016

View reviewed changes

Requested changes fix

b669770

Merge branch 'dev' into pr/4424

dfa3115

balloob merged commit 2a7bc0e into home-assistant:dev Nov 25, 2016

home-assistant locked and limited conversation to collaborators Mar 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced Ip filtering #4424

Advanced Ip filtering #4424

vkorn commented Nov 16, 2016

mention-bot commented Nov 16, 2016

SEJeff Nov 19, 2016 •

edited

vkorn Nov 20, 2016

SEJeff commented Nov 21, 2016

Danielhiversen commented Nov 21, 2016 •

edited

vkorn commented Nov 22, 2016

balloob Nov 23, 2016

balloob Nov 23, 2016

vkorn Nov 23, 2016

balloob Nov 23, 2016

balloob Nov 23, 2016

balloob Nov 23, 2016

vkorn Nov 23, 2016

balloob Nov 23, 2016

balloob Nov 23, 2016

vkorn Nov 23, 2016 •

edited

balloob Nov 23, 2016

balloob Nov 23, 2016

bagobones commented Nov 24, 2016

vkorn commented Nov 24, 2016

bagobones commented Nov 24, 2016

		@@ -297,6 +297,13 @@ def time(value):
		return time_val


		def exact_time(value):

Advanced Ip filtering #4424

Advanced Ip filtering #4424

Conversation

vkorn commented Nov 16, 2016

mention-bot commented Nov 16, 2016

SEJeff Nov 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEJeff commented Nov 21, 2016

Danielhiversen commented Nov 21, 2016 • edited

vkorn commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkorn Nov 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagobones commented Nov 24, 2016

vkorn commented Nov 24, 2016

bagobones commented Nov 24, 2016

SEJeff Nov 19, 2016 •

edited

Danielhiversen commented Nov 21, 2016 •

edited

vkorn Nov 23, 2016 •

edited