3 files changed, 108 insertions, 62 deletions
diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.lua
index 0573549..2ef2328 100644
--- a/custom/extensions/auth-file.lua
+++ b/custom/extensions/auth-file.lua
@@ -42,10 +42,12 @@
 --     luarocks install luaossl OPENSSL_DIR="$(brew --prefix openssl)"
 --     luarocks install luaposix
 --
--- There is no server-side session store and no CSRF token, so a deleted
--- account's cookie stays valid until it expires and instances sharing a
--- secret file accept each other's cookies. That is acceptable for gating
--- read access to a git browser, so weigh it before guarding anything more.
+-- A session is signed together with its account's hash, so changing the
+-- password or deleting the account ends it. There is no server-side session
+-- store and no CSRF token, so instances sharing a secret file accept each
+-- other's cookies, and only a new secret file ends every session at once.
+-- That is acceptable for gating read access to a git browser, so weigh it
+-- before guarding anything more.
 
 -- The values that follow are the configuration and are meant to be edited.
 
@@ -323,8 +325,19 @@ function is_safe_redirect(url)
 	return true
 end
 
+-- The signature covers the payload and a binding that the cookie never
+-- carries, so a value only verifies while its binding stays the same.
+local function sign(payload, binding)
+	local hmac = require("openssl.hmac")
+	local signed = payload .. "|" .. (binding or "")
+	return tohex(hmac.new(get_secret(), "sha256"):final(signed))
+end
+
 -- Return the value carried by a signed cookie, or nil if it does not verify.
-function validate_value(expected_field, cookie)
+-- bind, when given, maps the value to the binding its signature has to cover,
+-- or to nil to turn it down, which is how a session meets its account's
+-- current hash.
+function validate_value(expected_field, cookie, bind)
 	local i = 0
 	local value = ""
 	local field = ""
@@ -364,46 +377,50 @@ function validate_value(expected_field, cookie)
 		return nil
 	end
 
-	local payload = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
-	local hmac = require("openssl.hmac")
-	local expected_signature = tohex(hmac.new(get_secret(), "sha256"):final(payload))
-	if not constant_equals(signature, expected_signature) then
+	if url_decode(field) ~= expected_field then
 		return nil
 	end
 
-	-- An expiration of 0 never expires and is used for the redirect token.
-	if expiration ~= 0 and expiration <= os.time() then
+	local decoded = url_decode(value)
+	-- Reject values carrying control characters so a signed value cannot
+	-- smuggle CR or LF into a response header.
+	if decoded:find("%c") then
 		return nil
 	end
 
-	if url_decode(field) ~= expected_field then
+	local binding = ""
+	if bind ~= nil then
+		binding = bind(decoded)
+		if binding == nil then
+			return nil
+		end
+	end
+
+	local payload = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
+	if not constant_equals(signature, sign(payload, binding)) then
 		return nil
 	end
 
-	local decoded = url_decode(value)
-	-- Reject values carrying control characters so a signed value cannot
-	-- smuggle CR or LF into a response header.
-	if decoded:find("%c") then
+	-- An expiration of 0 never expires and is used for the redirect token.
+	if expiration ~= 0 and expiration <= os.time() then
 		return nil
 	end
 	return decoded
 end
 
 -- The layout built here is what validate_value takes apart again, so the two
--- have to move together.
-function secure_value(field, value, expiration)
+-- have to move together. The binding, if any, is signed but left out.
+function secure_value(field, value, expiration, binding)
 	if value == nil or value:len() <= 0 then
 		return ""
 	end
 
 	local rand = require("openssl.rand")
-	local hmac = require("openssl.hmac")
 	local salt = tohex(rand.bytes(16))
 	value = url_encode(value)
 	field = url_encode(field)
 	local payload = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
-	local signature = tohex(hmac.new(get_secret(), "sha256"):final(payload))
-	return payload .. "|" .. signature
+	return payload .. "|" .. sign(payload, binding)
 end
 
 -- A control character in a header value would let that value split the
@@ -470,7 +487,7 @@ function authenticate_post()
 
 	local username = post["username"]
 	local password = post["password"]
-	local ok = false
+	local session = ""
 	if username ~= nil and password ~= nil and #password <= max_password_bytes then
 		local unistd = require("posix.unistd")
 		local hash, stand_in = account_hash(username)
@@ -480,15 +497,12 @@ function authenticate_post()
 				unistd.crypt(password, stand_in)
 			end
 		elseif constant_equals(hash, unistd.crypt(password, hash)) then
-			ok = true
+			session = secure_value("username", username, os.time() + session_seconds, hash)
 		end
 	end
 
-	if ok then
-		set_cookie(cookie_name, secure_value("username", username, os.time() + session_seconds))
-	else
-		set_cookie(cookie_name, "")
-	end
+	-- A failed login answers with an empty session, which clears the cookie.
+	set_cookie(cookie_name, session)
 
 	html("\n")
 	return 0
@@ -503,7 +517,8 @@ function authenticate_cookie()
 		return 1
 	end
 
-	local username = validate_value("username", get_cookie(http["cookie"], cookie_name))
+	local cookie = get_cookie(http["cookie"], cookie_name)
+	local username = validate_value("username", cookie, account_hash)
 	if username == nil or not accepted_users[username:lower()] then
 		return 0
 	end
diff --git a/custom/extensions/auth-inline.lua b/custom/extensions/auth-inline.lua
index 1b46fba..bce2472 100644
--- a/custom/extensions/auth-inline.lua
+++ b/custom/extensions/auth-inline.lua
@@ -42,10 +42,12 @@
 --     luarocks install luaossl OPENSSL_DIR="$(brew --prefix openssl)"
 --     luarocks install luaposix
 --
--- There is no server-side session store and no CSRF token, so a deleted
--- account's cookie stays valid until it expires and instances sharing a
--- secret file accept each other's cookies. That is acceptable for gating
--- read access to a git browser, so weigh it before guarding anything more.
+-- A session is signed together with its account's hash, so changing the
+-- password or deleting the account ends it. There is no server-side session
+-- store and no CSRF token, so instances sharing a secret file accept each
+-- other's cookies, and only a new secret file ends every session at once.
+-- That is acceptable for gating read access to a git browser, so weigh it
+-- before guarding anything more.
 
 -- The values that follow are the configuration and are meant to be edited.
 
@@ -284,8 +286,19 @@ function is_safe_redirect(url)
 	return true
 end
 
+-- The signature covers the payload and a binding that the cookie never
+-- carries, so a value only verifies while its binding stays the same.
+local function sign(payload, binding)
+	local hmac = require("openssl.hmac")
+	local signed = payload .. "|" .. (binding or "")
+	return tohex(hmac.new(get_secret(), "sha256"):final(signed))
+end
+
 -- Return the value carried by a signed cookie, or nil if it does not verify.
-function validate_value(expected_field, cookie)
+-- bind, when given, maps the value to the binding its signature has to cover,
+-- or to nil to turn it down, which is how a session meets its account's
+-- current hash.
+function validate_value(expected_field, cookie, bind)
 	local i = 0
 	local value = ""
 	local field = ""
@@ -325,46 +338,50 @@ function validate_value(expected_field, cookie)
 		return nil
 	end
 
-	local payload = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
-	local hmac = require("openssl.hmac")
-	local expected_signature = tohex(hmac.new(get_secret(), "sha256"):final(payload))
-	if not constant_equals(signature, expected_signature) then
+	if url_decode(field) ~= expected_field then
 		return nil
 	end
 
-	-- An expiration of 0 never expires and is used for the redirect token.
-	if expiration ~= 0 and expiration <= os.time() then
+	local decoded = url_decode(value)
+	-- Reject values carrying control characters so a signed value cannot
+	-- smuggle CR or LF into a response header.
+	if decoded:find("%c") then
 		return nil
 	end
 
-	if url_decode(field) ~= expected_field then
+	local binding = ""
+	if bind ~= nil then
+		binding = bind(decoded)
+		if binding == nil then
+			return nil
+		end
+	end
+
+	local payload = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
+	if not constant_equals(signature, sign(payload, binding)) then
 		return nil
 	end
 
-	local decoded = url_decode(value)
-	-- Reject values carrying control characters so a signed value cannot
-	-- smuggle CR or LF into a response header.
-	if decoded:find("%c") then
+	-- An expiration of 0 never expires and is used for the redirect token.
+	if expiration ~= 0 and expiration <= os.time() then
 		return nil
 	end
 	return decoded
 end
 
 -- The layout built here is what validate_value takes apart again, so the two
--- have to move together.
-function secure_value(field, value, expiration)
+-- have to move together. The binding, if any, is signed but left out.
+function secure_value(field, value, expiration, binding)
 	if value == nil or value:len() <= 0 then
 		return ""
 	end
 
 	local rand = require("openssl.rand")
-	local hmac = require("openssl.hmac")
 	local salt = tohex(rand.bytes(16))
 	value = url_encode(value)
 	field = url_encode(field)
 	local payload = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
-	local signature = tohex(hmac.new(get_secret(), "sha256"):final(payload))
-	return payload .. "|" .. signature
+	return payload .. "|" .. sign(payload, binding)
 end
 
 -- A control character in a header value would let that value split the
@@ -431,7 +448,7 @@ function authenticate_post()
 
 	local username = post["username"]
 	local password = post["password"]
-	local ok = false
+	local session = ""
 	if username ~= nil and password ~= nil and #password <= max_password_bytes then
 		local unistd = require("posix.unistd")
 		local hash, stand_in = account_hash(username)
@@ -441,15 +458,12 @@ function authenticate_post()
 				unistd.crypt(password, stand_in)
 			end
 		elseif constant_equals(hash, unistd.crypt(password, hash)) then
-			ok = true
+			session = secure_value("username", username, os.time() + session_seconds, hash)
 		end
 	end
 
-	if ok then
-		set_cookie(cookie_name, secure_value("username", username, os.time() + session_seconds))
-	else
-		set_cookie(cookie_name, "")
-	end
+	-- A failed login answers with an empty session, which clears the cookie.
+	set_cookie(cookie_name, session)
 
 	html("\n")
 	return 0
@@ -464,7 +478,8 @@ function authenticate_cookie()
 		return 1
 	end
 
-	local username = validate_value("username", get_cookie(http["cookie"], cookie_name))
+	local cookie = get_cookie(http["cookie"], cookie_name)
+	local username = validate_value("username", cookie, account_hash)
 	if username == nil or not accepted_users[username:lower()] then
 		return 0
 	end
diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.lua
index eb402da..92e82eb 100644
--- a/tests/extensions/test-auth.lua
+++ b/tests/extensions/test-auth.lua
@@ -28,6 +28,7 @@ if variant == "file" then
 	local users_file = io.open("auth-users", "w")
 	users_file:write("Alice:" .. stored_hash .. "\r\n")
 	users_file:write("not a parsable line\n")
+	users_file:write("carol:" .. stored_hash .. "\n")
 	users_file:close()
 	local groups_file = io.open("auth-groups", "w")
 	groups_file:write("devs:Alice, bob\n")
@@ -50,7 +51,8 @@ if variant == "inline" then
 			"\t[\"empty-repo\"] = {},\n" ..
 			"\t[\"bare-repo\"] = {},\n" },
 		{ "local users = {\n", "local users = {\n" ..
-			"\tAlice = \"" .. stored_hash .. "\",\n" },
+			"\tAlice = \"" .. stored_hash .. "\",\n" ..
+			"\tcarol = \"" .. stored_hash .. "\",\n" },
 	})
 else
 	h.load(script)
@@ -228,14 +230,28 @@ h.equals("a public repository needs no cookie", ret, 1)
 out, ret = run_action("authenticate-cookie", { repo = "secret-repo" })
 h.equals("a protected repository turns a bare request away", ret, 0)
 
-local session = secure_value("username", "Alice", now + 3600)
+local session = secure_value("username", "Alice", now + 3600, stored_hash)
 out, ret = run_action("authenticate-cookie", { repo = "secret-repo", cookie = "cgitauth=" .. session })
 h.equals("a signed session for a member is let through", ret, 1)
 
+-- A session is signed with its account's hash, so a new password or a
+-- removed account ends it.
+local stale = secure_value("username", "Alice", now + 3600, "$6$rounds=300000$old$hash")
+out, ret = run_action("authenticate-cookie", { repo = "secret-repo", cookie = "cgitauth=" .. stale })
+h.equals("a session from before a password change is turned away", ret, 0)
+
+local orphan = secure_value("username", "bob", now + 3600, stored_hash)
+out, ret = run_action("authenticate-cookie", { repo = "secret-repo", cookie = "cgitauth=" .. orphan })
+h.equals("a session for a member with no account is turned away", ret, 0)
+
+local unbound = secure_value("username", "Alice", now + 3600)
+h.equals("a session signed without its binding does not verify",
+	validate_value("username", unbound, account_hash), nil)
+
 out, ret = run_action("authenticate-cookie", { repo = "empty-repo", cookie = "cgitauth=" .. session })
 h.equals("a memberless repository denies even a valid session", ret, 0)
 
-local outsider = secure_value("username", "carol", now + 3600)
+local outsider = secure_value("username", "carol", now + 3600, stored_hash)
 out, ret = run_action("authenticate-cookie", { repo = "secret-repo", cookie = "cgitauth=" .. outsider })
 h.equals("a signed session for an outsider is turned away", ret, 0)
 
@@ -265,7 +281,7 @@ out, ret = run_action("authenticate-post", {
 h.contains("a good login redirects back", out, "Status: 302")
 h.contains("to where the form said", out, "Location: /repo/\n")
 local granted = out:match("Set%-Cookie: cgitauth=([^;]*);")
-h.equals("and grants a session that verifies", granted and validate_value("username", granted), "Alice")
+h.equals("and grants a session that verifies", granted and validate_value("username", granted, account_hash), "Alice")
 
 out, ret = run_action("authenticate-post", {
 	method = "POST",