From faf381b5293f226293685d0f14d9150f1185db28 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 14 Aug 2024 17:13:14 +0100 Subject: [PATCH 1/2] Bug 1911202 - Remove support for iprepd from the BMO code base as it is not being used since the move to GCP --- Bugzilla.pm | 31 +--------- Bugzilla/App/Plugin/Login.pm | 6 +- Bugzilla/Auth.pm | 1 - Bugzilla/Auth/Login/APIKey.pm | 4 +- Bugzilla/Auth/Login/CGI.pm | 2 +- Bugzilla/Auth/Login/Cookie.pm | 4 +- Bugzilla/Config/Admin.pm | 12 ---- Bugzilla/MFA/TOTP.pm | 4 +- Bugzilla/Token.pm | 10 ++-- external_test_api.pl | 15 ----- qa/t/3_test_rate_limit.t | 106 +--------------------------------- t/migrate-params.t | 1 - token.cgi | 14 ++--- 13 files changed, 24 insertions(+), 186 deletions(-) diff --git a/Bugzilla.pm b/Bugzilla.pm index 62a0b47a4f..db5f145629 100644 --- a/Bugzilla.pm +++ b/Bugzilla.pm @@ -767,6 +767,7 @@ sub datadog { sub check_rate_limit { my ($class, $name, $identifier, $throw_error) = @_; + $identifier ||= remote_ip(); $throw_error //= sub { ThrowUserError("rate_limit") }; my $params = Bugzilla->params; if ($params->{rate_limit_active}) { @@ -796,36 +797,6 @@ sub check_rate_limit { return 1; } -sub iprepd_report { - my ($class, $name) = @_; - my $params = Bugzilla->params; - - return 0 if !$params->{iprepd_base_url} || !$params->{iprepd_client_secret}; - - # Send information about this event to the iprepd API if active - my $ip = remote_ip(); - my $ua = mojo_user_agent({request_timeout => 5}); - my $payload = {object => $ip, type => "ip", violation => $name}; - - # We should also audit this so it is recorded in the logs - Bugzilla->audit(sprintf 'iprepd: violation %s from ip address %s', $name, $ip); - - try { - my $tx = $ua->put( - $params->{iprepd_base_url} . '/violations/type/ip/' . $ip, - {'Authorization' => 'APIKey ' . $params->{iprepd_client_secret}} => json => - $payload - ); - my $res = $tx->result; - die $res->message . ' ' . $res->body unless $res->is_success; - } - catch { - WARN("IPREPD ERROR: $_"); - }; - - return 1; -} - sub markdown { require Bugzilla::Markdown; state $markdown = Bugzilla::Markdown->new; diff --git a/Bugzilla/App/Plugin/Login.pm b/Bugzilla/App/Plugin/Login.pm index 0d16d149f8..a9f6eb0a90 100644 --- a/Bugzilla/App/Plugin/Login.pm +++ b/Bugzilla/App/Plugin/Login.pm @@ -90,7 +90,7 @@ sub register { || $api_key->revoked ) { - Bugzilla->iprepd_report('bmo.api_key_mismatch'); + Bugzilla->check_rate_limit('api_key_mismatch'); } else { $api_key->update_last_used($remote_ip); @@ -98,7 +98,7 @@ sub register { } } else { - Bugzilla->iprepd_report('bmo.api_key_mismatch'); + Bugzilla->check_rate_limit('api_key_mismatch'); } } @@ -109,7 +109,7 @@ sub register { $user_id = $user->id; } else { - Bugzilla->iprepd_report('bmo.api_key_mismatch'); + Bugzilla->check_rate_limit('api_key_mismatch'); } } } diff --git a/Bugzilla/Auth.pm b/Bugzilla/Auth.pm index f372a8b80d..2a8187617b 100644 --- a/Bugzilla/Auth.pm +++ b/Bugzilla/Auth.pm @@ -234,7 +234,6 @@ sub _handle_login_result { # to find account names by brute force) elsif ($fail_code == AUTH_LOGINFAILED or $fail_code == AUTH_NO_SUCH_USER) { my $remaining_attempts = MAX_LOGIN_ATTEMPTS - ($result->{failure_count} || 0); - Bugzilla->iprepd_report('bmo.username_password_mismatch', remote_ip()); ThrowUserError("invalid_username_or_password", {remaining => $remaining_attempts}); } diff --git a/Bugzilla/Auth/Login/APIKey.pm b/Bugzilla/Auth/Login/APIKey.pm index 06f302f885..6d26ecb9af 100644 --- a/Bugzilla/Auth/Login/APIKey.pm +++ b/Bugzilla/Auth/Login/APIKey.pm @@ -62,14 +62,14 @@ sub get_login_info { if (!$api_key or $api_key->api_key ne $api_key_text) { # The second part checks the correct capitalization. Silly MySQL - Bugzilla->iprepd_report('bmo.api_key_mismatch', $remote_ip); + Bugzilla->check_rate_limit('api_key_mismatch'); ThrowUserError("api_key_not_valid"); } elsif ($api_key->sticky && $api_key->last_used_ip && $api_key->last_used_ip ne $remote_ip) { - Bugzilla->iprepd_report('bmo.api_key_mismatch'); + Bugzilla->check_rate_limit('api_key_mismatch'); ThrowUserError("api_key_not_valid"); } elsif ($api_key->revoked) { diff --git a/Bugzilla/Auth/Login/CGI.pm b/Bugzilla/Auth/Login/CGI.pm index 0cb317b691..fead6743a0 100644 --- a/Bugzilla/Auth/Login/CGI.pm +++ b/Bugzilla/Auth/Login/CGI.pm @@ -53,7 +53,7 @@ sub get_login_info { # If the web browser doesn't accept cookies, we have no way to # make sure that the authentication request comes from the user. elsif ($login && $password) { - Bugzilla->iprepd_report('bmo.token_mismatch'); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError('auth_untrusted_request', {login => $login}); } diff --git a/Bugzilla/Auth/Login/Cookie.pm b/Bugzilla/Auth/Login/Cookie.pm index da73054f61..28ec89f801 100644 --- a/Bugzilla/Auth/Login/Cookie.pm +++ b/Bugzilla/Auth/Login/Cookie.pm @@ -65,7 +65,7 @@ sub get_login_info { || $token_type ne 'api_token' || $user_id != $token_user_id) { - Bugzilla->iprepd_report('bmo.token_mismatch'); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError('auth_invalid_token', {token => $api_token}); } $is_internal = 1; @@ -104,7 +104,7 @@ sub get_login_info { if (i_am_webservice() && !$is_internal) { my $user = Bugzilla::User->new({id => $user_id, cache => 1}); if ($user->settings->{api_key_only}->{value} eq 'on') { - Bugzilla->iprepd_report('bmo.token_mismatch'); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError('invalid_cookies_or_token'); } } diff --git a/Bugzilla/Config/Admin.pm b/Bugzilla/Config/Admin.pm index ac52e59269..0897a6d147 100644 --- a/Bugzilla/Config/Admin.pm +++ b/Bugzilla/Config/Admin.pm @@ -44,18 +44,6 @@ sub get_param_list { updater => \&update_rate_limit_rules, }, - { - name => 'iprepd_base_url', - type => 't', - default => '', - }, - - { - name => 'iprepd_client_secret', - type => 't', - default => '', - }, - {name => 'log_user_requests', type => 'b', default => 0}, {name => 'block_user_agent', type => 't', default => ''}, diff --git a/Bugzilla/MFA/TOTP.pm b/Bugzilla/MFA/TOTP.pm index 0e70d99b39..fa16bdaee2 100644 --- a/Bugzilla/MFA/TOTP.pm +++ b/Bugzilla/MFA/TOTP.pm @@ -85,12 +85,12 @@ sub check { my $code = $params->{code}; return if $self->_auth()->verify($code, 1); - Bugzilla->iprepd_report('bmo.mfa_mismatch', remote_ip()); + Bugzilla->check_rate_limit('mfa_mismatch', $self->{user}->id); + if ($params->{mfa_action} && $params->{mfa_action} eq 'enable') { ThrowUserError('mfa_totp_bad_enrollment_code'); } else { - Bugzilla->check_rate_limit('mfa_mismatch', $self->{user}->id); ThrowUserError('mfa_bad_code'); } } diff --git a/Bugzilla/Token.pm b/Bugzilla/Token.pm index d8f4596e98..91dcc58431 100644 --- a/Bugzilla/Token.pm +++ b/Bugzilla/Token.pm @@ -67,7 +67,7 @@ sub issue_new_user_account_token { my $vars = {}; # Report this event for possible rate limiting - Bugzilla->iprepd_report('bmo.create_account'); + Bugzilla->check_rate_limit('create_account'); # Is there already a pending request for this login name? If yes, do not throw # an error because the user may have lost their email with the token inside. @@ -110,7 +110,7 @@ sub IssueEmailChangeToken { my $old_email = $user->login; # Report this event for possible rate limiting - Bugzilla->iprepd_report('bmo.email_change'); + Bugzilla->check_rate_limit('email_change'); my ($token, $token_ts) = _create_token($user->id, 'emailold', $old_email . ":" . $new_email); @@ -155,7 +155,7 @@ sub IssuePasswordToken { my $dbh = Bugzilla->dbh; # Report this event for possible rate limiting - Bugzilla->iprepd_report('bmo.password_reset'); + Bugzilla->check_rate_limit('password_reset'); my $too_soon = $dbh->selectrow_array( 'SELECT 1 FROM tokens @@ -348,7 +348,7 @@ sub Cancel { $vars ||= {}; # Report this event for possible rate limiting - Bugzilla->iprepd_report('bmo.cancel_token'); + Bugzilla->check_rate_limit('cancel_token'); # Get information about the token being canceled. my ($db_token, $issuedate, $tokentype, $eventdata, $userid) @@ -363,7 +363,7 @@ sub Cancel { # Some DBs such as MySQL are case-insensitive by default so we do # a quick comparison to make sure the tokens are indeed the same. unless (defined $db_token && $db_token eq $token) { - Bugzilla->iprepd_report('bmo.token_mismatch', remote_ip()); + Bugzilla->check_rate_limit('token_mismatch'); ThrowCodeError("cancel_token_does_not_exist"); } diff --git a/external_test_api.pl b/external_test_api.pl index eb2f7be086..c8d6f10872 100644 --- a/external_test_api.pl +++ b/external_test_api.pl @@ -43,21 +43,6 @@ sub startup { my ($self) = @_; my $r = $self->routes; - # Mock the IPrepD API violations endpoint - $r->put( - '/violations/type/ip/*ip' => sub { - my $c = shift; - $c->app->defaults->{last_violation} = $c->req->json; - $c->render(text => 'OK ', status => 200); - } - ); - $r->get( - '/violations/last' => sub { - my $c = shift; - $c->render(json => $c->app->defaults->{last_violation}, status => 200); - } - ); - # Webhook endpoints for testing $r->post( '/webhooks/test/noauth' => sub { diff --git a/qa/t/3_test_rate_limit.t b/qa/t/3_test_rate_limit.t index 611ab22007..b54484b085 100644 --- a/qa/t/3_test_rate_limit.t +++ b/qa/t/3_test_rate_limit.t @@ -31,9 +31,7 @@ set_parameters( type => 'text', value => '{"get_attachments":[5,100],"get_comments":[5,100],"get_bug":[5,100],"show_bug":[5,100],"github":[5,100],"webpage_errors":[5,100], "token":[5,100], "api_key":[5,100], "username_password":[3,100]}' - }, - 'iprepd_base_url' => {type => 'text', value => 'http://externalapi.test:8001'}, - 'iprepd_client_secret' => {type => 'text', value => 'iprepd_client_secret'}, + } } } ); @@ -92,108 +90,6 @@ $t->get_ok($config->{browser_url} . "/rest/bug/$bug_id")->status_is(400) clear_memcache(); # So we can connect again -# IPREPD REPORTING TESTS - -# Incorrect username and/or password sends report to iprepd - -# Enter incorrect username and password 4 times where the -# 4th should be rate limited -$sel->open_ok('/login', undef, 'Go to the home page'); -$sel->title_is('Log in to Bugzilla'); -$sel->type_ok( - 'Bugzilla_login', - $config->{admin_user_login}, - 'Enter admin login name' -); -$sel->type_ok('Bugzilla_password', 'bad_password', 'Enter bad admin password'); -$sel->click_ok('log_in', undef, 'Submit credentials'); -$sel->title_is('Invalid Username Or Password', - 'Username or password error page displayed'); - -# Check that iprepd was properly notified -$t->get_ok('http://externalapi.test:8001/violations/last')->status_is(200) - ->json_is('/violation' => 'bmo.username_password_mismatch') - ->json_has('/object')->json_is('/type', 'ip'); - -# Invalid api keys via REST API should send report to iprepd - -# Use a bad api key gives the normal API key error. -$t->post_ok($config->{browser_url} - . '/rest/bug' => {'X-Bugzilla-API-Key' => 'bad_key'} => json => {}) - ->status_is(400) - ->json_like('/message', qr/The API key you specified is invalid/); - -# Check that iprepd was properly notified -$t->get_ok('http://externalapi.test:8001/violations/last')->status_is(200) - ->json_is('/violation' => 'bmo.api_key_mismatch')->json_has('/object') - ->json_is('/type', 'ip'); - -# Token errors should send report to iprepd - -# You should be able to use an invalid token 5 times but then fail the next -$t->get_ok( - $config->{browser_url} . '/token.cgi?t=bad_token&a=request_new_account') - ->status_is(200)->content_like(qr/Token Does Not Exist/); - -# Check that iprepd was properly notified -$t->get_ok('http://externalapi.test:8001/violations/last')->status_is(200) - ->json_is('/violation' => 'bmo.token_mismatch')->json_has('/object') - ->json_is('/type', 'ip'); - -# Check that iprepd is notified for new account emails -$sel->open_ok('/createaccount.cgi', undef, 'Go to the new account page'); -$sel->type_ok('login', 'you@example.com', 'Enter new account email'); -$sel->check_ok('etiquette', 'Accept etiquette'); -$sel->click_ok('//input[@value="Create Account"]', 'Submit'); -$sel->title_like(qr/Request for new user account/, - 'Request for new account sent'); - -# Check that iprepd was properly notified -$t->get_ok('http://externalapi.test:8001/violations/last')->status_is(200) - ->json_is('/violation' => 'bmo.create_account')->json_has('/object') - ->json_is('/type', 'ip'); - -# Check that iprepd is notified for password reset emails -$sel->open_ok('/home', 'Go to home page'); -$sel->click_ok('forgot_link_top', 'Show password reset field'); -$sel->type_ok( - '//input[@name="loginname"]', - $config->{admin_user_login}, - 'Enter email for password reset' -); -$sel->click_ok('forgot_button_top', 'Submit'); -$sel->title_is('Request to Change Password', 'Request for password reset sent'); - -# Check that iprepd was properly notified -$t->get_ok('http://externalapi.test:8001/violations/last')->status_is(200) - ->json_is('/violation' => 'bmo.password_reset')->json_has('/object') - ->json_is('/type', 'ip'); - -# Check that iprepd is notified for password reset emails -log_in($sel, $config, 'unprivileged'); -$sel->click_ok('header-account-menu-button'); -$sel->click_ok("link=Preferences"); -$sel->title_is("User Preferences"); -$sel->click_ok("link=Account"); -$sel->title_is("User Preferences"); -$sel->type_ok( - '//input[@name="new_login_name"]', - 'new-' . $config->{unprivileged_user_login}, - 'Enter new email' -); -$sel->type_ok( - '//input[@name="old_password"]', - $config->{unprivileged_user_passwd}, - 'Enter current password' -); -$sel->click_ok('update'); -logout($sel); - -# Check that iprepd was properly notified -$t->get_ok('http://externalapi.test:8001/violations/last')->status_is(200) - ->json_is('/violation' => 'bmo.email_change')->json_has('/object') - ->json_is('/type', 'ip'); - # Turn rate limiting off log_in($sel, $config, 'admin'); set_parameters($sel, diff --git a/t/migrate-params.t b/t/migrate-params.t index f64c0ee8a7..9dc5303583 100644 --- a/t/migrate-params.t +++ b/t/migrate-params.t @@ -54,6 +54,5 @@ __DATA__ 'bitly_token' => 'BITLY_TOKEN', 'github_client_id' => 'GITHUB_CLIENT_ID', 'honeypot_api_key' => 'HONEYPOT_API_KEY', - 'iprepd_client_secret' => 'IPREPD_CLIENT_SECRET', 'phabricator_api_key' => 'PHABRICATOR_API_KEY', ); diff --git a/token.cgi b/token.cgi index 3ce12cfef1..338459c41b 100755 --- a/token.cgi +++ b/token.cgi @@ -53,44 +53,44 @@ if ($token) { WHERE token = ?', undef, $token ); unless (defined $db_token && $db_token eq $token) { - Bugzilla->iprepd_report('bmo.token_mismatch', remote_ip()); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError("token_does_not_exist"); } # Make sure the token is the correct type for the action being taken. if (grep($action eq $_, qw(cfmpw cxlpw chgpw)) && $tokentype ne 'password') { Bugzilla::Token::Cancel($token, "wrong_token_for_changing_passwd"); - Bugzilla->iprepd_report('bmo.token_mismatch', remote_ip()); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError("wrong_token_for_changing_passwd"); } if ( ($action eq 'cxlem') && (($tokentype ne 'emailold') && ($tokentype ne 'emailnew'))) { Bugzilla::Token::Cancel($token, "wrong_token_for_cancelling_email_change"); - Bugzilla->iprepd_report('bmo.token_mismatch', remote_ip()); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError("wrong_token_for_cancelling_email_change"); } if (grep($action eq $_, qw(cfmem chgem)) && ($tokentype ne 'emailnew')) { Bugzilla::Token::Cancel($token, "wrong_token_for_confirming_email_change"); - Bugzilla->iprepd_report('bmo.token_mismatch', remote_ip()); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError("wrong_token_for_confirming_email_change"); } if ( ($action =~ /^(request|confirm|cancel)_new_account$/) && ($tokentype ne 'account')) { Bugzilla::Token::Cancel($token, 'wrong_token_for_creating_account'); - Bugzilla->iprepd_report('bmo.token_mismatch', remote_ip()); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError('wrong_token_for_creating_account'); } if (substr($action, 0, 4) eq 'mfa_' && $tokentype ne 'session.short') { Bugzilla::Token::Cancel($token, 'wrong_token_for_mfa'); - Bugzilla->iprepd_report('bmo.token_mismatch', remote_ip()); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError('wrong_token_for_mfa'); } if ($action eq 'verify_auto_account_creation' && $tokentype ne 'account_create') { - Bugzilla->iprepd_report('token', remote_ip()); + Bugzilla->check_rate_limit('token_mismatch'); ThrowUserError('wrong_token_for_account_creation'); } } From 6981d2ae2c718d3f10350f636cffe27a914a8fd5 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Tue, 27 Aug 2024 11:47:25 -0400 Subject: [PATCH 2/2] Added sensible defaults for the other rate limit identifiers. Removed unused indentifiers from rate limit test script config. --- Bugzilla/Config/Admin.pm | 20 ++++++++++++++------ qa/t/3_test_rate_limit.t | 2 +- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/Bugzilla/Config/Admin.pm b/Bugzilla/Config/Admin.pm index 0897a6d147..1c678f848f 100644 --- a/Bugzilla/Config/Admin.pm +++ b/Bugzilla/Config/Admin.pm @@ -59,12 +59,20 @@ sub get_param_list { sub default_rate_limit_rules { return encode_json({ - get_bug => [75, 60], - show_bug => [75, 60], - github => [10, 60], - get_attachments => [75, 60], - get_comments => [75, 60], - webpage_errors => [75, 60], + get_bug => [75, 60], + show_bug => [75, 60], + github => [10, 60], + get_attachments => [75, 60], + get_comments => [75, 60], + webpage_errors => [75, 60], + token_mismatch => [5, 60], + github => [5, 60], + create_account => [5, 60], + email_change => [5, 60], + password_reset => [5, 60], + cancel_token => [5, 60], + api_key_mismatch => [5, 60], + mfa_mismatch => [5, 60], }); } diff --git a/qa/t/3_test_rate_limit.t b/qa/t/3_test_rate_limit.t index b54484b085..7be26c5f42 100644 --- a/qa/t/3_test_rate_limit.t +++ b/qa/t/3_test_rate_limit.t @@ -30,7 +30,7 @@ set_parameters( 'rate_limit_rules' => { type => 'text', value => - '{"get_attachments":[5,100],"get_comments":[5,100],"get_bug":[5,100],"show_bug":[5,100],"github":[5,100],"webpage_errors":[5,100], "token":[5,100], "api_key":[5,100], "username_password":[3,100]}' + '{"get_attachments":[5,100],"get_comments":[5,100],"get_bug":[5,100],"show_bug":[5,100],"github":[5,100],"webpage_errors":[5,100]}' } } }