diff --git a/Gemfile b/Gemfile index 67452d9592b..a1ac6a19fbf 100644 --- a/Gemfile +++ b/Gemfile @@ -51,7 +51,7 @@ gem 'rack-headers_filter' gem 'rack-timeout', require: false gem 'redacted_struct' gem 'redis', '>= 3.2.0' -gem 'redis-session-store', github: '18F/redis-session-store', tag: 'v0.12-18f' +gem 'redis-session-store', github: '18F/redis-session-store', tag: 'v1.0.1-18f' gem 'retries' gem 'rotp', '~> 6.1' gem 'rqrcode' diff --git a/Gemfile.lock b/Gemfile.lock index a4f91a06781..97b4b58b0d4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -25,12 +25,12 @@ GIT GIT remote: https://github.com/18F/redis-session-store.git - revision: 253aa4f466cf61e129625ea01d1f120fb9c8685d - tag: v0.12-18f + revision: 9e3f8a22a1b5d1e835e5cba20c51e38b8965b836 + tag: v1.0.1-18f specs: - redis-session-store (0.12.pre.18f) + redis-session-store (1.0.1.pre.18f) actionpack (>= 6, < 8) - redis (>= 3, < 6) + redis (>= 4.3, < 6) GIT remote: https://github.com/18F/saml_idp.git @@ -223,7 +223,7 @@ GEM coercible (1.0.0) descendants_tracker (~> 0.0.1) concurrent-ruby (1.2.2) - connection_pool (2.3.0) + connection_pool (2.4.0) cose (1.3.0) cbor (~> 0.5.9) openssl-signature_algorithm (~> 1.0) @@ -372,7 +372,7 @@ GEM activesupport (>= 4) railties (>= 4) request_store (~> 1.0) - loofah (2.19.1) + loofah (2.20.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) lookbook (1.5.3) @@ -467,7 +467,7 @@ GEM nio4r (~> 2.0) raabro (1.4.0) racc (1.6.2) - rack (2.2.6.4) + rack (2.2.7) rack-attack (6.5.0) rack (>= 1.0, < 3) rack-cors (1.1.1) @@ -477,7 +477,7 @@ GEM rack (>= 1.2.0) rack-proxy (0.7.4) rack - rack-test (2.0.2) + rack-test (2.1.0) rack (>= 1.3) rack-timeout (0.6.0) rack_session_access (0.2.0) @@ -530,7 +530,7 @@ GEM redcarpet (3.6.0) redis (5.0.6) redis-client (>= 0.9.0) - redis-client (0.14.0) + redis-client (0.14.1) connection_pool regexp_parser (2.6.1) reline (0.2.7) diff --git a/app/services/out_of_band_session_accessor.rb b/app/services/out_of_band_session_accessor.rb index 28dc70df484..1df1730f898 100644 --- a/app/services/out_of_band_session_accessor.rb +++ b/app/services/out_of_band_session_accessor.rb @@ -5,14 +5,27 @@ class OutOfBandSessionAccessor attr_reader :session_uuid - def initialize(session_uuid) + PLACEHOLDER_REQUEST = ActionDispatch::TestRequest.create.freeze + + def initialize(session_uuid, session_store = nil) @session_uuid = session_uuid + @session_store = session_store end def ttl - uuid = session_uuid - session_store.instance_eval do - with_redis { |client| client.ttl(prefixed(uuid)) } + uuid = Rack::Session::SessionId.new(session_uuid) + if IdentityConfig.store.redis_session_read_public_id + session_store.instance_eval do + with_redis_connection do |client| + public_id_ttl = client.ttl(prefixed(uuid)) + return public_id_ttl if public_id_ttl >= 0 + client.ttl(prefixed_public_id(uuid)) + end + end + else + session_store.instance_eval do + with_redis_connection { |client| client.ttl(prefixed(uuid)) } + end end end @@ -28,7 +41,12 @@ def load_x509 end def destroy - session_store.send(:delete_session, {}, Rack::Session::SessionId.new(session_uuid), drop: true) + session_store.send( + :delete_session, + PLACEHOLDER_REQUEST, + Rack::Session::SessionId.new(session_uuid), + drop: true, + ) end # @api private @@ -68,7 +86,7 @@ def put(data, expiration = 5.minutes) session_store.send( :write_session, - {}, + PLACEHOLDER_REQUEST, Rack::Session::SessionId.new(session_uuid), session_data, expire_after: expiration.to_i, @@ -77,9 +95,17 @@ def put(data, expiration = 5.minutes) # @return [Hash] def session_data - @session_data ||= session_store.send( - :find_session, {}, Rack::Session::SessionId.new(session_uuid) - ).last || {} + return {} unless session_uuid + uuid = Rack::Session::SessionId.new(session_uuid) + @session_data ||= session_store.instance_eval do + with_redis_connection do |client| + load_session_from_redis( + client, + PLACEHOLDER_REQUEST, + uuid, + ) + end + end || {} end def session_store diff --git a/config/application.yml.default b/config/application.yml.default index 8ae93e01f47..7add35e97d9 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -263,6 +263,10 @@ redis_throttle_url: redis://localhost:6379/1 redis_url: redis://localhost:6379/0 redis_pool_size: 10 redis_session_pool_size: 10 +redis_session_read_public_id: false +redis_session_write_public_id: false +redis_session_read_private_id: true +redis_session_write_private_id: true redis_throttle_pool_size: 5 redis_irs_attempt_api_pool_size: 1 reg_confirmed_email_max_attempts: 20 @@ -473,6 +477,10 @@ production: reauthentication_for_second_factor_management_enabled: false recurring_jobs_disabled_names: "[]" redis_irs_attempt_api_url: redis://redis.login.gov.internal:6379/2 + redis_session_read_public_id: true + redis_session_write_public_id: true + redis_session_read_private_id: false + redis_session_write_private_id: false redis_throttle_url: redis://redis.login.gov.internal:6379/1 redis_url: redis://redis.login.gov.internal:6379 report_timeout: 1_000_000 diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index 958f9216e17..79307ecb2df 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -1,18 +1,18 @@ require 'session_encryptor' require 'legacy_session_encryptor' -require 'session_encryptor_error_handler' APPLICATION_SESSION_COOKIE_KEY = '_identity_idp_session'.freeze Rails.application.config.session_store( :redis_session_store, key: APPLICATION_SESSION_COOKIE_KEY, + # cookie expires with browser close + expire_after: nil, redis: { - # cookie expires with browser close - expire_after: nil, - - write_fallback: true, - read_fallback: true, + read_public_id: IdentityConfig.store.redis_session_read_public_id, + write_public_id: IdentityConfig.store.redis_session_write_public_id, + read_private_id: IdentityConfig.store.redis_session_read_private_id, + write_private_id: IdentityConfig.store.redis_session_write_private_id, # Redis expires session after N minutes ttl: IdentityConfig.store.session_timeout_in_minutes.minutes, @@ -20,7 +20,7 @@ key_prefix: "#{IdentityConfig.store.domain_name}:session:", client_pool: REDIS_POOL, }, - on_session_load_error: SessionEncryptorErrorHandler, - on_redis_down: proc { |error, _env, _sid| raise error }, + on_session_load_error: proc { |error, _sid| raise error }, + on_redis_down: proc { |error| raise error }, serializer: SessionEncryptor.new, ) diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 2098df2cdb5..fb5c073662e 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -357,6 +357,10 @@ def self.build_store(config_map) config.add(:recurring_jobs_disabled_names, type: :json) config.add(:redis_irs_attempt_api_url) config.add(:redis_irs_attempt_api_pool_size, type: :integer) + config.add(:redis_session_read_public_id, type: :boolean) + config.add(:redis_session_write_public_id, type: :boolean) + config.add(:redis_session_read_private_id, type: :boolean) + config.add(:redis_session_write_private_id, type: :boolean) config.add(:redis_throttle_url) config.add(:redis_url) config.add(:redis_pool_size, type: :integer) diff --git a/lib/session_encryptor_error_handler.rb b/lib/session_encryptor_error_handler.rb deleted file mode 100644 index 6550cf7d1e2..00000000000 --- a/lib/session_encryptor_error_handler.rb +++ /dev/null @@ -1,5 +0,0 @@ -class SessionEncryptorErrorHandler - def self.call(error, _sid) - raise error - end -end diff --git a/spec/features/remember_device/session_expiration_spec.rb b/spec/features/remember_device/session_expiration_spec.rb index 31825f01fca..708030d334c 100644 --- a/spec/features/remember_device/session_expiration_spec.rb +++ b/spec/features/remember_device/session_expiration_spec.rb @@ -28,7 +28,12 @@ # Simulate being idle on the sign in page long enough for the session to # be deleted from Redis, but since Redis doesn't respect ActiveSupport::Testing::TimeHelpers, # we need to expire the session manually. - session_store.send(:destroy_session_from_sid, session_cookie.value) + session_store.send( + :delete_session, + nil, + Rack::Session::SessionId.new(session_cookie.value), + drop: true, + ) # Simulate refreshing the page with JS to avoid a CSRF error visit new_user_session_url(request_id: request_id) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index dec07bbdf6b..09f7b40a00a 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -337,7 +337,12 @@ expect(page).to_not have_content(t('forms.buttons.continue')) # Redis doesn't respect ActiveSupport::Testing::TimeHelpers, so expire session manually. - session_store.send(:destroy_session_from_sid, session_cookie.value) + session_store.send( + :delete_session, + nil, + Rack::Session::SessionId.new(session_cookie.value), + drop: true, + ) fill_in_credentials_and_submit(user.email, user.password) expect(page).to have_content t('errors.general') @@ -472,7 +477,6 @@ email = user.email allow(FeatureManagement).to receive(:use_kms?).and_return(true) stub_aws_kms_client_invalid_ciphertext - allow(SessionEncryptorErrorHandler).to receive(:call) signin(email, 'invalid') diff --git a/spec/requests/secure_cookies_spec.rb b/spec/requests/secure_cookies_spec.rb index 8186b5e84fd..7a72fa05cea 100644 --- a/spec/requests/secure_cookies_spec.rb +++ b/spec/requests/secure_cookies_spec.rb @@ -22,4 +22,11 @@ expect(response.headers['Set-Cookie'].scan('; SameSite=Lax').size).to eq(cookie_count) end end + + it 'does not set an expiration on the session cookie' do + get root_url + cookies = response.headers['Set-Cookie'].split("\n") + session_cookie = cookies.find { |x| x.include?(APPLICATION_SESSION_COOKIE_KEY) } + expect(session_cookie).to_not include('expires=') + end end diff --git a/spec/services/out_of_band_session_accessor_spec.rb b/spec/services/out_of_band_session_accessor_spec.rb index dbbbf1a4e20..fefa0f6c1e5 100644 --- a/spec/services/out_of_band_session_accessor_spec.rb +++ b/spec/services/out_of_band_session_accessor_spec.rb @@ -17,6 +17,58 @@ expect(store.ttl).to eq(5.minutes.to_i) end + + it 'returns the remaining time-to-live of the session data in redis' do + store.put_pii({ first_name: 'Fakey' }, 5.minutes.to_i) + + expect(store.ttl).to eq(5.minutes.to_i) + end + + context 'with reading and writing public_id enabled' do + it 'returns the TTL' do + allow(IdentityConfig.store).to receive(:redis_session_read_public_id).and_return(true) + allow(IdentityConfig.store).to receive(:redis_session_write_public_id).and_return(true) + + options = Rails.application.config.session_options.deep_dup + options[:redis][:write_public_id] = true + options[:redis][:write_private_id] = false + session_store = RedisSessionStore.new({}, options) + old_store = described_class.new(session_uuid, session_store) + + old_store.put_pii({ first_name: 'Fakey' }, 5.minutes.to_i) + + expect(store.ttl).to eq(5.minutes.to_i) + end + end + + context 'with reading public_id enabled and write public_id disabled' do + it 'returns the TTL whether it was written to the private_id key or private_id key' do + allow(IdentityConfig.store).to receive(:redis_session_read_public_id).and_return(true) + allow(IdentityConfig.store).to receive(:redis_session_write_public_id).and_return(false) + + old_store = described_class.new(session_uuid) + old_store.put_pii({ first_name: 'Fakey' }, 5.minutes.to_i) + expect(old_store.ttl).to eq(5.minutes.to_i) + + allow(IdentityConfig.store).to receive(:redis_session_write_public_id).and_return(true) + + new_store = described_class.new(session_uuid) + new_store.put_pii({ first_name: 'Fakey2' }, 5.minutes.to_i) + + expect(old_store.ttl).to eq(5.minutes.to_i) + end + end + + context 'with reading and writing public_id disabled' do + it 'returns the TTL' do + allow(IdentityConfig.store).to receive(:redis_session_read_public_id).and_return(false) + allow(IdentityConfig.store).to receive(:redis_session_write_public_id).and_return(false) + + store.put_pii({ first_name: 'Fakey' }, 5.minutes.to_i) + + expect(store.ttl).to eq(5.minutes.to_i) + end + end end describe '#load_pii' do