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

Win32 port: fix udp socket creation, merge args & simulate control pipe #28

Merged
merged 6 commits into from
Oct 12, 2014
Merged

Conversation

linusyang
Copy link
Collaborator

Previous implementation will cause EMSGSIZE issue on Windows since it mistakenly sets the DF flag. Also it is not necessary and has the exact same function as vpn_udp_alloc() in vpn.c to bind the UDP port.

Issue reported by https://www.v2ex.com/t/138108#reply14.

@clowwindy
Copy link
Owner

We may support multiple configurations in the future. So it's better to move global variables to structures. i.e. args_tun_ip => args->tun_ip

@clowwindy
Copy link
Owner

p.s. Which timezone are you in? = =

@linusyang
Copy link
Collaborator Author

@clowwindy This args_tun_ip is the local IP address of the tun device. Currently there is only one to be configured on Windows, so that it is a global variable. (I'm in UTC +8, but sleep a bit late on weekends. :P)

@clowwindy
Copy link
Owner

I see. Still I think we should pass these arguments via the args structure to functions win32.c as other arguments. It will be more difficult to do this later when the code become complicated and messy.

@linusyang
Copy link
Collaborator Author

@clowwindy These arguments (tun ip, netmask and delegate port) are only win32 specific and useless to other platforms.

Tun device of win32 needs to be assigned an IP address to respond ARP requests before using netsh (like ifconfig for unix) to setup. Delegate UDP port is a workaround to use select() to communicate with tun device, as select() in windows doesn't support file handle.

So, I think putting them in win32.c will be safe and not interfere with other platforms. But it depends on your preference and moving them to args is not hard.

@clowwindy
Copy link
Owner

In the future there might be other platform-specific arguments as well. Define all of them on shadowvpn_args_t might be a good practice, making the code easier to read.
Else it's difficult to figure out how many arguments do we have, unless we have read all the code in the configuration file parser.

typedef struct {
  shadowvpn_mode mode;
  shadowvpn_cmd cmd;
  const char *conf_file;
  const char *pid_file;
  const char *log_file;
  const char *intf;
  const char *password;
  const char *server;
  uint16_t port;
  uint16_t mtu;
  const char *up_script;
  const char *down_script;
#ifdef TARGET_WIN32
  // Windows arguments goes here
#endif
#ifdef TARGET_ANDROID
  // Android arguments goes here
#endif
} shadowvpn_args_t;

@linusyang
Copy link
Collaborator Author

True. I agree with you. I'll drop another commit that merges the arguments.

@clowwindy
Copy link
Owner

Thanks :)

@cj1324
Copy link
Collaborator

cj1324 commented Oct 12, 2014

Difference tun_local_ip or tun_remote_ip?
Including tun_netmask.

ifconfig command requires .

const char *tun_local_ip;
const char *tun_remote_ip;  // if server mode equal tun_local_ip
const char *tun_netmask;

@linusyang
Copy link
Collaborator Author

@cj1324 Only on Windows and local TUN ip is needed to be set when opening the TUN device (see my explanation above). So there's no need to add the remote TUN IP address as a configurable argument which can be setup later using command lines.

@cj1324
Copy link
Collaborator

cj1324 commented Oct 12, 2014

@linusyang Execute ifconfig tunX need。 eg:. OSX, FreeBSD

@linusyang
Copy link
Collaborator Author

@cj1324 Yeah, I know. I'm talking about Windows. The Windows TUN/TAP driver must need to set TUN IP address using API but not command line tools, while other platforms can use ifconfig to setup.

@cj1324
Copy link
Collaborator

cj1324 commented Oct 12, 2014

@linusyang i put ifconfig integrated in the code, not the shell , Work in progress ,So it is necessary

@linusyang
Copy link
Collaborator Author

@cj1324 Oh, I see. Actually, I prefer not to use system() or execv() to call command line utilities since they may not exist or have different arguments (BSD vs GNU, or busybox version, etc.). Maybe we can directly use system calls to setup interface and add routes. For example, for Darwin, we can setup an interface like this: https://github.com/linusyang/shadowsocks-libev/blob/darwin-new/src/local.c#L1096

@linusyang
Copy link
Collaborator Author

And this is a bit off topic right now. This page is for a quick patch for current Windows implementation. We may start a new issue page for discussing about integrating other features.

@cj1324
Copy link
Collaborator

cj1324 commented Oct 12, 2014

@linusyang Thanks, I do not consider busybox, I switched to pure c coding.

@linusyang
Copy link
Collaborator Author

@cj1324 Brilliant. Thanks. :)

@clowwindy
Copy link
Owner

I prefer to only provide the tun interface in the C code, and leave the IP address empty, unless the IP address is necessary(i.e. on Windows & Android). Users can do anything they want by setting up their own rules in client_up.sh. Moving ifconfig from client_up.sh to C code seems provides no benefit. Remember our goal is not to write another OpenVPN.

For example, the user may want to assign both an IPv4 address and another IPv6 address to one interface. And (often in a large company) they may want to make use of multiple servers to balance the traffic load by routing traffic from different subnets. The less we assume, the more users can do.

The second reason is that on some systems like OpenWRT, the network settings, including IP addresses, zone forwarding rules, are managed by some other tools. The users may not want to use the scripts at all. They may want to use other tools, like LuCI, or GUI tools on desktop environments to configure the interface. Setting up our own rules may conflict with these tools:

The third reason is that debugging shell scripts is much easier than debugging C code. If people keep asking why our configuration won't work on their environments, and we can only help them by adding some logs and compiling a new version for them, we'll be dead.

The last reason is that I'm working on trying different protocols, since raw UDP seems not the best choice. I hope ShadowVPN will be a simple packet tunnel, with different protocols pluggable. And the new protocol might not be working on IP layer but layer 2 or layer 4. The configurations in client_up.sh and client_down.sh will be very different from what they look like now.

@linusyang
Copy link
Collaborator Author

@clowwindy Sounds reasonable. Keeping simple meets with the UNIX philosophy. Integrating too many features makes the project too heavy to port and hard to maintain.

(BTW, please don't close or merge this pull request now. I will have some other small fixes to be pushed here for the windows port. Thanks.)

@clowwindy
Copy link
Owner

@linusyang
By the way, have you worked on BPF or pcap? With them we can do some fancy stuff, but I'm afraid they are not as fast as sockets.

@linusyang
Copy link
Collaborator Author

@clowwindy Not really. Only tried to use the API of libpcap to modify packets. But I can give it a try if necessary.

@clowwindy
Copy link
Owner

@linusyang
In short: I want to bypass the Unix network stack and send VPN traffic directly over fake TCP packets. You can imagine that they work as normal UDP packets, but look like TCP packets to firewalls.
There're no TCP connections at all. They are just connectionless, stateless datagrams.

@linusyang
Copy link
Collaborator Author

@clowwindy UDP over TCP? Will the performance drop if tunneling through the TCP connection?

@clowwindy
Copy link
Owner

@linusyang Since we bypassed the network stack, those TCP packets do not belong to any TCP connections. They are just datagrams like UDP. They just have a fake TCP header to confuse the firewall.

@linusyang
Copy link
Collaborator Author

@clowwindy Oh, I got it. That's the reason why using bpf or pcap. So this implementation will be built directly on Layer 2? The packet processing library will be very critical to the performance. Or we can simply make it as a kernel module?

@clowwindy
Copy link
Owner

@linusyang

Doing that in the kernel performs best but if we did something wrong users will have kernel panic.

We can use raw sockets to send those fake TCP packets, but I'm afraid we can only using BPF to receive these pakcets and protect them from RST by the OS.

Another way to do this in user space is creating another tun device and writing these packets directly to the tun device(pretending these packets are from another host).

@cj1324
Copy link
Collaborator

cj1324 commented Oct 12, 2014

@clowwindy Keeping simple ok.

@linusyang linusyang changed the title Use vpn_udp_alloc() to bind UDP port for win32 Win32 fixes: fix udp socket creation, merge args & simulate control pipe Oct 12, 2014
@linusyang linusyang changed the title Win32 fixes: fix udp socket creation, merge args & simulate control pipe Windows: fix udp socket creation, merge args & simulate control pipe Oct 12, 2014
@linusyang linusyang changed the title Windows: fix udp socket creation, merge args & simulate control pipe Win32 port: fix udp socket creation, merge args & simulate control pipe Oct 12, 2014
@linusyang
Copy link
Collaborator Author

@clowwindy All recent patches for win32 have been uploaded here. If no further problems after testing, please consider to merge it. Thanks.

clowwindy added a commit that referenced this pull request Oct 12, 2014
Win32 port: fix udp socket creation, merge args & simulate control pipe
@clowwindy clowwindy merged commit 6b380d0 into clowwindy:master Oct 12, 2014
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.

3 participants