Harden and reorganise the auth filters

1 file changed, 345 insertions, 187 deletions

diff --git a/extensions/auth-file.lua b/extensions/auth-file.luaindex 71c8bc6..5415ca7 100644--- a/extensions/auth-file.lua+++ b/extensions/auth-file.lua@@ -1,204 +1,203 @@--- cgit auth-filter that gates repositories behind a login form and a--- signed session cookie.+-- cgit auth-filter that gates repositories behind a login form and a signed+-- session cookie. ----- This is the FILE-BACKED variant. The user accounts, the groups, and--- the per-repository access lists are read from files on disk. Set their--- paths below and edit those files without touching this script. This--- suits larger or externally managed user sets.+-- This is the FILE-BACKED variant. The user accounts, the groups, and the+-- per-repository access lists are read from files on disk, whose paths are set+-- in the CONFIGURATION block below. Edit those files without touching this+-- script. This suits larger or externally managed user sets. ----- The companion auth-inline.lua behaves identically but keeps its--- accounts and access lists inline in the script itself, which suits a--- small fixed set of users.+-- The companion auth-inline.lua behaves identically but keeps its accounts and+-- access lists inline in the script itself, which suits a small fixed set of+-- users. -- -- Enable it in cgitrc with--- 	auth-filter=lua:/path/to/auth-file.lua+--     auth-filter=lua:/path/to/auth-file.lua ----- Requirements--- 	luaossl--- 	<http://25thandclement.com/~william/projects/luaossl.html>--- 	luaposix--- 	<https://github.com/luaposix/luaposix>+-- SUPPORTED LUA --+-- Lua 5.1, 5.2, 5.3, 5.4 and LuaJIT. Lua 5.5 is not supported, because luaossl+-- has no 5.5 build. Match the runtime to the Lua that cgit is built against.+--+-- HTTPS IS RECOMMENDED+--+-- Serve cgit over HTTPS. Terminate TLS at the web server in front of cgit. The+-- session cookie is marked Secure by default, so a browser only sends it back+-- over HTTPS. If you genuinely run cgit over plain HTTP with no TLS anywhere,+-- set cookie_insecure below, otherwise the cookie is never returned and login+-- appears to loop.+--+-- DEPENDENCIES+--+--     luaossl    OpenSSL binding, provides openssl.rand and openssl.hmac+--                <https://github.com/wahern/luaossl>+--     luaposix   POSIX binding, provides posix.sys.stat and posix.unistd+--                <https://github.com/luaposix/luaposix>+--+-- The reliable cross-platform install is LuaRocks, matched to your Lua+-- version. luaossl also needs the OpenSSL development headers present.+--+--     # Debian and Ubuntu+--     sudo apt install luarocks libssl-dev+--     sudo luarocks --lua-version 5.1 install luaossl+--     sudo luarocks --lua-version 5.1 install luaposix+--+--     # Fedora+--     sudo dnf install luarocks openssl-devel+--     sudo luarocks --lua-version 5.1 install luaossl luaposix+--+--     # Alpine+--     sudo apk add luarocks openssl-dev+--     sudo luarocks-5.1 install luaossl luaposix+--+--     # macOS with Homebrew+--     brew install luarocks openssl+--     luarocks install luaossl OPENSSL_DIR="$(brew --prefix openssl)"+--     luarocks install luaposix+--+-- Some distributions also package these, for example lua-luaossl and lua-posix+-- on Debian. If you use a distribution package, make sure it is built for the+-- same Lua version as cgit.+--+-- SECURITY NOTES+--+-- The cookie carries only a username with no server-side session store, so+-- deleting an account does not revoke a cookie already issued until it+-- expires, and instances that share a secret file accept each other's cookies.+-- The login form carries no CSRF token. Both are acceptable for gating read+-- access to a git browser. Weigh them before guarding anything more sensitive.+ local sysstat = require("posix.sys.stat") local unistd = require("posix.unistd") local rand = require("openssl.rand") local hmac = require("openssl.hmac") --- This file should contain a series of lines in the form of:---	username1:hash1---	username2:hash2---	username3:hash3---	...--- Hashes can be generated using something like `mkpasswd -m sha-512 -R 300000`.+--+-- ========================= CONFIGURATION =========================+-- Edit the values in this block. Nothing below it needs changing for+-- ordinary use.+--++-- Accounts, one per line, as username:hash. Generate a hash with+--     mkpasswd -m sha-512 -R 300000 -- This file should not be world-readable. local users_filename = "/etc/cgit-auth/users" --- This file should contain a series of lines in the form of:--- 	groupname1:username1,username2,username3,...---	...+-- Group membership, one per line, as groupname:user1,user2,user3,... local groups_filename = "/etc/cgit-auth/groups" --- This file should contain a series of lines in the form of:--- 	reponame1:groupname1,groupname2,groupname3,...---	...+-- Per-repository access, one per line, as reponame:group1,group2,...+-- A repository listed here is protected. One not listed is public. local repos_filename = "/etc/cgit-auth/repos" --- Set this to a path this script can write to for storing a persistent--- cookie secret, which should not be world-readable.+-- Where the cookie-signing secret is stored. It is created on first use. It+-- must be persistent and writable by cgit. Prefer a path OUTSIDE the cache+-- root, because pruning the cache would delete a secret kept inside it and+-- invalidate every live session. This file should not be world-readable. local secret_filename = "/var/cache/cgit/auth-secret" +-- How long a login stays valid, in seconds. Default one week.+local session_seconds = 7 * 24 * 60 * 60++-- Name of the session cookie.+local cookie_name = "cgitauth"++-- Path the cookie is scoped to. "/" covers the whole host. Set it to the cgit+-- root to scope the cookie more tightly.+local cookie_path = "/"++-- Leave false so the cookie is marked Secure and only travels over HTTPS. Set+-- it true ONLY if cgit is served over plain HTTP with no TLS anywhere, see the+-- HTTPS note in the header.+local cookie_insecure = false++--+-- =================================================================+--++-- A throwaway hash of the documented shape, used only to spend the same work+-- on a missing account as on a present one, so a failed login does not reveal+-- by timing whether the username exists.+local dummy_hash = "$6$rounds=300000$0000000000000000$"++-- Module state shared across the open, write and close calls of one request.+local action, http, cgit, post+ -- ----- Authentication functions follow below. Swap these out if you want different authentication semantics.+-- Account and access-list storage. This is the ONLY part that differs from+-- auth-inline.lua. Swap these two functions to change where accounts live. -- -- --- Looks up a hash for a given user.-function lookup_hash(user)-	local line-	for line in io.lines(users_filename) do+local function trim(s)+	return (string.gsub(s, "^%s*(.-)%s*$", "%1"))+end++-- Return the stored password hash for a user, or nil. Reads the users file+-- fresh each call. A missing or unreadable file, and any unparsable line, are+-- skipped rather than fatal.+function account_hash(user)+	if user == nil then+		return nil+	end+	local wanted = user:lower()+	local f = io.open(users_filename, "r")+	if f == nil then+		return nil+	end+	for line in f:lines() do 		local u, h = string.match(line, "(.-):(.+)")-		if u:lower() == user:lower() then+		if u ~= nil and trim(u):lower() == wanted then+			f:close() 			return h 		end 	end+	f:close() 	return nil end --- Looks up users for a given repo.-function lookup_users(repo)-	local users = nil+-- Return the set of users allowed to access a repository, keyed by lowercased+-- username, or nil if the repository is not protected. A protected repository+-- whose groups resolve to no users returns an EMPTY table, which denies+-- everyone rather than falling through to public.+function repo_userset(repo)+	if repo == nil then+		return nil+	end 	local groups = nil-	local line, group, user-	for line in io.lines(repos_filename) do-		local r, g = string.match(line, "(.-):(.+)")-		if r == repo then-			groups = { }-			for group in string.gmatch(g, "([^,]+)") do-				groups[group:lower()] = true+	local f = io.open(repos_filename, "r")+	if f ~= nil then+		for line in f:lines() do+			local r, g = string.match(line, "(.-):(.+)")+			if r ~= nil and trim(r) == repo then+				groups = {}+				for group in string.gmatch(g, "([^,]+)") do+					groups[trim(group):lower()] = true+				end+				break 			end-			break 		end+		f:close() 	end 	if groups == nil then 		return nil 	end-	for line in io.lines(groups_filename) do-		local g, u = string.match(line, "(.-):(.+)")-		if groups[g:lower()] then-			if users == nil then-				users = { }-			end-			for user in string.gmatch(u, "([^,]+)") do-				users[user:lower()] = true+	local users = {}+	local gf = io.open(groups_filename, "r")+	if gf ~= nil then+		for line in gf:lines() do+			local g, u = string.match(line, "(.-):(.+)")+			if g ~= nil and groups[trim(g):lower()] then+				for user in string.gmatch(u, "([^,]+)") do+					users[trim(user):lower()] = true+				end 			end 		end+		gf:close() 	end 	return users end ---- Sets HTTP cookie headers based on post and sets up redirection.-function authenticate_post()-	local hash = lookup_hash(post["username"])-	local redirect = validate_value("redirect", post["redirect"])--	if redirect == nil then-		not_found()-		return 0-	end--	redirect_to(redirect)--	if hash == nil or hash ~= unistd.crypt(post["password"], hash) then-		set_cookie("cgitauth", "")-	else-		-- One week expiration time-		local username = secure_value("username", post["username"], os.time() + 604800)-		set_cookie("cgitauth", username)-	end--	html("\n")-	return 0-end----- Returns 1 if the cookie is valid and 0 if it is not.-function authenticate_cookie()-	accepted_users = lookup_users(cgit["repo"])-	if accepted_users == nil then-		-- We return as valid if the repo is not protected.-		return 1-	end--	local username = validate_value("username", get_cookie(http["cookie"], "cgitauth"))-	if username == nil or not accepted_users[username:lower()] then-		return 0-	else-		return 1-	end-end---- Prints the html for the login form.-function body()-	html("<h2>Authentication Required</h2>")-	html("<form method='post' action='")-	html_attr(cgit["login"])-	html("'>")-	html("<input type='hidden' name='redirect' value='")-	html_attr(secure_value("redirect", cgit["url"], 0))-	html("' />")-	html("<table>")-	html("<tr><td><label for='username'>Username:</label></td><td><input id='username' name='username' autofocus /></td></tr>")-	html("<tr><td><label for='password'>Password:</label></td><td><input id='password' name='password' type='password' /></td></tr>")-	html("<tr><td colspan='2'><input value='Login' type='submit' /></td></tr>")-	html("</table></form>")--	return 0-end------------ Wrapper around filter API, exposing the http table, the cgit table, and the post table to the above functions.--------local actions = {}-actions["authenticate-post"] = authenticate_post-actions["authenticate-cookie"] = authenticate_cookie-actions["body"] = body--function filter_open(...)-	action = actions[select(1, ...)]--	http = {}-	http["cookie"] = select(2, ...)-	http["method"] = select(3, ...)-	http["query"] = select(4, ...)-	http["referer"] = select(5, ...)-	http["path"] = select(6, ...)-	http["host"] = select(7, ...)-	http["https"] = select(8, ...)--	cgit = {}-	cgit["repo"] = select(9, ...)-	cgit["page"] = select(10, ...)-	cgit["url"] = select(11, ...)-	cgit["login"] = select(12, ...)--end--function filter_close()-	return action()-end--function filter_write(str)-	post = parse_qs(str)-end-- -- -- -- Utility functions based on keplerproject/wsapi.@@ -225,17 +224,23 @@ function url_encode(str) 	return str end +-- Parse an application/x-www-form-urlencoded body. A value may itself contain+-- '=', for example a base64 password, so the value runs to the next '&'. function parse_qs(qs) 	local tab = {}-	for key, val in string.gmatch(qs, "([^&=]+)=([^&=]*)&?") do+	for key, val in string.gmatch(qs or "", "([^&=]+)=([^&]*)") do 		tab[url_decode(key)] = url_decode(val) 	end 	return tab end +-- Return the value of the named cookie, or nil. The stored token was already+-- url-encoded by secure_value, so it is returned verbatim, which keeps the+-- write path (set_cookie) and the read path symmetric. Decoding it here would+-- break the signature check for any value carrying a percent escape. function get_cookie(cookies, name)-	cookies = string.gsub(";" .. cookies .. ";", "%s*;%s*", ";")-	return url_decode(string.match(cookies, ";" .. name .. "=(.-);"))+	cookies = string.gsub(";" .. (cookies or "") .. ";", "%s*;%s*", ";")+	return string.match(cookies, ";" .. name .. "=(.-);") end  function tohex(b)@@ -254,7 +259,9 @@ end  local secret = nil --- Loads a secret from a file, creates a secret, or returns one from memory.+-- Load the cookie-signing secret, creating it on first use. Failures raise,+-- which cgit turns into a request error, so a broken secret denies rather than+-- signs with nothing. function get_secret() 	if secret ~= nil then 		return secret@@ -265,27 +272,48 @@ function get_secret() 		local temporary_filename = secret_filename .. ".tmp." .. tohex(rand.bytes(16)) 		local temporary_file = io.open(temporary_filename, "w") 		if temporary_file == nil then-			os.exit(177)+			sysstat.umask(old_umask)+			error("cgit auth: cannot create secret file " .. secret_filename)+		end+		local wrote = temporary_file:write(tohex(rand.bytes(32)))+		local closed = temporary_file:close()+		if not wrote or not closed then+			os.remove(temporary_filename)+			sysstat.umask(old_umask)+			error("cgit auth: failed writing secret file " .. secret_filename) 		end-		temporary_file:write(tohex(rand.bytes(32)))-		temporary_file:close()-		unistd.link(temporary_filename, secret_filename) -- Intentionally fails in the case that another process is doing the same.+		unistd.link(temporary_filename, secret_filename) -- Intentionally fails if another worker won the race. 		unistd.unlink(temporary_filename) 		sysstat.umask(old_umask) 		secret_file = io.open(secret_filename, "r") 	end 	if secret_file == nil then-		os.exit(177)+		error("cgit auth: cannot read secret file " .. secret_filename) 	end-	secret = secret_file:read()+	secret = secret_file:read("*l") 	secret_file:close()-	if secret:len() ~= 64 then-		os.exit(177)+	if secret == nil or secret:len() ~= 64 then+		secret = nil+		error("cgit auth: secret file " .. secret_filename .. " is malformed, expected 64 hex characters") 	end 	return secret end --- Returns value of cookie if cookie is valid. Otherwise returns nil.+-- A redirect target is unsafe if a browser would read it as another origin.+-- The only such form cgit can be tricked into signing is a scheme-relative+-- "//host" or "/\host". Everything else stays on this host.+function is_safe_redirect(url)+	if type(url) ~= "string" then+		return false+	end+	local head = url:sub(1, 2)+	if head == "//" or head == "/\\" then+		return false+	end+	return true+end++-- Return the value carried by a signed cookie, or nil if it does not verify. function validate_value(expected_field, cookie) 	local i = 0 	local value = ""@@ -304,10 +332,13 @@ function validate_value(expected_field, cookie) 		elseif i == 1 then 			value = component 		elseif i == 2 then-			expiration = tonumber(component)-			if expiration == nil then-				expiration = -1+			-- The expiration must be a plain integer. Rejecting other forms+			-- keeps the signed bytes canonical, since tonumber and tostring of+			-- "1e9" or "100.0" differ across Lua versions.+			if not string.match(component, "^%d+$") then+				return nil 			end+			expiration = tonumber(component) 		elseif i == 3 then 			salt = component 		elseif i == 4 then@@ -327,7 +358,8 @@ function validate_value(expected_field, cookie) 		return nil 	end -	if expiration == -1 or (expiration ~= 0 and expiration <= os.time()) 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 @@ -336,8 +368,8 @@ function validate_value(expected_field, cookie) 	end  	local decoded = url_decode(value)-	-- Reject values carrying control characters so a signed cookie or-	-- redirect target cannot smuggle CR or LF into a response header.+	-- 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@@ -349,11 +381,10 @@ function secure_value(field, value, expiration) 		return "" 	end -	local authstr = "" 	local salt = tohex(rand.bytes(16)) 	value = url_encode(value) 	field = url_encode(field)-	authstr = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt+	local authstr = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt 	authstr = authstr .. "|" .. tohex(hmac.new(get_secret(), "sha256"):final(authstr)) 	return authstr end@@ -363,27 +394,31 @@ function strip_ctl(s) 	return (string.gsub(s or "", "%c", "")) end --- Compare two strings without stopping at the first mismatch, so the time--- taken does not reveal how many leading bytes already matched.+-- Compare two strings in time that does not depend on how many leading bytes+-- match, so a mismatch position is not revealed by timing. function constant_equals(a, b)-	if a == nil or b == nil or #a ~= #b then+	if type(a) ~= "string" or type(b) ~= "string" or #a ~= #b then 		return false 	end 	local diff = 0 	for i = 1, #a do-		if a:byte(i) ~= b:byte(i) then-			diff = diff + 1-		end+		local d = a:byte(i) - b:byte(i)+		diff = diff + d * d 	end 	return diff == 0 end  function set_cookie(cookie, value)-	html("Set-Cookie: " .. cookie .. "=" .. strip_ctl(value) .. "; HttpOnly; SameSite=Lax")-	if http["https"] == "yes" or http["https"] == "on" or http["https"] == "1" then-		html("; secure")+	local attrs = "; HttpOnly; SameSite=Lax; Path=" .. cookie_path+	if not cookie_insecure then+		attrs = attrs .. "; Secure" 	end-	html("\n")+	if value == "" then+		attrs = attrs .. "; Max-Age=0"+	elseif session_seconds > 0 then+		attrs = attrs .. "; Max-Age=" .. tostring(session_seconds)+	end+	html("Set-Cookie: " .. cookie .. "=" .. strip_ctl(value) .. attrs .. "\n") end  function redirect_to(url)@@ -396,3 +431,126 @@ function not_found() 	html("Status: 404 Not Found\n") 	html("Cache-Control: no-cache, no-store\n\n") end++--+--+-- Authentication actions. Identical to auth-inline.lua from here down.+--+--++-- Sets HTTP cookie headers based on post and sets up redirection.+function authenticate_post()+	local redirect = validate_value("redirect", post["redirect"])++	if redirect == nil or not is_safe_redirect(redirect) then+		not_found()+		return 0+	end++	redirect_to(redirect)++	local username = post["username"]+	local password = post["password"]+	local ok = false+	if username ~= nil and password ~= nil then+		local hash = account_hash(username)+		if hash == nil then+			-- Spend the work anyway, see dummy_hash.+			unistd.crypt(password, dummy_hash)+		elseif constant_equals(hash, unistd.crypt(password, hash)) then+			ok = true+		end+	end++	if ok then+		set_cookie(cookie_name, secure_value("username", username, os.time() + session_seconds))+	else+		set_cookie(cookie_name, "")+	end++	html("\n")+	return 0+end++-- Returns 1 if the cookie is valid and 0 if it is not.+function authenticate_cookie()+	local accepted_users = repo_userset(cgit["repo"])+	if accepted_users == nil then+		-- The repository is not protected.+		return 1+	end++	local username = validate_value("username", get_cookie(http["cookie"], cookie_name))+	if username == nil or not accepted_users[username:lower()] then+		return 0+	end+	return 1+end++-- Prints the html for the login form.+function body()+	local target = cgit["url"]+	if not is_safe_redirect(target) then+		target = cgit["login"]+	end++	html("<h2>Authentication Required</h2>")+	html("<form method='post' action='")+	html_attr(cgit["login"])+	html("'>")+	html("<input type='hidden' name='redirect' value='")+	html_attr(secure_value("redirect", target, 0))+	html("' />")+	html("<table>")+	html("<tr><td><label for='username'>Username:</label></td><td><input id='username' name='username' autofocus /></td></tr>")+	html("<tr><td><label for='password'>Password:</label></td><td><input id='password' name='password' type='password' /></td></tr>")+	html("<tr><td colspan='2'><input value='Login' type='submit' /></td></tr>")+	html("</table></form>")++	return 0+end++--+--+-- Wrapper around the filter API, exposing the http, cgit and post tables to+-- the functions above.+--+--++local actions = {}+actions["authenticate-post"] = authenticate_post+actions["authenticate-cookie"] = authenticate_cookie+actions["body"] = body++function filter_open(...)+	action = actions[select(1, ...)]++	post = {}++	http = {}+	http["cookie"] = select(2, ...)+	http["method"] = select(3, ...)+	http["query"] = select(4, ...)+	http["referer"] = select(5, ...)+	http["path"] = select(6, ...)+	http["host"] = select(7, ...)+	http["https"] = select(8, ...)++	cgit = {}+	cgit["repo"] = select(9, ...)+	cgit["page"] = select(10, ...)+	cgit["url"] = select(11, ...)+	cgit["login"] = select(12, ...)+end++function filter_close()+	if action == nil then+		-- Unknown action, deny rather than raise.+		return 0+	end+	return action()+end++function filter_write(str)+	post = parse_qs(str)+end
diff --git a/extensions/auth-file.lua b/extensions/auth-file.lua
index 71c8bc6..5415ca7 100644
--- a/extensions/auth-file.lua
+++ b/extensions/auth-file.lua
@@ -1,204 +1,203 @@
--- cgit auth-filter that gates repositories behind a login form and a
--- signed session cookie.
+-- cgit auth-filter that gates repositories behind a login form and a signed
+-- session cookie.
 --
--- This is the FILE-BACKED variant. The user accounts, the groups, and
--- the per-repository access lists are read from files on disk. Set their
--- paths below and edit those files without touching this script. This
--- suits larger or externally managed user sets.
+-- This is the FILE-BACKED variant. The user accounts, the groups, and the
+-- per-repository access lists are read from files on disk, whose paths are set
+-- in the CONFIGURATION block below. Edit those files without touching this
+-- script. This suits larger or externally managed user sets.
 --
--- The companion auth-inline.lua behaves identically but keeps its
--- accounts and access lists inline in the script itself, which suits a
--- small fixed set of users.
+-- The companion auth-inline.lua behaves identically but keeps its accounts and
+-- access lists inline in the script itself, which suits a small fixed set of
+-- users.
 --
 -- Enable it in cgitrc with
--- 	auth-filter=lua:/path/to/auth-file.lua
+--     auth-filter=lua:/path/to/auth-file.lua
 --
--- Requirements
--- 	luaossl
--- 	<http://25thandclement.com/~william/projects/luaossl.html>
--- 	luaposix
--- 	<https://github.com/luaposix/luaposix>
+-- SUPPORTED LUA
 --
+-- Lua 5.1, 5.2, 5.3, 5.4 and LuaJIT. Lua 5.5 is not supported, because luaossl
+-- has no 5.5 build. Match the runtime to the Lua that cgit is built against.
+--
+-- HTTPS IS RECOMMENDED
+--
+-- Serve cgit over HTTPS. Terminate TLS at the web server in front of cgit. The
+-- session cookie is marked Secure by default, so a browser only sends it back
+-- over HTTPS. If you genuinely run cgit over plain HTTP with no TLS anywhere,
+-- set cookie_insecure below, otherwise the cookie is never returned and login
+-- appears to loop.
+--
+-- DEPENDENCIES
+--
+--     luaossl    OpenSSL binding, provides openssl.rand and openssl.hmac
+--                <https://github.com/wahern/luaossl>
+--     luaposix   POSIX binding, provides posix.sys.stat and posix.unistd
+--                <https://github.com/luaposix/luaposix>
+--
+-- The reliable cross-platform install is LuaRocks, matched to your Lua
+-- version. luaossl also needs the OpenSSL development headers present.
+--
+--     # Debian and Ubuntu
+--     sudo apt install luarocks libssl-dev
+--     sudo luarocks --lua-version 5.1 install luaossl
+--     sudo luarocks --lua-version 5.1 install luaposix
+--
+--     # Fedora
+--     sudo dnf install luarocks openssl-devel
+--     sudo luarocks --lua-version 5.1 install luaossl luaposix
+--
+--     # Alpine
+--     sudo apk add luarocks openssl-dev
+--     sudo luarocks-5.1 install luaossl luaposix
+--
+--     # macOS with Homebrew
+--     brew install luarocks openssl
+--     luarocks install luaossl OPENSSL_DIR="$(brew --prefix openssl)"
+--     luarocks install luaposix
+--
+-- Some distributions also package these, for example lua-luaossl and lua-posix
+-- on Debian. If you use a distribution package, make sure it is built for the
+-- same Lua version as cgit.
+--
+-- SECURITY NOTES
+--
+-- The cookie carries only a username with no server-side session store, so
+-- deleting an account does not revoke a cookie already issued until it
+-- expires, and instances that share a secret file accept each other's cookies.
+-- The login form carries no CSRF token. Both are acceptable for gating read
+-- access to a git browser. Weigh them before guarding anything more sensitive.
+
 local sysstat = require("posix.sys.stat")
 local unistd = require("posix.unistd")
 local rand = require("openssl.rand")
 local hmac = require("openssl.hmac")
 
--- This file should contain a series of lines in the form of:
---	username1:hash1
---	username2:hash2
---	username3:hash3
---	...
--- Hashes can be generated using something like `mkpasswd -m sha-512 -R 300000`.
+--
+-- ========================= CONFIGURATION =========================
+-- Edit the values in this block. Nothing below it needs changing for
+-- ordinary use.
+--
+
+-- Accounts, one per line, as username:hash. Generate a hash with
+--     mkpasswd -m sha-512 -R 300000
 -- This file should not be world-readable.
 local users_filename = "/etc/cgit-auth/users"
 
--- This file should contain a series of lines in the form of:
--- 	groupname1:username1,username2,username3,...
---	...
+-- Group membership, one per line, as groupname:user1,user2,user3,...
 local groups_filename = "/etc/cgit-auth/groups"
 
--- This file should contain a series of lines in the form of:
--- 	reponame1:groupname1,groupname2,groupname3,...
---	...
+-- Per-repository access, one per line, as reponame:group1,group2,...
+-- A repository listed here is protected. One not listed is public.
 local repos_filename = "/etc/cgit-auth/repos"
 
--- Set this to a path this script can write to for storing a persistent
--- cookie secret, which should not be world-readable.
+-- Where the cookie-signing secret is stored. It is created on first use. It
+-- must be persistent and writable by cgit. Prefer a path OUTSIDE the cache
+-- root, because pruning the cache would delete a secret kept inside it and
+-- invalidate every live session. This file should not be world-readable.
 local secret_filename = "/var/cache/cgit/auth-secret"
 
+-- How long a login stays valid, in seconds. Default one week.
+local session_seconds = 7 * 24 * 60 * 60
+
+-- Name of the session cookie.
+local cookie_name = "cgitauth"
+
+-- Path the cookie is scoped to. "/" covers the whole host. Set it to the cgit
+-- root to scope the cookie more tightly.
+local cookie_path = "/"
+
+-- Leave false so the cookie is marked Secure and only travels over HTTPS. Set
+-- it true ONLY if cgit is served over plain HTTP with no TLS anywhere, see the
+-- HTTPS note in the header.
+local cookie_insecure = false
+
+--
+-- =================================================================
+--
+
+-- A throwaway hash of the documented shape, used only to spend the same work
+-- on a missing account as on a present one, so a failed login does not reveal
+-- by timing whether the username exists.
+local dummy_hash = "$6$rounds=300000$0000000000000000$"
+
+-- Module state shared across the open, write and close calls of one request.
+local action, http, cgit, post
+
 --
 --
--- Authentication functions follow below. Swap these out if you want different authentication semantics.
+-- Account and access-list storage. This is the ONLY part that differs from
+-- auth-inline.lua. Swap these two functions to change where accounts live.
 --
 --
 
--- Looks up a hash for a given user.
-function lookup_hash(user)
-	local line
-	for line in io.lines(users_filename) do
+local function trim(s)
+	return (string.gsub(s, "^%s*(.-)%s*$", "%1"))
+end
+
+-- Return the stored password hash for a user, or nil. Reads the users file
+-- fresh each call. A missing or unreadable file, and any unparsable line, are
+-- skipped rather than fatal.
+function account_hash(user)
+	if user == nil then
+		return nil
+	end
+	local wanted = user:lower()
+	local f = io.open(users_filename, "r")
+	if f == nil then
+		return nil
+	end
+	for line in f:lines() do
 		local u, h = string.match(line, "(.-):(.+)")
-		if u:lower() == user:lower() then
+		if u ~= nil and trim(u):lower() == wanted then
+			f:close()
 			return h
 		end
 	end
+	f:close()
 	return nil
 end
 
--- Looks up users for a given repo.
-function lookup_users(repo)
-	local users = nil
+-- Return the set of users allowed to access a repository, keyed by lowercased
+-- username, or nil if the repository is not protected. A protected repository
+-- whose groups resolve to no users returns an EMPTY table, which denies
+-- everyone rather than falling through to public.
+function repo_userset(repo)
+	if repo == nil then
+		return nil
+	end
 	local groups = nil
-	local line, group, user
-	for line in io.lines(repos_filename) do
-		local r, g = string.match(line, "(.-):(.+)")
-		if r == repo then
-			groups = { }
-			for group in string.gmatch(g, "([^,]+)") do
-				groups[group:lower()] = true
+	local f = io.open(repos_filename, "r")
+	if f ~= nil then
+		for line in f:lines() do
+			local r, g = string.match(line, "(.-):(.+)")
+			if r ~= nil and trim(r) == repo then
+				groups = {}
+				for group in string.gmatch(g, "([^,]+)") do
+					groups[trim(group):lower()] = true
+				end
+				break
 			end
-			break
 		end
+		f:close()
 	end
 	if groups == nil then
 		return nil
 	end
-	for line in io.lines(groups_filename) do
-		local g, u = string.match(line, "(.-):(.+)")
-		if groups[g:lower()] then
-			if users == nil then
-				users = { }
-			end
-			for user in string.gmatch(u, "([^,]+)") do
-				users[user:lower()] = true
+	local users = {}
+	local gf = io.open(groups_filename, "r")
+	if gf ~= nil then
+		for line in gf:lines() do
+			local g, u = string.match(line, "(.-):(.+)")
+			if g ~= nil and groups[trim(g):lower()] then
+				for user in string.gmatch(u, "([^,]+)") do
+					users[trim(user):lower()] = true
+				end
 			end
 		end
+		gf:close()
 	end
 	return users
 end
 
-
--- Sets HTTP cookie headers based on post and sets up redirection.
-function authenticate_post()
-	local hash = lookup_hash(post["username"])
-	local redirect = validate_value("redirect", post["redirect"])
-
-	if redirect == nil then
-		not_found()
-		return 0
-	end
-
-	redirect_to(redirect)
-
-	if hash == nil or hash ~= unistd.crypt(post["password"], hash) then
-		set_cookie("cgitauth", "")
-	else
-		-- One week expiration time
-		local username = secure_value("username", post["username"], os.time() + 604800)
-		set_cookie("cgitauth", username)
-	end
-
-	html("\n")
-	return 0
-end
-
-
--- Returns 1 if the cookie is valid and 0 if it is not.
-function authenticate_cookie()
-	accepted_users = lookup_users(cgit["repo"])
-	if accepted_users == nil then
-		-- We return as valid if the repo is not protected.
-		return 1
-	end
-
-	local username = validate_value("username", get_cookie(http["cookie"], "cgitauth"))
-	if username == nil or not accepted_users[username:lower()] then
-		return 0
-	else
-		return 1
-	end
-end
-
--- Prints the html for the login form.
-function body()
-	html("<h2>Authentication Required</h2>")
-	html("<form method='post' action='")
-	html_attr(cgit["login"])
-	html("'>")
-	html("<input type='hidden' name='redirect' value='")
-	html_attr(secure_value("redirect", cgit["url"], 0))
-	html("' />")
-	html("<table>")
-	html("<tr><td><label for='username'>Username:</label></td><td><input id='username' name='username' autofocus /></td></tr>")
-	html("<tr><td><label for='password'>Password:</label></td><td><input id='password' name='password' type='password' /></td></tr>")
-	html("<tr><td colspan='2'><input value='Login' type='submit' /></td></tr>")
-	html("</table></form>")
-
-	return 0
-end
-
-
-
---
---
--- Wrapper around filter API, exposing the http table, the cgit table, and the post table to the above functions.
---
---
-
-local actions = {}
-actions["authenticate-post"] = authenticate_post
-actions["authenticate-cookie"] = authenticate_cookie
-actions["body"] = body
-
-function filter_open(...)
-	action = actions[select(1, ...)]
-
-	http = {}
-	http["cookie"] = select(2, ...)
-	http["method"] = select(3, ...)
-	http["query"] = select(4, ...)
-	http["referer"] = select(5, ...)
-	http["path"] = select(6, ...)
-	http["host"] = select(7, ...)
-	http["https"] = select(8, ...)
-
-	cgit = {}
-	cgit["repo"] = select(9, ...)
-	cgit["page"] = select(10, ...)
-	cgit["url"] = select(11, ...)
-	cgit["login"] = select(12, ...)
-
-end
-
-function filter_close()
-	return action()
-end
-
-function filter_write(str)
-	post = parse_qs(str)
-end
-
-
 --
 --
 -- Utility functions based on keplerproject/wsapi.
@@ -225,17 +224,23 @@ function url_encode(str)
 	return str
 end
 
+-- Parse an application/x-www-form-urlencoded body. A value may itself contain
+-- '=', for example a base64 password, so the value runs to the next '&'.
 function parse_qs(qs)
 	local tab = {}
-	for key, val in string.gmatch(qs, "([^&=]+)=([^&=]*)&?") do
+	for key, val in string.gmatch(qs or "", "([^&=]+)=([^&]*)") do
 		tab[url_decode(key)] = url_decode(val)
 	end
 	return tab
 end
 
+-- Return the value of the named cookie, or nil. The stored token was already
+-- url-encoded by secure_value, so it is returned verbatim, which keeps the
+-- write path (set_cookie) and the read path symmetric. Decoding it here would
+-- break the signature check for any value carrying a percent escape.
 function get_cookie(cookies, name)
-	cookies = string.gsub(";" .. cookies .. ";", "%s*;%s*", ";")
-	return url_decode(string.match(cookies, ";" .. name .. "=(.-);"))
+	cookies = string.gsub(";" .. (cookies or "") .. ";", "%s*;%s*", ";")
+	return string.match(cookies, ";" .. name .. "=(.-);")
 end
 
 function tohex(b)
@@ -254,7 +259,9 @@ end
 
 local secret = nil
 
--- Loads a secret from a file, creates a secret, or returns one from memory.
+-- Load the cookie-signing secret, creating it on first use. Failures raise,
+-- which cgit turns into a request error, so a broken secret denies rather than
+-- signs with nothing.
 function get_secret()
 	if secret ~= nil then
 		return secret
@@ -265,27 +272,48 @@ function get_secret()
 		local temporary_filename = secret_filename .. ".tmp." .. tohex(rand.bytes(16))
 		local temporary_file = io.open(temporary_filename, "w")
 		if temporary_file == nil then
-			os.exit(177)
+			sysstat.umask(old_umask)
+			error("cgit auth: cannot create secret file " .. secret_filename)
+		end
+		local wrote = temporary_file:write(tohex(rand.bytes(32)))
+		local closed = temporary_file:close()
+		if not wrote or not closed then
+			os.remove(temporary_filename)
+			sysstat.umask(old_umask)
+			error("cgit auth: failed writing secret file " .. secret_filename)
 		end
-		temporary_file:write(tohex(rand.bytes(32)))
-		temporary_file:close()
-		unistd.link(temporary_filename, secret_filename) -- Intentionally fails in the case that another process is doing the same.
+		unistd.link(temporary_filename, secret_filename) -- Intentionally fails if another worker won the race.
 		unistd.unlink(temporary_filename)
 		sysstat.umask(old_umask)
 		secret_file = io.open(secret_filename, "r")
 	end
 	if secret_file == nil then
-		os.exit(177)
+		error("cgit auth: cannot read secret file " .. secret_filename)
 	end
-	secret = secret_file:read()
+	secret = secret_file:read("*l")
 	secret_file:close()
-	if secret:len() ~= 64 then
-		os.exit(177)
+	if secret == nil or secret:len() ~= 64 then
+		secret = nil
+		error("cgit auth: secret file " .. secret_filename .. " is malformed, expected 64 hex characters")
 	end
 	return secret
 end
 
--- Returns value of cookie if cookie is valid. Otherwise returns nil.
+-- A redirect target is unsafe if a browser would read it as another origin.
+-- The only such form cgit can be tricked into signing is a scheme-relative
+-- "//host" or "/\host". Everything else stays on this host.
+function is_safe_redirect(url)
+	if type(url) ~= "string" then
+		return false
+	end
+	local head = url:sub(1, 2)
+	if head == "//" or head == "/\\" then
+		return false
+	end
+	return true
+end
+
+-- Return the value carried by a signed cookie, or nil if it does not verify.
 function validate_value(expected_field, cookie)
 	local i = 0
 	local value = ""
@@ -304,10 +332,13 @@ function validate_value(expected_field, cookie)
 		elseif i == 1 then
 			value = component
 		elseif i == 2 then
-			expiration = tonumber(component)
-			if expiration == nil then
-				expiration = -1
+			-- The expiration must be a plain integer. Rejecting other forms
+			-- keeps the signed bytes canonical, since tonumber and tostring of
+			-- "1e9" or "100.0" differ across Lua versions.
+			if not string.match(component, "^%d+$") then
+				return nil
 			end
+			expiration = tonumber(component)
 		elseif i == 3 then
 			salt = component
 		elseif i == 4 then
@@ -327,7 +358,8 @@ function validate_value(expected_field, cookie)
 		return nil
 	end
 
-	if expiration == -1 or (expiration ~= 0 and expiration <= os.time()) 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
 
@@ -336,8 +368,8 @@ function validate_value(expected_field, cookie)
 	end
 
 	local decoded = url_decode(value)
-	-- Reject values carrying control characters so a signed cookie or
-	-- redirect target cannot smuggle CR or LF into a response header.
+	-- 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
@@ -349,11 +381,10 @@ function secure_value(field, value, expiration)
 		return ""
 	end
 
-	local authstr = ""
 	local salt = tohex(rand.bytes(16))
 	value = url_encode(value)
 	field = url_encode(field)
-	authstr = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
+	local authstr = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
 	authstr = authstr .. "|" .. tohex(hmac.new(get_secret(), "sha256"):final(authstr))
 	return authstr
 end
@@ -363,27 +394,31 @@ function strip_ctl(s)
 	return (string.gsub(s or "", "%c", ""))
 end
 
--- Compare two strings without stopping at the first mismatch, so the time
--- taken does not reveal how many leading bytes already matched.
+-- Compare two strings in time that does not depend on how many leading bytes
+-- match, so a mismatch position is not revealed by timing.
 function constant_equals(a, b)
-	if a == nil or b == nil or #a ~= #b then
+	if type(a) ~= "string" or type(b) ~= "string" or #a ~= #b then
 		return false
 	end
 	local diff = 0
 	for i = 1, #a do
-		if a:byte(i) ~= b:byte(i) then
-			diff = diff + 1
-		end
+		local d = a:byte(i) - b:byte(i)
+		diff = diff + d * d
 	end
 	return diff == 0
 end
 
 function set_cookie(cookie, value)
-	html("Set-Cookie: " .. cookie .. "=" .. strip_ctl(value) .. "; HttpOnly; SameSite=Lax")
-	if http["https"] == "yes" or http["https"] == "on" or http["https"] == "1" then
-		html("; secure")
+	local attrs = "; HttpOnly; SameSite=Lax; Path=" .. cookie_path
+	if not cookie_insecure then
+		attrs = attrs .. "; Secure"
 	end
-	html("\n")
+	if value == "" then
+		attrs = attrs .. "; Max-Age=0"
+	elseif session_seconds > 0 then
+		attrs = attrs .. "; Max-Age=" .. tostring(session_seconds)
+	end
+	html("Set-Cookie: " .. cookie .. "=" .. strip_ctl(value) .. attrs .. "\n")
 end
 
 function redirect_to(url)
@@ -396,3 +431,126 @@ function not_found()
 	html("Status: 404 Not Found\n")
 	html("Cache-Control: no-cache, no-store\n\n")
 end
+
+--
+--
+-- Authentication actions. Identical to auth-inline.lua from here down.
+--
+--
+
+-- Sets HTTP cookie headers based on post and sets up redirection.
+function authenticate_post()
+	local redirect = validate_value("redirect", post["redirect"])
+
+	if redirect == nil or not is_safe_redirect(redirect) then
+		not_found()
+		return 0
+	end
+
+	redirect_to(redirect)
+
+	local username = post["username"]
+	local password = post["password"]
+	local ok = false
+	if username ~= nil and password ~= nil then
+		local hash = account_hash(username)
+		if hash == nil then
+			-- Spend the work anyway, see dummy_hash.
+			unistd.crypt(password, dummy_hash)
+		elseif constant_equals(hash, unistd.crypt(password, hash)) then
+			ok = true
+		end
+	end
+
+	if ok then
+		set_cookie(cookie_name, secure_value("username", username, os.time() + session_seconds))
+	else
+		set_cookie(cookie_name, "")
+	end
+
+	html("\n")
+	return 0
+end
+
+-- Returns 1 if the cookie is valid and 0 if it is not.
+function authenticate_cookie()
+	local accepted_users = repo_userset(cgit["repo"])
+	if accepted_users == nil then
+		-- The repository is not protected.
+		return 1
+	end
+
+	local username = validate_value("username", get_cookie(http["cookie"], cookie_name))
+	if username == nil or not accepted_users[username:lower()] then
+		return 0
+	end
+	return 1
+end
+
+-- Prints the html for the login form.
+function body()
+	local target = cgit["url"]
+	if not is_safe_redirect(target) then
+		target = cgit["login"]
+	end
+
+	html("<h2>Authentication Required</h2>")
+	html("<form method='post' action='")
+	html_attr(cgit["login"])
+	html("'>")
+	html("<input type='hidden' name='redirect' value='")
+	html_attr(secure_value("redirect", target, 0))
+	html("' />")
+	html("<table>")
+	html("<tr><td><label for='username'>Username:</label></td><td><input id='username' name='username' autofocus /></td></tr>")
+	html("<tr><td><label for='password'>Password:</label></td><td><input id='password' name='password' type='password' /></td></tr>")
+	html("<tr><td colspan='2'><input value='Login' type='submit' /></td></tr>")
+	html("</table></form>")
+
+	return 0
+end
+
+--
+--
+-- Wrapper around the filter API, exposing the http, cgit and post tables to
+-- the functions above.
+--
+--
+
+local actions = {}
+actions["authenticate-post"] = authenticate_post
+actions["authenticate-cookie"] = authenticate_cookie
+actions["body"] = body
+
+function filter_open(...)
+	action = actions[select(1, ...)]
+
+	post = {}
+
+	http = {}
+	http["cookie"] = select(2, ...)
+	http["method"] = select(3, ...)
+	http["query"] = select(4, ...)
+	http["referer"] = select(5, ...)
+	http["path"] = select(6, ...)
+	http["host"] = select(7, ...)
+	http["https"] = select(8, ...)
+
+	cgit = {}
+	cgit["repo"] = select(9, ...)
+	cgit["page"] = select(10, ...)
+	cgit["url"] = select(11, ...)
+	cgit["login"] = select(12, ...)
+end
+
+function filter_close()
+	if action == nil then
+		-- Unknown action, deny rather than raise.
+		return 0
+	end
+	return action()
+end
+
+function filter_write(str)
+	post = parse_qs(str)
+end