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.

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

Open
wants to merge 1 commit into
base: wireapp/4.6.2-federation
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.turnserver
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,8 @@ Options with values:
Defaults is 1000.
--401-window=<seconds> Set the time window duration in seconds for rate limiting 401 Unauthorized responses.
Defaults is 120.
--401-allowlist=<filename> Set the path of the allow-list, one IP per line allowed to bypass the 401 rate-limit settings.
Default is none.


==================================
Expand Down
17 changes: 17 additions & 0 deletions examples/etc/turnserver.conf
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,23 @@
#no-tlsv1_1
#no-tlsv1_2


# Set the maximum number of 401 Unauthorized responses allowed per rate-limiting
# window. When set to 0 disalbes 401 rate-limiting.
#
#401-req-limit=1000

# Set the time window duration in seconds for rate limiting 401 Unauthorized
# responses.
#
#401-window=120

# Set the path of the allow-list, one IP per line allowed to bypass the 401
# rate-limit settings. By default this is disabled. Uncomment to enable use
# of the 401 rate-limit allow list:
#
#401-allowlist=/etc/coturn/allowlist.txt

# Disable RFC5780 (NAT behavior discovery).
#
# Originally, if there are more than one listener address from the same
Expand Down
14 changes: 11 additions & 3 deletions src/apps/relay/mainrelay.c
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,8 @@ turn_params_t turn_params = {

///////// Ratelimt /////////
RATELIMIT_DEFAULT_MAX_REQUESTS_PER_WINDOW, /* 401-req-limit */
RATELIMIT_DEFAULT_WINDOW_SECS /* 401-window */
RATELIMIT_DEFAULT_WINDOW_SECS, /* 401-window */
NULL /* 401-allowlist */
};

//////////////// OpenSSL Init //////////////////////
Expand Down Expand Up @@ -1311,6 +1312,8 @@ static char Usage[] =
" per rate-limiting window. If set to 0 disables rate limiting. Default is 1000.\n"
" --401-window=<seconds>\t\t\t\tSet the time window duration in seconds for rate limiting 401 Unauthorized responses.\n"
" Default is 120.\n"
" --401-allowlist=<filename>\t\t\tSet the path of the allow-list, one IP per line allowed to bypass the 401\n"
" rate-limit settings. Default is none.\n"
" --version Print version (and exit).\n"
" -h Help\n"
"\n";
Expand Down Expand Up @@ -1476,9 +1479,9 @@ enum EXTRA_OPTS {
FEDERATION_PKEY_OPT,
FEDERATION_PKEY_PWD_OPT,
FEDERATION_REMOTE_WHITELIST_OPT,
RATELIMIT_OPT,
RATELIMIT_REQUESTS_OPT,
RATELIMIT_WINDOW_OPT
RATELIMIT_WINDOW_OPT,
RATELIMIT_ALLOWLIST_OPT
};

struct myoption {
Expand Down Expand Up @@ -1633,6 +1636,7 @@ static const struct myoption long_options[] = {
{"syslog-facility", required_argument, NULL, SYSLOG_FACILITY_OPT},
{"401-req-limit", optional_argument, NULL, RATELIMIT_REQUESTS_OPT},
{"401-window", optional_argument, NULL, RATELIMIT_WINDOW_OPT},
{"401-allowlist", optional_argument, NULL, RATELIMIT_ALLOWLIST_OPT},
{NULL, no_argument, NULL, 0}};

static const struct myoption admin_long_options[] = {
Expand Down Expand Up @@ -2397,6 +2401,10 @@ static void set_option(int c, char *value) {
turn_params.ratelimit_401_window_seconds = get_int_value(value, RATELIMIT_DEFAULT_WINDOW_SECS);
TURN_LOG_FUNC(TURN_LOG_LEVEL_INFO, "Setting 401 ratelimit window to: %i seconds\n", turn_params.ratelimit_401_window_seconds);
break;
case RATELIMIT_ALLOWLIST_OPT:
STRCPY(turn_params.ratelimit_401_allowlist, value);
TURN_LOG_FUNC(TURN_LOG_LEVEL_INFO, "Setting 401 ratelimit allow list to: %s\n", turn_params.ratelimit_401_allowlist);
break;
/* these options have been already taken care of before: */
case 'l':
case NO_STDOUT_LOG_OPT:
Expand Down
1 change: 1 addition & 0 deletions src/apps/relay/mainrelay.h
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,7 @@ typedef struct _turn_params_ {

vint ratelimit_401_requests_per_window;
vint ratelimit_401_window_seconds;
char ratelimit_401_allowlist[1025];
} turn_params_t;

extern turn_params_t turn_params;
Expand Down
3 changes: 2 additions & 1 deletion src/apps/relay/netengine.c
Original file line number Diff line number Diff line change
Expand Up @@ -1696,7 +1696,8 @@ static void setup_relay_server(struct relay_server *rs, ioa_engine_handle e, int
turn_params.oauth_server_name, turn_params.acme_redirect,
turn_params.allocation_default_address_family, &turn_params.log_binding,
&turn_params.no_stun_backward_compatibility, &turn_params.response_origin_only_with_rfc5780,
&turn_params.ratelimit_401_requests_per_window, &turn_params.ratelimit_401_window_seconds);
&turn_params.ratelimit_401_requests_per_window, &turn_params.ratelimit_401_window_seconds,
&turn_params.ratelimit_401_allowlist);


// Intentionally performed outside init_turn_server to help avoid future merge conflicts
Expand Down
102 changes: 100 additions & 2 deletions src/server/ns_turn_ratelimit.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Collaborator

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. :)

Copy link
Collaborator

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.

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
Expand All @@ -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();

Expand All @@ -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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

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

correct, we shouldn't need it on get, only the put

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

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.

free(address_new);
ratelimit_entry *rateLimitEntry = (ratelimit_entry *)(void *)(ur_map_value_type)ratelimit_ptr;
TURN_MUTEX_LOCK(&(rateLimitEntry->mutex));

Expand All @@ -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++;
Expand Down
3 changes: 2 additions & 1 deletion src/server/ns_turn_ratelimit.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@
extern "C" {
#endif

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);
void ratelimit_add_node(ioa_addr *address);
int ratelimit_delete_expired(ur_map_value_type value);
void ratelimit_init_map(void);
Expand Down
7 changes: 5 additions & 2 deletions src/server/ns_turn_server.c
Original file line number Diff line number Diff line change
Expand Up @@ -3921,7 +3921,8 @@ static int handle_turn_command(turn_turnserver *server, ts_ur_super_session *ss,
}
if(err_code == 401 && *server->ratelimit_401_requests_per_window > 0) {
ioa_addr *rate_limit_address = get_remote_addr_from_ioa_socket(ss->client_socket);
if (ratelimit_is_address_limited(rate_limit_address, *server->ratelimit_401_requests_per_window, *server->ratelimit_401_window_seconds)) {
if (ratelimit_is_address_limited(rate_limit_address, *server->ratelimit_401_requests_per_window,
*server->ratelimit_401_window_seconds, server->ratelimit_401_allowlist)) {
no_response = 1;
char raddr[129];
addr_to_string_no_port(rate_limit_address, (unsigned char *)raddr);
Expand Down Expand Up @@ -4929,7 +4930,8 @@ void init_turn_server(turn_turnserver *server, turnserver_id id, int verbose, io
const char *acme_redirect, ALLOCATION_DEFAULT_ADDRESS_FAMILY allocation_default_address_family,
vintp log_binding, vintp no_stun_backward_compatibility,
vintp response_origin_only_with_rfc5780,
vintp ratelimit_401_requests_per_window, vintp ratelimit_401_window_seconds) {
vintp ratelimit_401_requests_per_window, vintp ratelimit_401_window_seconds,
const char *ratelimit_401_allowlist) {

if (!server)
return;
Expand Down Expand Up @@ -5011,6 +5013,7 @@ void init_turn_server(turn_turnserver *server, turnserver_id id, int verbose, io
server->is_draining = 0;
server->ratelimit_401_requests_per_window = ratelimit_401_requests_per_window;
server->ratelimit_401_window_seconds = ratelimit_401_window_seconds;
server->ratelimit_401_allowlist = ratelimit_401_allowlist;
}

ioa_engine_handle turn_server_get_engine(turn_turnserver *s) {
Expand Down
4 changes: 3 additions & 1 deletion src/server/ns_turn_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ struct _turn_turnserver {

vintp ratelimit_401_requests_per_window;
vintp ratelimit_401_window_seconds;
const char *ratelimit_401_allowlist;
};

const char *get_version(turn_turnserver *server);
Expand All @@ -234,7 +235,8 @@ void init_turn_server(turn_turnserver *server, turnserver_id id, int verbose, io
allocate_bps_cb allocate_bps_func, int oauth, const char *oauth_server_name,
const char *acme_redirect, ALLOCATION_DEFAULT_ADDRESS_FAMILY allocation_default_address_family,
vintp log_binding, vintp no_stun_backward_compatibility, vintp response_origin_only_with_rfc5780,
vintp ratelimit_401_requests_per_window, vintp ratelimit_401_window_seconds);
vintp ratelimit_401_requests_per_window, vintp ratelimit_401_window_seconds,
const char *ratelimit_401_allowlist);

ioa_engine_handle turn_server_get_engine(turn_turnserver *s);

Expand Down