feat: add forward-auth plugin by bzp2010 · Pull Request #6037 · apache/apisix

bzp2010 · 2022-01-06T18:44:18Z

What this PR does / why we need it:

Add the forward-auth plugin.

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

shuaijinchao · 2022-01-07T01:39:30Z

+    end
+
+    local params = {
+        method = "GET",


Should the request method be mapped to the client request method?

I don't think it's needed unless we provide other special features for this, the implementation in other software is the same.

In our or other forward-auth plugins, authentication-related data is sent via fixed requests, not POST data, so I think that's enough too.

ping @shuaijinchao

OK, use GET as a fixed request method you should remove it because the default request method is GET

OK, use GET as a fixed request method you should remove it because the default request method is GET

Oh, I get it. Modify will be made later.

shuaijinchao · 2022-01-07T01:44:14Z

+    if not res or err then
+        core.log.error("failed to process forward auth, err: ", err)
+        return 403
+    end


Suggested change

if not res or err then

core.log.error("failed to process forward auth, err: ", err)

return 403

end

if not res then

core.log.error("failed to process forward auth, err: ", err)

return 403

end

Would it be better to judge this way?

I don't understand how to do it really right, I noticed that in other plugin implementations there are those that use not res alone, those that use err alone and those that use not res or err, which one should we take?

apisix/apisix/plugins/authz-keycloak.lua

Lines 251 to 254 in 58fec8f

local res, error = httpc:request_uri(conf.discovery, params)

if not res then

err = "Accessing discovery URL (" .. conf.discovery .. ") failed: " .. error

apisix/apisix/plugins/serverless/generic-upstream.lua

Lines 110 to 113 in 58fec8f

local res, err = httpc:request_uri(conf.function_uri, params)

if not res or err then

core.log.error("failed to process ", plugin_name, ", err: ", err)

apisix/apisix/plugins/wolf-rbac.lua

Lines 145 to 147 in 58fec8f

local res, err = httpc:request_uri(uri, params)

if err then

core.log.error("FAIL REQUEST [ ",core.json.delay_encode(

I hope to get your guidance.
cc @spacewander

I read through each branch of https://github.com/api7/lua-resty-http/blob/b63a9bbf2a3361836b2322e3c689d6d7fd83ec55/lib/resty/http.lua#L900, look like there are only two cases:

nil, err

res, nil, while res is a table

Referring to the way it is usually used, I will check if err is not nil.

if err then xxx end

Also, I think there needs to make a rules that developers should follow to handle error checking (at least on the http client), the mix of different usages is confusing.

Currently, we have to enforce it with code review. Maybe we can invest in linter in the future.

I think it should be used

if res then err end

instead of

if err then err end

this way of writing may be more suitable for languages such as Go

the common exception function return in OpenResty is return nil, err, the first value is enough to know whether to handle err, and you can refer to more code and test cases in lua-resty-core, such as: https://github.com/openresty/lua-resty-core/blob/e5217414669c100b334940b250d5340911a02dd0/t/ctx.t#L133-L143

spacewander · 2022-01-07T01:44:40Z

 end


+function _M.get_uri(ctx)


We should not add a common method that is only used by a place. Actually, I think we should remove the get_path too.

It is strange that get_path fetches $uri but get_uri fetches $request_uri. Look like it is a premature optimization that brings inconsistent names.

So I should just use ctx.var.request_uri instead of wrapping it? Can I merge the functionality of get_path into get_uri and add a with_args parameter indicating whether or not I need to carry queries.

Don't make thing complex!
The name of request_uri and uri already show the difference. There is no need to wrap them into a function which is rarely used. Premature optimization is the source of evil.

We can remove the get_path in the next PR. Better to get rid of it before the release, so that people won't ask why we use get_path in one place but use ctx.var.uri in the most elsewhere.

removed, I will remove get_path in a later PR.

spacewander · 2022-01-07T01:45:04Z

+            type = "array",
+            default = {},
+            items = {type = "string"},
+            description = "client request header that will be sent to the authorization"


Suggested change

description = "client request header that will be sent to the authorization"

description = "client request header that will be sent to the authorization service"

spacewander · 2022-01-07T01:52:57Z

+            default = {},
+            items = {type = "string"},
+            description = "authorization response header that will be sent to"
+                           .. "the client when authorize failure"


Suggested change

.. "the client when authorize failure"

.. "the client when authorizing failed"

spacewander · 2022-01-07T02:01:05Z

+    -- append headers that need to be get from the client request header
+    if #conf.request_headers > 0 then
+        for _, header in ipairs(conf.request_headers) do
+            auth_headers[header] = core.request.header(ctx, header)


Better not trust the client's X-Forwarded-XX by default

fixed, because auth_headers has been set in our concern headers, after the judgment, the request header from the client can not override them, that's not trust the client header. L89-L91

apisix/apisix/plugins/forward-auth.lua

Lines 78 to 93 in 8a853d3

local auth_headers = {

["X-Forwarded-Proto"] = core.request.get_scheme(ctx),

["X-Forwarded-Method"] = core.request.get_method(),

["X-Forwarded-Host"] = core.request.get_host(ctx),

["X-Forwarded-Uri"] = core.request.get_uri(ctx),

["X-Forwarded-For"] = core.request.get_remote_client_ip(ctx),

}

-- append headers that need to be get from the client request header

if #conf.request_headers > 0 then

for _, header in ipairs(conf.request_headers) do

if not auth_headers[header] then

auth_headers[header] = core.request.header(ctx, header)

end

end

else

spacewander · 2022-01-07T02:01:11Z

+            auth_headers[header] = core.request.header(ctx, header)
+        end
+    else
+        auth_headers = core.table.merge(core.request.headers(), auth_headers)


Because, merge the list of defined headers into the client request header, any key headers passed in by the client are overwritten, so I implement the untrusted client header.

spacewander · 2022-01-07T02:10:34Z

+}
+```
+
+3. **client_headers** Send `authorization` service response header to `client` when authorize failure


Suggested change

3. **client_headers** Send `authorization` service response header to `client` when authorize failure

3. **client_headers** Send `authorization` service response header to `client` when authorizing failed

spacewander · 2022-01-07T02:14:19Z

+                        "plugins": {
+                            "serverless-pre-function": {
+                                "phase": "rewrite",
+                                "functions": ["return function (conf, ctx) local core = require(\"apisix.core\"); local authorization = core.request.header(ctx, \"Authorization\"); if authorization == \"123\" then core.response.exit(200); elseif authorization == \"321\" then core.response.set_header(\"X-User-ID\", \"i-am-an-user\"); core.response.exit(200); else core.response.set_header(\"Location\", \"http://example.com/auth\"); core.response.exit(403); end end"]


Let's log down the X-Forwarded-XX received by the service. And better to break down the very long code.

I used another method to record these heads. I also added a simple echo function under /auth route.

spacewander · 2022-01-07T02:14:51Z

+GET /hello
+--- error_code: 403
+--- response_headers
+Location: http://example.com/auth


Let's add a test that the client passes X-Forwarded-XX by itself

added, I added a test case to demonstrate that these client-side incoming key headers are indeed dropped in APISIX.

spacewander · 2022-01-07T02:15:26Z

+| -- | -- | -- | -- | -- | -- |
+| host | string | required |  |  | Authorization service host (eg. https://localhost:9188) |
+| ssl_verify | boolean | optional | true |   | Whether to verify the certificate |
+| request_headers | array[string] | optional |  |  | `client` request header that will be sent to the `authorization` service |


Let's doc the behavior when this field is empty

spacewander · 2022-01-07T02:15:42Z

+| host | string | required |  |  | Authorization service host (eg. https://localhost:9188) |
+| ssl_verify | boolean | optional | true |   | Whether to verify the certificate |
+| request_headers | array[string] | optional |  |  | `client` request header that will be sent to the `authorization` service |
+| upstream_headers | array[string] | optional |  |  | `authorization` service response header that will be sent to the `upstream` |


Co-authored-by: 罗泽轩 <[email protected]>

spacewander · 2022-01-09T12:23:32Z

+                            "serverless-pre-function": {
+                                "phase": "rewrite",
+                                "functions": [
+                                    "return function (conf, ctx) local core = require(\"apisix.core\"); if core.request.header(ctx, \"Authorization\") == \"111\" then core.response.exit(200); end end",


Let's indent the code. Like:

diff --git t/plugin/forward-auth.t t/plugin/forward-auth.t index dca35b76..285e60fb 100644 --- t/plugin/forward-auth.t +++ t/plugin/forward-auth.t @@ -74,7 +74,12 @@ property "request_headers" validation failed: wrong type: expected array, got st "serverless-pre-function": { "phase": "rewrite", "functions": [ - "return function (conf, ctx) local core = require(\"apisix.core\"); if core.request.header(ctx, \"Authorization\") == \"111\" then core.response.exit(200); end end", + "return function (conf, ctx) + local core = require(\"apisix.core\"); + if core.request.header(ctx, \"Authorization\") == \"111\" then + core.response.exit(200) + end + end", "return function (conf, ctx) local core = require(\"apisix.core\"); if core.request.header(ctx, \"Authorization\") == \"222\" then core.response.set_header(\"X-User-ID\", \"i-am-an-user\"); core.response.exit(200); end end", "return function (conf, ctx) local core = require(\"apisix.core\"); if core.request.header(ctx, \"Authorization\") == \"333\" then core.response.set_header(\"Location\", \"http://example.com/auth\"); core.response.exit(403); end end", "return function (conf, ctx) local core = require(\"apisix.core\"); if core.request.header(ctx, \"Authorization\") == \"444\" then core.response.exit(403, core.request.headers(ctx)); end end"

Would be better to log down the "X-Forwarded-XX" headers and check it

~~Log check for X-Forwarded-XX, I will add it later.~~

Splitting the single line long code is done, which uses ]]..[[ truncates the data code, the code is so long that it cannot be parsed, so I modified it by referring to the explanation in this issue.

openresty/lua-nginx-module#1622

~~Log check for X-Forwarded-XX, I will add it later.~~

In the session of adding more cases, I found out that we won't actually use it, and that the function to print the request header is already implemented using /echo.

spacewander · 2022-01-09T12:30:30Z

+        },
+        request_headers = {
+            type = "array",
+            default = {},


Look like we can't support forward no headers from the client?
If this field is empty, all headers are forwarded. What about don't provide a default value? (Use empty array if no headers are need)

I think it's rightfully so, and that's an oversight on my part. I have reversed the meaning of request_headers and client_headers, if the user does not set this parameter, APISIX will send nothing.

spacewander · 2022-01-09T12:32:02Z

+                client_headers[header] = res.headers[header]
+            end
+        else
+            client_headers = res.headers


IMHO, would be better to use allowlist? If no client_headers, no headers will be returned to the client. Some headers are not expected to be overridden, like Server / Date.

Ditto, changed

spacewander · 2022-01-10T08:20:32Z

+| -- | -- | -- | -- | -- | -- |
+| host | string | required |  |  | Authorization service host (eg. https://localhost:9188) |
+| ssl_verify | boolean | optional | true |   | Whether to verify the certificate |
+| request_headers | array[string] | optional |  |  | `client` request header that will be sent to the `authorization` service. When it is not set, all `client` request headers are sent to the `authorization` service, except for those provided by APISIX (X-Forwarded-XXX). |


Need to update the doc

spacewander · 2022-01-10T08:23:23Z

+                            "serverless-pre-function": {
+                                "phase": "rewrite",
+                                "functions": [
+                                    "return function(conf, ctx)


Would be better to log down the APISIX generated "X-Forwarded-XX" headers and check it

I've merged it with the checkcase for overriding user input requests, which will check all APISIX generated headers at once. TEST 6: hit route (check APISIX generated headers and ignore client headers), It is provided by our echo function.

apisix/t/plugin/forward-auth.t

Lines 221 to 225 in beea582

--- error_code: 403

--- response_body eval

qr/\"x-forwarded-proto\":\"http\"/ and qr/\"x-forwarded-method\":\"GET\"/ and

qr/\"x-forwarded-host\":\"localhost\"/ and qr/\"x-forwarded-uri\":\"\\\/hello\"/ and

qr/\"x-forwarded-for\":\"127.0.0.1\"/

spacewander · 2022-01-11T01:12:03Z

+| -- | -- | -- | -- | -- | -- |
+| host | string | required |  |  | Authorization service host (eg. https://localhost:9188) |
+| ssl_verify | boolean | optional | true |   | Whether to verify the certificate |
+| request_headers | array[string] | optional |  |  | `client` request header that will be sent to the `authorization` service. When it is not set, no `client` request headers are sent to the `authorization` service, except for those provided by APISIX (X-Forwarded-XXX). |


Let's provide a list of the X-Forwarded-XXX headers

the Data Definition section added

bzp2010 added 11 commits January 6, 2022 23:23

feat: add forward auth plugin

98d9671

test: add get uri case

da4c5ef

feat: support client_headers

8525a76

test: add forward-auth case

868fced

test: add plugin list

1060fcb

feat: add default plugin list

1a14ab1

fix: description typo

5952350

test: remove unused code

ea6d529

fix: typo

24882f8

docs: add forward-auth document

eeaf575

feat: improve config check

bf5ade0

bzp2010 added enhancement New feature or request plugin labels Jan 6, 2022

bzp2010 requested review from shuaijinchao, spacewander and tzssangglass January 6, 2022 18:44

bzp2010 self-assigned this Jan 6, 2022

bzp2010 added 6 commits January 7, 2022 02:46

docs: fix lint

e31e6da

Merge remote-tracking branch 'upstream/master' into feat-forward-auth

4209f5a

docs: add category

c2257c4

fix: lint

7ce1bac

fix: lint

e9d1fa6

feat: remove unused code

18db978

shuaijinchao reviewed Jan 7, 2022

View reviewed changes

spacewander reviewed Jan 7, 2022

View reviewed changes

bzp2010 and others added 5 commits January 7, 2022 10:33

docs: fix typo

211c37f

Co-authored-by: 罗泽轩 <[email protected]>

feat: ignore client X headers

8a853d3

docs: remove unused code

a84ddf9

docs: improve document

67b1a7e

fix: wrong code push

a4b05d9

bzp2010 added 2 commits January 7, 2022 14:36

test: add ignore client X-Forwarded headers case

63811b5

test: improve ignore client X-Forwarded headers case

ac3ae02

bzp2010 requested review from shuaijinchao and spacewander January 7, 2022 06:56

bzp2010 added 2 commits January 7, 2022 21:57

feat: remove get uri

8fef52c

chore: change http client error check

07eb935

spacewander reviewed Jan 9, 2022

View reviewed changes

bzp2010 added 7 commits January 10, 2022 11:49

test: split single line of long code

4fb97f7

feat: reverse request_headers and client_headers meaning

868fa43

test: add headers logger

01f33f2

test: add no upsteam_headers and client_headers case

53f736b

test: remove headers logger

6c6304c

chore: remove default GET method

96a23ba

chore: change error check

ba64f8f

bzp2010 requested a review from spacewander January 10, 2022 06:54

spacewander reviewed Jan 10, 2022

View reviewed changes

bzp2010 added 2 commits January 10, 2022 16:46

docs: update outdatad desc

9565348

test: extend APISIX generated headers check

beea582

bzp2010 requested a review from spacewander January 10, 2022 09:07

shuaijinchao approved these changes Jan 11, 2022

View reviewed changes

spacewander reviewed Jan 11, 2022

View reviewed changes

bzp2010 added 3 commits January 11, 2022 09:29

docs: add data definitions

c49890f

docs: add more data desc

a83e2b9

docs: fix lint

8d11c22

bzp2010 requested a review from spacewander January 11, 2022 01:46

spacewander approved these changes Jan 11, 2022

View reviewed changes

spacewander merged commit 8dbdd1f into apache:master Jan 11, 2022

shuaijinchao mentioned this pull request Jan 12, 2022

chore: update use resty-http syntax question #6077

Merged

4 tasks

bisakhmondal mentioned this pull request Jan 14, 2022

chore: streamline error checking process #6103

Merged

4 tasks

tao12345666333 mentioned this pull request Feb 15, 2022

Feature suggestion: support forward auth for Ingress resource apache/apisix-ingress-controller#767

Closed

	local res, error = httpc:request_uri(conf.discovery, params)

	if not res then
	err = "Accessing discovery URL (" .. conf.discovery .. ") failed: " .. error

	local res, err = httpc:request_uri(conf.function_uri, params)

	if not res or err then
	core.log.error("failed to process ", plugin_name, ", err: ", err)

	local res, err = httpc:request_uri(uri, params)
	if err then
	core.log.error("FAIL REQUEST [ ",core.json.delay_encode(

	description = "client request header that will be sent to the authorization"
	description = "client request header that will be sent to the authorization service"

	.. "the client when authorize failure"
	.. "the client when authorizing failed"

	local auth_headers = {
	["X-Forwarded-Proto"] = core.request.get_scheme(ctx),
	["X-Forwarded-Method"] = core.request.get_method(),
	["X-Forwarded-Host"] = core.request.get_host(ctx),
	["X-Forwarded-Uri"] = core.request.get_uri(ctx),
	["X-Forwarded-For"] = core.request.get_remote_client_ip(ctx),
	}

	-- append headers that need to be get from the client request header
	if #conf.request_headers > 0 then
	for _, header in ipairs(conf.request_headers) do
	if not auth_headers[header] then
	auth_headers[header] = core.request.header(ctx, header)
	end
	end
	else

	3. client_headers Send `authorization` service response header to `client` when authorize failure
	3. client_headers Send `authorization` service response header to `client` when authorizing failed

	--- error_code: 403
	--- response_body eval
	qr/\"x-forwarded-proto\":\"http\"/ and qr/\"x-forwarded-method\":\"GET\"/ and
	qr/\"x-forwarded-host\":\"localhost\"/ and qr/\"x-forwarded-uri\":\"\\\/hello\"/ and
	qr/\"x-forwarded-for\":\"127.0.0.1\"/

Uh oh!

Conversation

bzp2010 commented Jan 6, 2022 • edited by spacewander Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzp2010 Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzp2010 Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzp2010 Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzp2010 Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzp2010 Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bzp2010 commented Jan 6, 2022 •

edited by spacewander

Loading

bzp2010 Jan 7, 2022 •

edited

Loading

bzp2010 Jan 7, 2022 •

edited

Loading

bzp2010 Jan 7, 2022 •

edited

Loading

bzp2010 Jan 7, 2022 •

edited

Loading

bzp2010 Jan 7, 2022 •

edited

Loading

bzp2010 Jan 10, 2022 •

edited

Loading

bzp2010 Jan 10, 2022 •

edited

Loading