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

Feature(socks5): support abstract namespace unix sock #436

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Feature(socks5): support abstract namespace unix sock #436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion proxy/socks5.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,21 @@ type Socks5 struct {
}

func NewSocks5(addr, user, pass string) (*Socks5, error) {
unix := len(addr) > 0 && addr[0] == '/'
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
unix := len(addr) > 0 && addr[0] == '/'
unix := len(addr) > 0 && (addr[0] == '/' || addr[0] == '@' || addr[0] == 0x00)

Just wondering why not ^?

Copy link
Author

Choose a reason for hiding this comment

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

You pass the url.path from url instance when url.host is empty. If we use the format like socks5:///@test, the address passed to NewSocks will be /@test, which requires additional processing logic. and for len(address)>2, it is only for a more safe in case the out of index error. The approach here checks if the first char after / is @ or null(0x00), and remove the leading /, since it will cause error.

Copy link
Author

Choose a reason for hiding this comment

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

And the reason we can’t avoid using socks5:/// is the string before @ will be parsed as username/password whenever is has valid user:pass or not. The @ char will be omitted by parser.

Copy link
Owner

@xjasonlyu xjasonlyu Dec 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I see. that makes sense. But I think such hack adjustment might be better to add here in the parseSocks5 function:

tun2socks/engine/parse.go

Lines 123 to 132 in ec85410

func parseSocks5(u *url.URL) (proxy.Proxy, error) {
address, username := u.Host, u.User.Username()
password, _ := u.User.Password()
// Socks5 over UDS
if address == "" {
address = u.Path
}
return proxy.NewSocks5(address, username, password)
}

And NewSocks5 func should keep its straightforwardness. what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

It makes sense, in this case, the unix check logic may also move to parser function as well. And I will need to make a new arg for unix flag for NewSocks5 , else it will need redundant code to test if the address is unix or not.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, it sounds good


// For support Linux abstract namespace
if len(addr) > 2 && addr[1] == '@' || addr[1] == 0x00 {
addr = addr[1:]
}

return &Socks5{
Base: &Base{
addr: addr,
proto: proto.Socks5,
},
user: user,
pass: pass,
unix: len(addr) > 0 && addr[0] == '/',
unix: unix,
}, nil
}

Expand Down
Loading