Add early FreeBSD, NetBSD, OpenBSD support #37

neilalexander · 2018-03-01T12:38:32Z

This adds support for OpenBSD (tested) and also possibly FreeBSD (untested, but builds without error).

With some very minor tweaks, this code could also potentially support NetBSD, DragonflyBSD and Solaris.

songgao

Thanks for working on BSD support! It'd be exciting to have them. I don't think what's currently in the PR would work on its own though. Could you please further explain you setup?

params_bsd.go

ipv4_test.go

if.go

songgao · 2018-03-01T22:29:01Z

syscalls_bsd.go

+ "os"
+)
+
+const bsdTUNSIFINFO = (0x80000000) | ((4 & 0x1fff) << 16) | uint32(byte('t'))<<8 | 91


If this is from a BSD source, please kindly mention the original header name and quote some important definitions, so that it's easier to understand if somebody else needs to change this.

I will look to add appropriate comments for it.

songgao · 2018-03-01T22:31:29Z

syscalls_bsd.go

+ MTU int16
+ Type uint8
+ Dummy uint8
+}


Do these fields need to be exported?

Actually it seems lines above this are not used at all. I was imagining you needed to make some syscalls to setup the tun/tap device. In your use case, do you call external commands to set them up? I think if that's the preferred use case, you wouldn't need this package at all. You can just use os.OpenFile directly.

I see there the second commit removed a ioctl function. Did you attempt to implement it first and removed it? I think it'd be nice to have syscalls handled in Go!

I do appear to have left this definition in by accident, as this would be handled elsewhere in my code (with the help of FD() above).

I did attempt to implement an ioctl function although then realised that other platforms do not necessarily have this implemented either, hence removal.

then realised that other platforms do not necessarily have this implemented either, hence removal.

I think all three platforms that this package has support for already handle syscalls properly. They might be structured in different ways because of different underlying designs of the OS APIs, but the goal is a user of the package doesn't need to make syscalls before or after creating the interface using this package.

I imagine the OpenBSD version could be similar to syscalls_darwin.go and syscalls_linux.go.

neilalexander · 2018-03-01T23:51:59Z

I have addressed the change reviews, including reverting the ipv4_test.go import.

The only exception is the definition of FD(), which I have left as-is. This has allowed simple access to the descriptor for later ioctls, even through an Interface{}.

I'd appreciate your thoughts.

Arceliar · 2018-03-04T20:29:24Z

@songgao @neilalexander It looks like we can use fd := iface.ReadWriteCloser.(*os.File).Fd(), so the interface changes aren't required and have been removed.

neilalexander · 2018-05-09T16:21:41Z

@songgao Do you mind re-reviewing?

FD() was removed and we have since added support for selecting between multiple tap0901 adapters on Windows.

neilalexander · 2018-11-17T11:15:14Z

Do you mind to re-review?

songgao · 2018-11-17T22:16:17Z

Sorry for the lack of response. You seem to have removed all the BSD syscalls, and the BSD portion of this PR is now basically a wrapper around os.OpenFile, which doesn't provide much value. In other words, one could just use os.OpenFile directly instead of this package.

As I mentioned in a comment above, all of three other platform supports take care of those syscalls setup. Do you have OpenBSD code using this package running somewhere? I assume some syscall/iotl are required there. If so moving the system calls into this PR would be great!

songgao

Some initial feedback on the Windows code here. Also, would you mind splitting it into a separate PR? It's different enough from the BSD code. Also please run golint on the code.

cc @lixin9311 for Windows code.

songgao · 2018-11-17T22:18:16Z

syscalls_windows.go

@@ -170,12 +170,33 @@ func getdeviceid(componentID string) (deviceid string, err error) {
 key.Close()
 continue
 }
+ if len(interfaceName) > 0 {
+ key2 := `SYSTEM\CurrentControlSet\Control\Network\{4D36E972-E325-11CE-BFC1-08002BE10318}\`+val+`\Connection`


The SYSTEM...318} part is also used above. Could probably make it a const and reuse?

songgao · 2018-11-17T22:19:39Z

params_windows.go

@@ -11,6 +11,10 @@ type PlatformSpecificParams struct {
 // use the default ComponentId. The default ComponentId is set to tap0901,
 // the one used by OpenVPN.
 ComponentID string
+ // Of course, you may have multiple tap0901 adapters on the system, in which
+ // case we need a friendlier way to identify them. In that case we can use
+ // the friendly name of the network adapter as set in Control Panel.


Please make this comment start with InterfaceName, and explain what this parameter is. Otherwise it won't pass linter.

songgao · 2018-11-17T22:21:59Z

params_windows.go

@@ -27,6 +31,7 @@ type PlatformSpecificParams struct {
 func defaultPlatformSpecificParams() PlatformSpecificParams {
 return PlatformSpecificParams{
 ComponentID: "tap0901",
+ InterfaceName: "",


"" is the default value of string, so it's not necessary to set this here.

songgao · 2018-11-17T22:29:55Z

syscalls_windows.go

+ key2 := `SYSTEM\CurrentControlSet\Control\Network\{4D36E972-E325-11CE-BFC1-08002BE10318}\`+val+`\Connection`
+ k2, err := registry.OpenKey(registry.LOCAL_MACHINE, key2, registry.READ)
+ if err != nil {
+ fmt.Println("Unable to open", key2)


Remove this line? Same as the fmt.Println below. Reason is that code using this package likely has stdout for other stuff, and writing into it without a way to turn it off doesn't seem like a good idea.

soffokl · 2019-03-07T06:21:03Z

@neilalexander @songgao Do you have plans to continue working on this PR?
I depend on using multiple tap0901 on Windows at that moment.
And it will be good to keep it in the main project instead of a fork.

I can continue working on it if you don't want to finish it in the scope of the PR.

neilalexander · 2019-03-07T09:22:37Z

Hi @soffokl, I don't have the time at the moment to refactor the PR to satisfy @songgao's comments. I'd like to get back to it soon. However our fork at yggdrasil-network/water supports named Windows interfaces for now.

songgao · 2019-03-07T16:10:45Z

@soffokl That’d be fantastic. Thanks!

…erface config yet)

neilalexander · 2019-07-20T10:04:31Z

I've updated this PR for now to bring it back in line with your master branch and to include the bare-bones needed for FreeBSD/NetBSD/OpenBSD support, which is just the ReadWriteCloser.

The syscalls are not here at the moment because I have not got them working in my own code so far for BSD, so I will submit a later PR to add those.

I would like to propose we merge this to make the library at half-way usable on these BSD platforms and then I can bring my own repository back into sync with yours (when #67 and #68 are also merged) for submitting future changes.

songgao · 2019-08-06T17:19:47Z

Sorry for dropping the ball here! Will look soon

songgao

Thanks Neil; Have you had more updates on the syscall part? I'd love to have at least a usable tun or tap device working before merging into master. Otherwise it feels a bit confusing.

songgao · 2019-08-11T21:19:11Z

params_bsd.go

+package water
+
+// PlatformSpecificParams defines parameters in Config that are specific to
+// Linux. A zero-value of such type is valid, yielding an interface


s/Linux/BSD

aki237 · 2020-08-08T17:43:06Z

@songgao Seems like tun implementation is little different from other (FreeBSD, NetBSD or DFly) tun implementations.

Each packet read or written is prefixed with a tunnel header consisting of a 4-byte network byte order integer containing the address family.

From their manpage (3rd Paragraph of the description)

Should this package handle the packet header or is it upto the application to remove it?

songgao requested changes Mar 1, 2018

View reviewed changes

neilalexander added a commit to yggdrasil-network/water that referenced this pull request Mar 1, 2018

Updates to PR songgao#37

509b830

neilalexander force-pushed the pr branch from 29558d3 to 78c5d97 Compare May 9, 2018 16:20

neilalexander changed the title ~~OpenBSD (and possibly FreeBSD) support~~ Add BSD support and ability to select specific tap0901 on Windows May 9, 2018

songgao reviewed Nov 17, 2018

View reviewed changes

soffokl mentioned this pull request Mar 14, 2019

Add support for Windows to allow selecting between multiple tap0901 adapters on the same system #59

Merged

Initial FreeBSD/OpenBSD/NetBSD support (doesn't have syscalls for int…

a31ac06

…erface config yet)

neilalexander force-pushed the pr branch from 78c5d97 to a31ac06 Compare July 20, 2019 10:00

neilalexander changed the title ~~Add BSD support and ability to select specific tap0901 on Windows~~ Add early FreeBSD, NetBSD, OpenBSD support Jul 20, 2019

songgao reviewed Aug 11, 2019

View reviewed changes

This was referenced Dec 17, 2019

Add support for FreeBSD #77

Open

Unbreak building for FreeBSD slackhq/nebula#103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add early FreeBSD, NetBSD, OpenBSD support #37

Add early FreeBSD, NetBSD, OpenBSD support #37

neilalexander commented Mar 1, 2018

songgao left a comment

songgao Mar 1, 2018

neilalexander Mar 1, 2018

songgao Mar 1, 2018

songgao Mar 1, 2018

neilalexander Mar 1, 2018

songgao Mar 4, 2018

neilalexander commented Mar 1, 2018

Arceliar commented Mar 4, 2018

neilalexander commented May 9, 2018

neilalexander commented Nov 17, 2018

songgao commented Nov 17, 2018 •

edited

songgao left a comment

songgao Nov 17, 2018

songgao Nov 17, 2018

songgao Nov 17, 2018

songgao Nov 17, 2018

soffokl commented Mar 7, 2019

neilalexander commented Mar 7, 2019 •

edited

songgao commented Mar 7, 2019

neilalexander commented Jul 20, 2019

songgao commented Aug 6, 2019

songgao left a comment

songgao Aug 11, 2019

aki237 commented Aug 8, 2020 •

edited

Add early FreeBSD, NetBSD, OpenBSD support #37

Are you sure you want to change the base?

Add early FreeBSD, NetBSD, OpenBSD support #37

Conversation

neilalexander commented Mar 1, 2018

songgao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilalexander commented Mar 1, 2018

Arceliar commented Mar 4, 2018

neilalexander commented May 9, 2018

neilalexander commented Nov 17, 2018

songgao commented Nov 17, 2018 • edited

songgao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soffokl commented Mar 7, 2019

neilalexander commented Mar 7, 2019 • edited

songgao commented Mar 7, 2019

neilalexander commented Jul 20, 2019

songgao commented Aug 6, 2019

songgao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aki237 commented Aug 8, 2020 • edited

songgao commented Nov 17, 2018 •

edited

neilalexander commented Mar 7, 2019 •

edited

aki237 commented Aug 8, 2020 •

edited