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

Add sniffer enhancement related to Fake DNS #697

Closed
wants to merge 2 commits into from

Conversation

yuhan6665
Copy link
Contributor

I have been thinking of for the cache/mapping loss when core is rebooted. Also got some idea from the initial user of the current fake DNS.
Basically the idea is to use the existing sniffers (HTTP, TLS) when the domain name cannot be recovered. When it works, logs will be like:

31872-31976/com.v2ray.ang I/GoLog: [Info] v2ray.com/core/app/dns: returning fake IP 240.0.0.2 for domain baidu.com
...reboot...
2020-11-29 22:55:56.300 31872-31976/com.v2ray.ang I/GoLog: [Info] [1895565757] v2ray.com/core/proxy/socks: TCP Connect request to tcp:240.0.0.2:80
2020-11-29 22:55:56.300 31872-31976/com.v2ray.ang I/GoLog: [Info] v2ray.com/core/app/dns/fakedns: A fake ip request to 240.0.0.2, however there is no matching domain name in fake DNS
2020-11-29 22:55:56.300 31872-31976/com.v2ray.ang I/GoLog: [Info] [1895565757] v2ray.com/core/app/dispatcher: Using sniffer http1 since the fake DNS missed
2020-11-29 22:55:56.300 31872-31976/com.v2ray.ang I/GoLog: [Info] [1895565757] v2ray.com/core/app/dispatcher: sniffed domain: baidu.com
2020-11-29 22:55:56.300 31872-31976/com.v2ray.ang I/GoLog: [Info] [1895565757] v2ray.com/core/app/dispatcher: taking detour [direct] for [tcp:baidu.com:80]

@Loyalsoldier
Copy link
Contributor

Loyalsoldier commented Feb 23, 2021

Maybe this behavior should be the default one in FakeDNS instead of adding a new DNS convention fakedns+others?

@yuhan6665
Copy link
Contributor Author

Maybe be this behavior should be the default one in FakeDNS instead of adding a new DNS convention fakedns+others?

I can make change this way. Commit will be simpler even.
The reason to add a new sniffer "fakedns+others" was to give user a way to opt out. But opt out will fail connection anyway..

@rurirei
Copy link
Contributor

rurirei commented Feb 23, 2021

but v2ray-sniffing enabled also on my side, as FakeDns not used anywhere.

{ "destOverride": [ "http", "tls", "fakedns" ] },
{ "dns": { "servers": [ "fakedns", "8.8.8.8" ] }
[v2ray] [Debug] v2ray.com/core/app/dns: domain dns.google will use DNS in order: [FakeDNS UDP:8.8.8.8:53]
[v2ray] [Info] [348177452] v2ray.com/core/app/dispatcher: fake dns got domain: dns.google for ip: 198.18.0.3
[v2ray] [Info] [348177452] v2ray.com/core/app/dispatcher: sniffed domain: dns.google

// not FakeDNS!
[v2ray] [Info] [2213317628] v2ray.com/core/app/dispatcher: sniffed domain: api.github.com

@Loyalsoldier
Copy link
Contributor

but v2ray-sniffing enabled also on my side, as FakeDns not used anywhere.

{ "destOverride": [ "http", "tls", "fakedns" ] },
{ "dns": { "servers": [ "fakedns", "8.8.8.8" ] }
[v2ray] [Debug] v2ray.com/core/app/dns: domain dns.google will use DNS in order: [FakeDNS UDP:8.8.8.8:53]
[v2ray] [Info] [348177452] v2ray.com/core/app/dispatcher: fake dns got domain: dns.google for ip: 198.18.0.3
[v2ray] [Info] [348177452] v2ray.com/core/app/dispatcher: sniffed domain: dns.google

// not FakeDNS!
[v2ray] [Info] [2213317628] v2ray.com/core/app/dispatcher: sniffed domain: api.github.com

There is an option metadataOnly for FakeDNS. Set it true, then v2ray can't sniff domains. But I don't know what this option for.

@yuhan6665
Copy link
Contributor Author

@rurirei
without b17dbd5, user need to turn on sniffer "http", "tls" for all IP range. Which might not be desirable in some cases.
c88d124 is just a small enhancement to make new fake IP unique.

@Loyalsoldier
Copy link
Contributor

Loyalsoldier commented Feb 24, 2021

Maybe add a boolean option fallback to SniffingObject. Set it true, then fallback to other sniffers?

(But as you can see for now, if the metadataOnly option in fakeDNS is set to false, other sniffers still work. Is it necessary to keep both fallback and metadataOnly options?)

@xiaokangwang

Fallback means if the request IP is in the range of fakes,
when fakeDNS cache missed (most likely due to reboot), "tls" and "http"
sniffer will be used to identify domain
With the default ip range, now it is guarentee to return different ip within
4 hours.
It helps when core is rebooted, new DNS request still get fresh ip,
If there is request to the old fake ips, fake DNS can't find the domain,
other sniffers will get a chance to work.

Also fix an edge case where element is wrongly brought to top of LRU
@rurirei
Copy link
Contributor

rurirei commented Feb 24, 2021

@yuhan6665 @Loyalsoldier what about defaults to fallback (if when metadataOnly: false), unless there are no "http and/or tls" sniffers appears in specified configs.

@Loyalsoldier
Copy link
Contributor

@yuhan6665 @Loyalsoldier what about defaults to fallback (if when metadataOnly: false), unless there are no "http and/or tls" sniffers appears in specified configs.

IMO, I like the word fallback more. The metadataOnly word to me is hard to understand. 😂

@rurirei
Copy link
Contributor

rurirei commented Feb 24, 2021

@Loyalsoldier metadataOnly is not option for fallback seems.

// Whether should only try to sniff metadata without waiting for client input.
// Can be used to support SMTP like protocol where server send the first message.
bool metadata_only = 3;
}

@yuhan6665
Copy link
Contributor Author

From what I can tell, metadataOnly = ture will turn off any fallback behavior, so I think we are good. Let's keep the existing configs as is.

@yuhan6665 yuhan6665 mentioned this pull request Feb 26, 2021
@klzgrad
Copy link

klzgrad commented Mar 6, 2021

This will stop working with TLS 1.3 ESNI and HTTP/3, no?

@rurirei
Copy link
Contributor

rurirei commented Mar 6, 2021

@klzgrad FakeDNS will not work with Encrypted DNS query. It may work with ESNI.

Copy link
Contributor

@kslr kslr left a comment

Choose a reason for hiding this comment

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

Looks good, but if we plan to add TUN Inbound, "fakedns+other" is better

@kslr
Copy link
Contributor

kslr commented Apr 3, 2021

@xiaokangwang Don't forget

@xiaokangwang
Copy link
Contributor

The reason I added a metadata only only is that without that, some protocol that require server to initiate the talk will not work(notably SMTP).

I have to say that the change in shouldOverride have break the abstraction of the sniffer. I will look more into how to incorporate this change into V2Ray.
https://github.com/v2fly/v2ray-core/pull/697/files#diff-c3820ebfbe4f1eb7e9d13008e995e0fa8912cad9f058c9f7767aed1df10533d8R181

@xiaokangwang
Copy link
Contributor

but v2ray-sniffing enabled also on my side, as FakeDns not used anywhere.

{ "destOverride": [ "http", "tls", "fakedns" ] },
{ "dns": { "servers": [ "fakedns", "8.8.8.8" ] }
[v2ray] [Debug] v2ray.com/core/app/dns: domain dns.google will use DNS in order: [FakeDNS UDP:8.8.8.8:53]
[v2ray] [Info] [348177452] v2ray.com/core/app/dispatcher: fake dns got domain: dns.google for ip: 198.18.0.3
[v2ray] [Info] [348177452] v2ray.com/core/app/dispatcher: sniffed domain: dns.google

// not FakeDNS!
[v2ray] [Info] [2213317628] v2ray.com/core/app/dispatcher: sniffed domain: api.github.com

Sorry my English is very bad and I cannot understand your meaning, but, when FakeDNS is not getting result, and metadataonly is false which allow V2Ray to prevent client from dialing connection before sending content, other sniffer's result will work.

@xiaokangwang
Copy link
Contributor

If agreed, I can rework the internal code of this PR so that:
A composite sniffer will sniff the domain name with tls and http if the ip address in the fake dns's ip pool but no result is given from fake dns.

@Loyalsoldier
Copy link
Contributor

If agreed, I can rework the internal code of this PR so that:
A composite sniffer will sniff the domain name with tls and http if the ip address in the fake dns's ip pool but no result is given from fake dns.

It's great to also implement this #806

@xiaokangwang
Copy link
Contributor

If agreed, I can rework the internal code of this PR so that:
A composite sniffer will sniff the domain name with tls and http if the ip address in the fake dns's ip pool but no result is given from fake dns.

It's great to also implement this #806

There are also domains like .internal that shouldn't be solved with fake dns. I think this should be something we require user to configure to suit their environment instead of hard code such logic. This issue is similar to some app don't respect fake dns's response and refuse to work when fake dns is enabled. We already have setting to solve all these problems, we shouldn't use hard coded logic then.

@Loyalsoldier
Copy link
Contributor

If agreed, I can rework the internal code of this PR so that:
A composite sniffer will sniff the domain name with tls and http if the ip address in the fake dns's ip pool but no result is given from fake dns.

It's great to also implement this #806

There are also domains like .internal that shouldn't be solved with fake dns. I think this should be something we require user to configure to suit their environment instead of hard code such logic. This issue is similar to some app don't respect fake dns's response and refuse to work when fake dns is enabled. We already have setting to solve all these problems, we shouldn't use hard coded logic then.

But V2Ray cannot deal with invalid domains with space now.

@yuhan6665
Copy link
Contributor Author

Close as work is done in #915

@yuhan6665 yuhan6665 closed this Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants