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.

Sign up for GitHub

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

Jump to bottom

proxy dns with NOTIMP error #2954

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

proxy dns with NOTIMP error #2954

wants to merge 5 commits into from
+120 −251

Conversation

rp-hello
Copy link

@rp-hello rp-hello commented Mar 31, 2024 •
edited
Loading

Chromium will send SVCB/HTTPS type dns query, which cause dns leak.

To prvent dns leak,

  • If the dns query type is not implemented, should return NOTIMP error, not just send data to original destination.
  • If the dns query has wrong format domain, should return FORMERR error, not send data to original destination.

@rp-hello
Copy link
Author

rp-hello commented Mar 31, 2024 •
edited
Loading

Here is similar issue XTLS/Xray-core#3006.
Could we just introduce nonIPQuery option?

  • when nonIPQuery=drop, just return NOTIMP, and this is default behavior
  • when nonIPQuery=skip, follow the original chain, call connWriter.WriteMessage

@github-actions 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 the Stale label Jul 30, 2024
@github-actions github-actions bot removed the Stale label Aug 4, 2024
@xiaokangwang
Copy link
Contributor

I think it would be better if we guard this new behavior behind a flag or option, otherwise it would take significantly longer time decide if a regression is justified.

@xiaokangwang xiaokangwang added the Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval label Aug 19, 2024
local added 4 commits December 11, 2024 17:41
proxy dns with NOTIMP error
5bac7bc
fix compile error when protoc is relative link
1fcf4cc 
```
$ go generate
...
proto.go
fork/exec ../Cellar/protobuf/28.3/bin/protoc: no such file or directory
exit status 1
proto.go:5: running "go": exit status 1
context_test.go
functions_test.go
v2ray_test.go
$ ls -l `which protoc`
lrwxr-xr-x 1 local admin 34 Nov  1 13:56 /opt/homebrew/bin/protoc -> ../Cellar/protobuf/28.3/bin/protoc
```
add nonIPQuery for dns config
ff1eb9f
go generate with only dns.proto
d266b05
@rp-hello
Copy link
Author

rp-hello commented Dec 13, 2024 •
edited
Loading

@xiaokangwang Please approve GitHub actions on this PR.

@xiaokangwang
Copy link
Contributor

xiaokangwang commented Jan 18, 2025 •
edited
Loading

Sadly I didn't see a place where I can approve it. Let's me have a try... Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rp-hello @xiaokangwang