-
Notifications
You must be signed in to change notification settings - Fork 186
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
🍁 UDP: Cache and reply from the same interface and IP #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we get too deep into this, could you clarify your use case?
Are you running outline-ss-server directly or the standard Outline Server?
Why do you need multiple interfaces?
Can you bind to a specific interface or address instead?
@@ -420,8 +428,8 @@ func (m *natmap) Close() error { | |||
var maxAddrLen int = len(socks.ParseAddr("[2001:db8::1]:12345")) | |||
|
|||
// copy from target to client until read timeout | |||
func timedCopy(clientAddr net.Addr, clientConn net.PacketConn, targetConn *natconn, | |||
keyID string, sm metrics.ShadowsocksMetrics) { | |||
func timedCopy(clientAddr *net.UDPAddr, clientConn onet.UDPPacketConn, oobCache []byte, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NAT will tie the targetConn
and the clientConn
, so every packet from targetConn
will go to the same clientConn
.
My understanding was that once you get the first packet the socket would be bound to the IP that received that packed. Is that not the case?
If it's not the case, this change can be made way less intrusive by implementing a PacketConn
that has that behavior. Right now your PR is spreading a bunch of implementation details around, while we just need to change how the packet connection works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change does not affect the behavior of targetConn
.
This change can be made way less intrusive by implementing a
PacketConn
that has that behavior.
I don't think this is possible. The write calls needs the OOB data to know which interface and IP to send from. You can't implement a PacketConn
with the knowledge of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not?
- Extend
UDPConn
and add a field to hold the address to use - Write a
ListenUDP
that will capture the address - Replace
WriteTo
to use the saved address
} | ||
|
||
func getOobForCache4(clientOob4 []byte) []byte { | ||
cmsg := (*oob4)(unsafe.Pointer(&clientOob4)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unsafe operations worry me. They may make the server less stable and less safe.
Can you use https://pkg.go.dev/golang.org/x/net/ipv4 or https://pkg.go.dev/golang.org/x/net/ipv6 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These unsafe operations are mostly identical with wireguard-go's implemention. I'm fairly confident these operations are safe. The OOB data is generated by the kernel, so there shouldn't be any security concerns.
} | ||
} | ||
|
||
func getOobForCache(clientOob []byte) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you get the address with PacketConn.LocalAddr()
?
https://pkg.go.dev/net#PacketConn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't. Because our clientConn
listens on ::
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked into this in great detail, but I think the cleanest solution here would be to launch a separate UDPService
for each network interface, each holding a clientConn
bound to one server IP address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Or simply bind to a single interface. @database64128 is support for multiple interfaces needed? Why?
We first identified this issue with UDP on some of our shadowbox deployments with multiple interfaces. Basically, outbound UDP packets are always sent via the gateway interface, causing them to be dropped by client firewall if the client is connecting from a non-gateway interface. Suppose we have interface
I'm not trying to bind the |
} | ||
} | ||
|
||
func getOobForCache(clientOob []byte) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Or simply bind to a single interface. @database64128 is support for multiple interfaces needed? Why?
@@ -420,8 +428,8 @@ func (m *natmap) Close() error { | |||
var maxAddrLen int = len(socks.ParseAddr("[2001:db8::1]:12345")) | |||
|
|||
// copy from target to client until read timeout | |||
func timedCopy(clientAddr net.Addr, clientConn net.PacketConn, targetConn *natconn, | |||
keyID string, sm metrics.ShadowsocksMetrics) { | |||
func timedCopy(clientAddr *net.UDPAddr, clientConn onet.UDPPacketConn, oobCache []byte, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not?
- Extend
UDPConn
and add a field to hold the address to use - Write a
ListenUDP
that will capture the address - Replace
WriteTo
to use the saved address
func ListenUDP(network string, laddr *net.UDPAddr) (onet.UDPPacketConn, error) { | ||
lc := &net.ListenConfig{ | ||
Control: func(network, address string, c syscall.RawConn) error { | ||
return c.Control(func(fd uintptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use https://pkg.go.dev/syscall#Bind or https://pkg.go.dev/syscall#BindToDevice at this point so you don't need to change the write code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or simply bind to a single interface.
How do you determine which interface to bind to? Most likely the user wants to bind to all interfaces.
Can you use https://pkg.go.dev/syscall#Bind or https://pkg.go.dev/syscall#BindToDevice at this point so you don't need to change the write code?
SO_BINDTODEVICE
does not allow you to specify a source IP. If an interface has multiple IP addresses of the same address family, outbound packets are always sent via the default IP. IP_PKTINFO
allows you to set a source IP in addition to an interface index. With the source IP setting, you don't have to bind the socket to an IP to send from that IP.
Why not?
For every interface you would have to create one clientConn
. My implementation creates one clientConn
that listens on all interfaces and IPs. This is also how wireguard-go
handles multiple interfaces.
Previously, outline-ss-server simply writes the UDP response, which gets routed via the default gateway. This behavior becomes a problem, when a server has multiple IP addresses of the same address family. Response UDP packets may get routed via a different interface or IP than the incoming packets. At the client's point of view, the outgoing UDP socket received packets from another IP. Most firewalls simply drop such packets. This commit borrows the idea from https://github.com/WireGuard/wireguard-go/blob/master/conn/bind_linux.go, where IP_PKTINFO and IPV6_PKTINFO are used to determine the interface and IP where the incoming packets are from. Then we send the response using the information as socket out-of-band data to inform the kernel how to route the packets. Currently it's only implemented for Linux. Implementing it for other platforms is simply not worth the effort, in my opinion.
As discussed in #106, a UDP socket bound to 0.0.0.0 results in pathological behavior when there are multiple interfaces of the same address family. This change binds a separate instance of the UDP service to each interface IP, ensuring that outbound Shadowsocks packets have the expected source IP.
@database64128 Would #109 work for your purpose? That implementation is perhaps less memory-efficient than yours, but it's entirely safe, platform-independent, and seems easier to reason about. I think the overhead (which should be much less than 1 MB of RAM) is not anything to worry about. |
The unsafe part in this PR is exactly the same as in wireguard-go. Thank you for your proposed changes, but I'm going to close this PR, since I have been maintaining and using my own fork for a long time. Shadowsocks-NET/outline-ss-server contains this fix, as well as a few other changes:
I'm planning to introduce some protocol changes in my fork. Backwards compatibility is just not a priority for me. My goals include changing key derivation method to drop sha1 and md5, adding timestamps to header to allow for proper replay protection, new optional UDP mode with handshake so we don't have to derive a new session key for every UDP packet, new optional UDP-over-TCP mode. |
Previously, outline-ss-server simply writes the UDP response, which gets routed via the default gateway.
This behavior becomes a problem, when a server has multiple IP addresses of the same address family. Response UDP packets may get routed via a different interface or IP than the incoming packets. At the client's point of view, the outgoing UDP socket received packets from another IP. Most firewalls simply drop such packets.
This commit borrows the idea from https://github.com/WireGuard/wireguard-go/blob/master/conn/bind_linux.go, where IP_PKTINFO and IPV6_PKTINFO are used to determine the interface and IP where the incoming packets are from. Then we send the response using the information as socket out-of-band data to inform the kernel how to route the packets.
Currently it's only implemented for Linux. Implementing it for other platforms is simply not worth the effort, in my opinion.