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.

Sign up for GitHub

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

Jump to bottom

[ENG-774] A Naive Solution for Waterbutler API Rate-limiting with Redis #380

Merged
merged 17 commits into from
Oct 1, 2019
Merged

[ENG-774] A Naive Solution for Waterbutler API Rate-limiting with Redis #380

merged 17 commits into from
Oct 1, 2019

Conversation

cslzchen
Copy link
Contributor

@cslzchen cslzchen commented Aug 5, 2019 •
edited

Ticket

https://openscience.atlassian.net/browse/ENG-774

Related Tickets

The parent EPIC ticket: https://openscience.atlassian.net/browse/ENG-451
The previous Analysis / POC ticket: https://openscience.atlassian.net/browse/ENG-468
The next ticket: https://openscience.atlassian.net/browse/ENG-775

Purpose

Add an naive solution to rate-limit Waterbutler API V1 using Redis.

Here is the OSF side support for local development: OSF-PR#9199.

DevOps tasks are out of scope for now.

Changes

Preparation

  • Rate-limiting algorithm: fixed Window.

    • Here is an article that explains different rate-limiting algorithms as well as their pros/cons.
    • This same algorithm is used by GitHub, as mentioned in its API Overview -> Rate Limiting.

  • Maintaining state: using a Redis server

What and How To Rate Limit

  • Window size is 1 hour and the limit is 3600 requests
  • Rate-limit applies to requests based on token auth, basic auth and no auth
  • Rate-limit does not apply to cookie auth requests (which are sent by the OSF API server)
  • Redis keys (token, cookie and base64 encoded username/password and IP address) are hashed for better security.

What's not included in this naive solution

  • What will WB do if Redis is down or Redis call returns an exception?
    • Naive: WB catches it and returns a 503
  • Applies stricter limits to no-auth and less strict ones to token and basic auth
    • Naive: same limit for all
  • Return X-WB-RL headers for every request so benign API users can back-off
    • Naive: only when a request hits the limit
  • Remove INFO level log
    • Naive: INFO level log are kept for convenience

Side effects

Please refer to Changes

Dev / QA Notes

  • assert needQa is False
  • The flag is turned off by default to make Travis happy

Deployment Notes

  • Merge into a feature branch instead of develop or next release
  • This feature should not go to production

@cslzchen cslzchen changed the title [WIP] Waterbutler API rate-limiting with Redis [Ticket-TBD] [WIP] Waterbutler API rate-limiting with Redis [ENG-451] [ ENG-468] Aug 6, 2019
@cslzchen cslzchen mentioned this pull request Aug 6, 2019
Closed
@cslzchen cslzchen force-pushed the feature/wb-rate-limiting-with-redis branch 3 times, most recently from b7b4325 to 3b555f0 Compare August 12, 2019 20:45
@cslzchen cslzchen changed the title [WIP] Waterbutler API rate-limiting with Redis [ENG-451] [ ENG-468] A Naive Solution for Waterbutler API Rate-limiting with Redis [ENG-774] Aug 15, 2019
@cslzchen cslzchen changed the title A Naive Solution for Waterbutler API Rate-limiting with Redis [ENG-774] [ENG-774] A Naive Solution for Waterbutler API Rate-limiting with Redis Aug 16, 2019
@cslzchen cslzchen mentioned this pull request Aug 21, 2019
Closed
3 tasks
@cslzchen cslzchen force-pushed the feature/wb-rate-limiting-with-redis branch from 3b555f0 to 2219ae1 Compare August 23, 2019 18:56
@coveralls
Copy link

coveralls commented Aug 23, 2019 •
edited

Coverage Status

Coverage decreased (-0.6%) to 91.268% when pulling e58a737 on cslzchen:feature/wb-rate-limiting-with-redis into 903f845 on CenterForOpenScience:feature/wb-rate-limiting.

@cslzchen cslzchen requested a review from felliott August 23, 2019 19:59
felliott and others added 17 commits September 4, 2019 15:08
@felliott
mark meta & revision requests as such
1f8a019 
 * Update the OSF auth handler to properly categorize metadata and
   revision requests as "metadata" and "revisions" actions in the
   request to the OSF.  They had been categorized as "download"
   actions by virtue of being GET requests.  HEAD requests were
   correctly categorized as "metadata" actions.

   The OSF relies on this action property to determine whether it
   should tally a download or view for a given file.
@felliott
Merge branch 'feature/auth-action-corrections' into develop
49269f0 
 [ENG-600]
@felliott
bump version & update changelog
c6911c1
@felliott
Merge branch 'release/19.1.0'
e44951a
@felliott
Merge tag '19.1.0' into develop
a3b7a0a
@cslzchen @felliott
A trivial implementaion of WB rate-limiting with redis
594d654 
This is a trivial but proof-of-concept implementation with the fixed
window algorithm. Users are identified by IP address only. WB allows
3600 requests per hour. Locally, the redis server runs with default
image and in default mode via OSF docker-compose locally.
@cslzchen @felliott
Implement a naive limiter that identifies different auth creds
1bbf4c3 
Rewrote the trivial rate limiter into a naive one that recognizes
different types of auth and rate-limits each type separately. The
4 types are OSF cookie, OAuth bearer token, basic auth w/ base64-
encoded username/password and w/o any of the 3 auth options.
@cslzchen @felliott
Use the "fixed window" algorithm and make parameters WB settings
8e8c164
@cslzchen @felliott
Add obfuscation to redis keys for rate-limiting
f4b176b 
OSF cookie, OAuth access token and base64-encoded username/password
are used as redis keys during rate-limiting. We must obfuscate them
for the same reason we store only password hashes in database. The
`hashlib.hash256().hexdigest()` is used in this case. In addition,
a prefix is added to the digest to identifty which type it is. The
"No Auth" case is hashed as well (though unnecessarily) so that the
keys all have the same look and length.
@cslzchen @felliott
Auth by OSF cookie now bypasses the rate limiter
44874d1
@cslzchen @felliott
Remove duplicate dev-requirements for redis
53b3f5e
@cslzchen @felliott
Return the Retry-After header in the 429 response if the limit is hit
dacd3f3 
- Use Redis TTL command to obtain the time until reset.
- Refactored `.rate_limit()` signature to return a tuple informing
  the handler whehter the limit is active and details of the limit.
- Use tornado request handler's `set_header()` and `write()` to set
  the "Retry-After" header and the response body.
@cslzchen @felliott
Remove redundant Redis .get() for key existence check
9181921 
In Redis, the INCR command increments the number stored at key by
1. If the key does not exist, it is set to 0 before performing the
operation. The python redis implementation makes this even simpler
by setting the value to 1 without doing the increment.
@cslzchen @felliott
The rate limiter now throws 503 if WB Redis fails
7a91a32
@cslzchen @felliott
Add X-WB-RL headers to responses for blocked requests
96169d7
@cslzchen @felliott
Disable WB API rate-limiting by default
3b1d3f3
@felliott
grab-bag of wb-rl updates
e58a737 
 * Bump redis dep to 3.3.8.  No consequential changes.

 * Update exceptions to survive unpickling, add to unpickling test.

 * Fix capitalization of WaterButler in exception class name.

 * Don't throw errors in the error handling: provide a fallback for
   the `resource` attribute if rate-limiting kicks in before that has
   been initialized.

 * Update some docstrings to clarify return values and process.

 * Refactor test rate-limiting auth testing to only extract data as
   needed.

 * Add docs to settings; use `config.get_bool()` on booleans.
@felliott felliott force-pushed the feature/wb-rate-limiting-with-redis branch from 9985713 to e58a737 Compare October 1, 2019 17:00
@felliott felliott changed the base branch from develop to feature/wb-rate-limiting October 1, 2019 17:02
@felliott
Copy link
Member

felliott commented Oct 1, 2019

LGTM! Added some minor changes / docs / refactors, but nothing earth-shattering. Shall merge now!

felliott added a commit that referenced this pull request Oct 1, 2019
@felliott
Merge branch 'feature/wb-rate-limiting-with-redis' into feature/wb-ra…
385133b 
…te-limiting

 [ENG-774]
 Closes: #380
@felliott felliott merged commit e58a737 into CenterForOpenScience:feature/wb-rate-limiting Oct 1, 2019
@felliott felliott mentioned this pull request Jun 29, 2021
Open
@cslzchen cslzchen deleted the feature/wb-rate-limiting-with-redis branch September 8, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felliott felliott Awaiting requested review from felliott

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cslzchen @coveralls @felliott