Skip to content

Remove short_term_phone_otp_rate_limiter_enabled configuration key #10432

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

Merged
mitchellhenke merged 2 commits into from
Apr 15, 2024
Merged

Remove short_term_phone_otp_rate_limiter_enabled configuration key #10432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99c2921
Remove short_term_phone_otp_rate_limiter_enabled configuration key
Apr 15, 2024
333c22c
fix tests
Apr 15, 2024
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
1 change: 0 additions & 1 deletion app/controllers/users/two_factor_authentication_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,6 @@ def short_term_otp_rate_limiter
end

def exceeded_short_term_otp_rate_limit?
return false unless IdentityConfig.store.short_term_phone_otp_rate_limiter_enabled
short_term_otp_rate_limiter.increment!
short_term_otp_rate_limiter.limited?
end
Expand Down
4 changes: 1 addition & 3 deletions config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,6 @@ personal_key_retired: true
phone_carrier_registration_blocklist_array: '[]'
short_term_phone_otp_max_attempt_window_in_seconds: 10
short_term_phone_otp_max_attempts: 2
short_term_phone_otp_rate_limiter_enabled: true
phone_confirmation_max_attempts: 20
phone_confirmation_max_attempt_window_in_minutes: 1_440
phone_service_check: true
Expand Down Expand Up @@ -490,7 +489,6 @@ production:
phone_recaptcha_mock_validator: false
piv_cac_verify_token_secret:
session_encryptor_alert_enabled: true
short_term_phone_otp_rate_limiter_enabled: false
redis_throttle_url: redis://redis.login.gov.internal:6379/1
redis_url: redis://redis.login.gov.internal:6379
report_timeout: 1_000_000
Expand Down Expand Up @@ -586,7 +584,7 @@ test:
scrypt_cost: 800$8$1$
secret_key_base: test_secret_key_base
session_encryption_key: 27bad3c25711099429c1afdfd1890910f3b59f5a4faec1c85e945cb8b02b02f261ba501d99cfbb4fab394e0102de6fecf8ffe260f322f610db3e96b2a775c120
short_term_phone_otp_rate_limiter_enabled: false
short_term_phone_otp_max_attempts: 100
Copy link
Copy Markdown
Contributor

@aduth aduth Apr 15, 2024

Choose a reason for hiding this comment

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

Maybe we should have something which resets all rate limits between tests? Unclear if that would actually help for the reason this was added.

That, and/or use a fake phone generator in the phone factory setup.

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Apr 15, 2024

Choose a reason for hiding this comment

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

I think some of them were sending too many SMS within the same test. ex:

identity-idp/spec/features/openid_connect/openid_connect_spec.rb

Lines 33 to 38 in 333c22c

it 'succeeds in forcing login with prompt login and prior session' do
user = oidc_end_client_secret_jwt(prompt: 'login')
travel_to((IdentityConfig.store.sp_handoff_bounce_max_seconds + 1).seconds.from_now) do
oidc_end_client_secret_jwt(prompt: 'login', user: user)
end
end

Copy link
Copy Markdown
Contributor

@aduth aduth Apr 15, 2024

Choose a reason for hiding this comment

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

Hm, fair enough. Seems like we may want to revisit those at some point to make them more realistic to what we'd expect in the real-world.

I started poking at my previous idea of resetting the rate limiter, though I don't love the idea here of multiple calls to Redis between each test:

# spec/support/rate_limiter.rb

RSpec.configure do |config|
  config.before(:each) do
    REDIS_THROTTLE_POOL.with do |client|
      keys = client.call [:keys, 'throttle:*']
      client.call [:del, *keys]
    end
  end
end

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Apr 15, 2024

Choose a reason for hiding this comment

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

do we need the .call? I think we can:

RSpec.configure do |config|
  config.before(:each) do
    REDIS_THROTTLE_POOL.with do |client|
      keys = client.keys('throttle:*')
      client.del(*keys)
    end
  end
end

Copy link
Copy Markdown
Contributor

@aduth aduth Apr 15, 2024

Choose a reason for hiding this comment

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

Stumbling my way through the solution led me to this issue, which seemed to imply that call is a bit safer when deleting many keys. Though I doubt we have so many matches that it'd become an issue.

redis/redis-rb#122

Copy link
Copy Markdown
Contributor Author

@mitchellhenke mitchellhenke Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Haven't confirmed that it does or doesn't work, but I think we intend to reset the rate limit pool between tests:

identity-idp/spec/rails_helper.rb

Line 112 in 333c22c

REDIS_THROTTLE_POOL.with { |client| client.flushdb }

Copy link
Copy Markdown
Contributor

@aduth aduth Apr 15, 2024

Choose a reason for hiding this comment

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

Ah, that would (likely) work too! As you mentioned, it's probably more to do with limits within a single test being exceeded.

skip_encryption_allowed_list: '[]'
state_tracking_enabled: true
team_ada_email: 'ada@example.com'
Expand Down
1 change: 0 additions & 1 deletion lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,6 @@ def self.build_store(config_map)
config.add(:show_user_attribute_deprecation_warnings, type: :boolean)
config.add(:short_term_phone_otp_max_attempts, type: :integer)
config.add(:short_term_phone_otp_max_attempt_window_in_seconds, type: :integer)
config.add(:short_term_phone_otp_rate_limiter_enabled, type: :boolean)
config.add(:skip_encryption_allowed_list, type: :json)
config.add(:sp_handoff_bounce_max_seconds, type: :integer)
config.add(:sp_issuer_user_counts_report_configs, type: :json)
Expand Down
2 changes: 0 additions & 2 deletions spec/controllers/users/two_factor_authentication_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,8 +609,6 @@ def index
stub_analytics
sign_in_before_2fa(@user)
subject.user_session[:context] = 'confirmation'
allow(IdentityConfig.store).to receive(:short_term_phone_otp_rate_limiter_enabled).
and_return(true)
allow(IdentityConfig.store).to receive(:short_term_phone_otp_max_attempts).and_return(2)
allow(IdentityConfig.store).to receive(:short_term_phone_otp_max_attempt_window_in_seconds).
and_return(5)
Expand Down