Allows to terminate the transaction if not succeeded

[jcmoraisjr]

We're improving the external authentication functionality provided by our haproxy cluster, and one of the changes is the ability to configure the external service to provide a response to the client, either a redirect, custom headers or even a small body if the authentication fails.

The diff below is the first attempt to update auth-request in order to allow us to implement this functionality without breaking compatibility. It's currently poorly tested and maybe something might be changed, but I can say that most of the job is done and it's apparently working fine for us.

I've currently some doubts, hence this issue instead of a pull request:

1. Is such change welcome to the component? I mean the functionality as a whole, I can easily change the approach if needed.
2. Backward compatibility doesn't allow us to simply change the parameters, maybe create two facade functions and move the current function outside the action declaration would be better than using haproxy vars.

Thoughts on this?

diff --git a/auth-request.lua b/auth-request.lua
index e07f9c3..16318b5 100644
--- a/auth-request.lua
+++ b/auth-request.lua
@@ -46,6 +46,21 @@ function sanitize_header_for_variable(header)
 	return header:gsub("[^a-zA-Z0-9]", "_")
 end

+function send(txn, response)
+	local reply = txn:reply()
+	if response then
+		reply:set_status(response.status_code)
+		for header, value in response:get_headers(true) do
+			reply:add_header(header, value)
+		end
+		if response.content then
+			reply:set_body(response.content)
+		end
+	else
+		reply:set_status(500)
+	end
+	txn:done(reply)
+end

 core.register_action("auth-request", { "http-req" }, function(txn, be, path)
 	set_var(txn, "txn.auth_response_successful", false)
@@ -89,28 +104,43 @@ core.register_action("auth-request", { "http-req" }, function(txn, be, path)
 	end

 	-- Make request to backend.
-	local response, err = http.head {
+	method = (txn:get_var("req.auth_request_method") or "HEAD"):upper()
+	local response, err = http.send(method, {
 		url = "http://" .. addr .. path,
 		headers = headers,
-	}
+	})
+
+	handle_failure = txn:get_var("req.auth_response_on_failure") ~= nil

 	-- Check whether we received a valid HTTP response.
 	if response == nil then
 		txn:Warning("Failure in auth-request backend '" .. be .. "': " .. err)
-		set_var(txn, "txn.auth_response_code", 500)
+		if handle_failure then
+			send(txn, nil)
+		else
+			set_var(txn, "txn.auth_response_code", 500)
+		end
 		return
 	end

-	set_var(txn, "txn.auth_response_code", response.status_code)
+	response_ok = 200 <= response.status_code and response.status_code < 300

-	for header, value in response:get_headers(true) do
-		set_var(txn, "req.auth_response_header." .. sanitize_header_for_variable(header), value)
+	-- Transaction is terminated if response is not 2xx and `req.auth_response_on_failure` was defined.
+	-- Only need to update variables if the transaction will not be terminated.
+	if response_ok or not handle_failure then
+		set_var(txn, "txn.auth_response_code", response.status_code)
+		for header, value in response:get_headers(true) do
+			set_var(txn, "req.auth_response_header." .. sanitize_header_for_variable(header), value)
+		end
 	end

 	-- 2xx: Allow request.
-	if 200 <= response.status_code and response.status_code < 300 then
+	if response_ok then
 		set_var(txn, "txn.auth_response_successful", true)
 	-- Don't allow other codes.
+	-- Build the response and send to the client if required.
+	elseif handle_failure then
+		send(txn, response)
 	-- Codes with Location: Passthrough location at redirect.
 	elseif response.status_code == 301 or response.status_code == 302 or response.status_code == 303 or response.status_code == 307 or response.status_code == 308 then
 		set_var(txn, "txn.auth_response_location", response:get_header("location", "last"))

[TimWolla]

Is such change welcome to the component? I mean the functionality as a whole, I can easily change the approach if needed.

I don't see why not! If it's useful to you, then it will certainly be useful to others.

maybe create two facade functions and move the current function outside the action declaration would be better than using haproxy vars.

I'd prefer a clean separation of the two behaviors. How about http-request auth-intercept for your proposal? Absolutely avoid using variables, because that is fragile.

---

Feel free to send a PR for this. Please make sure to add VTest tests (https://github.com/TimWolla/haproxy-auth-request/tree/main/test) and to adjust the README. I'm happy to advise if any further questions arise.

[TimWolla]

make sure to add VTest tests

One more thing: It might be that GitHub Actions CI is currently broken due to this: vtest/VTest#27. At the very least the builds for the https://github.com/TimWolla/action-install-haproxy/ action fail. I did not yet get around to working around this, yet.

[jcmoraisjr]

Hey Tim, I've just finished what I expect it's pretty close to my final implementation. The auth-request.lua diff can be found here. My main requisites are the ability to terminate the transaction with the client, and also to customize which headers should be copied to and from the external authentication service. This guided me to make the following changes:

• the new action has a bunch of params
• there are some magic behavior depending on the value of the params, see the auth_request function comment
• backward compatibility made the code somewhat harder to understand and maintain

I want to write the e2e tests, documentation, adjust anything that seems wrong or not optimal, so I can help other users with their own use cases, and also have other testers and maintainers to my code base. Does the actual changes continue to be interesting to auth-request? If so I'm planning to eg break the main function into small functionalities and use one distinct functions per HAProxy action, instead of one single, big and magic function/monolith.

[TimWolla]

I believe this is resolved with #37. Thanks!