feat: add expires timestamp for CSRF plugin by Baoyuantop · Pull Request #6201 · apache/apisix

Baoyuantop · 2022-01-25T14:23:05Z

What this PR does / why we need it:

resolve #6141

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

tokers · 2022-01-26T01:08:39Z

+
+=== TEST 10: expired csrf token
+--- config
+    location /t {


It seems that the test case doesn't access /t?

bisakhmondal

Hey, nice to see this PR.

bisakhmondal · 2022-01-26T07:08:06Z

+    local expires_timestamp = ngx_time() + conf.expires
+    local sign = gen_sign(random, expires_timestamp, conf.key)


I'd say sign with the current timestamp. and at line 116 perform a diff (ngx_time()-expires) and test if it is greater than conf.expires. In this way, expiry is localized to apisix as it supports the current need of the user. Any mistake in setting up conf expiry (maybe a big value) value and the tokens generated with that value can't be used to exploit the infra because generated tokens can be revoked by reconfiguring the expires field.

Also, how about ignoring the check when conf.expires = 0

Thank you for your guidance.

bisakhmondal

LGTM. Thanks for addressing the changes.

Also, how about ignoring the check when conf.expires = 0

What do you think?

Baoyuantop · 2022-01-27T10:09:21Z

LGTM. Thanks for addressing the changes.

Also, how about ignoring the check when conf.expires = 0

What do you think?

Agree

bisakhmondal · 2022-01-28T08:54:23Z

+    if conf.expires == 0 then
+        return
+    end


Ahh, sorry I confused you. I didn't mean to ignore csrf token validation. I meant we can ignore token has expired check.

bisakhmondal · 2022-01-28T08:55:03Z

+    if time_now - expires > conf.expires then
+        core.log.error("token has expired")
+        return false
+    end


Suggested change

if time_now - expires > conf.expires then

core.log.error("token has expired")

return false

end

if conf.expires > 0 and time_now - expires > conf.expires then

core.log.error("token has expired")

return false

end

Just that's it.

bisakhmondal · 2022-01-28T08:56:08Z

 | expires |  number | optional | `7200` | | Expiration time(s) of csrf cookie. |
 | key | string | required |  |  | The secret key used to encrypt the cookie. |

+**Note: When expires is set to 0 the plugin will ignore all checks**


Suggested change

**Note: When expires is set to 0 the plugin will ignore all checks**

**Note: When expires is set to 0 the plugin will ignore checking if the token is expired or not.**

bisakhmondal · 2022-01-28T08:57:58Z

+
+
+
+=== TEST 12: set expires 0


please update it accordingly. Split it into 2 cases

expires = 1 sec, sleep = 2 sec after drawing a token then check token has expired

expires = 0, sleep = 1, no expiration

Baoyuantop · 2022-02-24T06:41:38Z

I have a problem when I change the test: I need to carry the cookie returned from the previous response. Is there any way to do it? The cookie is now encoded in the test code.

spacewander · 2022-02-25T02:52:11Z

You can refer to:

One request after another:

https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md

bisakhmondal

lgtm. Thanks, @Baoyuantop for addressing the changes. Some minor nitpicks.

bisakhmondal · 2022-03-10T04:47:01Z

+            if res.status >= 300 then
+                ngx.status = res.status
+            end
+        }


set the ngx.status = res.status
ngx.say(res.body)

https://github.com/ledgetech/lua-resty-http

bisakhmondal · 2022-03-10T04:47:49Z

+=== TEST 12: set expires 1
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local code, body = t('/apisix/admin/routes/1',
+                ngx.HTTP_PUT,
+                [[{
+                    "uri": "/hello",
+                    "upstream": {
+                        "type": "roundrobin",
+                        "nodes": {
+                            "127.0.0.1:1980": 1
+                        }
+                    },
+                    "plugins": {
+                        "csrf": {
+                            "key": "userkey",
+                            "expires": 1
+                        }
+                    }
+                }]]
+            )
+
+            if code >= 300 then
+                ngx.status = code
+            end
+            ngx.say(body)
+        }
+    }
+--- response_body
+passed


redundant? /route/1 already has the same config.

Yes, modified

bisakhmondal · 2022-03-10T17:53:20Z

+            end
+            ngx.say(res.body)
+        }
+    }


Suggested change

end

ngx.say(res.body)

}

}

end

ngx.status = res.status

ngx.print(res.body)

}

}

--- response_body

hello world

I think you missed checking the response-body. Add this snippet. Thanks

bisakhmondal

LGTM. Thanks, @Baoyuantop

feat: add expires timestamp for csrf plugin

5078d9b

tzssangglass previously approved these changes Jan 26, 2022

View reviewed changes

tokers reviewed Jan 26, 2022

View reviewed changes

spacewander requested a review from bisakhmondal January 26, 2022 01:30

bisakhmondal reviewed Jan 26, 2022

View reviewed changes

Baoyuantop added 2 commits January 26, 2022 14:08

fix: change expires to timestamp

fbda806

Merge branch 'master' into feat-csrf-expires

1c9d129

spacewander requested a review from bisakhmondal January 27, 2022 07:27

bisakhmondal reviewed Jan 27, 2022

View reviewed changes

feat: expires is 0 ignore checks

bfc9922

docs: improve documentation

46e6c94

Baoyuantop requested a review from bisakhmondal January 28, 2022 08:05

bisakhmondal requested changes Jan 28, 2022

View reviewed changes

spacewander dismissed tzssangglass’s stale review via 46e6c94 February 11, 2022 03:41

fix: update codes

f023c1a

Baoyuantop added 3 commits March 9, 2022 06:20

test: supplemental expiration test

bde3363

fix: lint error

e518d3c

test: change sleep function

623a7c3

Baoyuantop requested review from bisakhmondal and tzssangglass March 9, 2022 06:59

tzssangglass previously approved these changes Mar 10, 2022

View reviewed changes

bisakhmondal reviewed Mar 10, 2022

View reviewed changes

test: fix review

d254b0b

Baoyuantop dismissed tzssangglass’s stale review via d254b0b March 10, 2022 09:48

Baoyuantop requested a review from bisakhmondal March 10, 2022 09:49

fix: lint error

d5b9a8d

bisakhmondal reviewed Mar 10, 2022

View reviewed changes

test: add res body check

d8ca727

bisakhmondal approved these changes Mar 12, 2022

View reviewed changes

bisakhmondal requested review from tokers and tzssangglass March 12, 2022 07:12

tzssangglass approved these changes Mar 13, 2022

View reviewed changes

spacewander merged commit cd3e7cd into apache:master Mar 13, 2022

Baoyuantop deleted the feat-csrf-expires branch March 13, 2022 14:13

		local expires_timestamp = ngx_time() + conf.expires
		local sign = gen_sign(random, expires_timestamp, conf.key)

	Note: When expires is set to 0 the plugin will ignore all checks
	Note: When expires is set to 0 the plugin will ignore checking if the token is expired or not.

Uh oh!

Conversation

Baoyuantop commented Jan 25, 2022

What this PR does / why we need it:

Pre-submission checklist:

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!

bisakhmondal left a comment

Choose a reason for hiding this comment

Uh oh!

Baoyuantop commented Jan 27, 2022

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!

Baoyuantop commented Feb 24, 2022

Uh oh!

spacewander commented Feb 25, 2022

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!

bisakhmondal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants