Session hardening: secret in production, real client binding, per-request cookies, authenticate, CI

.github/workflows/crystal.yml

@@ -33,8 +33,8 @@ jobs:
       - name: Install dependencies
         run: shards install
 
-      # - name: Check code style
-      #   run: crystal tool format ./src ./spec --check
+      - name: Check code style
+        run: crystal tool format --check src spec
 
       - name: Run tests
         run: crystal spec --error-trace
@@ -45,7 +45,9 @@ jobs:
     runs-on: ubuntu-latest
     needs:
       - test
-    if: ${{ success()}}
+    # Releasing tags the version and commits the bump to master, so only a push to master may do it
+    # (on a pull request the tag already exists and the job fails, or would write to master).
+    if: ${{ success() && github.event_name == 'push' }}
     steps:
       - name: Checkout
         uses: actions/checkout@v2

spec/binding_spec.cr

@@ -1,6 +1,11 @@
 require "./spec_helper"
 
 describe Session::ClientFingerprint do
+  # These examples describe a deployment behind a trusted proxy that names the client in
+  # X-Forwarded-For / X-Real-IP; client_binding_spec covers the default, which ignores them.
+  before_each { Session.config.trust_proxy_headers = true }
+  after_each { Session.config.trust_proxy_headers = false }
+
   describe ".binding_enabled?" do
     it "returns false when both bindings disabled" do
       Session.config.bind_to_ip = false

spec/client_binding_spec.cr

@@ -0,0 +1,95 @@
+require "./spec_helper"
+
+private class Seen
+  include HTTP::Handler
+
+  getter ids = [] of String
+
+  def call(context : HTTP::Server::Context)
+    ids << Session.config.store.as(Session::MemoryStore(UserSession)).current_session.session_id
+  end
+end
+
+private def request_from(cookie : String?, ip : String = "10.0.0.1", user_agent : String = "agent-a", headers : HTTP::Headers = HTTP::Headers.new) : HTTP::Server::Context
+  headers["User-Agent"] = user_agent
+  request = HTTP::Request.new("GET", "/", headers)
+  request.remote_address = Socket::IPAddress.new(ip, 40000)
+  request.cookies << HTTP::Cookie.new(name: Session.config.session_key, value: cookie) if cookie
+  HTTP::Server::Context.new(request, HTTP::Server::Response.new(IO::Memory.new))
+end
+
+private def with_handler(&)
+  reset_config
+  Session.config.bind_to_ip = true
+  Session.config.bind_to_user_agent = true
+  store = Session::MemoryStore(UserSession).new
+  Session.config.store = store
+  seen = Seen.new
+  handler = Session::SessionHandler(UserSession).new(store)
+  handler.next = seen
+  yield handler, seen
+ensure
+  Session.config.bind_to_ip = false
+  Session.config.bind_to_user_agent = false
+  Session.config.trust_proxy_headers = false
+end
+
+# The session id the first request of a new visitor was given.
+private def first_visit(handler, seen, **client) : String
+  handler.call(request_from(nil, **client))
+  seen.ids.last
+end
+
+describe "session binding in the handler" do
+  it "keeps a session for the client it was first used from" do
+    with_handler do |handler, seen|
+      id = first_visit(handler, seen)
+
+      handler.call(request_from(id))
+
+      seen.ids.last.should eq id
+    end
+  end
+
+  it "does not let another browser use the session" do
+    with_handler do |handler, seen|
+      id = first_visit(handler, seen)
+
+      handler.call(request_from(id, user_agent: "agent-b"))
+
+      seen.ids.last.should_not eq id
+    end
+  end
+
+  it "does not let another address use the session" do
+    with_handler do |handler, seen|
+      id = first_visit(handler, seen)
+
+      handler.call(request_from(id, ip: "203.0.113.9"))
+
+      seen.ids.last.should_not eq id
+    end
+  end
+
+  it "ignores X-Forwarded-For unless a trusted proxy is configured, since any client can send it" do
+    with_handler do |handler, seen|
+      id = first_visit(handler, seen)
+
+      forged = HTTP::Headers{"X-Forwarded-For" => "10.0.0.1"}
+      handler.call(request_from(id, ip: "203.0.113.9", headers: forged))
+
+      seen.ids.last.should_not eq id
+    end
+  end
+
+  it "believes X-Forwarded-For behind a trusted proxy" do
+    with_handler do |handler, seen|
+      Session.config.trust_proxy_headers = true
+      id = first_visit(handler, seen, headers: HTTP::Headers{"X-Forwarded-For" => "198.51.100.7"})
+
+      handler.call(request_from(id, ip: "172.16.0.1", headers: HTTP::Headers{"X-Forwarded-For" => "198.51.100.7"}))
+
+      seen.ids.last.should eq id
+    end
+  end
+end

spec/clustered_redis_store_spec.cr

@@ -489,10 +489,8 @@ if REDIS_AVAILABLE
         # Clear from node 1
         store1.clear
 
-        sleep(150.milliseconds)
-
         # Node 2's cache should be cleared
-        store2.coordinator.local_cache.size.should eq 0
+        eventually { store2.coordinator.local_cache.size == 0 }.should be_true
 
         store1.shutdown
         store2.shutdown

spec/configuration_spec.cr

@@ -234,3 +234,47 @@ describe Session::Configuration do
     end
   end
 end
+
+describe Session::Configuration do
+  describe "the default secret outside development" do
+    around_each do |example|
+      previous = ENV["CRYSTAL_ENV"]?
+      example.run
+      previous ? (ENV["CRYSTAL_ENV"] = previous) : ENV.delete("CRYSTAL_ENV")
+    end
+
+    {"production", "staging"}.each do |environment|
+      it "is refused in #{environment}" do
+        ENV["CRYSTAL_ENV"] = environment
+        config = Session::Configuration.new
+
+        config.require_secure_secret.should be_true
+        expect_raises(Session::InsecureSecretException) { config.validate_secret! }
+      end
+    end
+
+    it "is only warned about in development" do
+      ENV["CRYSTAL_ENV"] = "development"
+      config = Session::Configuration.new
+
+      config.require_secure_secret.should be_false
+      config.validate_secret!
+    end
+
+    it "can still be switched off explicitly" do
+      ENV["CRYSTAL_ENV"] = "production"
+      config = Session::Configuration.new
+      config.require_secure_secret = false
+
+      config.validate_secret!
+    end
+
+    it "does not get in the way of a real secret in production" do
+      ENV["CRYSTAL_ENV"] = "production"
+      config = Session::Configuration.new
+      config.secret = "a-secret-nobody-else-knows-0123456789"
+
+      config.validate_secret!
+    end
+  end
+end

spec/cookie_store_isolation_spec.cr

@@ -0,0 +1,50 @@
+require "./spec_helper"
+
+describe "CookieStore request isolation" do
+  it "keeps each request's cookies to the request that sent them" do
+    store = Session::CookieStore(UserSession).new
+    seen = Channel({String, String?}).new
+
+    %w(alice bob).each do |name|
+      spawn do
+        cookies = HTTP::Cookies.new
+        cookies << HTTP::Cookie.new("who", name)
+        store.cookies = cookies
+        Fiber.yield # the other request runs before this one reads its cookies
+        seen.send({name, store.cookies["who"]?.try(&.value)})
+      end
+    end
+
+    results = 2.times.map { seen.receive }.to_a.sort_by(&.first)
+    results.should eq [{"alice", "alice"}, {"bob", "bob"}]
+  end
+
+  it "lets a session written in one request be read back only from that request's cookies" do
+    store = Session::CookieStore(UserSession).new
+    alice = UserSession.new
+    alice.username = "alice"
+    store[alice.session_id] = alice
+    alices_cookies = store.cookies
+
+    other = Channel(String?).new
+    spawn do
+      store.cookies = HTTP::Cookies.new # a visitor without a session cookie
+      other.send(store[alice.session_id]?.try(&.username))
+    end
+
+    other.receive.should be_nil
+    store.cookies = alices_cookies
+    store[alice.session_id].username.should eq "alice"
+  end
+
+  it "does not keep the cookies of requests that are over" do
+    store = Session::CookieStore(UserSession).new
+    done = Channel(Nil).new
+    50.times { spawn { store.cookies = HTTP::Cookies.new; done.send(nil) } }
+    50.times { done.receive }
+    Fiber.yield
+
+    store.cookies = HTTP::Cookies.new
+    store.@request_cookies.size.should be < 5
+  end
+end

spec/spec_helper.cr

@@ -47,3 +47,14 @@ end
 Spec.before_each do
   reset_config
 end
+
+# Waits for something that happens in another fiber, such as a Pub/Sub message, to become true.
+# Returns as soon as it does (so the usual case is faster than a fixed sleep) and gives up after
+# `timeout` (so a loaded machine does not make the spec fail), returning the last answer.
+def eventually(timeout : Time::Span = 3.seconds, & : -> Bool) : Bool
+  deadline = Time.instant + timeout
+  until (result = yield) || Time.instant >= deadline
+    sleep 10.milliseconds
+  end
+  result
+end

spec/store_spec.cr

@@ -227,3 +227,20 @@ if REDIS_AVAILABLE
     end
   end
 end
+
+describe Session::Store do
+  describe "#authenticate" do
+    it "applies the change and moves the session to a new id, so the old one stops working" do
+      store = Session::MemoryStore(UserSession).new
+      store.create
+      before = store.session_id
+
+      store.authenticate { |session| session.username = "alice" }
+
+      store.session_id.should_not eq before
+      store.current_session.username.should eq "alice"
+      store[store.session_id].username.should eq "alice"
+      store[before]?.should be_nil
+    end
+  end
+end

src/base.cr

@@ -27,6 +27,9 @@ module Session
     # and reach only the visitor they were meant for.
     getter flash : Flash = Flash.new
 
+    # The client the session was first used from, when session binding is enabled.
+    property client_fingerprint : ClientFingerprint? = nil
+
     def initialize
     end
 

src/binding.cr

@@ -57,21 +57,20 @@ module Session
       @ip_hash.nil? && @user_agent_hash.nil?
     end
 
-    # Extract client IP from request, considering proxy headers
+    # The client's address: the peer of the connection, or, behind a trusted proxy
+    # (`trust_proxy_headers`), the one the proxy reports.
     protected def self.extract_client_ip(request : HTTP::Request) : String?
-      # Check X-Forwarded-For first (for proxied requests)
-      if forwarded = request.headers["X-Forwarded-For"]?
-        # Take the first IP (original client)
-        return forwarded.split(",").first.strip
-      end
+      if Session.config.trust_proxy_headers
+        if forwarded = request.headers["X-Forwarded-For"]?
+          return forwarded.split(",").first.strip
+        end
 
-      # Check X-Real-IP
-      if real_ip = request.headers["X-Real-IP"]?
-        return real_ip.strip
+        if real_ip = request.headers["X-Real-IP"]?
+          return real_ip.strip
+        end
       end
 
-      # Fall back to remote address (may not be available in all contexts)
-      nil
+      request.remote_address.try(&.as?(Socket::IPAddress)).try(&.address)
     end
 
     private def hash_value(value : String) : String

src/configuration.cr

@@ -32,8 +32,13 @@ module Session
     # Cluster configuration for multi-node deployments with Redis Pub/Sub
     property cluster : ClusterConfig = ClusterConfig.new
 
-    # Security: require a secure secret (non-default) - raises if default secret is used
-    property require_secure_secret : Bool = false
+    # Environments that must not run on the published default secret.
+    PRODUCTION_ENVIRONMENTS = %w(production staging)
+
+    # Security: require a secure secret (non-default) - raises if default secret is used.
+    # The default secret is public, so anyone could forge a session cookie signed with it:
+    # it is refused in production and staging (`CRYSTAL_ENV`) unless turned off explicitly.
+    property require_secure_secret : Bool = PRODUCTION_ENVIRONMENTS.includes?(ENV["CRYSTAL_ENV"]?)
 
     # Digest algorithm for HMAC signatures (:sha256 recommended, :sha1 for legacy)
     property digest_algorithm : Symbol = :sha256
@@ -73,6 +78,10 @@ module Session
     property bind_to_ip : Bool = false
     property bind_to_user_agent : Bool = false
 
+    # Whether `X-Forwarded-For` and `X-Real-IP` name the client for IP binding. Any client can
+    # send them, so they are believed only when a trusted proxy in front of the app sets them.
+    property trust_proxy_headers : Bool = false
+
     # Track if secret warning has been logged to avoid spamming
     @secret_warning_logged : Bool = false
     property on_started : Proc(String, Session::Base, Nil) = ->(sid : String, data : Session::Base) do

src/handlers/session_handler.cr

@@ -55,16 +55,16 @@ module Session
       Log.warn { "Failed to clear corrupted session: #{ex.message}" }
     end
 
+    # A session is tied to the client it was first used from: a request from a different
+    # client is refused (and answered with a fresh session by `load_session`'s rescue).
     private def validate_session_binding(request : HTTP::Request)
-      # Get stored fingerprint from session data if available
-      # Note: This requires the session data type to have a client_fingerprint property
-      # For now, we create a new fingerprint and validate against request
-      # In a full implementation, the fingerprint would be stored in the session
-      _fingerprint = ClientFingerprint.from_request(request)
+      session = @session.current_session
 
-      # If session has a stored fingerprint, validate it
-      # This is a simplified implementation - in practice you'd store the fingerprint
-      # in the session data when the session is created
+      if fingerprint = session.client_fingerprint
+        fingerprint.validate!(request)
+      else
+        session.client_fingerprint = ClientFingerprint.from_request(request)
+      end
     end
 
     private def generate_session_id

src/store.cr

@@ -106,6 +106,18 @@ module Session
       current_session
     end
 
+    # Change the session's privileges (log in, switch user, elevate) and give it a new id in
+    # one step. Re-using the id a visitor had before logging in lets anyone who planted or
+    # observed it (session fixation) share the authenticated session.
+    #
+    # ```
+    # store.authenticate { |session| session.user_id = user.id }
+    # ```
+    def authenticate(& : T -> _) : T
+      yield current_session
+      regenerate_id
+    end
+
     def create : T
       self.current_session = T.new
       self[session_id] = current_session