feat: Add the function to hide header for key-auth plugin #6670

bin-ya · 2022-03-21T02:23:10Z

Description

Add the function of hiding header for key-auth plugin

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

spacewander · 2022-03-21T05:56:41Z

Let's merge master to make CI pass.

…th-plugin-uograde Conflicts: docs/en/latest/plugins/key-auth.md docs/zh/latest/plugins/key-auth.md

tzssangglass · 2022-03-21T09:11:04Z

apisix/plugins/key-auth.lua

@@ -110,6 +114,10 @@ function _M.rewrite(conf, ctx)
    end
    core.log.info("consumer: ", core.json.delay_encode(consumer))

+    if conf.hide_credentials then
+        core.request.set_header(ctx, "apikey", nil)


Suggested change

core.request.set_header(ctx, "apikey", nil)

core.request.set_header(ctx, conf.header, nil)

tzssangglass · 2022-03-21T12:08:12Z

hi @bin-ya PTAL https://github.com/apache/apisix/runs/5624966335?check_suite_focus=true

tokers · 2022-03-23T09:14:10Z

docs/en/latest/plugins/key-auth.md

@@ -41,6 +41,7 @@ For route side:
 | ---- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------- |
 | header  | string | optional    | apikey        |       | the header we get the key from |
 | query   | string | optional    | apikey        |       | the query string we get the key from, which priority is lower than `header` |
+| hide_credentials   | bool | optional    | false        |       | Whether to pass the apikey request headers to the upstream. |


It's confusing, the header is not necessary to be "apikey", it's specified by the header. Also, the query should also be removed.

Query is the content modified by others. Can I delete it?

You should only handle the query that used to carry the credential.

juzhiyuan

Great! Would you like to share your Apache APISIX Development experience?

tzssangglass · 2022-03-24T13:35:53Z

oo, another file conflict @bin-ya

tzssangglass · 2022-03-25T07:20:17Z

docs/zh/latest/plugins/key-auth.md

@@ -41,6 +41,7 @@ router 端配置：
 | ---- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------------- |
 | header  | string | 可选| apikey |        | 设置我们从哪个 header 获取 key。 |
 | query  | string | 可选 | apikey |        | 设置我们从哪个 query string 获取 key，优先级低于 `header` |
+| hide_credentials  | bool | 可选 | false |        | 是否将请求头传递给 upstream。 |


tzssangglass · 2022-03-25T07:20:53Z

File conflicts have not been resolved @bin-ya

…th-plugin-uograde Conflicts: docs/zh/latest/plugins/key-auth.md

docs/en/latest/plugins/key-auth.md

…th-plugin-uograde Conflicts: docs/zh/latest/plugins/key-auth.md

…pisix into key-auth-plugin-uograde

spacewander · 2022-03-30T01:53:32Z

apisix/plugins/key-auth.lua

@@ -110,6 +114,10 @@ function _M.rewrite(conf, ctx)
    end
    core.log.info("consumer: ", core.json.delay_encode(consumer))

+    if conf.hide_credentials then


We should remove the key according to where the key comes from.

ok, I will.

tzssangglass · 2022-04-01T06:34:53Z

apisix/plugins/key-auth.lua

+        core.request.get_uri_args(ctx)
+        core.request.set_uri_args(ctx, {})


What is the purpose of doing this?

Hide information from query

I think we don't necessary to remove the all query params
If the key is in the header, we only need to delete the key in the headers.
If the key is in the query, we only need to delete the key in the query params.

cc @spacewander right?

Yes, we should only remove the key.

OK, I'll revise it right away.

feat: Add the function of hiding header for key-auth plugin

4dfabc9

spacewander requested a review from tzssangglass March 21, 2022 06:15

Merge branch 'master' of https://github.com/apache/apisix into key-au…

9be27cc

…th-plugin-uograde Conflicts: docs/en/latest/plugins/key-auth.md docs/zh/latest/plugins/key-auth.md

tzssangglass reviewed Mar 21, 2022

View reviewed changes

merge maste

7f47a59

fix ci

31fdc62

spacewander requested a review from tzssangglass March 23, 2022 06:08

tzssangglass previously approved these changes Mar 23, 2022

View reviewed changes

tokers reviewed Mar 23, 2022

View reviewed changes

juzhiyuan reviewed Mar 24, 2022

View reviewed changes

fix key-auth.md

c766814

bin-ya dismissed tzssangglass’s stale review via c766814 March 25, 2022 01:56

tzssangglass reviewed Mar 25, 2022

View reviewed changes

bin-ya added 3 commits March 27, 2022 22:30

modify information

8f6db40

fix previous error

17fb9ab

Merge branch 'master' of https://github.com/apache/apisix into key-au…

02cbe96

…th-plugin-uograde Conflicts: docs/zh/latest/plugins/key-auth.md

starsz reviewed Mar 28, 2022

View reviewed changes

docs/en/latest/plugins/key-auth.md Show resolved Hide resolved

Merge branch 'master' of https://github.com/apache/apisix into key-au…

4022efd

…th-plugin-uograde Conflicts: docs/zh/latest/plugins/key-auth.md

starsz previously approved these changes Mar 29, 2022

View reviewed changes

fix ci

448ce8f

bin-ya dismissed starsz’s stale review via 448ce8f March 29, 2022 05:34

bin-ya added 2 commits March 29, 2022 18:07

modify format

faea26e

Merge branch 'key-auth-plugin-uograde' of https://github.com/bin-ya/a…

2658e4d

…pisix into key-auth-plugin-uograde

spacewander reviewed Mar 30, 2022

View reviewed changes

bin-ya added 2 commits April 1, 2022 11:34

Add the function of hiding query information

5cb6ae5

fix ci

5d1f8d3

tzssangglass reviewed Apr 1, 2022

View reviewed changes

bin-ya added 3 commits April 2, 2022 12:21

modify hidden information and add test

a02d7ad

fix key-auth.t

97d754a

modify key-auth.t

a3a4ff9

spacewander requested a review from tzssangglass April 5, 2022 09:51

tzssangglass approved these changes Apr 6, 2022

View reviewed changes

starsz approved these changes Apr 6, 2022

View reviewed changes

spacewander approved these changes Apr 6, 2022

View reviewed changes

spacewander changed the title ~~feat: Add the function of hiding header for key-auth plugin~~ feat: Add the function to hide header for key-auth plugin Apr 6, 2022

spacewander merged commit 07d535d into apache:master Apr 6, 2022

Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request May 20, 2022

feat: Add the function to hide header for key-auth plugin (apache#6670)

3a3ce92

spacewander pushed a commit that referenced this pull request Jun 30, 2022

feat: Add the function to hide header for key-auth plugin (#6670)

bea9a0b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add the function to hide header for key-auth plugin #6670

feat: Add the function to hide header for key-auth plugin #6670

	core.request.set_header(ctx, "apikey", nil)
	core.request.set_header(ctx, conf.header, nil)

	\| hide_credentials \| bool \| 可选 \| false \| \| 是否将请求头传递给 upstream。 \|
	\| hide_credentials \| bool \| 可选 \| false \| \| 是否将含有认证信息的请求头传递给 upstream。 \|

		core.request.get_uri_args(ctx)
		core.request.set_uri_args(ctx, {})

feat: Add the function to hide header for key-auth plugin #6670

feat: Add the function to hide header for key-auth plugin #6670

Conversation

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment