WPB-11455 Allow List for 401 rate limiting #24
Conversation
echoes-hq
bot
added
the
echoes/initiative: federation-wire-cloud
| return 0; | ||
| } | ||
| /* Init allow_list map if needed */ | ||
| if (rate_limit_allowlist_map == NULL) { |
There was a problem hiding this comment.
I believe multiple threads can end up calling this code. I'm thinking this entire method (from line 76 down) should be under the rate_limit_allowlist_mutex - since it's currently possible for the list to change at runtime (if the file is updated). If we do that, then we can remove the mutex from ratelimit_init_allowlist_map. Or perhaps we can keep the filesystem checks out of the mutex to reduce the lock time.
There was a problem hiding this comment.
I'm of two minds of which way to go. In the last push I went ahead and split the locks up over the different parts that can conflict (the init, the clear/re-init, and put method of the map).
Let me know if you think it might be better for one big lock. I also question if the global for "modified time/mtime" should be in the lock as well.
Let me know your thoughts.
There was a problem hiding this comment.
We need to check if it's thread safe to iterate (ie: get operation) the list while it might be modified. I'll try to look at that later this week. :)
There was a problem hiding this comment.
@e-lisa I did some digging into the coturn code and from what I can tell, it is NOT thread safe to call ur_addr_map_get while another thread is calling ur_addr_map_put or other ur_addr_map API's. Therefor I think you will need to implement a single global lock.
| @@ -87,7 +174,6 @@ int ratelimit_is_address_limited(ioa_addr *address, int max_requests, int window | |||
| addr_set_port(address_new, 0); | |||
|
|
|||
| if (ur_addr_map_get(rate_limit_map, address_new, &ratelimit_ptr)) { | |||
There was a problem hiding this comment.
Sorry - this point is more related to the previous PR on rate limit. It seems we are doing a ur_addp_map_get without a mutex. I see you call rate_limit_add_node under the mutex.
There was a problem hiding this comment.
If that is true, perhaps we don't need the entry mutex - we can just keep the main mutex locked for all adjustments.
There was a problem hiding this comment.
correct, we shouldn't need it on get, only the put
There was a problem hiding this comment.
I'm not sure that is true, I would have thought any iterating of the list while it is being written too would be dangerous. I will try to take some more time to review the ur_addr_map code to confirm this.
There was a problem hiding this comment.
@e-lisa I did some digging into the coturn code and from what I can tell, it is NOT thread safe to call ur_addr_map_get while another thread is calling ur_addr_map_put or other ur_addr_map API's. Therefor I think you will need to implement a single global lock.
|
It would be good to add all your new command line / configuration options to the sample turnserver.conf file: |
Added a new `--401-allowlist` to allow adding a path of an allow-list of IPv4/IPv6 addresses that can bypass the 401 rate-limit settings. This can also be set in the configuration file via `401-allowlist=.` The allow-list is updated when the allowlist file is updated during runtime. The allow-list format is one (1) IP address per line.
added |
|
@e-lisa If you can please avoid squashing the commits (at least for now), it will be easier to see what has been changed from the commits that result from review comments. :) |
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Allowlist for the 401 rate-limit features.
Added a new
--401-allowlistto allow adding a path of an allow-list ofIPv4/IPv6 addresses that can bypass the 401 rate-limit settings. This
can also be set in the configuration file via
401-allowlist=.The allow-list is updated when the allowlist file is updated during
runtime.
The allow-list format is one (1) IP address per line.
Issues
An allow-list to bypass the 401 rate-limit feature is required for federated services and other high volume IP addresses.
Solutions
A new option
--401-allowlist=and configuration file setting401-allowlist=have been added to allow specifying a file to contain the allow list. The allow list is updated every time the allow list file is updated based on its mtime (modification time).The allow list is formatted with one IPv4/IPv6 address per line as seen in this example allow list:
Needs releases with:
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.