feat(hmac-auth): Add validate request body for hmac auth plugin by arthur-zhang · Pull Request #5038 · apache/apisix

arthur-zhang · 2021-09-10T11:45:35Z

What this PR does / why we need it:

Add validate request body for hmac auth plugin, by calculate body hmac hash and put it in Digest Header

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

starsz · 2021-09-10T15:39:47Z

+    if not req_body then
+        req_body = ""
+    end
+    local digest_header = core.request.header(ctx, DIGEST)


digest header looks not good for me.

The reason I use this header is that:
The Digest HTTP header provides a digest of the requested resource.

starsz · 2021-09-10T15:41:47Z

+Date: Tue, 19 Jan 2021 11:33:20 GMT
+Server: APISIX/2.2
+......
+```


Can you help to update the English doc?

tzssangglass · 2021-09-12T14:49:49Z

+    if not req_body then
+        req_body = ""
+    end


Suggested change

if not req_body then

req_body = ""

end

req_body = req_body or ""

tzssangglass · 2021-09-12T14:51:31Z

+    if not digest_header then
+        -- it's ok if there is no digest header and no body
+        return req_body == ""
+    end


Can we judge this first?

Maybe not, because we can omit digest header when body is empty.

tzssangglass · 2021-09-12T14:52:09Z


+### Body 校验
+
+把 `validate_request_body` 设置为 true 来进行请求 body 的校验。 插件将计算 hmac-sha 值，对比头部中的 Digest 头部值。


Suggested change

把 `validate_request_body` 设置为 true 来进行请求 body 的校验。插件将计算 hmac-sha 值，对比头部中的 Digest 头部值。

把 `validate_request_body` 设置为 true 来进行请求 body 的校验。插件将计算 hmac-sha 值，对比头部中的 Digest 头部值。

Digest in code is digest, better to keep same.

nic-chen · 2021-09-11T08:10:06Z

+        req_body = ""
+    end
+    local digest_header = core.request.header(ctx, DIGEST)
+    if not digest_header then


we could check this before reading the body

Maybe not, because we can omit digest header when body is empty.

tzssangglass · 2021-09-13T06:31:46Z

hi @arthur-zhang, I notice this 当无请求 body 时，可不传 Digest 头部，网关会校验是否确实无请求 body。如果要传 Digest 头部，可计算长度为 0 的空字符串的 hmac-sha 值。, should we add test cases about GET method?

arthur-zhang · 2021-09-13T07:16:38Z

hi @arthur-zhang, I notice this 当无请求 body 时，可不传 Digest 头部，网关会校验是否确实无请求 body。如果要传 Digest 头部，可计算长度为 0 的空字符串的 hmac-sha 值。, should we add test cases about GET method?

I Add test === TEST 4: missing digest header and body is empty for post request without body, GET is no difference.

tzssangglass · 2021-09-13T06:16:06Z


+### Body 校验
+
+把 `validate_request_body` 设置为 true 来进行请求 body 的校验。插件将计算 hmac-sha 值，对比头部中的 Digest 头部值。


Suggested change

把 `validate_request_body` 设置为 true 来进行请求 body 的校验。插件将计算 hmac-sha 值，对比头部中的 Digest 头部值。

`validate_request_body` 设置为 true 时，插件将计算请求 body 的 `hmac-sha` 值，并与请求 headers 中的 Digest 的值进行校验。

tzssangglass · 2021-09-13T06:17:55Z

+
+
+
+=== TEST 3: missing body digest when validate_request_body is true


Suggested change

=== TEST 3: missing body digest when validate_request_body is true

=== TEST 3: missing body digest when validate_request_body is enable

tzssangglass · 2021-09-13T06:22:37Z

+
+
+
+=== TEST 4: missing digest header and body is empty


Suggested change

=== TEST 4: missing digest header and body is empty

=== TEST 4: no digest header and request body is empty

tzssangglass · 2021-09-13T06:24:08Z

+
+            local code, body = t.test('/hello',
+                ngx.HTTP_POST,
+                body,


here body should be nil?

tzssangglass · 2021-09-13T06:30:33Z

+Digest: base64(hmac-sha(<body>))
+```
+
+当无请求 body 时，可不传 Digest 头部，网关会校验是否确实无请求 body。如果要传 Digest 头部，可计算长度为 0 的空字符串的 hmac-sha 值。


Suggested change

当无请求 body 时，可不传 Digest 头部，网关会校验是否确实无请求 body。如果要传 Digest 头部，可计算长度为 0 的空字符串的 hmac-sha 值。

当没有请求 body 时，可不传 Digest 头部，网关会校验是否确实无请求 body。如果要传 Digest 头部，插件将请求 body 默认为长度为 0 的空字符串，并参与到计算 hmac-sha 值。

spacewander · 2021-09-14T02:21:35Z

+            default = false,
+        },
+        max_req_body = {
+            type = "number",


Suggested change

type = "number",

type = "integer",

spacewander · 2021-09-14T02:22:27Z

 | 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.   |
 | encode_uri_params | boolean       | optional    | true          | [ true, false ]                             | Whether to encode the uri parameter in the signature, for example: `params1=hello%2Cworld` is encoded, `params2=hello,world` is not encoded. true: means to encode the uri parameter in the signature, false: not to encode the uri parameter in the signature.               |
+| validate_request_body | boolean  | optional   | false         | [ true, false ]                             | Whether to check request body. |
+| max_req_body     | number        | optional   | 512KB         |                                             | Max allowed body size. |


Better to use a number instead of string in the doc's default value cell

spacewander · 2021-09-14T02:23:31Z

+     -H 'Content-Type: text/plain; charset=utf-8' \
+     -d "{\"hello\":\"world\"}"
+
+HTTP/1.1 200 OK


Better to only show the first line? APISIX 2.2 doesn't support this feature. We can avoid confusing users by only show the first line.

spacewander · 2021-09-14T02:27:49Z

 local ngx_decode_base64 = ngx.decode_base64
+local ngx_encode_base64 = ngx.encode_base64

+local DIGEST = "Digest"


Can we make this header configurable and default to "X-HMAC-DIGEST"?

There is already a Digest header in the HTTP standard and it is not relative to the hmac.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Digest

nic-chen · 2021-09-14T06:13:07Z


+    local validate_request_body = get_conf_field(params.access_key, "validate_request_body")
+    if validate_request_body then
+        local max_req_body = get_conf_field(params.access_key, "max_req_body")


I think we could return here if no params.body_digest.

I think we should check whether the params.body_digest exists or not.

when validate_request_body == true and no params.body_digest, why not return false?

When there is no request body, the X-HMAC-DIGEST header can be omitted.

@arthur-zhang

OK. Thanks for your explanation. How about when validate_request_body is enabled, X-APISIX-HMAC-BODY-DIGEST is required? Just pass empty strings for it when no body.

spacewander · 2021-09-14T06:17:29Z

+            headers["X-HMAC-SIGNATURE"] = ngx_encode_base64(signature)
+            headers["X-HMAC-ALGORITHM"] = "hmac-sha256"
+            headers["Date"] = gmt
+            headers["X-Digest"] = ngx_encode_base64(body_digest)


Should be X-HMAC-DIGEST?

Any string here is ok, I will change it to X-HMAC-DIGEST.

spacewander · 2021-09-14T06:21:57Z


+### Request body checking
+
+When `validate_request_body` is assigned to `true`, the plugin will check the request body. The plugin will calculate the hmac-sha value of the request body，and check against the `X-HMAC-DIGEST` header.


Better to mention the max_req_body limitation

I add a note to that

spacewander · 2021-09-14T06:23:28Z

+X-HMAC-DIGEST: base64(hmac-sha(<body>))
+```
+
+When there is no request body, the `X-HMAC-DIGEST` header can be omitted. If you want to send request with this header whether the body is empty or not, you can set `X-HMAC-DIGEST` value to the hmac-sha of empty string.


Suggested change

When there is no request body, the `X-HMAC-DIGEST` header can be omitted. If you want to send request with this header whether the body is empty or not, you can set `X-HMAC-DIGEST` value to the hmac-sha of empty string.

When there is no request body, the `X-HMAC-DIGEST` header can be omitted. If you want to send a request with this header when the body is missing, you can set `X-HMAC-DIGEST` value to the hmac-sha of empty string.

tokers · 2021-09-14T14:20:45Z

+        },
+        max_req_body = {
+            type = "integer",
+            title = "Max request body allowed",


"Max request body size" would be better.

spacewander · 2021-09-15T02:03:57Z

+
+When there is no request body, the `X-HMAC-DIGEST` header can be omitted. If you want to send a request with this header when the body is missing, you can set `X-HMAC-DIGEST` value to the hmac-sha of empty string.
+
+**Note:** The plugin will load the request body to memory to calculate the digest of the request body, which wight cause high memory consumption with large bodies. You can limit the max allowed body size by the configuration of `max_req_body`(default=512KB), request body larger than that will be rejected.


Suggested change

**Note:** The plugin will load the request body to memory to calculate the digest of the request body, which wight cause high memory consumption with large bodies. You can limit the max allowed body size by the configuration of `max_req_body`(default=512KB), request body larger than that will be rejected.

**Note:** The plugin will load the request body to memory to calculate the digest of the request body, which might cause high memory consumption with large bodies. You can limit the max allowed body size by the configuration of `max_req_body`(default=512KB), request body larger than that will be rejected.

arthur-zhang added 5 commits September 10, 2021 19:42

fix doc

1bc110c

add new line to test file

8271bbd

indent test case

70b4813

fix test

eddf479

starsz reviewed Sep 10, 2021

View reviewed changes

arthur-zhang requested a review from starsz September 11, 2021 01:33

tzssangglass reviewed Sep 12, 2021

View reviewed changes

nic-chen reviewed Sep 12, 2021

View reviewed changes

Fix Review

304f3d5

arthur-zhang requested review from nic-chen and tzssangglass September 13, 2021 01:45

tzssangglass reviewed Sep 13, 2021

View reviewed changes

spacewander reviewed Sep 14, 2021

View reviewed changes

arthur-zhang added 2 commits September 14, 2021 12:04

Add custom key of body_digest_key to X-HMAC-DIGEST

d14aafa

Fix Doc

6cbef7e

tzssangglass approved these changes Sep 14, 2021

View reviewed changes

nic-chen reviewed Sep 14, 2021

View reviewed changes

spacewander reviewed Sep 14, 2021

View reviewed changes

Fix Doc

4623d2f

arthur-zhang requested a review from spacewander September 14, 2021 07:47

tokers reviewed Sep 14, 2021

View reviewed changes

Fix title

a4edde5

spacewander reviewed Sep 15, 2021

View reviewed changes

Force X-HMAC-DIGEST when validate_request_body is enabled

6863828

arthur-zhang requested a review from nic-chen September 15, 2021 02:13

Fix indent

9891f46

spacewander approved these changes Sep 15, 2021

View reviewed changes

nic-chen approved these changes Sep 15, 2021

View reviewed changes

tokers approved these changes Sep 16, 2021

View reviewed changes

starsz approved these changes Sep 16, 2021

View reviewed changes

spacewander merged commit d6c09fb into apache:master Sep 16, 2021

arthur-zhang deleted the dev-hmac-body-validate branch September 16, 2021 07:43


		### Body 校验

		把 `validate_request_body` 设置为 true 来进行请求 body 的校验。插件将计算 hmac-sha 值，对比头部中的 Digest 头部值。

	把 `validate_request_body` 设置为 true 来进行请求 body 的校验。插件将计算 hmac-sha 值，对比头部中的 Digest 头部值。
	`validate_request_body` 设置为 true 时，插件将计算请求 body 的 `hmac-sha` 值，并与请求 headers 中的 Digest 的值进行校验。




		=== TEST 3: missing body digest when validate_request_body is true

	=== TEST 4: missing digest header and body is empty
	=== TEST 4: no digest header and request body is empty

	当无请求 body 时，可不传 Digest 头部，网关会校验是否确实无请求 body。如果要传 Digest 头部，可计算长度为 0 的空字符串的 hmac-sha 值。
	当没有请求 body 时，可不传 Digest 头部，网关会校验是否确实无请求 body。如果要传 Digest 头部，插件将请求 body 默认为长度为 0 的空字符串，并参与到计算 hmac-sha 值。


		### Request body checking

		When `validate_request_body` is assigned to `true`, the plugin will check the request body. The plugin will calculate the hmac-sha value of the request body，and check against the `X-HMAC-DIGEST` header.

	When there is no request body, the `X-HMAC-DIGEST` header can be omitted. If you want to send request with this header whether the body is empty or not, you can set `X-HMAC-DIGEST` value to the hmac-sha of empty string.
	When there is no request body, the `X-HMAC-DIGEST` header can be omitted. If you want to send a request with this header when the body is missing, you can set `X-HMAC-DIGEST` value to the hmac-sha of empty string.


		When there is no request body, the `X-HMAC-DIGEST` header can be omitted. If you want to send a request with this header when the body is missing, you can set `X-HMAC-DIGEST` value to the hmac-sha of empty string.

		Note: The plugin will load the request body to memory to calculate the digest of the request body, which wight cause high memory consumption with large bodies. You can limit the max allowed body size by the configuration of `max_req_body`(default=512KB), request body larger than that will be rejected.

Uh oh!

Conversation

arthur-zhang commented Sep 10, 2021

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthur-zhang Sep 10, 2021 • 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

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!

tzssangglass commented Sep 13, 2021

Uh oh!

arthur-zhang commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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

arthur-zhang Sep 10, 2021 •

edited

Loading

arthur-zhang commented Sep 13, 2021 •

edited

Loading