Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EDNS: Client Subnet #1007

Merged
merged 130 commits into from Nov 20, 2023
Merged

EDNS: Client Subnet #1007

merged 130 commits into from Nov 20, 2023

Conversation

kwitsch
Copy link
Collaborator

@kwitsch kwitsch commented Apr 30, 2023

Closes #952
Closes #1146
Depends on #1245 merged

Changes:

  • added util for handling EDNS0 options
  • disable caching if the request contains a netmask size greater than 1
  • added config section for ECS handling and validation for it
  • added ecs_resolver for enhancing and cleaning subnet and client IP information

Old entry:
Tested it with some blocky containers in a chain and it works well.

I'm currently at a loss how to write propper unit tests as most of the testing has to be done in the mock resolver that is configured as next(?). 🤔

Ideas & hints how to do it properly are appreciate. 😅

@codecov
Copy link

codecov bot commented Apr 30, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (d52c598) 93.76% compared to head (4ce48f3) 93.85%.

Files Patch % Lines
util/edns0.go 95.89% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1007      +/-   ##
==========================================
+ Coverage   93.76%   93.85%   +0.08%     
==========================================
  Files          72       75       +3     
  Lines        5890     6041     +151     
==========================================
+ Hits         5523     5670     +147     
- Misses        284      287       +3     
- Partials       83       84       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ThinkChaos
Copy link
Collaborator

You can take a look at the rewriter, it has some tests that hook into the inner and next resolvers.
For example:
https://github.com/0xERR0R/blocky/blob/v0.21/resolver/rewriter_resolver_test.go#L144-L150

@kwitsch
Copy link
Collaborator Author

kwitsch commented May 1, 2023

You can take a look at the rewriter, it has some tests that hook into the inner and next resolvers. For example: https://github.com/0xERR0R/blocky/blob/v0.21/resolver/rewriter_resolver_test.go#L144-L150

Thanks, that's axactly what I was looking for. ❤️

@bjw-s
Copy link

bjw-s commented May 26, 2023

Having support for ECS would seriously benefit my use case for Blocky! Just wanted to drop a big thank you for working on this!

@github-actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Jul 11, 2023
@bjw-s
Copy link

bjw-s commented Jul 11, 2023

Still very much needed

@github-actions github-actions bot removed the Stale label Jul 12, 2023
@0xERR0R 0xERR0R added this to the future milestone Jul 14, 2023
@buroa
Copy link

buroa commented Jul 14, 2023

Would also love this in my network!

@fisherbln
Copy link

Would also love this in my network!

same!

@kwitsch
Copy link
Collaborator Author

kwitsch commented Jul 14, 2023

@bjw-s @buroa @fisherbln Sorry guys i got almost no time at least until end of next month. I'll work on it again as soon as time allows it.

@buroa
Copy link

buroa commented Sep 13, 2023

@kwitsch Any update on this?

@kwitsch
Copy link
Collaborator Author

kwitsch commented Sep 14, 2023

Sorry I replied to it yesterday but it seems it's missing? 🤨

@kwitsch Any update on this?

Sorry to let you down but I still had no time to work on blocky again. 😔

I'm currently only able to be online with my mobile phone which is not suitable for working on blocky. 🫣

@0xERR0R 0xERR0R changed the title Issues/952 EDNS: Client Subnet Sep 19, 2023
@0xERR0R
Copy link
Owner

0xERR0R commented Sep 19, 2023

@kwitsch I skimmed over your PR and I think I can continue to work on it.

I have some questions/discussion points:

  • As far I understand, ECS makes only sense, if public IP is used. If blocky runs on a local network, it receives only local client addresses. Does it make sense to forward a local address (like 192.168.x) to a remote upstream? Would it better to check in EcsResolver if client IP is a local network and only if not, apply it to request? How covers it the use case of original issue (Feature request: use RFC7871 to determine client IP #952)?
  • What is about caching? IMHO it not correct to store a server reply for one Client Submask and return the same answer to another one... Maybe it is better not to cache answers for ECS request?
  • Just idea: maybe it would make sense to provide a config option to set a constant ECS IP/subnet? Use case: local network, someone is behind CGNAT and has only NAT address as IPv4 (100.64.x.x). User can set custom subnet of his ISP and ECS aware upstream resolvers will serve better results.

@kwitsch
Copy link
Collaborator Author

kwitsch commented Sep 19, 2023

  • As far I understand, ECS makes only sense, if public IP is used. If blocky runs on a local network, it receives only local client addresses. Does it make sense to forward a local address (like 192.168.x) to a remote upstream? Would it better to check in EcsResolver if client IP is a local network and only if not, apply it to request? How covers it the use case of original issue (Feature request: use RFC7871 to determine client IP #952)?

I viewed it the other way around: especially in local network setups this feature is handy 😅

Example for an IPv4 network:
Multiple servers with blocky on port 53 and netmask 32 resolve against another blocky instance in a secured network which isn't accessible to the original requestor. This way the secured instance gets the whole IP of the original requestor and uses it for client name resolution instead of the blocky instance working as gateway/proxy. If the secured instance resolves against a local server the ECS info can be forwarded to use it there.

  • What is about caching? IMHO it not correct to store a server reply for one Client Submask and return the same answer to another one... Maybe it is better not to cache answers for ECS request?

Agreed 👍

  • Just idea: maybe it would make sense to provide a config option to set a constant ECS IP/subnet? Use case: local network, someone is behind CGNAT and has only NAT address as IPv4 (100.64.x.x). User can set custom subnet of his ISP and ECS aware upstream resolvers will serve better results.

I think for Nat purpose a second instance for resolution would be the better choice (as described in the first example).

@kwitsch
Copy link
Collaborator Author

kwitsch commented Sep 19, 2023

@0xERR0R I can provide an example project (docker compose & blocky config) tomorrow in a few days if it helps clarifying how I intended this feature.

Edit:
Sorry it'll take some more time for the upload since I stay in the hospital longer than expected. 😓

@kwitsch kwitsch linked an issue Sep 19, 2023 that may be closed by this pull request
@kwitsch
Copy link
Collaborator Author

kwitsch commented Sep 25, 2023

I'm back at home and should have more time working on blocky again. 🥳

@0xERR0R as promised an simple example: ecs-example.zip (should be placed in a sub directory of this branch)

Description:

  • blocky1: ecs with full net masks enabled
  • blocky1: uses blocky2 as upstream
  • blocky1: exposes port 5331
  • blocky2: useEcsAsClient enabled
  • blocky2: uses 1.1.1.1 as upstream
  • blocky2: exposes port 5332

Expected behavior: if a request is done to both ports blocky should log the same client_ip for both requests

I would only disable caching if the ecs mask contains more than one IP(v4 <32 & v6 <128).
Otherwise this feature wouldn't be usefull for most of the issues it was intended to solve.

@buroa
Copy link

buroa commented Sep 25, 2023

I would only disable caching if the ecs mask contains more than one IP(v4 <32 & v6 <128). Otherwise this feature wouldn't be usefull for most of the issues it was intended to solve.

Definitely. I have a lot of clients in my network and would love to capture the IP; but I also do a ton of caching... so if it's favor of caching vs ecs, then I would pick ecs. Hopefully we can have both of these features enabled at the same time :)

@ThinkChaos
Copy link
Collaborator

ThinkChaos commented Nov 19, 2023

I think 0 doesn't matter: why would we cache something to delete it immediately?
So I'd rather update the docs than the code.

Nevermind, you're right!

Copy link
Collaborator

@ThinkChaos ThinkChaos left a comment

Choose a reason for hiding this comment

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

Just a couple details I noticed going through everything again.

util/edns0.go Outdated Show resolved Hide resolved
model/models.go Outdated Show resolved Hide resolved
config/ecs.go Outdated Show resolved Hide resolved
util/edns0.go Outdated Show resolved Hide resolved
resolver/ede_resolver_test.go Outdated Show resolved Hide resolved
util/edns0.go Outdated Show resolved Hide resolved
resolver/ecs_resolver.go Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
kwitsch and others added 5 commits November 19, 2023 21:58
Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>
Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>
Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>
Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>
Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>
@kwitsch
Copy link
Collaborator Author

kwitsch commented Nov 20, 2023

@ThinkChaos I think I resolved all of your comments. :)

@ThinkChaos
Copy link
Collaborator

Yes looks all good to me!

@kwitsch kwitsch merged commit d37d183 into 0xERR0R:main Nov 20, 2023
11 checks passed
@kwitsch kwitsch deleted the issues/952 branch November 20, 2023 15:56
@buroa
Copy link

buroa commented Nov 21, 2023

@kwitsch Big BIG thanks to this!!

@kwitsch
Copy link
Collaborator Author

kwitsch commented Nov 21, 2023

@kwitsch Big BIG thanks to this!!

I'm glad I could finally finish it & that it's appreciated. 😊
Sorry again for taking this log...I had some RL struggles. 😖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 enhancement New feature or request
Projects
None yet
7 participants