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

Apply DomainStrategy to outbound target #2553

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

Vigilans
Copy link
Contributor

@Vigilans Vigilans commented Jun 6, 2023

Fixes #2448 (comment)

For outbounds like SOCKS5, VMESS, domainStrategy will only take effect on proxy server's address (because domainStrategy only applies for dialing).

This PR applies domainStrategy to outbound handler's Dispatch method, so all outbound targets will be affected by domainStrategy. Therefore domainStrategy for SOCKS5 and VMESS will behave like Freedom outbound.

Prior behavior for SOCKS5, VMESS:

  • √ Remote proxy server's address is affected by domainStrategy.
  • x Actual target address is not affected by domainStrategy.

After this PR

  • √ Remote proxy server's address is affected by domainStrategy.
  • √ Actual target address will be affected by domainStrategy.

@Vigilans Vigilans force-pushed the vigilans/refine-domain-strategy branch from 73b9ed1 to c9d3fd3 Compare June 6, 2023 11:51
@Vigilans
Copy link
Contributor Author

Vigilans commented Jun 6, 2023

  • Remote proxy server's address affected by domainStrategy.
  • Actual target address affected by domainStrategy.

A question raised is that do we need to make these two behaviors individually configurable?

E.g., we may need to resolve proxy server's address using own strategy in v2ray's DNS, but still delegate target domain to remote server for resolution.

If individually configurable, how to specify the config format?

Option 1:

{
  "domainStrategy": {
    "dialer": "UseIP",
    "target": "AsIs"
  }
}

{
  "domainStrategy": "UseIP"
}
# is equivalent as
{
  "domainStrategy": {
    "dialer": "UseIP",
    "target": "UseIP"
  }
}

Option 2

{
  "domainStrategy": "AsIs"
  "dialStrategy": "UseIP"
}

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2ac0188) 38.31% compared to head (d4db633) 38.31%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2553   +/-   ##
=======================================
  Coverage   38.31%   38.31%           
=======================================
  Files         637      637           
  Lines       38110    38118    +8     
=======================================
+ Hits        14601    14606    +5     
- Misses      21901    21904    +3     
  Partials     1608     1608           
Impacted Files Coverage Δ
app/proxyman/outbound/handler.go 33.33% <0.00%> (-1.50%) ⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dyhkwong
Copy link
Contributor

dyhkwong commented Jun 6, 2023

If DomainStrategy is not AsIs and outbound.Target.Addressis a domain, but resolveIP returns nil, please consider one of these two behaviors:

  • leave outbound.Target.Address as is and continue dispatching
  • close link and stop dispatching

SagerNet has done roughly the same thing as this PR before and named it DomainStrategy. IMO there is no need to make dialer and target DomainStrategy individually configurable. If you must make them individually configurable, please consider Option 2. Option 1 may need some json unmarshal hacking, make it troublesome to implement in some client software doing conversion between JSON and object of other languages.

@Vigilans
Copy link
Contributor Author

Vigilans commented Jun 6, 2023

If DomainStrategy is not AsIs and outbound.Target.Addressis a domain, but resolveIP returns nil, please consider one of these two behaviors:

  • leave outbound.Target.Address as is and continue dispatching
  • close link and stop dispatching

SagerNet has done roughly the same thing as this PR before and named it DomainStrategy. IMO there is no need to make dialer and target DomainStrategy individually configurable. If you must make them individually configurable, please consider Option 2. Option 1 may need some json unmarshal hacking, make it troublesome to implement in some client software doing conversion between JSON and object of other languages.

The previous behavior is AsIs, so I'll keep using AsIs here.

SagerNet@4f039c0 's PreferIPv4 and PreferIPv6 reminds me of another problem: Current v2ray's behavior will randomly select an IP from multiple results, which means ipv4 or ipv6 will be selected randomly for dual stack site. If using UseIP, remote server may not have IPv6 address so failed to process.

So we enumerate the usage possibilities:

1. Single stack proxy server + AsIs

  • √ Proxy server domain: no problem, result is either IPv4/IPv6
  • √ Target domain: no problem, delegated to proxy server

2. Single stack proxy server + UseIPv4/UseIPv6

  • √ Proxy server domain: no problem if IP version matches
  • √ Target domain: no problem, since proxy server domain already needs correct domain strategy to access

3. Single stack proxy server + UseIP

  • √ Proxy server domain: no problem, will return single stack result
  • x Target domain: problematic since IPv6/IPv4 result may be chosen for IPv4/IPv6 only proxy server

4. Dual stack proxy server + AsIs

  • √ Proxy server domain: no problem, system may prefer IPv6
  • √ Target domain: no problem, delegated to proxy server

5. Dual stack proxy server + UseIPv4/UseIPv6

  • √ Proxy server domain: no problem, configure preferred route for proxy server
  • x Target domain: problematic, since with UseIPv6 a site without IPv6 address will return nil

6. Dual stack proxy server + UseIP

  • √ Proxy server domain: no problem, preferred route can be configured with dns's queryStrategy
  • ? Target domain: IPv4/IPv6 will be selected randomly.

Case 3 is meaningless since Case 1 and Case 2 covers all scenarios for single-stack server. For proxy server, Case 5 can be implemented as Case 6 with DNS queryStrategy. Therefore, we may not need a separated config for dialer's staretegy.

On the other hand, with SagerNet@4f039c0 's PreferIPv4 and PreferIPv6, we can replace UseIP with PreferIPv- for following benifits:

  • Problem in Case 5 is avoided. Problem in Case 3 cannot be avoided since dual-stack host may still choose IPv6 address for single-stack proxy server. But case 3 is still meaningless since we know that proxy server is single-stack so we can choose UseIPv4 or UseIPv6.
  • Can use a definitive startegy for target domains in case 6.

Therefore PreferIPv4 and PreferIPv6 may be two strategies worthwhile to introduce. After #2550 merged, I may open a pull request to introduce these two strategies.

@Vigilans Vigilans closed this Jun 6, 2023
@Vigilans Vigilans force-pushed the vigilans/refine-domain-strategy branch from c9d3fd3 to 2ac0188 Compare June 6, 2023 17:44
@Vigilans Vigilans reopened this Jun 6, 2023
@github-actions
Copy link
Contributor

It has been open 120 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added Stale and removed Stale labels Oct 19, 2023
Copy link
Contributor

@xiaokangwang xiaokangwang left a comment

Choose a reason for hiding this comment

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

I think this pull request is ready to be merged. Thanks for your contribution. It will be included in v5.12.0.

@xiaokangwang xiaokangwang merged commit ce7dc23 into v2fly:master Nov 19, 2023
76 checks passed
Vigilans added a commit to Vigilans/v2ray-core that referenced this pull request Feb 19, 2024
This reverts commit ce7dc23.

Until implementation of `PreferIPv4` and `PreferIPv6`. See v2fly#2553
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.

DomainStrategy Setting Has no Effect in Freedom Outbound while Using jsonv5 Format
4 participants