Upgrade redis-session-store to improve performance and security

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'

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)

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

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

config/initializers/session_store.rb

@@ -1,26 +1,26 @@
 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,
 
     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,
 )

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)

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)
 

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')
 

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

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