Check that the auth secret is made safely

The checks replaced get_secret with a constant, so its success path
never ran, and the luaposix stand-in had no link or unlink and a umask
that ignored its argument. Dropping the tightened mask, which leaves
the secret readable by everyone under the usual 022, or accepting a
short secret, passed. The real get_secret now makes the secret in the
run's directory through stand-ins that behave like the calls they
replace, and the checks cover the mask and its restoration, the link
race, the temporary file, reuse and a secret written by hand.

1 file changed, 82 insertions, 8 deletions

diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.luaindex 97639ea..506babf 100644--- a/tests/extensions/test-auth.lua+++ b/tests/extensions/test-auth.lua@@ -5,7 +5,7 @@ -- load, so the harness meets both with deterministic stand-ins, which keeps -- every check about this script's own logic, the cookie layout, the expiry -- and field rules, the redirect vetting and the action flows, rather than--- about OpenSSL. The signing secret is pinned by replacing get_secret, and+-- about OpenSSL. The signing secret is made in the run's directory, and -- the accounts and access lists are filled through each variant's own -- channel, its commented-out examples switched on for the inline script and -- fixture files reached through a redirected io.open for the file one. Both@@ -58,8 +58,13 @@ local inline_config = { 		"\tcarol = \"" .. stored_hash .. "\",\n" }, } -local function load_script(changes)-	local all = {}+-- Every load keeps its signing secret in the run's directory, at the path+-- given or else at one shared by all of them.+local function load_script(changes, secret_path)+	local all = {+		{ "local secret_filename = \"/var/lib/cgit/auth-secret\"",+			"local secret_filename = " .. string.format("%q", secret_path or directory .. "/auth-secret") },+	} 	if variant == "inline" then 		for _, change in ipairs(inline_config) do 			all[#all + 1] = change@@ -69,9 +74,6 @@ local function load_script(changes) 		all[#all + 1] = change 	end 	h.load_with(script, all)-	function get_secret()-		return string.rep("0123456789abcdef", 4)-	end end  load_script()@@ -89,6 +91,79 @@ unistd.crypt = function(trypass, setting) 	return real_crypt(trypass, setting) end +local function read_file(path)+	local file = io.open(path, "rb")+	if file == nil then+		return nil+	end+	local text = file:read("*a")+	file:close()+	return text+end++local function write_file(path, text)+	local file = assert(io.open(path, "wb"))+	file:write(text)+	file:close()+end++-- The secret is made on first use, so the masks it is made under and the+-- temporary files it is linked from are watched the same way. A secret set+-- in race_winner is written just ahead of the link, the way another request+-- making the file at the same moment would.+local sysstat = require("posix.sys.stat")+local real_umask = sysstat.umask+local umasks = {}+sysstat.umask = function(mask)+	local old = real_umask(mask)+	umasks[#umasks + 1] = { mask = mask, old = old }+	return old+end+local real_link = unistd.link+local linked = {}+local race_winner = nil+unistd.link = function(from, to)+	linked[#linked + 1] = from+	if race_winner then+		write_file(to, race_winner)+	end+	return real_link(from, to)+end++local _, made = pcall(get_secret)+h.check("a new secret is 64 hex characters",+	type(made) == "string" and #made == 64 and not made:find("%X"), tostring(made))+h.equals("and is what its file holds", read_file(directory .. "/auth-secret"), made)+h.equals("it is written under a mask that keeps everyone else out", umasks[1] and umasks[1].mask, 63)+h.check("and the mask before it goes back after",+	#umasks == 2 and umasks[2].mask == umasks[1].old)+h.equals("it reaches its name through one link", #linked, 1)+h.equals("and the temporary file it was linked from is removed", linked[1] and read_file(linked[1]), nil)+load_script()+h.equals("a later load takes the same secret from the file", select(2, pcall(get_secret)), made)+h.equals("without making another", #linked, 1)++-- The request that made the secret first keeps it, since it may have+-- signed sessions with it already.+race_winner = string.rep("9", 64)+load_script(nil, directory .. "/raced-secret")+h.equals("a secret made meanwhile by another request wins", select(2, pcall(get_secret)), race_winner)+h.equals("and is left as it was", read_file(directory .. "/raced-secret"), race_winner)+h.equals("while the temporary file is still removed", read_file(linked[#linked]), nil)+race_winner = nil++-- A secret made by hand is read as its first line, so one saved with a+-- newline still serves, while one of the wrong length is refused.+write_file(directory .. "/typed-secret", made .. "\n")+load_script(nil, directory .. "/typed-secret")+h.equals("a secret ending in a newline is read without it", select(2, pcall(get_secret)), made)+write_file(directory .. "/short-secret", "0123456789\n")+load_script(nil, directory .. "/short-secret")+local short_ok, short_err = pcall(get_secret)+h.check("a secret of the wrong length is refused", not short_ok)+h.contains("as malformed", tostring(short_err), "is malformed, expected 64 hex characters")+load_script()+ -- The lookups themselves, which are the part the two variants do not share.  local hash = account_hash("ALICE")@@ -323,8 +398,7 @@ h.equals("and leaves the flag out of the way back", way_back_in(out), "/repo/log  -- The real get_secret, pointed at a directory nobody made, cannot sign the -- way back, so the page says so whole rather than breaking off in the form.-h.load_with(script, { { "local secret_filename = \"/var/lib/cgit/auth-secret\"",-	"local secret_filename = " .. string.format("%q", directory .. "/no-such-directory/auth-secret") } })+load_script(nil, directory .. "/no-such-directory/auth-secret") out = run_action("body", {}) h.contains("a secret that cannot be made leaves a page saying so", out, 	"<p class='login-error' role='alert'>Logins are unavailable. The server's error log says why.</p>")
diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.lua
index 97639ea..506babf 100644
--- a/tests/extensions/test-auth.lua
+++ b/tests/extensions/test-auth.lua
@@ -5,7 +5,7 @@
 -- load, so the harness meets both with deterministic stand-ins, which keeps
 -- every check about this script's own logic, the cookie layout, the expiry
 -- and field rules, the redirect vetting and the action flows, rather than
--- about OpenSSL. The signing secret is pinned by replacing get_secret, and
+-- about OpenSSL. The signing secret is made in the run's directory, and
 -- the accounts and access lists are filled through each variant's own
 -- channel, its commented-out examples switched on for the inline script and
 -- fixture files reached through a redirected io.open for the file one. Both
@@ -58,8 +58,13 @@ local inline_config = {
 		"\tcarol = \"" .. stored_hash .. "\",\n" },
 }
 
-local function load_script(changes)
-	local all = {}
+-- Every load keeps its signing secret in the run's directory, at the path
+-- given or else at one shared by all of them.
+local function load_script(changes, secret_path)
+	local all = {
+		{ "local secret_filename = \"/var/lib/cgit/auth-secret\"",
+			"local secret_filename = " .. string.format("%q", secret_path or directory .. "/auth-secret") },
+	}
 	if variant == "inline" then
 		for _, change in ipairs(inline_config) do
 			all[#all + 1] = change
@@ -69,9 +74,6 @@ local function load_script(changes)
 		all[#all + 1] = change
 	end
 	h.load_with(script, all)
-	function get_secret()
-		return string.rep("0123456789abcdef", 4)
-	end
 end
 
 load_script()
@@ -89,6 +91,79 @@ unistd.crypt = function(trypass, setting)
 	return real_crypt(trypass, setting)
 end
 
+local function read_file(path)
+	local file = io.open(path, "rb")
+	if file == nil then
+		return nil
+	end
+	local text = file:read("*a")
+	file:close()
+	return text
+end
+
+local function write_file(path, text)
+	local file = assert(io.open(path, "wb"))
+	file:write(text)
+	file:close()
+end
+
+-- The secret is made on first use, so the masks it is made under and the
+-- temporary files it is linked from are watched the same way. A secret set
+-- in race_winner is written just ahead of the link, the way another request
+-- making the file at the same moment would.
+local sysstat = require("posix.sys.stat")
+local real_umask = sysstat.umask
+local umasks = {}
+sysstat.umask = function(mask)
+	local old = real_umask(mask)
+	umasks[#umasks + 1] = { mask = mask, old = old }
+	return old
+end
+local real_link = unistd.link
+local linked = {}
+local race_winner = nil
+unistd.link = function(from, to)
+	linked[#linked + 1] = from
+	if race_winner then
+		write_file(to, race_winner)
+	end
+	return real_link(from, to)
+end
+
+local _, made = pcall(get_secret)
+h.check("a new secret is 64 hex characters",
+	type(made) == "string" and #made == 64 and not made:find("%X"), tostring(made))
+h.equals("and is what its file holds", read_file(directory .. "/auth-secret"), made)
+h.equals("it is written under a mask that keeps everyone else out", umasks[1] and umasks[1].mask, 63)
+h.check("and the mask before it goes back after",
+	#umasks == 2 and umasks[2].mask == umasks[1].old)
+h.equals("it reaches its name through one link", #linked, 1)
+h.equals("and the temporary file it was linked from is removed", linked[1] and read_file(linked[1]), nil)
+load_script()
+h.equals("a later load takes the same secret from the file", select(2, pcall(get_secret)), made)
+h.equals("without making another", #linked, 1)
+
+-- The request that made the secret first keeps it, since it may have
+-- signed sessions with it already.
+race_winner = string.rep("9", 64)
+load_script(nil, directory .. "/raced-secret")
+h.equals("a secret made meanwhile by another request wins", select(2, pcall(get_secret)), race_winner)
+h.equals("and is left as it was", read_file(directory .. "/raced-secret"), race_winner)
+h.equals("while the temporary file is still removed", read_file(linked[#linked]), nil)
+race_winner = nil
+
+-- A secret made by hand is read as its first line, so one saved with a
+-- newline still serves, while one of the wrong length is refused.
+write_file(directory .. "/typed-secret", made .. "\n")
+load_script(nil, directory .. "/typed-secret")
+h.equals("a secret ending in a newline is read without it", select(2, pcall(get_secret)), made)
+write_file(directory .. "/short-secret", "0123456789\n")
+load_script(nil, directory .. "/short-secret")
+local short_ok, short_err = pcall(get_secret)
+h.check("a secret of the wrong length is refused", not short_ok)
+h.contains("as malformed", tostring(short_err), "is malformed, expected 64 hex characters")
+load_script()
+
 -- The lookups themselves, which are the part the two variants do not share.
 
 local hash = account_hash("ALICE")
@@ -323,8 +398,7 @@ h.equals("and leaves the flag out of the way back", way_back_in(out), "/repo/log
 
 -- The real get_secret, pointed at a directory nobody made, cannot sign the
 -- way back, so the page says so whole rather than breaking off in the form.
-h.load_with(script, { { "local secret_filename = \"/var/lib/cgit/auth-secret\"",
-	"local secret_filename = " .. string.format("%q", directory .. "/no-such-directory/auth-secret") } })
+load_script(nil, directory .. "/no-such-directory/auth-secret")
 out = run_action("body", {})
 h.contains("a secret that cannot be made leaves a page saying so", out,
 	"<p class='login-error' role='alert'>Logins are unavailable. The server's error log says why.</p>")