Session: a memory store never needs the secret; safer encryptor cache

UPGRADE.md

@@ -8,18 +8,42 @@ cookies with a key anyone could use to forge a session. Now:
 - `config.secret` defaults to `SESSION_SECRET`. Without it, a random key for this process (one warning), so
   sessions end when the process restarts and are not shared between instances. Set `SESSION_SECRET` to keep them.
 - In production and staging a missing secret raises `Session::InsecureSecretException` wherever the secret is
-  used. With a `CookieStore`, or `encrypt_redis_data`, `Session::SessionHandler` then fails every request with `Session::InsecureSecretException` (a 500, logged by the server) instead of answering without a working session; a store that does not encrypt never reads the secret, so nothing is raised.
+  used. With a `CookieStore`, or a Redis store with `encrypt_redis_data`, `Session::SessionHandler` then fails every
+  request with `Session::InsecureSecretException` (a 500, logged by the server) instead of answering without a
+  working session; a store that does not encrypt never reads the secret, so nothing is raised. A `MemoryStore`
+  never encrypts, also under the `:production`, `:high_security` and `:clustered` presets, which turn
+  `encrypt_redis_data` on.
 - A secret shorter than 32 bytes raises `Session::InsecureSecretException` too, instead of an `ArgumentError` from
   AES that the handler logged and swallowed. This holds with `use_kdf` on (the `:production`, `:high_security` and
   `:clustered` presets): the HMAC signature uses the secret itself, so PBKDF2 does not make a short one safe.
-- `Session.configure` checks the secret when the configured store encrypts with it (a `CookieStore`, or
-  `encrypt_redis_data`), so a missing or short secret fails at boot instead of on every request. Set the secret (or
-  `SESSION_SECRET`) before, or inside, the `Session.configure` block that sets such a store.
+- `Session.configure` checks the secret when the configured store encrypts with it (a `CookieStore`, or a Redis
+  store with `encrypt_redis_data`), so a missing or short secret fails at boot instead of on every request. Set the
+  secret (or `SESSION_SECRET`) before, or inside, the `Session.configure` block that sets such a store.
+- For store authors: `Session::Store#encrypts_with_secret?` tells `Session.configure` and `Session::SessionHandler`
+  whether the store needs the secret. The default follows `encrypt_redis_data`; `MemoryStore` returns `false` and
+  `CookieStore` `true`. A custom store that encrypts with `Session.config.encryptor` regardless of
+  `encrypt_redis_data` should override it to return `true`, and one that never encrypts should return `false`.
 - `require_secure_secret` now reads `CRYSTAL_ENV` (case-insensitive) when it is checked, not when the shard loads;
   setting it explicitly still wins.
 - `config.secret = nil` goes back to the default. `Configuration::DEFAULT_SECRET` still exists and is still refused
   where `require_secure_secret` holds.
 
+## Expired sessions are refused when loaded (behavior change)
+
+No API change, but sessions can now end sooner than before. Semver impact: patch (security fix).
+
+- Every store now refuses a session whose `expires_at` has passed when it is loaded, and the request starts a fresh
+  session. A copied `CookieStore` cookie no longer stays signed in after the timeout.
+- `RedisStore` sessions now end at their `expires_at` even for a visitor who keeps using them, when
+  `sliding_expiration` is `false` (the default): `expires_at` is set when the session is created (or regenerated),
+  so the session lasts `timeout` from then. Before, every save ran `SETEX`, which reset the Redis TTL, and nothing
+  read `expires_at`, so an active session lived indefinitely. To keep sessions alive while they are used, set
+  `config.sliding_expiration = true`: each request then moves `expires_at` to `timeout` from now. The
+  `:development`, `:production`, `:high_security` and `:clustered` presets already set it, so an app that applies
+  one of them keeps sliding sessions.
+- A stored or signed payload without `expires_at` no longer loads. Payloads written by released versions always
+  carry it. A `Session::Base` subclass that overrides `after_initialize` must call `super`.
+
 ## Crystal 1.22.1 or newer (no code change)
 
 `shard.yml` now declares `crystal: ">= 1.22.1"`. It was `crystal: 1.4.1`, a 1.4.1 minimum that no CI job ever built.
@@ -38,20 +62,6 @@ Nothing here needs a code change. Semver impact: minor (new opt-in API, one bug
 - `Session::Store#current_session?` returns the request's session without creating one.
 - `Session::Flash#to_h` returns the messages a view shows.
 
-## Expired sessions are refused when loaded (behavior change)
-
-No API change, but sessions can now end sooner than before. Semver impact: patch (security fix).
-
-- Every store now refuses a session whose `expires_at` has passed when it is loaded, and the request starts a fresh
-  session. A copied `CookieStore` cookie no longer stays signed in after the timeout.
-- `RedisStore` sessions now end at their `expires_at` even for a visitor who keeps using them, when
-  `sliding_expiration` is `false` (the default): `expires_at` is set when the session is created (or regenerated),
-  so the session lasts `timeout` from then. Before, every save ran `SETEX`, which reset the Redis TTL, and nothing
-  read `expires_at`, so an active session lived indefinitely. To keep sessions alive while they are used, set
-  `config.sliding_expiration = true`: each request then moves `expires_at` to `timeout` from now.
-- A stored or signed payload without `expires_at` no longer loads. Payloads written by released versions always
-  carry it. A `Session::Base` subclass that overrides `after_initialize` must call `super`.
-
 ## Upgrading from the `SessionData` API to `Session::Base`
 
 Releases before v1.0.28 defined sessions with the `SessionData` module and the `SessionId(T)` wrapper. From v1.0.28 on, a single `Session::Base` class replaces both. There was no 2.0 release: this migration happens inside the 1.0.x line, and the "Before" snippets below show code that no longer compiles.

docs/api/store.md

@@ -40,6 +40,12 @@ Subclasses must implement:
 | `session_key` | `String` | Cookie name (default `"_session"`) |
 | `create_session_cookie` | `HTTP::Cookie` | Build a session cookie for a host |
 
+## Secret
+
+| Method | Return | Description |
+|--------|--------|-------------|
+| `encrypts_with_secret?` | `Bool` | Whether the store encrypts with `Session.config.secret`. `Session.configure` and `SessionHandler` require a usable secret only when it does. Default: `encrypt_redis_data`. `MemoryStore` returns `false`, `CookieStore` `true`. A custom store that encrypts with `Session.config.encryptor` regardless of `encrypt_redis_data` overrides it to return `true` |
+
 ## Callbacks
 
 `on(event, session_id, session)` dispatches to configured callbacks:

docs/security/encryption.md

@@ -51,7 +51,10 @@ The default digest is `:sha256`. If you're migrating from an older version that
   `InsecureSecretException`, from `Session.configure` when the store encrypts with it
 - Unset, `secret` is `SESSION_SECRET`. Without that, `InsecureSecretException` where `require_secure_secret` holds
   (production and staging by default, read from `CRYSTAL_ENV` each time the secret is read), and a random per-process
-  key with one warning elsewhere. With a `CookieStore`, or `encrypt_redis_data`, `Session::SessionHandler` then fails every request with `Session::InsecureSecretException` (a 500, logged by the server) instead of answering without a working session; a store that does not encrypt never reads the secret, so nothing is raised.
+  key with one warning elsewhere. With a `CookieStore`, or a Redis store with `encrypt_redis_data`,
+  `Session::SessionHandler` then fails every request with `Session::InsecureSecretException` (a 500, logged by the
+  server) instead of answering without a working session; a store that does not encrypt never reads the secret, so
+  nothing is raised. A `MemoryStore` never encrypts, also under the presets that turn `encrypt_redis_data` on.
 - The secret older releases used by default (`Configuration::DEFAULT_SECRET`) is public: set explicitly, it is refused
   where `require_secure_secret` holds and warned about elsewhere
 

spec/configuration_spec.cr

@@ -1,4 +1,5 @@
 require "./spec_helper"
+require "log/spec"
 
 describe Session::Configuration do
   describe "#using_default_secret?" do
@@ -385,6 +386,57 @@ describe Session::Configuration do
       config.secret = "another-secret-nobody-knows-0123456789"
       config.encryptor.should_not be second
     end
+
+    it "reads the secret once per call" do
+      config = SecretReadCounter.new
+      config.secret = "a-secret-nobody-else-knows-0123456789"
+
+      config.encryptor
+      config.reads.should eq 1
+      config.encryptor
+      config.reads.should eq 2
+    end
+
+    it "gives every fiber a working encryptor while the settings change" do
+      config = Session::Configuration.new
+      config.secret = "a-secret-nobody-else-knows-0123456789"
+      done = Channel(Exception?).new
+
+      8.times do |i|
+        spawn do
+          50.times { |j| config.kdf_salt = "salt-#{(i + j) % 3}"; config.encryptor.encrypt_and_sign("data") }
+          done.send(nil)
+        rescue ex
+          done.send(ex)
+        end
+      end
+
+      8.times { done.receive.should be_nil }
+    end
+  end
+
+  describe "the default secret messages" do
+    it "suggest a secret that is long enough" do
+      config = Session::Configuration.new
+      config.secret = Session::Configuration::DEFAULT_SECRET
+      config.require_secure_secret = true
+
+      error = expect_raises(Session::InsecureSecretException) { config.validate_secret! }
+      error.message.to_s.should contain "openssl rand -hex 32"
+      error.message.to_s.should_not contain "your-"
+    end
+
+    it "warn with a secret that is long enough" do
+      config = Session::Configuration.new
+      config.secret = Session::Configuration::DEFAULT_SECRET
+      config.require_secure_secret = false
+
+      Log.capture do |logs|
+        config.validate_secret!
+        logs.check(:warn, /openssl rand -hex 32/)
+        logs.entry.message.should_not contain "your-"
+      end
+    end
   end
 
   describe "#validate_secret_for!" do
@@ -404,10 +456,54 @@ describe Session::Configuration do
   end
 end
 
+# Counts the reads of `secret`: each one reads the environment when no secret is configured.
+class SecretReadCounter < Session::Configuration
+  getter reads = 0
+
+  def secret : String
+    @reads += 1
+    super
+  end
+end
+
+describe "reset_config" do
+  it "restores what a preset and the cookie options change" do
+    config = Session.config
+    config.apply_preset(:high_security)
+    config.kdf_salt = "another-salt"
+    config.cookie_secure = false
+    config.cookie_same_site = HTTP::Cookie::SameSite::Lax
+    config.allow_insecure_cookies = true
+    config.retry_config = Session::RetryConfig.new(max_attempts: 9)
+    config.circuit_breaker_config = Session::CircuitBreakerConfig.new(failure_threshold: 99)
+
+    reset_config
+
+    defaults = Session::Configuration.new
+    {% for name in %w(timeout encrypt_redis_data sliding_expiration log_errors circuit_breaker_enabled enable_retry
+                     compress_data compression_threshold bind_to_ip bind_to_user_agent use_kdf kdf_iterations kdf_salt
+                     digest_algorithm digest_fallback fail_fast_on_corruption cookie_secure cookie_same_site
+                     allow_insecure_cookies) %}
+      config.{{name.id}}.should eq defaults.{{name.id}}
+    {% end %}
+    config.retry_config.max_attempts.should eq defaults.retry_config.max_attempts
+    config.circuit_breaker_config.failure_threshold.should eq defaults.circuit_breaker_config.failure_threshold
+    config.cluster.enabled.should be_false
+  end
+end
+
+# Encrypts with the secret when `encrypt_redis_data` is on, as `RedisStore` does, without a Redis server.
+class RedisLikeStore < Session::MemoryStore(UserSession)
+  def encrypts_with_secret? : Bool
+    Session.config.encrypt_redis_data
+  end
+end
+
 describe "Session.configure" do
   it "fails at boot when the configured store encrypts with an unusable secret" do
     expect_raises(Session::InsecureSecretException, /at least 32 bytes/) do
       Session.configure do |c|
+        c.store = RedisLikeStore.new
         c.encrypt_redis_data = true
         c.secret = "short"
       end
@@ -418,16 +514,96 @@ describe "Session.configure" do
     expect_raises(Session::InsecureSecretException, /at least 32 bytes/) do
       Session.configure do |c|
         c.apply_preset(:production)
-        c.cookie_secure = true
-        c.encrypt_redis_data = true
+        c.store = RedisLikeStore.new
         c.secret = "abc"
       end
     end
   end
 
+  it "checks the secret without a store when encrypt_redis_data is on" do
+    expect_raises(Session::InsecureSecretException, /at least 32 bytes/) do
+      Session.configure do |c|
+        c.store = nil
+        c.encrypt_redis_data = true
+        c.secret = "short"
+      end
+    end
+  end
+
+  it "does not check the secret without a store when encrypt_redis_data is off" do
+    Session.configure do |c|
+      c.store = nil
+      c.secret = "short"
+    end
+  end
+
   it "does not check the secret when the store does not encrypt" do
     Session.configure do |c|
       c.secret = "short"
     end
   end
+
+  describe "a memory store in production without a secret" do
+    around_each do |example|
+      previous = {ENV["CRYSTAL_ENV"]?, ENV["SESSION_SECRET"]?}
+      ENV.delete("SESSION_SECRET")
+      ENV["CRYSTAL_ENV"] = "production"
+      example.run
+    ensure
+      {"CRYSTAL_ENV", "SESSION_SECRET"}.zip(previous.not_nil!).each do |name, value|
+        value ? (ENV[name] = value) : ENV.delete(name)
+      end
+    end
+
+    {:production, :high_security, :clustered}.each do |preset|
+      it "boots under the #{preset} preset: the memory store never reads the secret" do
+        Session.configure do |c|
+          c.apply_preset(preset)
+          c.store = Session::MemoryStore(UserSession).new
+          c.secret = nil
+        end
+
+        Session.config.encrypt_redis_data.should be_true
+      end
+    end
+
+    it "serves requests under the production preset" do
+      store = Session::MemoryStore(UserSession).new
+      Session.configure do |c|
+        c.apply_preset(:production)
+        c.store = store
+        c.secret = nil
+      end
+      handler = Session::SessionHandler.new(store)
+      handler.next = ->(context : HTTP::Server::Context) { context.response.status_code = 200 }
+      context = HTTP::Server::Context.new(HTTP::Request.new("GET", "/"), HTTP::Server::Response.new(IO::Memory.new))
+
+      handler.call(context)
+
+      context.response.status_code.should eq 200
+    end
+  end
+end
+
+describe "Store#encrypts_with_secret?" do
+  it "is false for a memory store, whatever encrypt_redis_data says" do
+    Session.config.encrypt_redis_data = true
+
+    Session::MemoryStore(UserSession).new.encrypts_with_secret?.should be_false
+  end
+
+  it "is true for a cookie store" do
+    Session::CookieStore(UserSession).new.encrypts_with_secret?.should be_true
+  end
+
+  if REDIS_AVAILABLE
+    it "follows encrypt_redis_data for a Redis store" do
+      store = Session::RedisStore(UserSession).new(client: redis_client)
+
+      Session.config.encrypt_redis_data = true
+      store.encrypts_with_secret?.should be_true
+      Session.config.encrypt_redis_data = false
+      store.encrypts_with_secret?.should be_false
+    end
+  end
 end

spec/spec_helper.cr

@@ -24,24 +24,39 @@ end
 
 Session.use_store(Session::MemoryStore(UserSession))
 
-# Reset configuration to defaults before each test
+# Reset configuration to defaults before each test: every setting a preset or a spec changes
+# goes back to the value a new `Session::Configuration` has.
 def reset_config
-  Session.config.store = Session::MemoryStore(UserSession).new
-  Session.config.timeout = 1.hour
-  Session.config.secret = Session::Configuration::DEFAULT_SECRET
-  Session.config.require_secure_secret = false
-  Session.config.digest_algorithm = :sha256
-  Session.config.digest_fallback = true
-  Session.config.use_kdf = false
-  Session.config.kdf_iterations = 100_000
-  Session.config.encrypt_redis_data = false
-  Session.config.circuit_breaker_enabled = false
-  Session.config.sliding_expiration = false
-  Session.config.compress_data = false
-  Session.config.bind_to_ip = false
-  Session.config.bind_to_user_agent = false
-  Session.config.metrics_backend = Session::Metrics::NullBackend.new
-  Session.config.cluster = Session::ClusterConfig.new
+  defaults = Session::Configuration.new
+  config = Session.config
+  config.store = Session::MemoryStore(UserSession).new
+  config.timeout = defaults.timeout
+  config.session_key = defaults.session_key
+  config.secret = Session::Configuration::DEFAULT_SECRET
+  config.require_secure_secret = false
+  config.cookie_secure = defaults.cookie_secure
+  config.cookie_same_site = defaults.cookie_same_site
+  config.allow_insecure_cookies = defaults.allow_insecure_cookies
+  config.digest_algorithm = defaults.digest_algorithm
+  config.digest_fallback = defaults.digest_fallback
+  config.use_kdf = defaults.use_kdf
+  config.kdf_iterations = defaults.kdf_iterations
+  config.kdf_salt = defaults.kdf_salt
+  config.encrypt_redis_data = defaults.encrypt_redis_data
+  config.circuit_breaker_enabled = defaults.circuit_breaker_enabled
+  config.circuit_breaker_config = defaults.circuit_breaker_config
+  config.sliding_expiration = defaults.sliding_expiration
+  config.compress_data = defaults.compress_data
+  config.compression_threshold = defaults.compression_threshold
+  config.bind_to_ip = defaults.bind_to_ip
+  config.bind_to_user_agent = defaults.bind_to_user_agent
+  config.trust_proxy_headers = defaults.trust_proxy_headers
+  config.enable_retry = defaults.enable_retry
+  config.retry_config = defaults.retry_config
+  config.log_errors = defaults.log_errors
+  config.fail_fast_on_corruption = defaults.fail_fast_on_corruption
+  config.metrics_backend = Session::Metrics::NullBackend.new
+  config.cluster = Session::ClusterConfig.new
 end
 
 Spec.before_each do