From 7055fa4da71a3bfa01a09a056d4046c3b44c17bd Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Nov 2022 14:46:24 -0700 Subject: [PATCH 1/7] Use a connection pool for redis-session-store - Via ConnectionPool::Wrapper which may be slightly slower [skip changelog] --- config/application.yml.default | 1 + config/initializers/redis.rb | 7 +++++++ config/initializers/session_store.rb | 2 +- lib/identity_config.rb | 1 + 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/config/application.yml.default b/config/application.yml.default index 5ea870d40fc..d37b63909ea 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -234,6 +234,7 @@ redis_irs_attempt_api_url: redis://localhost:6379/2 redis_throttle_url: redis://localhost:6379/1 redis_url: redis://localhost:6379/0 redis_pool_size: 10 +redis_session_pool_size: 10 redis_throttle_pool_size: 5 reg_confirmed_email_max_attempts: 20 reg_confirmed_email_window_in_minutes: 60 diff --git a/config/initializers/redis.rb b/config/initializers/redis.rb index 4edf5057a37..f7b166a38a7 100644 --- a/config/initializers/redis.rb +++ b/config/initializers/redis.rb @@ -11,3 +11,10 @@ redis: Redis.new(url: IdentityConfig.store.redis_throttle_url), ) end + +REDIS_SESSION_POOL_WRAPPER = ConnectionPool::Wrapper.new( + size: IdentityConfig.store.redis_session_pool_size, +) do + # redis-session-store does its own namespacing in session_store.rb + Redis.new(url: IdentityConfig.store.redis_url) +end diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index ac1b4f63e7f..a1c3ab5bbca 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -13,7 +13,7 @@ ttl: IdentityConfig.store.session_timeout_in_minutes.minutes, key_prefix: "#{IdentityConfig.store.domain_name}:session:", - url: IdentityConfig.store.redis_url, + client: REDIS_SESSION_POOL_WRAPPER, }, on_session_load_error: SessionEncryptorErrorHandler, serializer: SessionEncryptor.new, diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 191d47d0001..eff7656fb00 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -322,6 +322,7 @@ def self.build_store(config_map) config.add(:redis_throttle_url) config.add(:redis_url) config.add(:redis_pool_size, type: :integer) + config.add(:redis_session_pool_size, type: :integer) config.add(:redis_throttle_pool_size, type: :integer) config.add(:reg_confirmed_email_max_attempts, type: :integer) config.add(:reg_confirmed_email_window_in_minutes, type: :integer) From 7f1fc018700cde24244324104f6abbdf07469c2e Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Nov 2022 15:31:24 -0700 Subject: [PATCH 2/7] Use forked redis-session-store gem that can accept a connection pool --- Gemfile | 2 +- Gemfile.lock | 14 ++++++++++---- config/initializers/redis.rb | 4 +--- config/initializers/session_store.rb | 2 +- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Gemfile b/Gemfile index 3fe635d9d58..0fb7c2ff94e 100644 --- a/Gemfile +++ b/Gemfile @@ -51,7 +51,7 @@ gem 'rack-timeout', require: false gem 'redacted_struct' gem 'redis', '>= 3.2.0' gem 'redis-namespace' -gem 'redis-session-store', '>= 0.11.4' +gem 'redis-session-store', github: '18f/redis-session-store', ref: 'margolis-use-connection-pool' gem 'retries' gem 'rotp', '~> 6.1' gem 'rqrcode' diff --git a/Gemfile.lock b/Gemfile.lock index d233fcfe4ec..eda9ada3981 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -36,6 +36,15 @@ GIT pkcs11 uuid +GIT + remote: https://github.com/18f/redis-session-store.git + revision: 1e97057f3d860c4dce758c61e7de66b8450b5f94 + ref: margolis-use-connection-pool + specs: + redis-session-store (0.12.pre.18f) + actionpack (>= 6, < 8) + redis (>= 3, < 5) + GIT remote: https://github.com/hashrocket/capybara-webmock.git revision: 63d790a0b6c779b9700634bfc153e25ccdeb3688 @@ -533,9 +542,6 @@ GEM redis (4.7.1) redis-namespace (1.8.1) redis (>= 3.0.4) - redis-session-store (0.11.4) - actionpack (>= 3, < 8) - redis (>= 3, < 5) regexp_parser (2.6.0) reline (0.2.7) io-console (~> 0.5) @@ -800,7 +806,7 @@ DEPENDENCIES redacted_struct redis (>= 3.2.0) redis-namespace - redis-session-store (>= 0.11.4) + redis-session-store! retries rotp (~> 6.1) rqrcode diff --git a/config/initializers/redis.rb b/config/initializers/redis.rb index f7b166a38a7..6a1632f650b 100644 --- a/config/initializers/redis.rb +++ b/config/initializers/redis.rb @@ -12,9 +12,7 @@ ) end -REDIS_SESSION_POOL_WRAPPER = ConnectionPool::Wrapper.new( - size: IdentityConfig.store.redis_session_pool_size, -) do +REDIS_SESSION_POOL = ConnectionPool.new(size: IdentityConfig.store.redis_session_pool_size) do # redis-session-store does its own namespacing in session_store.rb Redis.new(url: IdentityConfig.store.redis_url) end diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index a1c3ab5bbca..18f518f2ff5 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -13,7 +13,7 @@ ttl: IdentityConfig.store.session_timeout_in_minutes.minutes, key_prefix: "#{IdentityConfig.store.domain_name}:session:", - client: REDIS_SESSION_POOL_WRAPPER, + client_pool: REDIS_SESSION_POOL, }, on_session_load_error: SessionEncryptorErrorHandler, serializer: SessionEncryptor.new, From 555fd437be8b3b6cc50dd395db00a62b37a20203 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Nov 2022 16:17:08 -0700 Subject: [PATCH 3/7] Update private API usage :see_no_evil: --- app/services/out_of_band_session_accessor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/out_of_band_session_accessor.rb b/app/services/out_of_band_session_accessor.rb index 2f5981cf128..64dad10a8bf 100644 --- a/app/services/out_of_band_session_accessor.rb +++ b/app/services/out_of_band_session_accessor.rb @@ -12,7 +12,7 @@ def initialize(session_uuid) def ttl uuid = session_uuid - session_store.instance_eval { redis.ttl(prefixed(uuid)) } + session_store.instance_eval { with_redis { |redis| redis.ttl(prefixed(uuid)) } } end def load From 7ae9f78a8bd8f5d0dce3d3fec1e8804268707efa Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Nov 2022 17:50:48 -0700 Subject: [PATCH 4/7] Refactor X509::SessionStore to use OutOfBandSessionStore - just like Pii::SessionStore - they probably should not be separate classes in the first place --- app/services/out_of_band_session_accessor.rb | 2 +- app/services/x509/session_store.rb | 30 ++++++-------------- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/app/services/out_of_band_session_accessor.rb b/app/services/out_of_band_session_accessor.rb index 64dad10a8bf..60a779838e3 100644 --- a/app/services/out_of_band_session_accessor.rb +++ b/app/services/out_of_band_session_accessor.rb @@ -25,7 +25,7 @@ def destroy # @api private # Only used for convenience in tests - # @param [Pii::Attributes] pii + # @param [#to_h] data def put(data, expiration = 5.minutes) session_data = { 'warden.user.user.session' => data.to_h, diff --git a/app/services/x509/session_store.rb b/app/services/x509/session_store.rb index 174b6cfb9d4..c8184aef549 100644 --- a/app/services/x509/session_store.rb +++ b/app/services/x509/session_store.rb @@ -5,41 +5,29 @@ # See X509::Cacher for accessing PII inside of a normal browser session module X509 class SessionStore - attr_reader :session_uuid + attr_reader :session_accessor - def initialize(session_uuid) - @session_uuid = session_uuid - end + delegate :ttl, :destroy, to: :session_accessor - def ttl - uuid = session_uuid - session_store.instance_eval { redis.ttl(prefixed(uuid)) } + def initialize(session_uuid) + @session_accessor = OutOfBandSessionAccessor.new(session_uuid) end + # @return [X509::Attributes] def load - session = session_store.send(:load_session_from_redis, session_uuid) || {} + session = session_accessor.load X509::Attributes.new_from_json(session.dig('warden.user.user.session', :decrypted_x509)) end # @api private # Only used for convenience in tests - # @param [X509::Attributes] x509 + # @param [X509::Attributes] piv_cert_info def put(piv_cert_info, expiration = 5.minutes) session_data = { - 'warden.user.user.session' => { - decrypted_x509: piv_cert_info.to_h.to_json, - }, + decrypted_x509: piv_cert_info.to_h.to_json, } - session_store. - send(:set_session, {}, session_uuid, session_data, expire_after: expiration.to_i) - end - - private - - def session_store - config = Rails.application.config - config.session_store.new({}, config.session_options) + session_accessor.put(session_data, expiration) end end end From 2dd1e1ebd529073ac81ffd1f8ed8820c8378caab Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 3 Nov 2022 17:52:49 -0700 Subject: [PATCH 5/7] Remove reference to X509::Cacher class that does not exist --- app/services/x509/session_store.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/services/x509/session_store.rb b/app/services/x509/session_store.rb index c8184aef549..6985bc9ca35 100644 --- a/app/services/x509/session_store.rb +++ b/app/services/x509/session_store.rb @@ -2,7 +2,6 @@ # in an out-of-band fashion (using only the session UUID) instead of having access # to the user_session from Devise/Warden # Should only be used outside of a normal browser session (such as the OpenID Connect API) -# See X509::Cacher for accessing PII inside of a normal browser session module X509 class SessionStore attr_reader :session_accessor From 62dc63f5b19caaf8c4c8b8c50ddc5767cd4da1dc Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Fri, 4 Nov 2022 10:05:16 -0700 Subject: [PATCH 6/7] Revert "Refactor X509::SessionStore to use OutOfBandSessionStore" This reverts commit 7ae9f78a8bd8f5d0dce3d3fec1e8804268707efa. --- app/services/out_of_band_session_accessor.rb | 2 +- app/services/x509/session_store.rb | 30 ++++++++++++++------ 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/app/services/out_of_band_session_accessor.rb b/app/services/out_of_band_session_accessor.rb index 60a779838e3..64dad10a8bf 100644 --- a/app/services/out_of_band_session_accessor.rb +++ b/app/services/out_of_band_session_accessor.rb @@ -25,7 +25,7 @@ def destroy # @api private # Only used for convenience in tests - # @param [#to_h] data + # @param [Pii::Attributes] pii def put(data, expiration = 5.minutes) session_data = { 'warden.user.user.session' => data.to_h, diff --git a/app/services/x509/session_store.rb b/app/services/x509/session_store.rb index 6985bc9ca35..569196ca6c6 100644 --- a/app/services/x509/session_store.rb +++ b/app/services/x509/session_store.rb @@ -4,29 +4,41 @@ # Should only be used outside of a normal browser session (such as the OpenID Connect API) module X509 class SessionStore - attr_reader :session_accessor - - delegate :ttl, :destroy, to: :session_accessor + attr_reader :session_uuid def initialize(session_uuid) - @session_accessor = OutOfBandSessionAccessor.new(session_uuid) + @session_uuid = session_uuid + end + + def ttl + uuid = session_uuid + session_store.instance_eval { redis.ttl(prefixed(uuid)) } end - # @return [X509::Attributes] def load - session = session_accessor.load + session = session_store.send(:load_session_from_redis, session_uuid) || {} X509::Attributes.new_from_json(session.dig('warden.user.user.session', :decrypted_x509)) end # @api private # Only used for convenience in tests - # @param [X509::Attributes] piv_cert_info + # @param [X509::Attributes] x509 def put(piv_cert_info, expiration = 5.minutes) session_data = { - decrypted_x509: piv_cert_info.to_h.to_json, + 'warden.user.user.session' => { + decrypted_x509: piv_cert_info.to_h.to_json, + }, } - session_accessor.put(session_data, expiration) + session_store. + send(:set_session, {}, session_uuid, session_data, expire_after: expiration.to_i) + end + + private + + def session_store + config = Rails.application.config + config.session_store.new({}, config.session_options) end end end From 42e831278484d4799cd8af6e036b8137791cf40f Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Fri, 4 Nov 2022 10:06:02 -0700 Subject: [PATCH 7/7] Fix private API again --- app/services/x509/session_store.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/x509/session_store.rb b/app/services/x509/session_store.rb index 569196ca6c6..64aca718f34 100644 --- a/app/services/x509/session_store.rb +++ b/app/services/x509/session_store.rb @@ -12,7 +12,7 @@ def initialize(session_uuid) def ttl uuid = session_uuid - session_store.instance_eval { redis.ttl(prefixed(uuid)) } + session_store.instance_eval { with_redis { |redis| redis.ttl(prefixed(uuid)) } } end def load