feat: Add rate limiter #1976

bolkedebruin · 2023-01-09T11:12:18Z

Description

This allows regulation of the number of times a user can request a particular API or view given a time-frame. It protects against brute-forcing entry and denial of service.

Both Airflow and Superset have a long standing issue of not being able to rate limit access to APIs and views allowing for brute forcing access.

apache/superset#10560
https://lists.apache.org/thread/hdpyr3y5cq0djcogn1ql7gj7vq09xo0j
apache/superset#10620

This change allows users to decorate their APIs and Views with @limit.

Note I have turned to default to be ON for authentication for AuthDB and AuthLDAP (Oauth/oidc have their own protection mechanisms) as FAB should be secure out of the box I assume.

cc @potiuk

ADDITIONAL INFORMATION

codecov · 2023-01-09T11:24:51Z

Codecov Report

Merging #1976 (ea19c87) into master (57e9d10) will increase coverage by 0.16%.
The diff coverage is 98.57%.

❗ Current head ea19c87 differs from pull request most recent head 7723ff1. Consider uploading reports for the commit 7723ff1 to get more accurate results

@@            Coverage Diff             @@
##           master    #1976      +/-   ##
==========================================
+ Coverage   78.10%   78.27%   +0.16%     
==========================================
  Files          71       72       +1     
  Lines        8611     8667      +56     
==========================================
+ Hits         6726     6784      +58     
+ Misses       1885     1883       -2

Flag	Coverage Δ
python	`78.27% <98.57%> (+0.16%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
flask_appbuilder/security/views.py	`62.63% <50.00%> (ø)`
flask_appbuilder/api/__init__.py	`96.34% <100.00%> (+0.03%)`	⬆️
flask_appbuilder/base.py	`84.85% <100.00%> (+0.27%)`	⬆️
flask_appbuilder/baseviews.py	`89.90% <100.00%> (+0.25%)`	⬆️
flask_appbuilder/security/decorators.py	`94.68% <100.00%> (+0.77%)`	⬆️
flask_appbuilder/security/manager.py	`75.64% <100.00%> (+0.56%)`	⬆️
flask_appbuilder/utils/limit.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dpgaspar

This is great! left some non blocking comments. Can you please revisit?

flask_appbuilder/security/manager.py

flask_appbuilder/security/decorators.py

flask_appbuilder/security/manager.py

This allows regulation of the number of times a user can request a particular API or view given a time-frame. It protects against brute-forcing entry and denial of service.

bolkedebruin · 2023-02-16T12:04:53Z

@dpgaspar ptl, i have addressed your comments and fixed the linting (can I suggest using a newer flake8, the one used here is really really old).

bolkedebruin · 2023-02-17T12:34:20Z

Hi @dpgaspar PR is finished. Would be great if it was included in 4.2.1 :-)

dpgaspar · 2023-02-17T14:20:34Z

sorry, it won't, but I'll make a 4.3.0 next week just for this, no worries

bolkedebruin · 2023-02-17T14:40:53Z

awesome, that'll work :-)

dpgaspar · 2023-02-17T16:07:49Z

flask_appbuilder/security/manager.py

@@ -255,6 +257,10 @@ def __init__(self, appbuilder):
            app.config.setdefault("AUTH_LDAP_LASTNAME_FIELD", "sn")
            app.config.setdefault("AUTH_LDAP_EMAIL_FIELD", "mail")

+        # Rate limiting
+        app.config.setdefault("AUTH_RATE_LIMITED", True)
+        app.config.setdefault("AUTH_RATE_LIMIT", "2 per 5 second")


This seems a bit agressive, what do you think about keeping the same rate but increasing the number of attempts needed. Example: "10 per 25 second"?

I don't think it's aggressive. A human being will have trouble meeting this if they have to retype the password and otherwise a short pause won't hurt. An automated system configured wrongly will remain wrong.

So if you realize "omg ive typed in the password/username twice" you probably start thinking a bit longer and cross the 5s barrier (try it!) and if you do make it a slight pause won't hurt IMHO.

An alternative is to redirect to a captcha when reaching the limit but I'll leave that to someone else.

Anyways I'm not married to this limit but I do not think it's too aggressive.

I was able to reach it pretty easily and manually

Easily as in normal human behavior or as in trying to trigger it? Triple entry isn't common.

But if you think it's better we can switch.

Trying to trigger it, but If you agree, I think it would be more conservative and still safe against brute force and DoS

FAB 4.3.0 added rate limiting and we would like to upgrade to this version. This requires to bring some of the changes from the PRs merged in Flask App Builder: * dpgaspar/Flask-AppBuilder#1976 * dpgaspar/Flask-AppBuilder#1997 * dpgaspar/Flask-AppBuilder#1999 While Flask App Builder disabled rate limitig by default, Airlfow is "end product" using it and we decided to enable it.

FAB 4.3.0 added rate limiting and we would like to upgrade to this version. This requires to bring some of the changes from the PRs merged in Flask App Builder: * dpgaspar/Flask-AppBuilder#1976 * dpgaspar/Flask-AppBuilder#1997 * dpgaspar/Flask-AppBuilder#1999 While Flask App Builder disabled rate limitig by default, Airlfow is "end product" using it and we decided to enable it. GitOrigin-RevId: b7f05008ae45fc208456f7f64c19d08ad1cf7313

bolkedebruin mentioned this pull request Jan 9, 2023

Cannot integrate with Flask-Application builder as viewfunc(endpoint) != "{obj.__module__}.{obj.__name__}" alisaifee/flask-limiter#377

Closed

bolkedebruin force-pushed the master branch from aae57a6 to a2060c6 Compare January 9, 2023 11:27

dpgaspar reviewed Feb 15, 2023

View reviewed changes

bolkedebruin added 5 commits February 16, 2023 10:51

Add rate limiter

6e4e99e

This allows regulation of the number of times a user can request a particular API or view given a time-frame. It protects against brute-forcing entry and denial of service.

Lint

82b5809

sigh conflicting linters

7a1ef04

further linting

418ff08

Address comments

c357412

bolkedebruin force-pushed the master branch from 2e15de9 to c357412 Compare February 16, 2023 09:53

bolkedebruin added 4 commits February 16, 2023 10:58

ParamSpec is only available > 3.8

537a7e8

Use latest flask-limiter

55998b8

black

f170dd4

old style flake8

2952544

Fix timeline when inserting

ea19c87

dpgaspar reviewed Feb 17, 2023

View reviewed changes

dpgaspar added 3 commits February 22, 2023 13:18

Merge branch 'master' into master

2fe7f08

Merge branch 'master' into master

5228392

Merge branch 'master' into master

7723ff1

dpgaspar approved these changes Feb 23, 2023

View reviewed changes

dpgaspar merged commit bef2421 into dpgaspar:master Feb 23, 2023

dpgaspar mentioned this pull request Feb 24, 2023

fix: bump FAB to 4.3.0 apache/superset#23184

Merged

9 tasks

potiuk mentioned this pull request Feb 25, 2023

Upgrade FAB to 4.3.0 apache/airflow#29766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add rate limiter #1976

feat: Add rate limiter #1976

bolkedebruin commented Jan 9, 2023 •

edited

Loading

codecov bot commented Jan 9, 2023 •

edited

Loading

dpgaspar left a comment

bolkedebruin commented Feb 16, 2023

bolkedebruin commented Feb 17, 2023

dpgaspar commented Feb 17, 2023

bolkedebruin commented Feb 17, 2023

dpgaspar Feb 17, 2023

bolkedebruin Feb 17, 2023

dpgaspar Feb 17, 2023

bolkedebruin Feb 17, 2023

dpgaspar Feb 22, 2023

feat: Add rate limiter #1976

feat: Add rate limiter #1976

Conversation

bolkedebruin commented Jan 9, 2023 • edited Loading

Description

ADDITIONAL INFORMATION

codecov bot commented Jan 9, 2023 • edited Loading

Codecov Report

dpgaspar left a comment

Choose a reason for hiding this comment

bolkedebruin commented Feb 16, 2023

bolkedebruin commented Feb 17, 2023

dpgaspar commented Feb 17, 2023

bolkedebruin commented Feb 17, 2023

dpgaspar Feb 17, 2023

Choose a reason for hiding this comment

bolkedebruin Feb 17, 2023

Choose a reason for hiding this comment

dpgaspar Feb 17, 2023

Choose a reason for hiding this comment

bolkedebruin Feb 17, 2023

Choose a reason for hiding this comment

dpgaspar Feb 22, 2023

Choose a reason for hiding this comment

bolkedebruin commented Jan 9, 2023 •

edited

Loading

codecov bot commented Jan 9, 2023 •

edited

Loading