-
Notifications
You must be signed in to change notification settings - Fork 294
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
Add windows support. #13
Conversation
"golang.org/x/sys/windows/registry" | ||
) | ||
|
||
var ( |
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.
Perhaps const
would be better than var
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.
Go doesn't have const
functions, so using const
means you can't reuse these bit-wise code through tap_control_code
below.
All those tap_*
are only used within the water
package, so I think it's fine to leave this as var
.
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.
Thanks @lixin9311. This is awesome! Just some questions and suggestions.
Also, I don't have a Windows machine to test this; would you mind adding a ipv4_windows_test.go
like this? That would enable a simple test on Windows.
} | ||
|
||
for _, v := range ifces { | ||
if hwaddr_equal(v.HardwareAddr[:6], mac[:6]) { |
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.
Would you mind using bytes.Equal([]byte(v.HardwareAddr), mac)
here? Then you can also get rid of the hwaddr_equal
func literals above.
|
||
// openDev find and open an interface. | ||
func openDev(isTAP bool) (ifce *Interface, err error) { | ||
// ifName won't work |
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.
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.
Fixed, and it should pass the test in about 4s.
And a little change to ipv4_linux_test.go and ipv4_test.go, please check.
if err != nil { | ||
goto next | ||
} | ||
if val == "tap0901" { |
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 wonder what tap0901
is. Is it a special ID on Windows for all TAP interfaces, or is it like tap0
on unix where the number gets auto-incremented?
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.
It's a driver maker specified value, defined here.
Many other VPN programs are using different values to distinguish their virtual net adapter from OpenVPN.
func getdeviceid() (string, error) { | ||
// TAP driver key location | ||
regkey := `SYSTEM\CurrentControlSet\Control\Class\{4D36E972-E325-11CE-BFC1-08002BE10318}` | ||
k, err := registry.OpenKey(registry.LOCAL_MACHINE, regkey, registry.ALL_ACCESS) |
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 don't know this well enough, but it seems that the code below only reads values but does not update them. Do we still need ALL_ACCESS
here? Is there a READ_ACCESS
or equivalent?
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.
Fixed.
I just found that READ
is enough for this.
And privileged right is not required anymore.
} | ||
// find the one with ComponentId == "tap0901" | ||
for _, v := range keys { | ||
key, err := registry.OpenKey(registry.LOCAL_MACHINE, regkey+"\\"+v, registry.ALL_ACCESS) |
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.
same here
// TAP driver key location | ||
regkey := `SYSTEM\CurrentControlSet\Control\Class\{4D36E972-E325-11CE-BFC1-08002BE10318}` | ||
k, err := registry.OpenKey(registry.LOCAL_MACHINE, regkey, registry.ALL_ACCESS) | ||
if err != nil { |
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.
It looks like it errors here when the TAP driver is not installed. Would you mind checking the error here and return a more descriptive error? Perhaps something like this:
switch(err) {
case nil:
case XXXKeyNotFound: // whatever error indicating the key is not found in registry
return nil, NoTAPDriverInstalled // add this error above
default:
return nil, nil
}
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 can only roughly check the err, because it will go deeply to the system errorno.
key, err := registry.OpenKey(registry.LOCAL_MACHINE, regkey+"\\"+v, registry.ALL_ACCESS) | ||
if err != nil { | ||
continue | ||
} |
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 think if you do something like this here, you can avoid all the goto next
in this loop:
defer func(k registry.Key) {
k.Close()
}(key)
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.
Fixed
@lixin9311 also feel free to add yourself to the end of the CONTRIBUTORS file! |
Refined the code. |
Forgot to say, the Linux example code works very well on windows. |
Thanks @lixin9311 ! I'll make a few more small changes before merging it. Also, I'm a bit reluctant on introducing those error types. Do you think we can un-export them? I can't think of a scenario where a developer would want to confirm those errors and handle differently. Introducing Windows-specific (exported) errors means we have different interface on different platforms. We need to be careful on introducing exported types, as once you have it in master, it'd be hard to remove them since you never know if somebody is using the type in the wild. What do you think? (you don't have to remove them; I'm just asking for opinions) |
I think you can unexport them, and include underlying error in the error message. I think if a developer really want to use this on windows, he will definitely handle the error message on himself. I can only provide rough information, such as "driver may be not installed", because you never know what weird errors can come out from Windows. |
Sorry; By "handle the error message himself" did you mean the developer needs to know which one of the three errors it is, or he/she needs to generate the message based on use case and these types wouldn't matter much? |
I mean, we can provide a rough information, like "driver may not be installed", and include more underlying detailed, then let the developers grep the message themselves. |
I see. Thanks! Can there be only one virtual adapter on Windows? What if you are creating the interface for the second time? |
TUN/TAP driver work differently on Windows from Linux. So the code I've added is more like |
And for your question, if try to open an opened device, it will fail here(syscalls_windows.go#L102). |
Is OpenVPN's GUI the only place to install new adaptors? Is there an API to do it? On Linux/macOS this library creates an interface and attaches to it. It also cleans up automatically when the device is closed. It'd be nice to have the same semantics on Windows if possible. Probably should be a different PR though. |
I'm afraid to say, the virtual adapter is persisted on Windows, unless you uninstall the driver. Actually, there is a standalone installer of the driver, you can find it at the bottom of this page. Also, developers can build their own driver from the source code. |
Ah I see. I guess we'll have to accept the fact and document it then. Thanks for the details! |
I think you might try it on a Windows machine if you have chance and time. |
Yup; will do! |
|
One thing I want to discuss, about the way creating TUN device on Windows. Upon an L3 communication, the TAP driver will reply the ARP with its own MAC address, as you can see here.
See If we use a different address from the address we provided when creating the TUN devices, there will be address unreachable, because there is no ARP reply and no L3 packet. If we add more arguments for windows, to ask users to provide an address when creating TUN devices on Windows, I think it will break the consistency. Any suggestions? |
|
OK, now, the only question is how to keep consistency due to the TUN issue. |
Thanks @lixin9311 ! Sorry I hadn't merged this. The ARP thing looks like normal packet handling for me. With TUN/TAP device, user of the library should be able to handle the packets themselves. Traditionally
Good point! Part of the reason I haven't merged this is actually that I was thinking about providing a new interface for setting up TUN/TAP devices, that has platform specific parameters. I'd also like to allow user to set the driver ID. This way, applications can use their own network adapter device on Windows instead of fighting for the one for OpenVPN. I'll push a PR for that new interface this evening or tomorrow and tag you for review. |
On Linux, a TUN interface is marked with NOARP flag, because it's a PtP
interface.
But Windows doesn't have such design. The TAP driver handles the ARP
request itself, reply if the IP matches the address provided when opening
the device and redirect the packets to itself. So that, user can handle L3
packets as same as the TUN way on Linux.
Good news is, we can reconfigure the IP after opening the device although
the TAP driver cannot perceive IP changes caused by dhcp or iptools.
…On Jan 5, 2017 3:05 AM, "Song Gao" ***@***.***> wrote:
Thanks @lixin9311 <https://github.com/lixin9311> ! Sorry I hadn't merged
this.
The ARP thing looks like normal packet handling for me. With TUN/TAP
device, user of the library should be able to handle the packets
themselves. Traditionally water only handles setting up the device, but
leaves L3 setup to the user. I think IP setup is orthogonal to the TUN/TAP
devices themselves. I suppose we could let the library potentially handle
IP setup for the interface as well, but then we should probably also
provide tools for changing those settings.
If we add more arguments for windows, to ask users to provide an address
when creating TUN devices on Windows, I think it will break the consistency.
Good point! Part of the reason I haven't merged this is actually that I
was thinking about providing a new interface for setting up TUN/TAP
devices, that has platform specific parameters. I'd also like to allow user
to set the driver ID. This way, applications can use their own network
adapter device on Windows instead of fighting for the one for OpenVPN.
I'll push a PR for that new interface this evening or tomorrow and tag you
for review.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAWrE7rjD5vTdEe5TVoOn0BrQ3MGiXoZks5rO99ygaJpZM4LVXek>
.
|
An ARP packet is just a L2 frame. If user choose to use TAP device, she has access to all L2 frames coming in, including the ones containing IP packets, ARP frames, or other types. Thus the user should handle all frames she's interested in, including the ARP ones. The TAP driver itself provides necessary parts for user to setup and use the TAP device, but I don't see how the driver should handle any frames for the user. OpenVPN ships with TAP drivers, but it's also an "application" of the TAP driver. In other words, the VPN consumes frames from the TAP adapter, and writes frames into it. That's why it needs to handle ARP frames in addition to IP traffic. On the other hand, if we choose to provide tools for setting up L3 stuff, like configuring IP for the adapters, I suppose it's useful to provide relevant tools for user to quickly plug into their code and handle ARP automatically. Please let me know if this makes sense! |
Yes, you are right. But I mean, when it comes to a TUN interface, the TAP
driver on Windows will handle ARP requests, and redirect packets to
itself, so that user can see L3 packets in their code.
On Linux, there will be a NOARP flag.
|
If it's a TUN device, there shouldn't be TAP drivers involved right? If I understand correctly, it's the network stack in Windows kernel that handles the ARP. In this case the user of the library works in L3, thus doesn't handle any L2 frames (including ARPs). Or did you mean when a TUN device is used, Windows doesn't automatically handle ARP frames for it? That would be surprising |
Yes, Windows won't handle it. The driver handles it instead. Because Windows is born with no TAP/TUN support, people build an virtual adapter driver to provide such functionalities. |
hmm.. interesting! But Windows has PPP adapters for dial-up networks. Aren't they L3? How does it handle ARP then? Also the code you linked above from OpenVPN is from TAP path. I thought that was when OpenVPN handles ARP frames when TAP driver is used, not when TUN is used. Did I misunderstand? |
TAP and TUN are using the same driver and the same code on Windows. I think PPP on Windows is using another black box driver. |
If we use TUN, the driver handles ARP instead of the Windows kernel. |
If we..
Whether we use the virtual adapter in TAP or TUN mode, Windows will still consider the adapter as a real NIC. Which means the driver will receive and process L2 frames. |
Ah I see. Sorry for totally missing the point earlier! So back to the original discussion, is the problem that if we ever update address on an adapter, the TUN/TAP driver wouldn't get notified, and when responding ARP it still assumes the old address? |
Yes, unless we reconfigure it like |
Hi @lixin9311, I suggest you to migrate to use |
Hi @LionNatsu, thank you for your suggestion, but I've encountered some bugs in my recent project with |
Sounds good; I think we can keep using |
To use it on windows, you need a tap driver, or just install OpenVPN.
Refined 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.
Rebased and updated
// For detail, please refer | ||
// https://github.com/OpenVPN/tap-windows6/blob/master/src/device.c#L431 | ||
// and https://github.com/songgao/water/pull/13#issuecomment-270341777 | ||
Network string |
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 added a new field to help creating a TUN interface.
@@ -143,6 +144,68 @@ You'd see the ICMP packets printed out: | |||
2016/10/23 20:22:40 Packet Received: 00 00 00 02 45 00 00 54 4a 2e 00 00 40 01 1c 5c 0a 01 00 0a 0a 01 00 14 08 00 31 51 f0 f9 00 00 58 0d 7e 80 00 03 14 21 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 | |||
``` | |||
|
|||
### TAP on Windows: |
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 think we should update the README.
Sorry I forgot to merge this. I'll also change README a little bit to note Windows support is experimental and API might change. (I was hoping the interface for TUN could be better and less cross-layered.) @lixin9311 Thanks again for your help! |
OMG. It takes me the whole night to understand why I should set the extra addresses when creating a TUN device on Windows until I find this discussion. From my limited NDIS driver knowledge, I think there should be some possibility for the driver to get the addresses of the interface. Hope the tap driver for windows (and its doc) could improve a little bit. |
To use it on windows, you need a tap driver
https://github.com/OpenVPN/tap-windows6
or just install OpenVPN.
https://github.com/OpenVPN/openvpn