diff --git a/UPGRADE.md b/UPGRADE.md index c4f8982..369ecae 100644 --- a/UPGRADE.md +++ b/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. diff --git a/docs/api/store.md b/docs/api/store.md index 91d8003..95471d6 100644 --- a/docs/api/store.md +++ b/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: diff --git a/docs/security/encryption.md b/docs/security/encryption.md index 57d8b07..1d8a999 100644 --- a/docs/security/encryption.md +++ b/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 diff --git a/spec/configuration_spec.cr b/spec/configuration_spec.cr index 82d1b24..cbb8b24 100644 --- a/spec/configuration_spec.cr +++ b/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 diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index c036fd3..0689098 100644 --- a/spec/spec_helper.cr +++ b/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 diff --git a/src/configuration.cr b/src/configuration.cr index bfaac14..69da7f2 100644 --- a/src/configuration.cr +++ b/src/configuration.cr @@ -236,24 +236,25 @@ module Session # The encryptor for the current settings. It is reused while they stay the same, so PBKDF2 # (`use_kdf`) runs once per secret, salt and iteration count, not on every encrypt and decrypt. def encryptor : Message::Encryptor - validate_secret! key = {secret, digest_algorithm, digest_fallback, use_kdf, kdf_iterations, kdf_salt} - cached = @encryptor_cache + validate_secret!(key[0]) + cached = @encryptor_cache.get(:acquire) return cached.encryptor if cached && cached.key == key encryptor = Message::Encryptor.new( key[0], - digest: digest_algorithm, - fallback_digest: digest_fallback ? :sha1 : nil, - use_kdf: use_kdf, - kdf_iterations: kdf_iterations, - kdf_salt: kdf_salt + digest: key[1], + fallback_digest: key[2] ? :sha1 : nil, + use_kdf: key[3], + kdf_iterations: key[4], + kdf_salt: key[5] ) - @encryptor_cache = EncryptorCache.new(key, encryptor) + @encryptor_cache.set(EncryptorCache.new(key, encryptor), :release) encryptor end - # One reference, so a fiber on another thread sees the old pair or the new one, never half of each. + # The settings and the encryptor built from them, published together through one atomic + # reference: a fiber on another thread sees the old pair or the new one, never half of each. private class EncryptorCache getter key : {String, Symbol, Bool, Bool, Int32, String} getter encryptor : Message::Encryptor @@ -262,10 +263,11 @@ module Session end end - @encryptor_cache : EncryptorCache? = nil + @encryptor_cache = Atomic(EncryptorCache?).new(nil) - # Raises `InsecureSecretException` when `store` encrypts with the secret (a `CookieStore`, or - # any store with `encrypt_redis_data`) and the secret is missing, published or too short. + # Raises `InsecureSecretException` when `store` encrypts with the secret (`Store#encrypts_with_secret?`: + # a `CookieStore`, or a Redis store with `encrypt_redis_data`) and the secret is missing, published + # or too short. def validate_secret_for!(store : Store) : Nil validate_secret! if store.encrypts_with_secret? end @@ -277,7 +279,11 @@ module Session # Validate secret security - warns or raises based on configuration def validate_secret! : Nil - key = secret + validate_secret!(secret) + end + + # Checks `key`, the secret already read: `secret` reads the environment on every call. + private def validate_secret!(key : String) : Nil if key.bytesize < MIN_SECRET_BYTES raise InsecureSecretException.new( "The session secret is #{key.bytesize} bytes; it must be at least #{MIN_SECRET_BYTES} bytes " \ @@ -289,7 +295,8 @@ module Session if require_secure_secret raise InsecureSecretException.new( "Using default secret is not allowed when require_secure_secret is enabled. " \ - "Please configure a secure secret using Session.configure { |c| c.secret = \"your-secure-secret\" }" + "Set SESSION_SECRET to a random value of at least #{MIN_SECRET_BYTES} bytes (`openssl rand -hex 32`), " \ + "or configure one with Session.configure { |c| c.secret = ENV[\"SESSION_SECRET\"] }" ) end @@ -297,7 +304,8 @@ module Session unless @secret_warning_logged Log.warn { "SECURITY WARNING: Using default session secret. This is insecure for production. " \ - "Configure a secure secret: Session.configure { |c| c.secret = \"your-32-byte-secret\" }" + "Set SESSION_SECRET to a random value of at least #{MIN_SECRET_BYTES} bytes (`openssl rand -hex 32`), " \ + "or configure one with Session.configure { |c| c.secret = ENV[\"SESSION_SECRET\"] }" } @secret_warning_logged = true end diff --git a/src/store.cr b/src/store.cr index 12ff9f7..81e1a75 100644 --- a/src/store.cr +++ b/src/store.cr @@ -22,7 +22,10 @@ module Session # Storage abstraction - must be implemented by subclasses abstract def storage : String - # Whether this store encrypts sessions with `Session.config.secret`: with `encrypt_redis_data`. + # Whether this store encrypts sessions with `Session.config.secret`. `Session.configure` and + # `SessionHandler` require a usable secret only when it does. The default follows + # `encrypt_redis_data` (`RedisStore`, `ClusteredRedisStore`); a store that never encrypts + # returns false (`MemoryStore`), and one that always does returns true (`CookieStore`). def encrypts_with_secret? : Bool Session.config.encrypt_redis_data end diff --git a/src/stores/memory_store.cr b/src/stores/memory_store.cr index 19c9749..a10cdc2 100644 --- a/src/stores/memory_store.cr +++ b/src/stores/memory_store.cr @@ -17,6 +17,12 @@ module Session self.class.name end + # Sessions stay in this process as objects: nothing is encrypted, so the secret is never read + # (`encrypt_redis_data`, set by the :production, :high_security and :clustered presets, does not apply). + def encrypts_with_secret? : Bool + false + end + def [](key : String) : T if session = sessions[key]? if session.valid?