You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/10/21 16:42:00 UTC
[GitHub] [apisix] Firstsawyou opened a new pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Firstsawyou opened a new pull request #2491:
URL: https://github.com/apache/apisix/pull/2491
…ader related to the plugin.
fix #2490
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#discussion_r509830993
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -316,9 +317,17 @@ local function get_params(ctx)
local signed_headers = core.request.header(ctx, signed_headers_key)
core.log.info("signature_key: ", signature_key)
+ core.request.set_header(access_key, nil)
Review comment:
These request headers are related to the `hmac-auth` plugin, and upstream does not need to care about their existence.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#discussion_r509792569
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -316,9 +317,17 @@ local function get_params(ctx)
local signed_headers = core.request.header(ctx, signed_headers_key)
core.log.info("signature_key: ", signature_key)
+ core.request.set_header(access_key, nil)
Review comment:
Why delete these request headers? Are there any protocol specifications?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#discussion_r510570816
##########
File path: doc/plugins/hmac-auth.md
##########
@@ -42,6 +42,7 @@ The `consumer` then adds its key to request header to verify its request.
| algorithm | string | optional | "hmac-sha256" | ["hmac-sha1", "hmac-sha256", "hmac-sha512"] | Encryption algorithm. |
| clock_skew | integer | optional | 0 | | The clock skew allowed by the signature in seconds. For example, if the time is allowed to skew by 10 seconds, then it should be set to `10`. especially, `0` means not checking `Date` |
| signed_headers | array[string] | optional | | | Restrict the headers that are added to the encrypted calculation. After the specified, the client request can only specify the headers within this range. When this item is empty, all the headers specified by the client request will be added to the encrypted calculation |
+| keep_headers | boolean | optional | false | [ true, false ] | Whether it is necessary to keep the request headers of `X-HMAC-SIGNATURE`, `X-HMAC-ALGORITHM` and `X-HMAC-SIGNED-HEADERS` in the http request after successful authentication. true: means to keep the http request header, false: means to remove the http request header. |
Review comment:
Keep what headers?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#issuecomment-714148542
> before sending a PR, we should reach a consensus hmm
yes, a related issue was established, but it was established in a hurry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#discussion_r510576893
##########
File path: doc/plugins/hmac-auth.md
##########
@@ -42,6 +42,7 @@ The `consumer` then adds its key to request header to verify its request.
| algorithm | string | optional | "hmac-sha256" | ["hmac-sha1", "hmac-sha256", "hmac-sha512"] | Encryption algorithm. |
| clock_skew | integer | optional | 0 | | The clock skew allowed by the signature in seconds. For example, if the time is allowed to skew by 10 seconds, then it should be set to `10`. especially, `0` means not checking `Date` |
| signed_headers | array[string] | optional | | | Restrict the headers that are added to the encrypted calculation. After the specified, the client request can only specify the headers within this range. When this item is empty, all the headers specified by the client request will be added to the encrypted calculation |
+| keep_headers | boolean | optional | false | [ true, false ] | Whether it is necessary to keep the request headers of `X-HMAC-SIGNATURE`, `X-HMAC-ALGORITHM` and `X-HMAC-SIGNED-HEADERS` in the http request after successful authentication. true: means to keep the http request header, false: means to remove the http request header. |
Review comment:
Whether to keep the three http request headers `X-HMAC-SIGNATURE`, `X-HMAC-ALGORITHM` and `X-HMAC-SIGNED-HEADERS`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#issuecomment-714073616
before sending a PR, we should reach a consensus hmm
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#issuecomment-714168202
> > before sending a PR, we should reach a consensus hmm
>
> yes, a related issue was established, but it was established in a hurry.
this `pr` related issue： https://github.com/apache/apisix/issues/2490
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] gxthrj commented on a change in pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#discussion_r510574841
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -343,6 +370,19 @@ local function get_params(ctx)
params.date = date or ""
params.signed_headers = signed_headers and ngx_re.split(signed_headers, ";")
+ local keep_headers = get_keep_headers(params.access_key)
+ core.log.info("keep_headers: ", keep_headers)
+
+ if not keep_headers then
+ remove_headers(signature_key, algorithm_key, signed_headers_key)
+
+ if params.signed_headers then
Review comment:
we do not need to remove the headers defined in `signed_headers`.
It may cause bugs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on a change in pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#discussion_r510591193
##########
File path: doc/plugins/hmac-auth.md
##########
@@ -42,6 +42,7 @@ The `consumer` then adds its key to request header to verify its request.
| algorithm | string | optional | "hmac-sha256" | ["hmac-sha1", "hmac-sha256", "hmac-sha512"] | Encryption algorithm. |
| clock_skew | integer | optional | 0 | | The clock skew allowed by the signature in seconds. For example, if the time is allowed to skew by 10 seconds, then it should be set to `10`. especially, `0` means not checking `Date` |
| signed_headers | array[string] | optional | | | Restrict the headers that are added to the encrypted calculation. After the specified, the client request can only specify the headers within this range. When this item is empty, all the headers specified by the client request will be added to the encrypted calculation |
+| keep_headers | boolean | optional | false | [ true, false ] | Whether it is necessary to keep the request headers of `X-HMAC-SIGNATURE`, `X-HMAC-ALGORITHM` and `X-HMAC-SIGNED-HEADERS` in the http request after successful authentication. true: means to keep the http request header, false: means to remove the http request header. |
Review comment:
Please add in doc.
And Do we really need this switch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#discussion_r510596853
##########
File path: doc/plugins/hmac-auth.md
##########
@@ -42,6 +42,7 @@ The `consumer` then adds its key to request header to verify its request.
| algorithm | string | optional | "hmac-sha256" | ["hmac-sha1", "hmac-sha256", "hmac-sha512"] | Encryption algorithm. |
| clock_skew | integer | optional | 0 | | The clock skew allowed by the signature in seconds. For example, if the time is allowed to skew by 10 seconds, then it should be set to `10`. especially, `0` means not checking `Date` |
| signed_headers | array[string] | optional | | | Restrict the headers that are added to the encrypted calculation. After the specified, the client request can only specify the headers within this range. When this item is empty, all the headers specified by the client request will be added to the encrypted calculation |
+| keep_headers | boolean | optional | false | [ true, false ] | Whether it is necessary to keep the request headers of `X-HMAC-SIGNATURE`, `X-HMAC-ALGORITHM` and `X-HMAC-SIGNED-HEADERS` in the http request after successful authentication. true: means to keep the http request header, false: means to remove the http request header. |
Review comment:
It has been added in doc. It would be better if there is a switch, and we default to false to remove the request header.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis merged pull request #2491: feat: remove auth headers for hmac-auth plugin.
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #2491:
URL: https://github.com/apache/apisix/pull/2491
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2491: feat: In the request of the hmac-auth plugin, remove the request he…
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2491:
URL: https://github.com/apache/apisix/pull/2491#discussion_r510619897
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -343,6 +370,19 @@ local function get_params(ctx)
params.date = date or ""
params.signed_headers = signed_headers and ngx_re.split(signed_headers, ";")
+ local keep_headers = get_keep_headers(params.access_key)
+ core.log.info("keep_headers: ", keep_headers)
+
+ if not keep_headers then
+ remove_headers(signature_key, algorithm_key, signed_headers_key)
+
+ if params.signed_headers then
Review comment:
has changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org