feat: add CSRF plugin by Baoyuantop · Pull Request #5727 · apache/apisix

Baoyuantop · 2021-12-07T10:27:08Z

What this PR does / why we need it:

Add CSRF plugin for APISIX
Discuss in https://lists.apache.org/thread/ddj53svylg9s541vf2gpysxwgyrhwlzx

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

Baoyuantop · 2021-12-16T05:54:28Z

Would you please take a look when you have time? Thanks.

leslie-tsang · 2021-12-16T08:59:16Z

Would you please take a look when you have time? Thanks.

Hello there, Would you mind to pass the CI test first ?

spacewander

Let's use 4 spaces to indent and remove trailing ';'

spacewander · 2021-12-23T02:15:55Z

+
+function _M.access(conf, ctx)
+    local method = ctx.var.request_method
+    if method == 'GET' then


Why skip GET?

Here a port is needed for getting the csrf token, and if all of them are blocked, the cookie may not be placed. Therefore, CORS on this port should be configured carefully, as I will explain in the documentation.

This type of request has no side effects and does not need to accept protection, but of course, I will add the corresponding instructions in the documentation.

spacewander · 2021-12-23T02:16:14Z

+
+function _M.header_filter(conf, ctx)
+    local method = ctx.var.request_method
+    if method == 'GET' then


Why only set cookie for GET?

OPTIONS has been added to take into account cross-domain requests.

Co-authored-by: tzssangglass <[email protected]>

spacewander · 2021-12-30T02:03:56Z

+
+
+function _M.header_filter(conf, ctx)
+    local csrf_token = gen_csrf_token(conf)


Better to emphasize in the doc that a new cookie is generated for each request

This problem is not addressed?

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

Co-authored-by: 琚致远 <[email protected]>

spacewander

Great. Let's add some tests.
You can refer to
https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md
https://github.com/apache/apisix/blob/master/t/plugin/opa.t

spacewander · 2022-01-10T06:42:43Z

+no_shuffle();
+no_root_location();
+run_tests();
+


Let's add

apisix/t/plugin/opa.t

Line 23 in e481fc7

add_block_preprocessor(sub {

and remove duplicated --- request.

spacewander · 2022-01-10T06:45:41Z

+--- request
+GET /hello
+--- response_header
+Set-Cookie


We can verify part of the value with https://metacpan.org/pod/Test%3A%3ANginx%3A%3ASocket#response_headers_like

spacewander · 2022-01-10T06:46:07Z

+
+
+
+=== TEST8: invalid csrf token


Let's add a test which token matches the header

spacewander · 2022-01-12T06:37:18Z

+    }
+--- response_body
+done
+--- no_error_log


We can do the same with --- no_error_log too?

spacewander · 2022-01-14T09:59:38Z

+
+
+
+=== TEST4: block request


Let's add a space before the number

bisakhmondal

lgtm%(some nitpicking)
Thank you.

bisakhmondal · 2022-01-16T06:20:48Z

+local ngx_encode_base64 = ngx.encode_base64
+local ngx_decode_base64 = ngx.decode_base64
+local ngx_time = ngx.time
+local cookie_time = ngx.cookie_time


For code style consistency can we use ngx_cookie_time instead of cookie_time?

bisakhmondal · 2022-01-16T06:22:12Z

+    sha256:update(sign)
+    local digest = sha256:final()
+
+    return str.to_hex(digest)


Please localize the method as from resty.string module we are using this single method.

bisakhmondal · 2022-01-16T06:25:37Z

+
+
+function _M.access(conf, ctx)
+    local safe_methods = {"GET", "HEAD", "OPTIONS"}


Can we extract this out from the access phase and treat it as a constant array?
local SAFE_METHODS = {"GET", "HEAD", "OPTIONS"}

bisakhmondal · 2022-01-16T06:28:11Z

+
+## Description
+
+The `CSRF` plugin based on the `Double Submit Cookie` way, protect your API from CSRF attacks. This plugin considers the `GET`, `HEAD` and `OPTIONS` methods to be safe operations. Therefore calls to the `GET`, `HEAD` and `OPTIONS` methods are not checked for interception.


I guess it would be better to add a hyperlink at Double Submit Cookie. https://en.wikipedia.org/wiki/Cross-site_request_forgery#Double_Submit_Cookie

bisakhmondal · 2022-01-16T06:30:30Z

+}'
+```
+
+CSRF plugin have been disabled.


Suggested change

CSRF plugin have been disabled.

The CSRF plugin has been disabled.

juzhiyuan

LGTM

bisakhmondal · 2022-01-16T10:03:30Z

+    local cookie = conf.name .. "=" .. csrf_token .. ";path=/;SameSite=Lax;Expires="
+                   .. cookie_time(ngx_time() + conf.expires)


Instead of Expires I think we can use Max-Age attribute here. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie

Since, the operation is performed for each request, at the long run it will save some computation.

bisakhmondal · 2022-01-16T10:07:48Z

+    local token = {
+        random = random,
+        expires = conf.expires,
+        sign = sign,
+    }
+
+    local cookie = ngx_encode_base64(core.json.encode(token))
+    return cookie
+end
+
+
+local function check_csrf_token(conf, ctx, token)
+    local token_str = ngx_decode_base64(token)
+    if token_str == nil then
+        core.log.error("csrf token base64 decode error")
+        return false
+    end
+
+    local token_table, err = core.json.decode(token_str)
+    if err then
+        core.log.error("decode token error: ", err)
+        return false
+    end
+
+    local random = token_table["random"]
+    if not random then
+        core.log.error("no random in token")
+        return false
+    end
+
+    local expires = token_table["expires"]
+    if not expires then
+        core.log.error("no expires in token")
+        return false
+    end


Just a proposal here, in the token object instead of total expiration seconds, can we use the expiration timestamp? So during the checking phase inside the check_csrf_token method, we could potentially check/prevent/[minimize the window] of replay attacks as an added benefit by figuring out if the generated cookie is long been expired or not. WDYT
Thank you. cc @spacewander

Yes. Thanks for @bisakhmondal 's suggestion.

Hey @Baoyuantop, thanks for making the changes. Are you going to address this in this PR or do you think it would be better to have it on a separate PR? Thank you.

Hi @bisakhmondal, I would prefer a separate PR to solve this problem, this PR is already a bit big.

Sure. Sounds good to me. Thanks

Please open an issue for keeping track of the progress.

Done @bisakhmondal. #6141

Thank you. Merging.

Co-authored-by: Bisakh <[email protected]>

moonming · 2022-01-18T10:04:28Z

@Baoyuantop please add this plugin to README.md too

Baoyuantop · 2022-01-19T02:02:02Z

@Baoyuantop please add this plugin to README.md too

Done #6144

feat: add csrf plugin

ad8c54d

juzhiyuan reviewed Dec 7, 2021

View reviewed changes

Comment thread apisix/plugins/csrf.lua Outdated

fix: format code

843a6ff

leslie-tsang requested changes Dec 10, 2021

View reviewed changes

Comment thread apisix/plugins/csrf.lua Outdated

Comment thread apisix/plugins/csrf.lua Outdated

Comment thread apisix/plugins/csrf.lua Outdated

Comment thread apisix/plugins/csrf.lua Outdated

Baoyuantop added 2 commits December 13, 2021 16:33

feat: add plugin docs

e40347b

fix: adjust code

af25b19

Baoyuantop marked this pull request as ready for review December 14, 2021 13:56

Baoyuantop added 3 commits December 20, 2021 14:12

fix: add docs in config.json

b1032e7

Merge branch 'master' into feat-csrf

0ea8477

fix: add plugin in plugins.t

2a52163

spacewander reviewed Dec 23, 2021

View reviewed changes

fix: format code

55d656f

tzssangglass reviewed Dec 28, 2021

View reviewed changes

Comment thread docs/zh/latest/plugins/csrf.md Outdated

tzssangglass reviewed Dec 28, 2021

View reviewed changes

Comment thread apisix/plugins/csrf.lua Outdated

Comment thread apisix/plugins/csrf.lua Outdated

tzssangglass reviewed Dec 28, 2021

View reviewed changes

Comment thread apisix/plugins/csrf.lua Outdated

tzssangglass mentioned this pull request Dec 29, 2021

chore(jwt-auth): get JWT by ctx.var.cookie_jwt instead of resty.cookie #5947

Merged

4 tasks

Baoyuantop and others added 3 commits December 29, 2021 16:44

Update apisix/plugins/csrf.lua

7a61185

Co-authored-by: tzssangglass <[email protected]>

docs: modify inappropriate descriptions

a03da0a

Merge branch 'feat-csrf' of github.com:Baoyuantop/apisix into feat-csrf

7d14375

spacewander reviewed Dec 30, 2021

View reviewed changes

Baoyuantop and others added 6 commits December 30, 2021 03:22

fix: change read cookie method

6413427

fix: change variable name and add two blank lines

15c9802

Update docs/en/latest/plugins/csrf.md

b52d042

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

fix: add SameSite for cookie

9c42231

fix: move plugin name

45065bb

test: fix plugins test

a4b284e

juzhiyuan reviewed Dec 31, 2021

View reviewed changes

Comment thread docs/zh/latest/plugins/csrf.md Outdated

Update docs/zh/latest/plugins/csrf.md

6c999e6

Co-authored-by: 琚致远 <[email protected]>

spacewander reviewed Jan 7, 2022

View reviewed changes

Baoyuantop added 2 commits January 10, 2022 04:31

test: add some tests

a219794

fix: lint test code

7eb2e9f

Baoyuantop requested review from spacewander and tzssangglass January 10, 2022 06:00

spacewander reviewed Jan 10, 2022

View reviewed changes

Baoyuantop added 2 commits January 12, 2022 05:15

test: perfect test

7eb2524

fix: lint code

2454909

spacewander reviewed Jan 12, 2022

View reviewed changes

fix: update test

d439243

Baoyuantop requested a review from spacewander January 14, 2022 08:47

spacewander approved these changes Jan 14, 2022

View reviewed changes

Comment thread t/plugin/csrf.t Outdated

=== TEST4: block request

spacewander Jan 14, 2022

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a space before the number

test: add space before number

d0f1f71

juzhiyuan requested a review from bisakhmondal January 15, 2022 03:07

bzp2010 approved these changes Jan 16, 2022

View reviewed changes

bisakhmondal approved these changes Jan 16, 2022

View reviewed changes

juzhiyuan approved these changes Jan 16, 2022

View reviewed changes

bisakhmondal reviewed Jan 16, 2022

View reviewed changes

fix: optimization code

cfff42a

shuaijinchao approved these changes Jan 17, 2022

View reviewed changes

shuaijinchao requested a review from bisakhmondal January 17, 2022 02:01

tzssangglass approved these changes Jan 17, 2022

View reviewed changes

Baoyuantop mentioned this pull request Jan 18, 2022

improve: use the expiration timestamp in csrf plugin #6141

Closed

bisakhmondal reviewed Jan 18, 2022

View reviewed changes

Comment thread apisix/plugins/csrf.lua Outdated

Update apisix/plugins/csrf.lua

9305e2b

Co-authored-by: Bisakh <[email protected]>

bisakhmondal merged commit 4d52c3e into apache:master Jan 18, 2022

Baoyuantop deleted the feat-csrf branch January 18, 2022 08:26

Baoyuantop mentioned this pull request Jan 19, 2022

docs: add CSRF plugin info to README #6144

Merged

4 tasks



		function _M.header_filter(conf, ctx)
		local csrf_token = gen_csrf_token(conf)



		function _M.access(conf, ctx)
		local safe_methods = {"GET", "HEAD", "OPTIONS"}


		## Description

		The `CSRF` plugin based on the `Double Submit Cookie` way, protect your API from CSRF attacks. This plugin considers the `GET`, `HEAD` and `OPTIONS` methods to be safe operations. Therefore calls to the `GET`, `HEAD` and `OPTIONS` methods are not checked for interception.

	CSRF plugin have been disabled.
	The CSRF plugin has been disabled.

		local cookie = conf.name .. "=" .. csrf_token .. ";path=/;SameSite=Lax;Expires="
		.. cookie_time(ngx_time() + conf.expires)

Uh oh!

Conversation

Baoyuantop commented Dec 7, 2021 • edited 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Baoyuantop commented Dec 16, 2021

Uh oh!

leslie-tsang commented Dec 16, 2021

Uh oh!

spacewander left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spacewander left a comment

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!

bisakhmondal left a comment

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!

juzhiyuan left a comment

Baoyuantop commented Dec 7, 2021 •

edited

Loading

bisakhmondal Jan 16, 2022 •

edited

Loading