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

Scheduler rate limiting #1413

Closed
wants to merge 53 commits into from
Closed

Scheduler rate limiting #1413

wants to merge 53 commits into from

Conversation

jpbruinsslot
Copy link
Contributor

@jpbruinsslot jpbruinsslot commented Jul 18, 2023 •
edited
Loading

Changes

This PR adds rate-limiting capabilities to the scheduler for tasks. Boefjes that are subject to rate limiting (defined in the boefje manifest) are being tracked by an in-memory rate limiter leveraging the package limits. When a task is subject to a rate-limit it will get the DELAYED status and will be postponed until it is allowed again. A thread will check for delayed tasks and put them on the queue when they're ready.

This will expect some changes in the katalogus mainly exposing a grouping/namespacing on which to rate limit and the parseable rate limit.

Issue link

Closes #1317

@jpbruinsslot jpbruinsslot self-assigned this Jul 18, 2023
@jpbruinsslot jpbruinsslot added the mula Issues related to the scheduler label Jul 18, 2023
@jpbruinsslot jpbruinsslot linked an issue Jul 18, 2023 that may be closed by this pull request
[mula] Rate limiting by interval and grouper as set in boefje manifest. #1317
Open
@jpbruinsslot
Leverage tasks instead of in memory datastructure to store delayed tasks
10a7f9f
@jpbruinsslot
Fix consuming of the rate limiter
0b552a0
@jpbruinsslot
Add optional consume parameter
fda6991
@jpbruinsslot
Fix rate limit tests
8ec4416
@jpbruinsslot
Fix formatting and type hint suggestions
f5a71ea
@jpbruinsslot
Debugging and writing tests for edge cases
90786b9
@jpbruinsslot
Add order_by argument for task store
2c4a1c2
@jpbruinsslot
Add migration
46c1e6e
@jpbruinsslot
Fix migration
83e8f30
@jpbruinsslot
Merge branch 'main' into feature/mula/rate-limit
f70c8fb 
* main:
  Fix robot test (#1420)
  Use the correct clearance level variable in organization member list template (#1427)
  Fix translation in Debian package (#1432)
  Reschedule tasks when no results in bytes are found after grace period (#1410)
  Don't scan hostname nmap in nmap boefje (#1415)
  Add and use our own CVE API (#1383)
  Add `task_id` as a query parameter to the `GET /origins` endpoint (#1414)
  Remove member group checks and check for permission instead (#1275)
  Bump cryptography from 41.0.0 to 41.0.2 in /boefjes/boefjes/plugins/kat_ssl_certificates (#1396)
  Bump cryptography from 41.0.1 to 41.0.2 in /bytes (#1397)
  Build the Debian build image on the main branch (#1387)
  Add explicit `black` config to all modules (#1395)
  Fix <no title> in the user guide docs (#1391)
  Add configurable octpoes request timeout (#1382)
  Remove hardcoded clearance level in member list for superusers (#1390)
  Add Debian build depends for CVE API package (#1384)
  Add buttons to manual rerun tasks, both boefjes or normalizers (#1339)
  Use fix multiprocessing bug on macOS where `qsize()` is not implemented (#1374)
@jpbruinsslot
Fix tests
7f31f24
@jpbruinsslot
Update requirements.txt
7b162ac
@jpbruinsslot
Update tests timeout
8485870
@jpbruinsslot
Finaly made the tests work
fc17de7
@jpbruinsslot
Fixes for precommit
d659a16
@jpbruinsslot
Update tests
81223a8
@jpbruinsslot
Implement lock
fe1197d
@jpbruinsslot
Remove comment
1362292
@jpbruinsslot jpbruinsslot mentioned this pull request Jul 25, 2023
Open
praseodym
praseodym reviewed Aug 9, 2023
View reviewed changes
Copy link
Contributor

@praseodym praseodym left a comment

Choose a reason for hiding this comment

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

As far as I understand, the rate limiter is a property of the BoefjeScheduler class. This class has a separate rate_limiter instance per organisation, which means that rate limiting only works per organisation.

In larger deployments, API keys such as for Shodan will likely be shared by multiple KAT organisations. How do we implement rate limiting for that scenario?

ammar92 and others added 11 commits August 14, 2023 14:28
@ammar92 @jpbruinsslot
Fix rstcheck hook (#1584)
d055d80 
Signed-off-by: ammar <[email protected]>
@TwistMeister @Darwinkel @jpbruinsslot
OOI Detail page: Remember page position after clicking the "show inhe…
ae268a3 
…ritance" link (#1590)

Co-authored-by: Patrick <[email protected]>
@jpbruinsslot @Darwinkel
Upgrade FastAPI (#1576)
5777629 
Signed-off-by: ammar <[email protected]>
Co-authored-by: Patrick Darwinkel <[email protected]>
Co-authored-by: Patrick <[email protected]>
@praseodym @Darwinkel
@dekkers @jpbruinsslot
Fix human-readable name for ImageMetadata (#1558)
93a8f1b 
Co-authored-by: Patrick <[email protected]>
Co-authored-by: Jeroen Dekkers <[email protected]>
@noamblitz @dekkers @jpbruinsslot
fix many ports open normalizer (#1592)
1ea9154 
Co-authored-by: Jeroen Dekkers <[email protected]>
@dekkers @praseodym @jpbruinsslot
Configure github actions in dependabot (#1594)
c31e6a8 
Co-authored-by: Mark Janssen <[email protected]>
@dekkers @ammar92 @jpbruinsslot
Run docker-compose pull in make pull (#1585)
7c97142 
Co-authored-by: ammar92 <[email protected]>
@dependabot @dekkers @jpbruinsslot
Bump actions/checkout from 1 to 3 (#1598)
0751d10 
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jeroen Dekkers <[email protected]>
@ring-ring-ring @RobboRob
@Darwinkel @underdarknl @dekkers @jpbruinsslot
Add community install/update scripts (#1309)
eb3779f 
Co-authored-by: Robjen <[email protected]>
Co-authored-by: Patrick Darwinkel <[email protected]>
Co-authored-by: Patrick <[email protected]>
Co-authored-by: Jan Klopper <[email protected]>
Co-authored-by: Jeroen Dekkers <[email protected]>
@jpbruinsslot @ring-ring-ring
@ammar92 @Darwinkel @underdarknl
Update scheduler documentation (#1476)
5fc6107 
Co-authored-by: ring-ring-ring <[email protected]>
Co-authored-by: ammar92 <[email protected]>
Co-authored-by: Patrick <[email protected]>
Co-authored-by: Jan Klopper <[email protected]>
@weblate @Darwinkel
@underdarknl @jpbruinsslot
Translations update from Hosted Weblate (#1604)
681d6de 
Co-authored-by: Patrick <[email protected]>
Co-authored-by: Jan Klopper <[email protected]>
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
7 out of 8 committers have signed the CLA.

✅ jpbruinsslot
✅ praseodym
✅ TwistMeister
✅ ammar92
✅ noamblitz
✅ ring-ring-ring
✅ dekkers
❌ weblate
You have signed the CLA already but the status is still pending? Let us recheck it.

@jpbruinsslot
Copy link
Contributor Author

A merge main and rebase resulted in an unfortunate branch state. Until this is solved, in the meantime changes will be made on the following PR: #1611

@jpbruinsslot
Copy link
Contributor Author

Please refer to #1611

@praseodym praseodym mentioned this pull request Aug 17, 2023
Draft
@jpbruinsslot jpbruinsslot removed a link to an issue Jun 4, 2024
[mula] Rate limiting by interval and grouper as set in boefje manifest. #1317
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@praseodym praseodym praseodym left review comments

Assignees

@jpbruinsslot jpbruinsslot

Labels
mula Issues related to the scheduler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jpbruinsslot @CLAassistant @praseodym @ammar92 @TwistMeister @noamblitz @dekkers @ring-ring-ring @weblate