-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
WPB-11455 Allow List for 401 rate limiting #24
base: wireapp/4.6.2-federation
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,15 +28,104 @@ | |
* SUCH DAMAGE. | ||
*/ | ||
|
||
#include <sys/stat.h> | ||
|
||
#include "ns_turn_maps.h" | ||
#include "ns_turn_ioalib.h" | ||
#include "ns_turn_ratelimit.h" | ||
|
||
/////////////////// rate limit ////////////////////////// | ||
|
||
ur_addr_map *rate_limit_map = NULL; | ||
ur_addr_map *rate_limit_allowlist_map = NULL; | ||
|
||
int ratelimit_window_secs = RATELIMIT_DEFAULT_WINDOW_SECS; | ||
time_t last_mtime = 0; | ||
|
||
TURN_MUTEX_DECLARE(rate_limit_main_mutex); | ||
TURN_MUTEX_DECLARE(rate_limit_allowlist_mutex); | ||
|
||
void ratelimit_remove_newlines(char *str) { | ||
char *src = str; | ||
char *dst = str; | ||
|
||
while (*src != '\0') { | ||
if (*src != '\r' && *src != '\n') { | ||
*dst++ = *src; | ||
} | ||
src++; | ||
} | ||
*dst = '\0'; | ||
} | ||
|
||
void ratelimit_init_allowlist_map() { | ||
TURN_MUTEX_INIT(&rate_limit_allowlist_mutex); | ||
TURN_MUTEX_LOCK(&rate_limit_allowlist_mutex); | ||
|
||
rate_limit_allowlist_map = (ur_addr_map*)malloc(sizeof(ur_addr_map)); | ||
ur_addr_map_init(rate_limit_allowlist_map); | ||
|
||
TURN_MUTEX_UNLOCK(&rate_limit_allowlist_mutex); | ||
} | ||
|
||
int ratelimit_is_on_allowlist(const char *allowlist, ioa_addr *addr) { | ||
/* If no allowlist provided, return early */ | ||
if (!allowlist) { | ||
return 0; | ||
} | ||
/* Init allow_list map if needed */ | ||
if (rate_limit_allowlist_map == NULL) { | ||
ratelimit_init_allowlist_map(); | ||
} | ||
|
||
/* Check the mtime of the allow list, do we need to update? */ | ||
struct stat fstat; | ||
if (stat(allowlist, &fstat) == 0) { | ||
lwille marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (fstat.st_mtime != last_mtime) { | ||
last_mtime = fstat.st_mtime; | ||
|
||
FILE *file = NULL; | ||
file = fopen(allowlist, "r"); | ||
if (file != NULL) { | ||
char line[1024]; | ||
|
||
/* Rebuild map */ | ||
TURN_MUTEX_LOCK(&rate_limit_allowlist_mutex); | ||
ur_addr_map_clean(rate_limit_allowlist_map); | ||
ur_addr_map_init(rate_limit_allowlist_map); | ||
TURN_MUTEX_UNLOCK(&rate_limit_allowlist_mutex); | ||
/* loop over file and add entries */ | ||
while (fgets(line, sizeof(line) - 1, file) != NULL) { | ||
if (!line) { | ||
break; | ||
} | ||
|
||
ioa_addr new_address; | ||
ratelimit_remove_newlines(line); | ||
if(make_ioa_addr_from_full_string((const uint8_t *)line, 0, &new_address) != 0) { | ||
TURN_LOG_FUNC(TURN_LOG_LEVEL_ERROR, "Malformed address in 401 ratelimit allow list file %s: %s\n", allowlist, line); | ||
} else { | ||
TURN_MUTEX_LOCK(&rate_limit_allowlist_mutex); | ||
ur_addr_map_put(rate_limit_allowlist_map, &new_address, (ur_addr_map_value_type)1); | ||
TURN_MUTEX_UNLOCK(&rate_limit_allowlist_mutex); | ||
} | ||
} | ||
if (file) { | ||
fclose(file); | ||
} | ||
} else { | ||
TURN_LOG_FUNC(TURN_LOG_LEVEL_ERROR, "Could not open 401 ratelimit allow list file: %s\n", allowlist); | ||
} | ||
} | ||
} | ||
|
||
ur_addr_map_value_type ratelimit_ptr = 0; | ||
if (ur_addr_map_get(rate_limit_allowlist_map, addr, &ratelimit_ptr)) { | ||
return 1; | ||
} else { | ||
return 0; | ||
} | ||
} | ||
|
||
void ratelimit_add_node(ioa_addr *address) { | ||
// copy address | ||
|
@@ -63,7 +152,7 @@ void ratelimit_init_map() { | |
TURN_MUTEX_UNLOCK(&rate_limit_main_mutex); | ||
} | ||
|
||
int ratelimit_is_address_limited(ioa_addr *address, int max_requests, int window_seconds) { | ||
int ratelimit_is_address_limited(ioa_addr *address, int max_requests, int window_seconds, const char *allowlist) { | ||
/* Housekeeping, prune the map when ADDR_MAP_SIZE is hit and delete expired items */ | ||
turn_time_t current_time = turn_time(); | ||
|
||
|
@@ -87,7 +176,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct, we shouldn't need it on get, only the put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
free(address_new); | ||
ratelimit_entry *rateLimitEntry = (ratelimit_entry *)(void *)(ur_map_value_type)ratelimit_ptr; | ||
TURN_MUTEX_LOCK(&(rateLimitEntry->mutex)); | ||
|
||
|
@@ -96,15 +184,25 @@ int ratelimit_is_address_limited(ioa_addr *address, int max_requests, int window | |
rateLimitEntry->request_count = 1; | ||
rateLimitEntry->last_request_time = current_time; | ||
TURN_MUTEX_UNLOCK(&(rateLimitEntry->mutex)); | ||
free(address_new); | ||
return 0; | ||
} else if (rateLimitEntry->request_count < max_requests) { | ||
/* Check if request count is below requests per window; increment the count */ | ||
if (rateLimitEntry->request_count < UINT32_MAX) | ||
rateLimitEntry->request_count++; | ||
rateLimitEntry->last_request_time = current_time; | ||
TURN_MUTEX_UNLOCK(&(rateLimitEntry->mutex)); | ||
free(address_new); | ||
return 0; | ||
} else { | ||
/* Before ratelimit, check allow list */ | ||
if(ratelimit_is_on_allowlist(allowlist, address_new)) { | ||
sgodin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
free(address_new); | ||
TURN_MUTEX_UNLOCK(&(rateLimitEntry->mutex)); | ||
return 0; | ||
} | ||
|
||
free(address_new); | ||
/* Request is outside of defined window and count, request is ratelimited */ | ||
if (rateLimitEntry->request_count < UINT32_MAX) | ||
rateLimitEntry->request_count++; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.