Say when logins are unavailable

The way back was signed part way through the form, so while the secret's
directory did not exist, or a library was missing, cgit's error landed
inside the hidden input and the page broke off there with no fields.

5 files changed, 78 insertions, 15 deletions

diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.luaindex ba05875..3814a1f 100644--- a/custom/extensions/auth-file.lua+++ b/custom/extensions/auth-file.lua@@ -91,10 +91,11 @@ local groups_filename = "/etc/cgit-auth/groups" -- protected ones apart. local repos_filename = "/etc/cgit-auth/repos" --- Where the cookie-signing secret is stored, created on first use. It must be--- persistent and writable by cgit, and it is worth keeping 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.+-- Where the cookie-signing secret is stored, created on first use in a+-- directory that has to exist already. It must be persistent and writable by+-- cgit, and it is worth keeping 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/lib/cgit/auth-secret"  -- How long a login stays valid, in seconds. The default is one week.@@ -295,7 +296,9 @@ function get_secret() 		local temporary_file = io.open(temporary_filename, "w") 		if temporary_file == nil then 			sysstat.umask(old_umask)-			error("cgit auth: cannot create secret file " .. secret_filename)+			local directory = secret_filename:match("^(.*)/") or "."+			error("cgit auth: cannot create secret file " .. secret_filename ..+				", as " .. directory .. " is missing or not writable by cgit") 		end 		local wrote = temporary_file:write(tohex(rand.bytes(32))) 		local closed = temporary_file:close()@@ -520,6 +523,16 @@ function body() 	end  	html("<h2>Authentication Required</h2>")+	-- The token is signed before any of the form is written, since an error+	-- raised part way through would land inside its markup. A missing+	-- library or a secret that cannot be made leaves a page saying logins+	-- are unavailable instead, with the reason in the error log.+	local signed, token = pcall(secure_value, "redirect", target, 0)+	if not signed then+		io.stderr:write("[cgit] " .. tostring(token):match("[^\n]*") .. "\n")+		html("<p class='login-error' role='alert'>Logins are unavailable. The server's error log says why.</p>")+		return 0+	end 	if failed then 		html("<p class='login-error' role='alert'>Wrong username or password.</p>") 	end@@ -527,7 +540,7 @@ function body() 	html_attr(cgit["login"]) 	html("'>") 	html("<input type='hidden' name='redirect' value='")-	html_attr(secure_value("redirect", target, 0))+	html_attr(token) 	html("'>") 	html("<table>") 	html("<tr><td><label for='username'>Username:</label></td><td>")
diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.lua
index ba05875..3814a1f 100644
--- a/custom/extensions/auth-file.lua
+++ b/custom/extensions/auth-file.lua
@@ -91,10 +91,11 @@ local groups_filename = "/etc/cgit-auth/groups"
 -- protected ones apart.
 local repos_filename = "/etc/cgit-auth/repos"
 
--- Where the cookie-signing secret is stored, created on first use. It must be
--- persistent and writable by cgit, and it is worth keeping 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.
+-- Where the cookie-signing secret is stored, created on first use in a
+-- directory that has to exist already. It must be persistent and writable by
+-- cgit, and it is worth keeping 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/lib/cgit/auth-secret"
 
 -- How long a login stays valid, in seconds. The default is one week.
@@ -295,7 +296,9 @@ function get_secret()
 		local temporary_file = io.open(temporary_filename, "w")
 		if temporary_file == nil then
 			sysstat.umask(old_umask)
-			error("cgit auth: cannot create secret file " .. secret_filename)
+			local directory = secret_filename:match("^(.*)/") or "."
+			error("cgit auth: cannot create secret file " .. secret_filename ..
+				", as " .. directory .. " is missing or not writable by cgit")
 		end
 		local wrote = temporary_file:write(tohex(rand.bytes(32)))
 		local closed = temporary_file:close()
@@ -520,6 +523,16 @@ function body()
 	end
 
 	html("<h2>Authentication Required</h2>")
+	-- The token is signed before any of the form is written, since an error
+	-- raised part way through would land inside its markup. A missing
+	-- library or a secret that cannot be made leaves a page saying logins
+	-- are unavailable instead, with the reason in the error log.
+	local signed, token = pcall(secure_value, "redirect", target, 0)
+	if not signed then
+		io.stderr:write("[cgit] " .. tostring(token):match("[^\n]*") .. "\n")
+		html("<p class='login-error' role='alert'>Logins are unavailable. The server's error log says why.</p>")
+		return 0
+	end
 	if failed then
 		html("<p class='login-error' role='alert'>Wrong username or password.</p>")
 	end
@@ -527,7 +540,7 @@ function body()
 	html_attr(cgit["login"])
 	html("'>")
 	html("<input type='hidden' name='redirect' value='")
-	html_attr(secure_value("redirect", target, 0))
+	html_attr(token)
 	html("'>")
 	html("<table>")
 	html("<tr><td><label for='username'>Username:</label></td><td>")
diff --git a/custom/extensions/auth-inline.lua b/custom/extensions/auth-inline.luaindex 971d662..e7f12cd 100644--- a/custom/extensions/auth-inline.lua+++ b/custom/extensions/auth-inline.lua@@ -92,10 +92,11 @@ local users = { 	-- bob   = "$6$rounds=300000$REPLACE_THIS_SALT$REPLACE_THIS_HASH", } --- Where the cookie-signing secret is stored, created on first use. It must be--- persistent and writable by cgit, and it is worth keeping 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.+-- Where the cookie-signing secret is stored, created on first use in a+-- directory that has to exist already. It must be persistent and writable by+-- cgit, and it is worth keeping 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/lib/cgit/auth-secret"  -- How long a login stays valid, in seconds. The default is one week.@@ -258,7 +259,9 @@ function get_secret() 		local temporary_file = io.open(temporary_filename, "w") 		if temporary_file == nil then 			sysstat.umask(old_umask)-			error("cgit auth: cannot create secret file " .. secret_filename)+			local directory = secret_filename:match("^(.*)/") or "."+			error("cgit auth: cannot create secret file " .. secret_filename ..+				", as " .. directory .. " is missing or not writable by cgit") 		end 		local wrote = temporary_file:write(tohex(rand.bytes(32))) 		local closed = temporary_file:close()@@ -483,6 +486,16 @@ function body() 	end  	html("<h2>Authentication Required</h2>")+	-- The token is signed before any of the form is written, since an error+	-- raised part way through would land inside its markup. A missing+	-- library or a secret that cannot be made leaves a page saying logins+	-- are unavailable instead, with the reason in the error log.+	local signed, token = pcall(secure_value, "redirect", target, 0)+	if not signed then+		io.stderr:write("[cgit] " .. tostring(token):match("[^\n]*") .. "\n")+		html("<p class='login-error' role='alert'>Logins are unavailable. The server's error log says why.</p>")+		return 0+	end 	if failed then 		html("<p class='login-error' role='alert'>Wrong username or password.</p>") 	end@@ -490,7 +503,7 @@ function body() 	html_attr(cgit["login"]) 	html("'>") 	html("<input type='hidden' name='redirect' value='")-	html_attr(secure_value("redirect", target, 0))+	html_attr(token) 	html("'>") 	html("<table>") 	html("<tr><td><label for='username'>Username:</label></td><td>")
diff --git a/custom/extensions/auth-inline.lua b/custom/extensions/auth-inline.lua
index 971d662..e7f12cd 100644
--- a/custom/extensions/auth-inline.lua
+++ b/custom/extensions/auth-inline.lua
@@ -92,10 +92,11 @@ local users = {
 	-- bob   = "$6$rounds=300000$REPLACE_THIS_SALT$REPLACE_THIS_HASH",
 }
 
--- Where the cookie-signing secret is stored, created on first use. It must be
--- persistent and writable by cgit, and it is worth keeping 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.
+-- Where the cookie-signing secret is stored, created on first use in a
+-- directory that has to exist already. It must be persistent and writable by
+-- cgit, and it is worth keeping 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/lib/cgit/auth-secret"
 
 -- How long a login stays valid, in seconds. The default is one week.
@@ -258,7 +259,9 @@ function get_secret()
 		local temporary_file = io.open(temporary_filename, "w")
 		if temporary_file == nil then
 			sysstat.umask(old_umask)
-			error("cgit auth: cannot create secret file " .. secret_filename)
+			local directory = secret_filename:match("^(.*)/") or "."
+			error("cgit auth: cannot create secret file " .. secret_filename ..
+				", as " .. directory .. " is missing or not writable by cgit")
 		end
 		local wrote = temporary_file:write(tohex(rand.bytes(32)))
 		local closed = temporary_file:close()
@@ -483,6 +486,16 @@ function body()
 	end
 
 	html("<h2>Authentication Required</h2>")
+	-- The token is signed before any of the form is written, since an error
+	-- raised part way through would land inside its markup. A missing
+	-- library or a secret that cannot be made leaves a page saying logins
+	-- are unavailable instead, with the reason in the error log.
+	local signed, token = pcall(secure_value, "redirect", target, 0)
+	if not signed then
+		io.stderr:write("[cgit] " .. tostring(token):match("[^\n]*") .. "\n")
+		html("<p class='login-error' role='alert'>Logins are unavailable. The server's error log says why.</p>")
+		return 0
+	end
 	if failed then
 		html("<p class='login-error' role='alert'>Wrong username or password.</p>")
 	end
@@ -490,7 +503,7 @@ function body()
 	html_attr(cgit["login"])
 	html("'>")
 	html("<input type='hidden' name='redirect' value='")
-	html_attr(secure_value("redirect", target, 0))
+	html_attr(token)
 	html("'>")
 	html("<table>")
 	html("<tr><td><label for='username'>Username:</label></td><td>")
diff --git a/tests/extensions/harness.lua b/tests/extensions/harness.luaindex 2fe722f..8c1ba6c 100644--- a/tests/extensions/harness.lua+++ b/tests/extensions/harness.lua@@ -54,14 +54,30 @@ html_url_path = sink(function(s) 	end)) end) +-- What a script writes to standard error goes to the web server's error log+-- under cgit, so it is kept here for a check to read instead.+local logged = {}++io.stderr = {+	write = function(self, ...)+		logged[#logged + 1] = table.concat({ ... })+		return self+	end,+}+ function harness.reset() 	pieces = {}+	logged = {} end  function harness.output() 	return table.concat(pieces) end +function harness.error_log()+	return table.concat(logged)+end+ function harness.load(script) 	dofile(script) end@@ -182,6 +198,11 @@ function harness.stub_auth_modules() 	preload("posix.unistd", { 		crypt = fake_crypt, 	})+	preload("posix.sys.stat", {+		umask = function()+			return 18+		end,+	}) 	preload("openssl.rand", { 		bytes = function(count) 			local out = {}
diff --git a/tests/extensions/harness.lua b/tests/extensions/harness.lua
index 2fe722f..8c1ba6c 100644
--- a/tests/extensions/harness.lua
+++ b/tests/extensions/harness.lua
@@ -54,14 +54,30 @@ html_url_path = sink(function(s)
 	end))
 end)
 
+-- What a script writes to standard error goes to the web server's error log
+-- under cgit, so it is kept here for a check to read instead.
+local logged = {}
+
+io.stderr = {
+	write = function(self, ...)
+		logged[#logged + 1] = table.concat({ ... })
+		return self
+	end,
+}
+
 function harness.reset()
 	pieces = {}
+	logged = {}
 end
 
 function harness.output()
 	return table.concat(pieces)
 end
 
+function harness.error_log()
+	return table.concat(logged)
+end
+
 function harness.load(script)
 	dofile(script)
 end
@@ -182,6 +198,11 @@ function harness.stub_auth_modules()
 	preload("posix.unistd", {
 		crypt = fake_crypt,
 	})
+	preload("posix.sys.stat", {
+		umask = function()
+			return 18
+		end,
+	})
 	preload("openssl.rand", {
 		bytes = function(count)
 			local out = {}
diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.luaindex 974544d..3a4fabf 100644--- a/tests/extensions/test-auth.lua+++ b/tests/extensions/test-auth.lua@@ -302,6 +302,18 @@ h.contains("a failed login says so", out, 	"<p class='login-error' role='alert'>Wrong username or password.</p>") h.equals("and leaves the flag out of the way back", way_back_in(out), "/repo/log/?q=x") +-- 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 = \"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>")+h.excludes("with no form to fill in", out, "<form")+h.contains("and the error log names the directory", h.error_log(),+	"as no-such-directory is missing or not writable by cgit\n")+load_script()+ local way_back = secure_value("redirect", "/repo/", 0)  out = login("Alice", password, way_back)@@ -373,6 +385,9 @@ for _, name in ipairs({ "openssl.rand", "openssl.hmac", "posix.sys.stat", "posix end h.equals("without the libraries a public repository is served", visit("unlisted"), 1) h.check("while a protected one fails closed", not pcall(visit, "secret-repo", session))-h.check("and so does its login form", not pcall(run_action, "body", {}))+out = run_action("body", {})+h.contains("and its login page says logins are unavailable", out, "Logins are unavailable.")+h.excludes("and no form to fill in", out, "<form")+h.contains("while the error log names the missing library", h.error_log(), "openssl.rand")  h.finish()
diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.lua
index 974544d..3a4fabf 100644
--- a/tests/extensions/test-auth.lua
+++ b/tests/extensions/test-auth.lua
@@ -302,6 +302,18 @@ h.contains("a failed login says so", out,
 	"<p class='login-error' role='alert'>Wrong username or password.</p>")
 h.equals("and leaves the flag out of the way back", way_back_in(out), "/repo/log/?q=x")
 
+-- 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 = \"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>")
+h.excludes("with no form to fill in", out, "<form")
+h.contains("and the error log names the directory", h.error_log(),
+	"as no-such-directory is missing or not writable by cgit\n")
+load_script()
+
 local way_back = secure_value("redirect", "/repo/", 0)
 
 out = login("Alice", password, way_back)
@@ -373,6 +385,9 @@ for _, name in ipairs({ "openssl.rand", "openssl.hmac", "posix.sys.stat", "posix
 end
 h.equals("without the libraries a public repository is served", visit("unlisted"), 1)
 h.check("while a protected one fails closed", not pcall(visit, "secret-repo", session))
-h.check("and so does its login form", not pcall(run_action, "body", {}))
+out = run_action("body", {})
+h.contains("and its login page says logins are unavailable", out, "Logins are unavailable.")
+h.excludes("and no form to fill in", out, "<form")
+h.contains("while the error log names the missing library", h.error_log(), "openssl.rand")
 
 h.finish()
diff --git a/tests/t0505-auth.sh b/tests/t0505-auth.shindex e9349d4..3b49c7e 100755--- a/tests/t0505-auth.sh+++ b/tests/t0505-auth.sh@@ -69,9 +69,10 @@ test_expect_success CGIT_LUA_NOAUTH 'without the modules a protected repository 	repo.url=authy 	repo.path=$PWD/repos/authy/.git 	EOF-	cgit_rc guardedrc "url=authy/log/" >tmp &&+	cgit_rc guardedrc "url=authy/log/" >tmp 2>err && 	! grep "guarded commit" tmp &&-	grep "Unable to complete the request" tmp+	grep "<p class=.login-error. role=.alert.>Logins are unavailable" tmp &&+	grep "^\[cgit\] " err '  # A filter of the test's own turns every visitor away, which needs no Lua.
diff --git a/tests/t0505-auth.sh b/tests/t0505-auth.sh
index e9349d4..3b49c7e 100755
--- a/tests/t0505-auth.sh
+++ b/tests/t0505-auth.sh
@@ -69,9 +69,10 @@ test_expect_success CGIT_LUA_NOAUTH 'without the modules a protected repository
 	repo.url=authy
 	repo.path=$PWD/repos/authy/.git
 	EOF
-	cgit_rc guardedrc "url=authy/log/" >tmp &&
+	cgit_rc guardedrc "url=authy/log/" >tmp 2>err &&
 	! grep "guarded commit" tmp &&
-	grep "Unable to complete the request" tmp
+	grep "<p class=.login-error. role=.alert.>Logins are unavailable" tmp &&
+	grep "^\[cgit\] " err
 '
 
 # A filter of the test's own turns every visitor away, which needs no Lua.