apernet / OpenGFW

OpenGFW is a flexible, easy-to-use, open source implementation of GFW (Great Firewall of China) on Linux
https://gfw.dev/
Mozilla Public License 2.0
9.47k stars 711 forks source link

Add GeoIP and GeoSite support for expr #38

Closed KujouRinka closed 7 months ago

KujouRinka commented 7 months ago

I add geoip and geosite support:

- name: geosite block bilibili
  action: block
  expr: geosite(string(tls?.req?.sni), "bilibili")

- name: geoip block some ip
  action: block
  expr: geoip(string(ip.dst), "cn")

But I think it's better to make this as a draft, for more things that might have further discussion:

KujouRinka commented 7 months ago

Here's what I do:

So it looks like:

ruleset
    ├── buildins
    │   └── geo
    │       ├── geo_loader.go
    │       ├── geo_matcher.go
    │       ├── host_matchers.go
    │       ├── interface.go
    │       ├── matchers_v2geo.go
    │       └── v2geo/
    ├── expr.go
    └── interface.go

If no further problem, I will continue working on this:

Need a way to expose config options for users to specify local geoip/geosite db files, similar to Hysteria.

But may I ask that it's better to put this option to config.yaml in rules.yaml?

Thanks for your reading.

tobyxdd commented 7 months ago

👍 The changes look good but maybe remove compileHostMatcher entirely (extract the code that handles geoip/geosite into separate methods in GeoMatcher). btw "buildins" should be "builtins".

But may I ask that it's better to put this option to config.yaml in rules.yaml?

In config.yaml, perhaps create a new section for ruleset settings as we may have more in the future

KujouRinka commented 7 months ago

To specify local geoip/geosite db files:

# config.yaml

# other field ....

geo:
  geoip: geoip.dat
  geosite: geosite.dat
tobyxdd commented 7 months ago

Just tested it and can confirm that it works great 💯

I just have one last nitpicking to make - can you make it only download GeoIP/GeoSite db if at least one rule uses these functions? (lazy loading similar to ACL in Hysteria)

We already have AST visitor for analyzer dependency check, so it should be easy to implement

KujouRinka commented 7 months ago

I didn't find way to lazily build geosite and geoip by only calling expr.Compile() so I use parser.Parse() and ast.Walk to walk on AST before compile. I also move original expr.Compile() 's argument to depVisitor but I am not sure is this right.

https://github.com/KujouRinka/OpenGFW/blob/c26e04b5a6aa5ee7f4b9405702b873a0c4b4accd/ruleset/expr.go#L104-L123

I think geosite and geoip should differ from Analyzer so I add a function func isBuiltInFunction(name string) bool. If I am wrong please correct me.

tobyxdd commented 7 months ago

Is it possible to always provide these functions when calling Compile, but only actually load the geo files when the identifier is seen by the visitor during compilation? I think this works the same, but can keep the code the way it was before

KujouRinka commented 7 months ago

I agree with you, but I have a more question that loading geo files may counter with internet error, while ast.Visitor interface doesn't provide us how to return this. I think we should introduce a field in depVisitor for recording.

tobyxdd commented 7 months ago

Yes, feel free to modify depVisitor as you need.

tobyxdd commented 7 months ago

Or maybe just keep depVisitor a visitor (only record identifiers) and do the loading in the outer compile function?

KujouRinka commented 7 months ago

I rebase it from 1eb52f7 for making change clear.

KujouRinka commented 7 months ago

Or maybe just keep depVisitor a visitor (only record identifiers) and do the loading in the outer compile function?

You mean mark whether geosite and geoip is needed in depVisitor, then after compile function to decide whether to load? I think this is better, for we have no need to record geo.GeoMatcher in depVisitor. Just regard it as "only record identifiers" you mentioned. 😊

tobyxdd commented 7 months ago

Yes, maybe depVisitor can have a Functions similar to Analyzers

KujouRinka commented 7 months ago

I introduce UseGeoSite and UseGeoIp to depVisitor to mark this.

KujouRinka commented 7 months ago

I think I should rebase these commits from first commit of this PR right?

tobyxdd commented 7 months ago

I think I should rebase these commits from first commit of this PR right?

What do you mean

KujouRinka commented 7 months ago

I think these commits are a bit messy, so before merging, maybe I need to use "git rebase" to merge all commits into one.

tobyxdd commented 7 months ago

That's fine, we can use squash merge.

KujouRinka commented 7 months ago

I think this draft is about to be completed so I convert it to a PR. If have any problem, please feel free to call me. ❤️

tobyxdd commented 7 months ago

Thanks again for the effort. I plan to release v0.0.5 with this & WireGuard analyzer