Avoid resetting the session key with each call to put/4 (#72)

What changed?
============

After upgrading to the new v0.7.0 of redbird, I found I consistently 
got `Plug.Conn.CookieOverflowErrors` after a few requests.

This is strange—the session key is supposed to be stable, yet it 
was changing and growing in size with each request.

The problem
-------------

The redis key gets re-signed by Redbird.Crypto.sign with every 
call to put/4—even if that key was already signed, leading to a 
new (larger) key with each request! 

This stems from calling `Redbird.Key.sign_key/2` unconditionally
(as of 299906f), which results in signing the already-signed
user-supplied key.

We end up with a key with arbitrary levels of nested signatures.
This is bad in its own right (redbird isn't supposed to move to a new
key each `put`), but also leads to the key growing without bound,
as each new layer of signature results in a somewhat-longer key.

Eventually, this overflows the maximum length of 4096 bytes which 
plug is willing to allow in a cookie, leading to a 
`Plug.Conn.CookieOverflowError`.

The fix
-------

The fix is to sign the key exactly once, right after generating it,
 then verify that the key is signed properly in `put/4` in the same 
way as in `get/3` or `delete/2`.

Implementation notes
----------------------

The function previously known as Redbird.Key.deletable?/2 is
now Redbird.Key.accessible?/2, and is used in put/4, get/3, and
delete/2.

A key is now signed exactly once, immediately after being generated,
and verified each time it is used. This fixes the bug described
in the previous commit, where the session key is re-signed
with each call to put/4, causing it to grow without bound and eventually
overflow.

This leaves us with the question of what to do if a key passed to put/4
is invalid. At that point, the redis key sent in the cookie can't be trusted 
(e.g.  it's unsigned or signed incorrectly). We can't allow
malicious users to set arbitrary redis keys.

Therefore, if the signature can't be validate, we treat it as though it was 
never sent. So, we start over with a new generated key in the same way 
as when the key isn't specified, and that's the key we return. 

Keeping expiration in sync with Plug.Crypto
--------------------------------------------

We use `Plug.Crypto` to check a key's validity. 

But Plug.Crypto defaults to a validity period of 24 hours.
So we would effectively expire sessions after 24 hours, irrespective
of redbird's configured expiration time.

To address that, we pass our expiration time through as the max_age 
when verifying the signature. That way, we avoid expiring sessions 
early when redbird is configured to keep session for longer than 24 
hours (as indeed it is by default.)

lib/redbird/key.ex

@@ -11,13 +11,9 @@ defmodule Redbird.Key do
     to_string(namespace) <> Crypto.sign_key(key, conn)
   end
 
-  def verify(key, conn) do
-    Crypto.verify_key(key, conn)
-  end
-
-  def deletable?(key, conn) do
+  def accessible?(key, conn, opts \\ []) do
     with {:ok, key, _} <- extract_key(key),
-         {:ok, _verified_key} <- Crypto.verify_key(key, conn) do
+         {:ok, _verified_key} <- Crypto.verify_key(key, conn, opts) do
       true
     else
       _ -> false

lib/redbird/plug/session/redis.ex

@@ -14,9 +14,10 @@ defmodule Plug.Session.REDIS do
     opts
   end
 
-  def get(conn, prospective_key, _init_options) do
-    with {:ok, key, _} <- Key.extract_key(prospective_key),
-         {:ok, _verified_key} <- Key.verify(key, conn),
+  def get(conn, prospective_key, init_options) do
+    max_age = session_expiration(init_options)
+
+    with true <- Key.accessible?(prospective_key, conn, max_age: max_age),
          value when is_binary(value) <- get(prospective_key) do
       {prospective_key, Value.deserialize(value)}
     else
@@ -25,17 +26,28 @@ defmodule Plug.Session.REDIS do
   end
 
   def put(conn, nil, data, init_options) do
-    put(conn, Key.generate(), data, init_options)
+    key =
+      Key.generate()
+      |> Key.sign_key(conn)
+
+    put(conn, key, data, init_options)
   end
 
   def put(conn, key, data, init_options) do
-    key
-    |> Key.sign_key(conn)
-    |> set_key_with_retries(Value.serialize(data), session_expiration(init_options), 1)
+    max_age = session_expiration(init_options)
+
+    if Key.accessible?(key, conn, max_age: max_age) do
+      key
+      |> set_key_with_retries(Value.serialize(data), max_age, 1)
+    else
+      put(conn, nil, data, init_options)
+    end
   end
 
-  def delete(conn, redis_key, _init_options) do
-    if Key.deletable?(redis_key, conn), do: del(redis_key)
+  def delete(conn, redis_key, init_options) do
+    max_age = session_expiration(init_options)
+
+    if Key.accessible?(redis_key, conn, max_age: max_age), do: del(redis_key)
 
     :ok
   end

test/redbird/key_test.exs

@@ -102,34 +102,34 @@ defmodule Redbird.KeyTest do
     end
   end
 
-  describe "deletable?/2" do
-    test "verifiable keys with a namespace are deletable" do
+  describe "accessible?/2" do
+    test "verifiable keys with a namespace are accessible" do
       conn = Redbird.ConnCase.signed_conn()
       generated_key = Key.generate(Key.namespace())
       signed_key = Key.sign_key(generated_key, conn)
 
-      actual = Key.deletable?(signed_key, conn)
+      actual = Key.accessible?(signed_key, conn)
 
       assert actual == true
     end
 
-    test "verifiable keys without a namespace are deletable" do
+    test "verifiable keys without a namespace are accessible" do
       conn = Redbird.ConnCase.signed_conn()
       generated_key = Key.generate("")
       signed_key = Key.sign_key(generated_key, conn)
 
-      actual = Key.deletable?(signed_key, conn)
+      actual = Key.accessible?(signed_key, conn)
 
       assert actual == true
     end
 
-    test "non-verifiable keys are not-deletable" do
+    test "non-verifiable keys are not-accessible" do
       conn = Redbird.ConnCase.signed_conn()
       generated_key = Key.generate("")
       signed_key = Key.sign_key(generated_key, conn)
       tampered_key = signed_key <> "sneaky"
 
-      actual = Key.deletable?(tampered_key, conn)
+      actual = Key.accessible?(tampered_key, conn)
 
       assert actual == false
     end

test/redbird_test.exs

@@ -57,6 +57,29 @@ defmodule RedbirdTest do
       assert get_session(conn, :foo) == "bar"
     end
 
+    test "it reuses the session key between requests" do
+      secret = generate_secret()
+
+      conn =
+        :get
+        |> conn("/")
+        |> sign_conn_with(secret)
+        |> put_session(:foo, "bar")
+        |> send_resp(200, "")
+
+      session_key = conn.resp_cookies["_session_key"].value
+
+      conn =
+        :get
+        |> conn("/")
+        |> recycle_cookies(conn)
+        |> sign_conn_with(secret)
+        |> put_session(:foo, "baz")
+        |> send_resp(200, "")
+
+      assert conn.resp_cookies["_session_key"].value == session_key
+    end
+
     test "it allows configuring session expiration" do
       secret = generate_secret()