feat(plugin): Add new plugin ua-restriction for bot spider restriction #4587

arthur-zhang · 2021-07-12T05:02:18Z

What this PR does / why we need it:

Support bot spider restriction, eg Baidu spider、go-httpclient etc

Mozilla/5.0 (iPhone; CPU iPhone OS 9_1 like Mac OS X) AppleWebKit/601.1.46 (KHTML, like Gecko) Version/9.0 Mobile/13B143 Safari/601.1 (compatible; Baiduspider-render/2.0; +http://www.baidu.com/search/spider.html)

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

conf/config-default.yaml

spacewander · 2021-07-12T06:59:14Z

apisix/plugins/bot-restriction.lua

+    additionalProperties = false,
+}
+
+local plugin_name = "bot-restriction"


The bot-restriction is confusing. It just checks the UA. What about renaming it to ua-restriction?

I do not think so，this plugin is for spider detection，and include most common spider ua.

A real bot-detection in the industry area is not just spider detection and UA check.

The plugin is for the common BaiduSpider、360Spider and some dev tools detection. We have to use the product of professional security company to do the feature you mentioned.

I found similar function in krakend

https://www.krakend.io/docs/throttling/botdetector/

Don't believe the nonsense bullshit. This way can only kick out script boys. But for the real hacker, checking the UA is definitely not enough.

We have to use the product of professional security company to do the feature you mentioned.

That's it. A real bot detection system should be as professional as them instead of just doing UA checks and declaring this solves the problem. People will laugh at APISIX. We should provide a mechanism that the professional security company can use to build a gateway, but not declare we are a security gateway.

is it more suitable to change plugin name to ua-restriction and remove the hard-coded ua list?

Of course, yes.

spacewander · 2021-07-12T07:00:42Z

apisix/plugins/bot-restriction.lua

+}
+
+-- List taken from https://github.com/ua-parser/uap-core/blob/master/regexes.yaml
+local well_known_bots = {


I think we should not hard code the UA list, as it could not be updated in time. It would be better to provide a mechanism but not the tool to check the UA.

Most spider bot UA contains “bot” or “spider” or “crawler” or dev http client, the regex expression covers most common case，it will not update very frequently.

But it will be updated, isn't it? Better to require the user to choose their list instead of shipping a stale one.

User can update the list in whitelist or blacklist configuration to support the ua not listed in our package

So why ship a stale one and ask the user to update it?

We can support other plugin for client restrction like ua, ip, or other infomartion. This plugin is just for users not want to add bunch of ua regex rules.

This plugin is only for simplify usage.
If user do not want to use the plugin, they can use other restriction plugin and modify the rules the want.

spacewander · 2021-07-12T07:04:53Z

t/plugin/bot-restriction.t

+        }
+    }
+
+    location /disable {


the /disable is unused.

spacewander · 2021-07-12T07:05:59Z

t/plugin/bot-restriction.t

+
+
+=== TEST 6: set blacklist
+


The blank line can be removed?

spacewander · 2021-07-12T07:06:27Z

t/plugin/bot-restriction.t

+
+
+=== TEST 7: hit route and user-agent in blacklist
+


spacewander · 2021-07-12T07:21:55Z

apisix/plugins/bot-restriction.lua

+    end
+    -- ignore multiple instances of request headers
+    if type(user_agent) == "table" then
+        return


Why ignore the UA?

this corner case is that the user-agent become table when send multiple user-agent. Almost all the bot or http-client will not send request like this，i think we ignore it is a better choice.

So if they send other UA, the check can be bypassed? This is not a good idea, especially in an open source project...

ok, I will check the table

spacewander · 2021-07-12T07:22:27Z

apisix/plugins/bot-restriction.lua

+    end
+    local match, err = lrucache_useragent(user_agent, conf, match_user_agent, user_agent, conf)
+    if err then
+        return


Better to log the err?

spacewander

Provide a mechanism but not the tool to check the UA

tokers · 2021-07-12T11:10:19Z

apisix/plugins/bot-restriction.lua

+            type = "array",
+            minItems = 1
+        },
+        blacklist = {


What about allowlist and blocklist, we should avoid using these sensitive words.

spacewander · 2021-07-13T02:20:37Z

docs/en/latest/plugins/ua-restriction.md

+| --------- | ------------- | ----------- | ------- | ----- | ---------------------------------------- |
+| allowlist | array[string] | optional    |         |       | List of User-Agent of allowlist. |
+| denylist | array[string] | optional    |         |       | List of User-Agent of denylist. |
+| message | string | optional             | Not allowed. | [1, 1024] | Message of deny reason. |


We should clarify that [1, 1024] is the length.

spacewander · 2021-07-13T02:21:25Z

apisix/plugins/ua-restriction.lua

+        for _, v in ipairs(user_agent) do
+            if type(v) == "string" then
+                match = lrucache_useragent(v, conf, match_user_agent, v, conf)
+                if match > MATCH_ALLOW then


Why not use match > MATCH_DENY?

i think this check should be MATCH_ALLOW, if ua in deny list, the result should be MATCH_DENY, otherwise, the result is MATCH_ALLOW or MATCH_NONE

spacewander · 2021-07-13T02:22:05Z

apisix/plugins/ua-restriction.lua

+        {required = {"allowlist"}},
+        {required = {"denylist"}},
+    },
+    minProperties = 1,


The minProperties is useless?

apisix/plugins/ua-restriction.lua

spacewander · 2021-07-19T01:11:18Z

@arthur-zhang
Would you like to update the PR? Thanks!

arthur-zhang · 2021-07-19T10:13:53Z

@arthur-zhang
Would you like to update the PR? Thanks!

I will update soon.

tzssangglass · 2021-07-20T06:46:17Z

t/plugin/ua-restriction.t

+
+
+
+=== TEST 16: hit route and user-agent in both allowlist and denylist, part 1


Suggested change

=== TEST 16: hit route and user-agent in both allowlist and denylist, part 1

=== TEST 16: hit route and user-agent in both allowlist and denylist, part 1

Suggested change

=== TEST 16: hit route and user-agent in both allowlist and denylist, part 1

=== TEST 16: hit route and user-agent in both allowlist and denylist, pass(part 1)

tzssangglass · 2021-07-20T06:46:29Z

t/plugin/ua-restriction.t

+
+
+
+=== TEST 17: hit route and user-agent in both allowlist and denylist, part 2


tzssangglass · 2021-07-20T06:54:21Z

t/plugin/ua-restriction.t

+                            "denylist": [
+                                "foo"
+                            ],
+                            "disable": true


What does disable mean here? I don't see it in the plugin properties doc.

disable is built-in properties of plugin schema

spacewander · 2021-07-21T00:55:18Z

Let's merge master to make CI pass

arthur-zhang · 2021-07-21T00:55:26Z

Seems CI system is not working

spacewander · 2021-07-21T01:00:26Z

Err, since the server is changed again...

arthur-zhang · 2021-07-21T01:11:07Z

Err, since the server is changed again...

apisix/plugins/ua-restriction.lua

tokers · 2021-07-21T01:08:02Z

docs/en/latest/plugins/ua-restriction.md

+
+## Name
+
+The `ua-restriction` can restrict access to a Service or a Route by either `allowlist` or `denylist` `User-Agent`.


Broken sentence? What does the "by either allowlist or denylist User-Agent" mean?

tokers · 2021-07-21T01:09:28Z

docs/en/latest/plugins/ua-restriction.md

+
+| Name      | Type          | Requirement | Default | Valid | Description                              |
+| --------- | ------------- | ----------- | ------- | ----- | ---------------------------------------- |
+| allowlist | array[string] | optional    |         |       | List of User-Agent of allowlist. |


tokers · 2021-07-21T01:10:32Z

docs/en/latest/plugins/ua-restriction.md

+| Name      | Type          | Requirement | Default | Valid | Description                              |
+| --------- | ------------- | ----------- | ------- | ----- | ---------------------------------------- |
+| allowlist | array[string] | optional    |         |       | List of User-Agent of allowlist. |
+| denylist | array[string] | optional    |         |       | List of User-Agent of denylist. |


tokers · 2021-07-21T01:11:29Z

docs/en/latest/plugins/ua-restriction.md

+...
+```
+
+Requests from bot User-Agent:


Suggested change

Requests from bot User-Agent:

Requests with the bot User-Agent:

t/plugin/ua-restriction.t

tokers · 2021-07-21T01:13:59Z

t/plugin/ua-restriction.t

+
+
+
+=== TEST 8: hit route and user-agent in denylist with multiple


With multiple what?

t/plugin/ua-restriction.t

arthur-zhang added 6 commits July 12, 2021 13:01

feat(plugin): Add new plugin bot-restriction for bot spider restrction

f2a8bf1

fix doc

830b621

fix doc

bfe2d7c

Fix lint

cb6d5d8

fix lint

8be944e

Fix lint

dc331cd

spacewander reviewed Jul 12, 2021

View reviewed changes

Fix Review

7ea7c87

spacewander requested changes Jul 12, 2021

View reviewed changes

tokers reviewed Jul 12, 2021

View reviewed changes

rename plugin to ua-restriction

0c6dcb2

arthur-zhang requested review from spacewander and tokers July 12, 2021 13:22

spacewander reviewed Jul 13, 2021

View reviewed changes

tzssangglass reviewed Jul 13, 2021

View reviewed changes

apisix/plugins/ua-restriction.lua Show resolved Hide resolved

spacewander added the wait for update wait for the author's response in this issue/PR label Jul 19, 2021

fix Review

420e676

arthur-zhang requested a review from spacewander July 19, 2021 12:13

spacewander approved these changes Jul 20, 2021

View reviewed changes

tzssangglass reviewed Jul 20, 2021

View reviewed changes

spacewander removed the wait for update wait for the author's response in this issue/PR label Jul 20, 2021

arthur-zhang added 4 commits July 20, 2021 15:46

Fix Review

f7a61bd

Fix Test

9ace564

Merge remote-tracking branch 'origin/master' into feat-bot-detection

1ba8c12

Fix ci

a0fb6dd

tokers reviewed Jul 21, 2021

View reviewed changes

Add bypass_missing and fix test case

6837ed1

arthur-zhang requested review from tokers and spacewander July 21, 2021 09:32

tokers approved these changes Jul 21, 2021

View reviewed changes

spacewander approved these changes Jul 21, 2021

View reviewed changes

spacewander changed the title ~~feat(plugin): Add new plugin bot-restriction for bot spider restriction~~ feat(plugin): Add new plugin ua-restriction for bot spider restriction Jul 21, 2021

spacewander merged commit 71bc27c into apache:master Jul 21, 2021

arthur-zhang deleted the feat-bot-detection branch July 21, 2021 13:50

liuxiran mentioned this pull request Aug 25, 2021

Feature request: support config plugin ua-restriction apache/apisix-dashboard#2091

Closed




		=== TEST 16: hit route and user-agent in both allowlist and denylist, part 1




		=== TEST 17: hit route and user-agent in both allowlist and denylist, part 2


		## Name

		The `ua-restriction` can restrict access to a Service or a Route by either `allowlist` or `denylist` `User-Agent`.

	\| allowlist \| array[string] \| optional \| \| \| List of User-Agent of allowlist. \|
	\| allowlist \| array[string] \| optional \| \| \| A list of allowed `User-Agent` headers . \|

	\| denylist \| array[string] \| optional \| \| \| List of User-Agent of denylist. \|
	\| denylist \| array[string] \| optional \| \| \| A list of denied `User-Agent` headers. \|

	Requests from bot User-Agent:
	Requests with the bot User-Agent:




		=== TEST 8: hit route and user-agent in denylist with multiple

feat(plugin): Add new plugin ua-restriction for bot spider restriction #4587

feat(plugin): Add new plugin ua-restriction for bot spider restriction #4587

Conversation

arthur-zhang commented Jul 12, 2021 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

Choose a reason for hiding this comment

arthur-zhang Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthur-zhang Jul 12, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander left a 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

spacewander commented Jul 19, 2021

arthur-zhang commented Jul 19, 2021

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

spacewander commented Jul 21, 2021

arthur-zhang commented Jul 21, 2021

spacewander commented Jul 21, 2021

arthur-zhang commented Jul 21, 2021

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

arthur-zhang commented Jul 12, 2021 •

edited

Loading

arthur-zhang Jul 12, 2021 •

edited

Loading

arthur-zhang Jul 12, 2021 •

edited

Loading