feat: support hide the authentication header in basic-auth with a config #6039

mangoGoForward · 2022-01-07T03:07:59Z

What this PR does / why we need it:

Resolves #5900

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

when eureka server return compressed data, use lua-zlib to unzip received response data

…to unzip received response data" This reverts commit dc2570a

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

feat: support hide the authentication header in basic-auth

spacewander · 2022-01-07T06:35:14Z

apisix/plugins/basic-auth.lua

@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
        return 401, { message = "Password is error" }
    end

+    -- 5. hide `Authentication` header if `hide_auth_header` is `true`
+    if conf.hide_auth_header == true then
+        core.response.set_header("Authentication", "")


It seems you misunderstand the original issue. We want to hide the request header.

My fault. Done.

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

tokers · 2022-01-07T08:16:07Z

apisix/plugins/basic-auth.lua

+    properties = {
+        hide_auth_header = {
+            type = "boolean",
+            default = true,


I think the default value should be false.

leslie-tsang · 2022-01-07T08:08:36Z

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

@@ -43,6 +43,7 @@ title: basic-auth
 | -------- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------------------ |
 | username | string | 必须   |        |        | 不同的 `consumer` 对象应有不同的值，它应当是唯一的。不同 consumer 使用了相同的 `username` ，将会出现请求匹配异常。 |
 | password | string | 必须   |        |        | 用户的密码                                                                                                         |
+| hide_auth_header | boolean | 可选    | true   |       | 是否将 Authentication 请求头返回给客户端.                                                                                             |


leslie-tsang · 2022-01-07T08:11:27Z

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

+| Name             | Type    | Requirement | Default | Valid | Description                                                                                                                                                      |
+| --------         | ------  | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username         | string  | required    |         |       | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password         | string  | required    |         |       | the user's password                                                                                                                                              |
+| hide_auth_header | boolean | optional    | true    |       | Whether to return the Authentication request headers to the client.                                                                                             |


Suggested change

| Name | Type | Requirement | Default | Valid | Description |

| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |

| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |

| password | string | required | | | the user's password |

| hide_auth_header | boolean | optional | true | | Whether to return the Authentication request headers to the client. |

| Name | Type | Requirement | Default | Valid | Description |

| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |

| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |

| password | string | required | | | the user's password |

| hide_auth_header | boolean | optional | true | | Whether to return the Authentication request headers to the upstream. |

leslie-tsang · 2022-01-07T08:15:27Z

apisix/plugins/basic-auth.lua

@@ -39,6 +44,10 @@ local consumer_schema = {
    properties = {
        username = { type = "string" },
        password = { type = "string" },
+        hide_auth_header = {
+            type = "boolean",
+            default = true,


Suggested change

default = true,

default = false,

Need to discuss it in maillist if use true as default value

leslie-tsang · 2022-01-07T08:17:32Z

apisix/plugins/basic-auth.lua

+    -- 5. hide `Authentication` request header if `hide_auth_header` is `true`
+    if conf.hide_auth_header == true then
+        core.request.set_header(ctx, "Authentication", "")
+    end
+


The original issue seems to avoid to send the header to upstream.

Yes, use core.request.set_header(ctx, "Authentication", nil) can avoid.

leslie-tsang · 2022-01-07T08:19:34Z

apisix/plugins/basic-auth.lua

+    properties = {
+        hide_auth_header = {
+            type = "boolean",
+            default = true,


Suggested change

default = true,

default = true,

Ditto

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

spacewander · 2022-01-07T13:47:54Z

apisix/plugins/basic-auth.lua

@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
 local schema = {
    type = "object",
    title = "work with route or service object",
-    properties = {},
+    properties = {
+        hide_auth_header = {


hide_credentials would be better? Kong uses this field in their basic-auth.

spacewander · 2022-01-07T13:49:25Z

apisix/plugins/basic-auth.lua

@@ -39,6 +44,10 @@ local consumer_schema = {
    properties = {
        username = { type = "string" },
        password = { type = "string" },
+        hide_auth_header = {


We don't need to configure it in the consumer

spacewander · 2022-01-07T13:51:37Z

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

+| --------         | ------  | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username         | string  | required    |         |       | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password         | string  | required    |         |       | the user's password                                                                                                                                              |
+| hide_auth_header | boolean | optional    | false   |       | Whether to return the Authentication request headers to the upstream.                                                                                             |


Let's distinguish route conf from the consumer's like this one: https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/ldap-auth.md#attributes

…e docs Signed-off-by: mango <xu.weiKyrie@foxmail.com>

tzssangglass · 2022-01-10T04:35:29Z

apisix/plugins/basic-auth.lua

@@ -161,6 +166,11 @@ function _M.rewrite(conf, ctx)
        return 401, { message = "Password is error" }
    end

+    -- 5. hide `Authentication` request header if `hide_credentials` is `true`
+    if conf.hide_credentials == true then


Suggested change

if conf.hide_credentials == true then

if conf.hide_credentials then

is ok?

tzssangglass · 2022-01-10T04:38:15Z

and need to add test cases to cover this. I see that you have test cases in your previous submission records……

spacewander · 2022-01-10T06:06:23Z

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

+
+| Name             | Type    | Requirement | Default | Valid | Description                                                                                                                                                      |
+| --------         | ------  | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| hide_credentials | boolean | optional    | false   |       | Whether to return the Authentication request headers to the upstream.                                                                                            |


Signed-off-by: mango <xu.weiKyrie@foxmail.com>

imjoey · 2022-01-12T08:35:31Z

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

+For consumer side:
+
+| Name             | Type    | Requirement | Default | Valid | Description                                                                                                                                                      |
+| --------         | ------  | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username         | string  | required    |         |       | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password         | string  | required    |         |       | the user's password                                                                                                                                              |


@mangoGoForward IMHO there's no need to change this. How about revert it?

imjoey

@mangoGoForward Please revert some changes in English docs, others look good to me. Thanks.

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

imjoey · 2022-01-12T09:06:56Z

t/plugin/basic-auth.t

@@ -20,6 +20,7 @@ repeat_each(2);
 no_long_string();
 no_root_location();
 no_shuffle();
+log_level('info');


@mangoGoForward please take a check whether this change is needed. Thanks.

Seems unnecessary, removed.

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

spacewander · 2022-01-13T06:49:30Z

@mangoGoForward
Please make lint pass.

mangoGoForward · 2022-01-13T07:21:40Z

@mangoGoForward Please make lint pass.

That's strange, it's good when I run reindex on local(MacOS) and Centos environment, please see the screenshot below:

I will test again on ubuntu later, If you know the error's location, please help me to improve, Thanks.

mangoGoForward · 2022-01-13T08:32:57Z

I run reindex to check test code style on Ubuntu Server 20.04 LTS, also is right, so strangely. please see:

tzssangglass · 2022-01-13T16:05:32Z

Strange, I had no problem checking the format of this test case.

…header revert: "feat: support hide the authentication header in basic-auth"

…-auth"" This reverts commit a1deeef Signed-off-by: mango <xu.weiKyrie@foxmail.com>

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

mangoGoForward · 2022-01-14T02:03:16Z

Strange, I had no problem checking the format of this test case.

I have run workflow in my fork repo, it also failed.

imjoey · 2022-01-14T02:34:20Z

@mangoGoForward The changes look good to me. Could you explain why you committed several other commits after Zexuan's approval? Thanks.

mangoGoForward · 2022-01-14T02:45:50Z

@mangoGoForward The changes look good to me. Could you explain why you committed several other commits? Thanks.

I want to trigger the actions of my forked repo, so I commit a PR to my repo's master branch, and got file conflict when merged from master, so i commit 8440045 to resolved. but the master branch's code is polluted, so I revert the commit to 49b7850, and I also checked those file changes, it is same as 49b7850, I'm sorry about troubled your review work.

imjoey · 2022-01-14T03:29:07Z

@mangoGoForward The changes look good to me. Could you explain why you committed several other commits? Thanks.

I want to trigger the actions of my forked repo, so I commit a PR to my repo's master branch, and got file conflict when merged from master, so i commit 8440045 to resolved. but the master branch's code is polluted, so I revert the commit to 49b7850, and I also checked those file changes, it is same as 49b7850, I'm sorry about troubled your review work.

@mangoGoForward Got it and thanks. Let's wait for the CI. :-)

imjoey · 2022-01-14T03:49:48Z

@spacewander @mangoGoForward The CI job failure of linux_openresty on ubuntu-18.04 seemed not related to the code and I restarted that job. I would prefer to leave an approval in prior and merge this after that job passed. Thanks.

imjoey

The changes look good to me.

lijing-21 · 2022-01-18T02:23:31Z

Hi @mangoGoForward , thank you for your contribution!

Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :)

[1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt

mangoGoForward · 2022-01-18T05:55:43Z

Hi @mangoGoForward , thank you for your contribution!

Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :)

[1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt

Thanks.

xuwei and others added 5 commits September 1, 2021 13:46

fix:

dc2570a

when eureka server return compressed data, use lua-zlib to unzip received response data

merge from upstream master

f0d6f6f

Merge remote-tracking branch 'upstream/master'

3845a27

feature: support hide the authentication header in basic-auth plugin

db0c9e7

add test case with hide auth header

0cee869

mangoGoForward marked this pull request as draft January 7, 2022 03:08

mangoGoForward and others added 4 commits January 7, 2022 11:15

Revert "fix: when eureka server return compressed data, use lua-zlib …

79ca875

…to unzip received response data" This reverts commit dc2570a

remove blank line

9a9565c

change default value of hide_auth_header to true

10d35fd

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

change default value of hide_auth_header to true

c642635

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

mangoGoForward marked this pull request as ready for review January 7, 2022 03:21

mangoGoForward marked this pull request as draft January 7, 2022 03:29

mangoGoForward and others added 3 commits January 7, 2022 11:32

fix code lint error

29d3077

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

fix code lint error

1c42db0

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

Merge pull request #1 from mangoGoForward/feature/hide-auth-header

7a663fa

feat: support hide the authentication header in basic-auth

mangoGoForward marked this pull request as ready for review January 7, 2022 06:10

spacewander reviewed Jan 7, 2022

View reviewed changes

hide Authentication request header if hide_auth_header is true

6e8d684

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

tokers reviewed Jan 7, 2022

View reviewed changes

leslie-tsang requested changes Jan 7, 2022

View reviewed changes

leslie-tsang reviewed Jan 7, 2022

View reviewed changes

hide Authentication request header if hide_auth_header is true

ec3bec6

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

spacewander reviewed Jan 7, 2022

View reviewed changes

mangoGoForward and others added 3 commits January 10, 2022 09:20

Merge branch 'apache:master' into master

37076a7

change config item hide_auth_header to hide_credentials and updat…

484d05d

…e docs Signed-off-by: mango <xu.weiKyrie@foxmail.com>

change config item hide_auth_header to hide_credentials and updat…

63aa704

…e docs Signed-off-by: mango <xu.weiKyrie@foxmail.com>

tzssangglass reviewed Jan 10, 2022

View reviewed changes

spacewander reviewed Jan 10, 2022

View reviewed changes

add test cases to cover it

210c9d5

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

imjoey reviewed Jan 12, 2022

View reviewed changes

imjoey requested changes Jan 12, 2022

View reviewed changes

update basic-auth doc

41bb17e

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

imjoey reviewed Jan 12, 2022

View reviewed changes

remove log_level

49b7850

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

spacewander approved these changes Jan 14, 2022

View reviewed changes

spacewander requested a review from imjoey January 14, 2022 01:28

mangoGoForward added 3 commits January 14, 2022 09:33

Revert "feat: support hide the authentication header in basic-auth"

a1deeef

Merge pull request #2 from mangoGoForward/revert-1-feature/hide-auth-…

4fc743f

…header revert: "feat: support hide the authentication header in basic-auth"

Merge branch 'master' into feature/hide-auth-header

8440045

mangoGoForward marked this pull request as draft January 14, 2022 01:45

mangoGoForward added 3 commits January 14, 2022 09:51

Revert "Revert "feat: support hide the authentication header in basic…

8531227

…-auth"" This reverts commit a1deeef Signed-off-by: mango <xu.weiKyrie@foxmail.com>

change en doc

31af04d

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

change zh doc

ccc46b6

Signed-off-by: mango <xu.weiKyrie@foxmail.com>

mangoGoForward marked this pull request as ready for review January 14, 2022 01:56

Merge branch 'apache:master' into feature/hide-auth-header

1b0ffe4

imjoey approved these changes Jan 14, 2022

View reviewed changes

spacewander merged commit d7fda7e into apache:master Jan 14, 2022

tzssangglass mentioned this pull request Jan 14, 2022

chore: reindex format of basic_auth.t #6101

Merged

4 tasks

	\| hide_auth_header \| boolean \| 可选 \| true \| \| 是否将 Authentication 请求头返回给客户端. \|
	\| hide_auth_header \| boolean \| 可选 \| false \| \| 是否将 Authentication 请求头传递给 upstream。 \|

	if conf.hide_credentials == true then
	if conf.hide_credentials then

	\| hide_credentials \| boolean \| optional \| false \| \| Whether to return the Authentication request headers to the upstream. \|
	\| hide_credentials \| boolean \| optional \| false \| \| Whether to pass the Authentication request headers to the upstream. \|

feat: support hide the authentication header in basic-auth with a config #6039

feat: support hide the authentication header in basic-auth with a config #6039

Conversation

mangoGoForward commented Jan 7, 2022

What this PR does / why we need it:

Pre-submission 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

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

tzssangglass commented Jan 10, 2022

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

imjoey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander commented Jan 13, 2022

mangoGoForward commented Jan 13, 2022 • edited Loading

mangoGoForward commented Jan 13, 2022

tzssangglass commented Jan 13, 2022

mangoGoForward commented Jan 14, 2022

imjoey commented Jan 14, 2022 • edited Loading

mangoGoForward commented Jan 14, 2022

imjoey commented Jan 14, 2022

imjoey commented Jan 14, 2022

imjoey left a comment

Choose a reason for hiding this comment

lijing-21 commented Jan 18, 2022

mangoGoForward commented Jan 18, 2022

mangoGoForward commented Jan 13, 2022 •

edited

Loading

imjoey commented Jan 14, 2022 •

edited

Loading