Skip to content
Merged
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
46 changes: 46 additions & 0 deletions app/controllers/users/two_factor_authentication_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,10 @@ def handle_valid_otp_params(otp_delivery_selection_result, method, default = nil
context: context,
)
end

if exceeded_short_term_otp_rate_limit?
return handle_too_many_short_term_otp_sends(method: method, default: default)
end
return handle_too_many_confirmation_sends if exceeded_phone_confirmation_limit?

@telephony_result = send_user_otp(method)
Expand Down Expand Up @@ -261,6 +265,19 @@ def phone_confirmation_rate_limiter
)
end

def short_term_otp_rate_limiter
@short_term_otp_rate_limiter ||= RateLimiter.new(
user: current_user,
rate_limit_type: :short_term_phone_otp,
)
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

def exceeded_phone_confirmation_limit?
return false unless UserSessionContext.confirmation_context?(context)
phone_confirmation_rate_limiter.increment!
Expand Down Expand Up @@ -344,7 +361,36 @@ def webauthn_params
{ platform: current_user.webauthn_configurations.platform_authenticators.present? }
end

def handle_too_many_short_term_otp_sends(method:, default:)
analytics.rate_limit_reached(
limiter_type: short_term_otp_rate_limiter.rate_limit_type,
country_code: parsed_phone.country,
phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164),
context: context,
otp_delivery_preference: method,
)

flash[:error] = t(
'errors.messages.phone_confirmation_limited',
timeout: distance_of_time_in_words(
Time.zone.now,
[short_term_otp_rate_limiter.expires_at, Time.zone.now].compact.max,
),
)

redirect_to login_two_factor_url(
otp_delivery_preference: method,
otp_make_default_number: default,
)
end

def handle_too_many_confirmation_sends
analytics.rate_limit_reached(
limiter_type: phone_confirmation_rate_limiter.rate_limit_type,
country_code: parsed_phone.country,
phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164),
)

flash[:error] = t(
'errors.messages.phone_confirmation_limited',
timeout: distance_of_time_in_words(
Expand Down
5 changes: 5 additions & 0 deletions app/services/rate_limiter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,11 @@ def self.load_rate_limit_config
max_attempts: IdentityConfig.store.otp_delivery_blocklist_maxretry + 1,
attempt_window: IdentityConfig.store.otp_delivery_blocklist_findtime,
},
short_term_phone_otp: {
max_attempts: IdentityConfig.store.short_term_phone_otp_max_attempts,
attempt_window: IdentityConfig.store.
short_term_phone_otp_max_attempt_window_in_seconds.seconds.in_minutes.to_f,
},
}.with_indifferent_access
end

Expand Down
5 changes: 5 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,9 @@ participate_in_dap: false
password_max_attempts: 3
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My initial impression was that this would allow someone to request 2 codes within 10 seconds, but I can only request 1 code within 10 seconds. I think either would be fine, but the configuration value felt a little misleading to me (or at least unexpected), i.e. I would expect this value to be 1 if I can only request 1 code every 10 seconds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the way the RateLimiter class is kind of weird in that way unfortunately :/

We could switch

attempts && attempts >= RateLimiter.max_attempts(rate_limit_type)
to be > rather than >= and reduce everything by one 😬

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 @@ -483,6 +486,7 @@ production:
phone_recaptcha_mock_validator: false
piv_cac_verify_token_secret:
session_encryptor_alert_enabled: true
short_term_phone_otp_rate_limiter_enabled: false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Curious the rationale for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd rather default to off for deployed environments and opt-in manually for now. We can/should remove it if we're happy with it though.

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 @@ -579,6 +583,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
skip_encryption_allowed_list: '[]'
state_tracking_enabled: true
team_ada_email: 'ada@example.com'
Expand Down
3 changes: 3 additions & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,9 @@ def self.build_store(config_map)
config.add(:session_total_duration_timeout_in_minutes, type: :integer)
config.add(:show_unsupported_passkey_platform_authentication_setup, type: :boolean)
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
Original file line number Diff line number Diff line change
Expand Up @@ -572,13 +572,15 @@ def index
end

it 'rate limits confirmation OTPs on sign up' do
parsed_phone = Phonelib.parse(@unconfirmed_phone)
stub_analytics
sign_in_before_2fa(@user)
subject.user_session[:context] = 'confirmation'
allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(999)

freeze_time do
(IdentityConfig.store.phone_confirmation_max_attempts + 1).times do
subject.user_session[:unconfirmed_phone] = '+1 (202) 555-1213'
IdentityConfig.store.phone_confirmation_max_attempts.times do
subject.user_session[:unconfirmed_phone] = @unconfirmed_phone
get :send_code, params: otp_delivery_form_sms
end

Expand All @@ -594,6 +596,52 @@ def index
)
expect(response).to redirect_to authentication_methods_setup_url
end
expect(@analytics).to have_logged_event(
'Rate Limit Reached',
country_code: parsed_phone.country,
limiter_type: :phone_confirmation,
phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164),
)
end

it 'rate limits between OTPs' do
parsed_phone = Phonelib.parse(@unconfirmed_phone)
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)

freeze_time do
IdentityConfig.store.short_term_phone_otp_max_attempts.times do
subject.user_session[:unconfirmed_phone] = @unconfirmed_phone
get :send_code, params: otp_delivery_form_sms
end

timeout = distance_of_time_in_words(
RateLimiter.attempt_window_in_minutes(:short_term_phone_otp).minutes,
)

expect(flash[:error]).to eq(
I18n.t(
'errors.messages.phone_confirmation_limited',
timeout: timeout,
),
)
expect(response).to redirect_to login_two_factor_url(otp_delivery_preference: 'sms')
end

expect(@analytics).to have_logged_event(
'Rate Limit Reached',
context: 'confirmation',
country_code: parsed_phone.country,
limiter_type: :short_term_phone_otp,
otp_delivery_preference: 'sms',
phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164),
)
end

it 'marks the user as locked out after too many attempts on sign up' do
Expand Down