diff options
| author | Bryce Kwon <bryce@brycekwon.com> | |
|---|---|---|
| committer | Bryce Kwon <bryce@brycekwon.com> | |
| commit | ||
| parent | ||
| tree | ||
| download | ||
End a session when its account's hash changes
A session stayed good for its whole week after the password changed or
the account went away, which is when ending it matters most.
| -rw-r--r-- | custom/extensions/auth-file.lua | 73 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| -rw-r--r-- | custom/extensions/auth-inline.lua | 73 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| -rw-r--r-- | tests/extensions/test-auth.lua | 24 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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", |
