[REF] auth_brute_force: Cover all auth entrypoints #1219

yajo · 2018-04-12T08:23:42Z

To fix #1125 I needed to refactor the addon.

The fix is affected by odoo/odoo#24183 and will not work until it gets fixed upstream due to the technical limitations implied. ~~This PR is implemented assuming odoo/odoo#24187 will be merged and backported.~~

@Tecnativa

yajo · 2018-04-17T06:49:52Z

According to odoo/odoo#24187 (comment), I monkey-patched Odoo to get this safely working. As you might know, monkey patches affect all databases in the same instance, which can be bad in a SaaS environment. However, the patch is completely harmless in such case.

pedrobaeza · 2018-04-17T08:15:49Z

Using _register_hook y _apply_patch you do it only for the current DB.

yajo · 2018-04-18T11:33:42Z

Yikes, tricky tests! It's all ✔️ now, ready to merge IMHO.

rafaelbn

Yes! Very important in terms of GDPR 👍 :

chienandalu

With users behind the same IP (or bad configured proxies): if a user gets banned, would it put the other's sessions (already logged) down?

yajo · 2018-04-27T08:19:39Z

@chienandalu Those already logged in would retain their sessions, although new login would get locked, yes.

This is a side effect of this addon, docummented in Known Issues. Do you think there would be a better approach on the matter?

chienandalu

No, it's ok like that :)

yajo · 2018-04-27T08:38:42Z

I'm gonna think some more on the subject. It could be really problematic. Imagine:

One office with 1 single public IP
200 users in that office
At the same time, 5 of them fail passwords or users twice
Nobody else from the office will be able to log in

So, the problem is quite big. I think it should be blocked just by user. Then maybe a notification could be sent to him.

LeartS

Just code review, great code!

If someone tested this functionally, considering also the unittest, for me it's ok to merge.

LeartS · 2018-04-27T08:41:47Z

auth_brute_force/models/res_authentication_attempt.py

        last_ok = self.search(
-            [('result', '=', 'successfull'), ('remote', '=', remote)],
+            [('result', '=', 'successfull'), ('remote', '=', remote),
+             ("attempt_date", "<=", until)],


Maybe at least some of the fields used in this search should be setted as index=True ? On a medium/big installation running for years there could be potentially thousands (or even tens or hundreds of thousands) of login attempts to search in..

In the other hand, I don't see any interest to keep attempts with a date > X monthes. So maybe a cron to clean this table should be great. (I think there is a generic module that does that task for any table in server-tools.)

But Indeed, set an index=True is a good idea !

legalsylvain · 2018-04-27T09:00:22Z

So, the problem is quite big. I think it should be blocked just by user. Then maybe a notification could be sent to him.

I fully understand your use case. But if what you say is implemented, it should be optional. When I wrote this module, it was not designed to block user that doesn't remember their password, but to prevent attack and the only thing we have is the IP. (it's not perfect, it doesn't prevent Distributed attack, but it's a first step).

If it is only blocked by user and not by IP (as you suggest), it will generate the following use case :

I want to attack a company that use odoo.
I know that the login is made with name@mycompany.com
I try to log in with all the login I know. (easy social attack).
Result : I can block all the login because my IP is not banned.

I see the implementation of the "block by login" as a big regression.

Solution 0 (no dev)

set a max_try to 30 (for exemple)
improve password quality (to avoid attack by dictionnary)

-> I don't see any users trying to log in 30 times. After some tries, he will contact the administrator.

solution 1

add a option "block by login" and / or "block by IP". The admin user can choose the policy, that depends on the network implementation of Odoo.

solution 2

set hard_max_try to 10 (for exemple)
add a soft_max_try to 5
make a improvment (by JS) to block user to attempt to login after 5 tries during 10 minutes, then during 1 hours, etc...

-> only one browser is blocked.

maybe quite hard to develop

solution 3

set hard_max_try to 10 (for exemple)
add a soft_max_try to 5
make a improvment (by JS) to block users to log after 5 tries and redirect the user to a page to reinit the password, via a form. (classical token process).

what do you think ?

kind regards.

LeartS · 2018-04-27T09:01:56Z

Yes, that could be a problem. Adding the user on login attempts is in my opinion an ok solution; it would just mean that a potential attacker could try 10 passwords per user instead of 10 in general; which shouldn't be a problem.

LeartS · 2018-04-27T09:03:58Z

If it is only blocked by user and not by IP (as you suggest)

I think @yajo meant to block just login attempts for that specific user to that IP, not to block the user in general. So the IP is still considered, and you can't use this to block access to odoo (unless you have the same IP as the real user, but in that case it's an internal problem and can't really be avoided..)

yajo · 2018-04-27T09:10:29Z

Yep, I think I'm gonna go that path: just block the user+IP combination instead of just the IP.

legalsylvain · 2018-04-27T09:10:47Z

I think @yajo meant to block just login attempts for that specific user to that IP, not to block the user in general. So the IP is still considered.

How ! I didn't understood ! Yes indeed. An option still is a good implementation. like

block_user_policy :

option 1 : block the IP ;
option 2 : block the IP for the current login only.

it would just mean that a potential attacker could try 10 passwords per user instead of 10 in general; which shouldn't be a problem.

It can be a problem. if you have a company with 1000 users, you let an attacker to make an attack with up to 10.000 attempts. he has more chance to succeed, statisticaly.

LeartS · 2018-04-27T09:21:52Z

It can be a problem. if you have a company with 1000 users, you let an attacker to make an attack with up to 10.000 attempts. he has more chance to succeed, statisticaly.

Yes, 10000 in total, but still 10 per user. But I get what you mean: if an attacker tries the 10 most common passwords, the probability of at least 1 user in 1000 using it is much higher than the probability of a single specific user using it.
Maybe we could solve it this way, with two config values:

Block remote-user after x invalid attempts (the param already implemented)
Block remote generally after y invalid attempts

So, for example, block ip 1.1.1.1 for user 'admin' after 5 incorrect attempts for user 'admin', and block 1.1.1.1 in general after 50 consecutive invalid attempts between all users.
Setting x = y would mean to always block the IP, setting y to -1 could mean to never block the entire IP.

LeartS · 2018-04-27T09:25:28Z

@yajo I was thinking it could be useful to save and show the login attempt channel (web - xmlrpc -jsonrpc). But it's ok to do it in a future PR as to not overload this one.

yajo · 2018-04-27T09:28:48Z

You'd be interested in v12 approach, currently in RD phase: odoo/odoo#22212

yajo · 2018-05-09T09:24:58Z

OK Travis is ✔️ now. I digged a lot and couldn't discover the conflict, so I just triggered separate test suites.

Shall we squash+merge now?

rafaelbn · 2018-05-09T11:34:35Z

We need here the last approval of a PSC 👍

pedrobaeza · 2018-05-09T11:39:50Z

auth_brute_force/README.rst

-* If some users are behind the same Internet Service Provider, if a user is
-  banned, all the other users will be banned too;
+
+  * If the Odoo server is behind an Apache / NGinx proxy and it is not properly


Incorrect indentation

It's because it's a sub-point for previous point.

pedrobaeza · 2018-05-09T11:42:34Z

auth_brute_force/data/ir_config_parameter.xml

-    <data>
-
-        <record id="max_attempt_qty" model="ir.config_parameter">
-            <field name="key">auth_brute_force.max_attempt_qty</field>


How do we control this now? No change in README?

I updated the README.

naglis

@yajo thank you so much for the effort, can't wait to test it out!

Added my 2¢, hopefully some of it will be helpful.

naglis · 2018-05-09T09:30:54Z

auth_brute_force/models/res_authentication_attempt.py

+            try:
+                res = json.loads(urlopen(url, timeout=5).read())
+            except Exception:
+                _logger.error(


I think you can use _logger.exception instead of _logger.error with exc_info=True.

I didn't want to give so much importance to this failure, which is not such a big problem. In fact, I think I'm gonna lower it to a warning instead 🤔

naglis · 2018-05-09T09:34:05Z

auth_brute_force/models/res_authentication_attempt.py

+            ("remote", "=", remote),
+        ]
+        last_ok = self.search(domain_good, order='create_date desc', limit=1)
+        recent_failures = self.search(


Nitpick: you may also use the .search_count() shortcut.

naglis · 2018-05-09T09:36:45Z

auth_brute_force/models/res_users.py

+    _inherit = "res.users"
+
+    # HACK https://github.com/odoo/odoo/issues/24183
+    # TODO Remvove when fixed, and use normal odoo.http.request to get details


Typo: Remvove

naglis · 2018-05-09T09:40:47Z

auth_brute_force/models/res_users.py

+        """Update a given auth attempt if we still ignore its result."""
+        auth_id = getattr(current_thread(), "auth_attempt_id", False)
+        if not auth_id:
+            return dict()  # No running auth attempt; nothing to do


Nitpick: {} is faster

naglis · 2018-05-09T09:43:10Z

auth_brute_force/tests/test_brute_force.py

+        # Some tests could retain environ from last test and produce fake
+        # results without this patch
+        # HACK https://github.com/odoo/odoo/issues/24183
+        # TODO Remvove when fixed


Typo: Remvove.

naglis · 2018-05-09T09:58:40Z

auth_brute_force/views/view.xml

+    <record id="view_res_authentication_attempt_tree" model="ir.ui.view">
+        <field name="model">res.authentication.attempt</field>
+        <field name="arch" type="xml">
+            <tree colors="orange: result == 'failed';red: result == 'banned'">


Nitpick: colors attribute has been deprecated since v9.

naglis · 2018-05-09T10:49:42Z

auth_brute_force/models/res_authentication_attempt.py

+                    exc_info=True,
+                )
+            else:
+                item.metadata = "\n".join(


Shouldn't it be remote_metadata?

naglis · 2018-05-09T11:52:53Z

auth_brute_force/models/res_authentication_attempt.py

+from odoo import api, fields, models
+from odoo.osv.expression import TRUE_LEAF
+
+GEOLOCALISATION_URL = u"http://ip-api.com/json/{}"


Not sure about pros/cons of existing online service alternatives, but maybe it would be better to use a service which has HTTPS in their free plan, eg. https://ipapi.co/json?

Also, I have tested http://ip-api.com/json/ locally and after a couple of hundreds of requests I got banned (I guess timeout based), so in a real world case I believe this could potentially break if the limit is reached within the time window, resulting in empty metadata?

IIUC this would also be stored upon successful logins? In that case, each new res.authentication.attempt record could potentially take ~5 seconds? What if we try to fill the metadata asynchronously afterwards, eg. with Cron or connector? This way you could also group records by remote address and reduce the number of requests in this way (now it is repeated for the same IP).

Also, I am wondering why an external service is used here instead of eg. GeoIP?

Hi. I implemented the call of ip-api some years ago, but I did'nt made a benchmark about all the solutions, so maybe it's not the best solution.
Fill the metadata via a cron could be a great improvment indeed. And using GeoIP too ! (but maybe not the objective of this PR).

I agree with both: this should be improved, but this PR targets other purpose. I added it to known issues.

naglis · 2018-05-09T11:55:15Z

auth_brute_force/tests/test_brute_force.py

+                ),
+            )
+        # Attempts recorded
+        with self.cursor() as cr:


Readabillty: These test cases seem quite repetitive, maybe it would be possible to factor out the common parts to reduce code repetition?

Possibly, but the little details make it quite difficult to refactor, and after all repeating code in test cases is not so bad as in other parts of the code.

naglis · 2018-05-09T11:58:02Z

auth_brute_force/models/res_authentication_attempt.py

+                login,
+            )
+            return False
+        # Check if remote + login combination is banned


Readability: The # Check if remote is banned and # Check if remote + login combination is banned parts share much of the code, might be worth factoring out into separate helper function.

MiquelRForgeFlow · 2018-05-09T12:14:51Z

auth_brute_force/__manifest__.py

    'author': "GRAP, "
              "Tecnativa, "
              "Odoo Community Association (OCA)",
-    'website': 'http://www.grap.coop',
+    'website': 'https://github.com/OCA/server-auth',


But this PR is in serve-tools 🤔

MiquelRForgeFlow · 2018-05-09T12:16:00Z

auth_brute_force/README.rst

@@ -69,14 +69,15 @@ For further information, please visit:
 Known issues / Roadmap
 ======================

-* The ID used to identify a remote request is the IP provided in the request
-  (key 'REMOTE_ADDR').
+* Remove 🐒 patch for https://github.com/odoo/odoo/issues/24183 when fixed.


This issue was closed. Solved for v12

rafaelbn · 2018-05-10T17:42:55Z

@yajo which is the state of this? OCA need this great work!

yajo · 2018-05-17T08:49:19Z

Updated with all remarks attended.

…1219) To fix OCA/server-tools#1125 I needed to refactor the addon. To whitelist IPs now you use a config parameter, which renders res.banned.remote model unneeded. The fix is affected by odoo/odoo#24183 and will not work until it gets fixed upstream due to the technical limitations implied. [FIX] auth_brute_force: Small typos (OCA/server-tools#1250) - The `whitelisted` field needs to exist in view to be usable. - The correct class is `decoration-danger` for tree views.

…1219) To fix OCA/server-tools#1125 I needed to refactor the addon. To whitelist IPs now you use a config parameter, which renders res.banned.remote model unneeded. The fix is affected by odoo/odoo#24183 and will not work until it gets fixed upstream due to the technical limitations implied. [FIX] auth_brute_force: Small typos (OCA/server-tools#1250) - The `whitelisted` field needs to exist in view to be usable. - The correct class is `decoration-danger` for tree views. [FIX] auth_brute_force: Fix addon requirement computation (OCA/server-tools#1251) Include HACK for odoo/odoo#24833, which explains the false positive problem we were having here: an addon being importable doesn't mean it is installed.

…1219) To fix OCA/server-tools#1125 I needed to refactor the addon. To whitelist IPs now you use a config parameter, which renders res.banned.remote model unneeded. The fix is affected by odoo/odoo#24183 and will not work until it gets fixed upstream due to the technical limitations implied.

To fix OCA#1125 I needed to refactor the addon. To whitelist IPs now you use a config parameter, which renders res.banned.remote model unneeded. The fix is affected by odoo/odoo#24183 and will not work until it gets fixed upstream due to the technical limitations implied.

* '10.0' of https://github.com/OCA/server-tools: (47 commits) [UPD] addons table in README.md [ci skip] [UPD] README.rst module_auto_update: readme fragments module_auto_update: sbidoul as maintainer [FIX] rst syntax errors [10.0][FIX] module_auto_update: Don't set 'to upgrade' on void recordset modules [UPD] addons table in README.md [ci skip] [FIX] auth_brute_force: Avoid storing false login attempts (OCA#1258) [UPD] addons table in README.md [ci skip] [FIX] password_security: Create user as admin in tests [UPD] addons table in README.md [ci skip] [FIX] auth_brute_force: Fix addon requirement computation (OCA#1251) [UPD] addons table in README.md [ci skip] [FIX] auth_brute_force: Small typos [UPD] addons table in README.md [ci skip] [REF] auth_brute_force: Cover all auth entrypoints (OCA#1219) [UPD] addons table in README.md [ci skip] module_auto_update: bump version module_auto_update: check a post condition [UPD] addons table in README.md [ci skip] ...

To fix OCA#1125 I needed to refactor the addon. To whitelist IPs now you use a config parameter, which renders res.banned.remote model unneeded. The fix is affected by odoo/odoo#24183 and will not work until it gets fixed upstream due to the technical limitations implied.

…1219) To fix OCA/server-tools#1125 I needed to refactor the addon. To whitelist IPs now you use a config parameter, which renders res.banned.remote model unneeded. The fix is affected by odoo/odoo#24183 and will not work until it gets fixed upstream due to the technical limitations implied.

yajo self-assigned this Apr 12, 2018

yajo added bug enhancement needs review work in progress and removed needs review labels Apr 12, 2018

yajo added this to the 10.0 milestone Apr 12, 2018

pedrobaeza mentioned this pull request Apr 12, 2018

[IMP] Convert XMLRPC handler to a regular HTTP controller odoo/odoo#24187

Merged

yajo added needs review and removed work in progress labels Apr 17, 2018

yajo requested review from legalsylvain and LeartS April 18, 2018 11:33

yajo mentioned this pull request Apr 27, 2018

auth_brute_force doesn't consider xmlrpc/jsonrpc login attempts #1125

Closed

rafaelbn approved these changes Apr 27, 2018

View reviewed changes

chienandalu reviewed Apr 27, 2018

View reviewed changes

chienandalu approved these changes Apr 27, 2018

View reviewed changes

LeartS approved these changes Apr 27, 2018

View reviewed changes

fixup! [REF] auth_brute_force: Cover all auth entrypoints

e445ffa

pedrobaeza requested changes May 9, 2018

View reviewed changes

naglis approved these changes May 9, 2018

View reviewed changes

MiquelRForgeFlow reviewed May 9, 2018

View reviewed changes

yajo mentioned this pull request May 10, 2018

[11.0][MIG] auth_brute_force #1078

Closed

fixup! [REF] auth_brute_force: Cover all auth entrypoints

cf64c2c

yajo added 2 commits May 17, 2018 12:15

fixup! [REF] auth_brute_force: Cover all auth entrypoints

3d47554

fixup! [REF] auth_brute_force: Cover all auth entrypoints

c8379c4

pedrobaeza approved these changes May 18, 2018

View reviewed changes

yajo merged commit 53e61d3 into OCA:10.0 May 18, 2018

yajo deleted the 10.0-auth_brute_force-rpc_protection branch May 18, 2018 12:42

yajo mentioned this pull request May 21, 2018

[MIG] auth_brute_force: Migrate to v11 OCA/server-auth#19

Merged

[REF] auth_brute_force: Cover all auth entrypoints #1219

[REF] auth_brute_force: Cover all auth entrypoints #1219

Conversation

yajo commented Apr 12, 2018 • edited

yajo commented Apr 17, 2018

pedrobaeza commented Apr 17, 2018

yajo commented Apr 18, 2018

rafaelbn left a comment

Choose a reason for hiding this comment

chienandalu left a comment

Choose a reason for hiding this comment

yajo commented Apr 27, 2018 • edited

chienandalu left a comment

Choose a reason for hiding this comment

yajo commented Apr 27, 2018

LeartS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legalsylvain commented Apr 27, 2018

LeartS commented Apr 27, 2018

LeartS commented Apr 27, 2018 • edited

yajo commented Apr 27, 2018

legalsylvain commented Apr 27, 2018 • edited

LeartS commented Apr 27, 2018 • edited

LeartS commented Apr 27, 2018

yajo commented Apr 27, 2018

yajo commented May 9, 2018

rafaelbn commented May 9, 2018

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

naglis left a 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

Choose a reason for hiding this comment

naglis May 9, 2018 • edited

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

rafaelbn commented May 10, 2018

yajo commented May 17, 2018

yajo commented Apr 12, 2018 •

edited

yajo commented Apr 27, 2018 •

edited

LeartS commented Apr 27, 2018 •

edited

legalsylvain commented Apr 27, 2018 •

edited

LeartS commented Apr 27, 2018 •

edited

naglis May 9, 2018 •

edited