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

Update client and server to allow forwarding IPv6 packets within the tunnel #65

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

chris-hellberg
Copy link

  1. Adds a new -S flag to both iodine and iodined to indicate desire to forward IPv6 packets within the tunnel
  2. If V6 tunnel forwarding is requested (i.e. forward IPv6 packets within the tunnel and the tunnel interface comes up, add an IPv6 address to the interface.
    a) The IPv6 address is the same as the IPv4 tunnel address but in the form ::/64. The mask is always /64 and is configured on the interface using ifconfig/ipconfig (WINDOWS32 ipconfig support pending).
  3. Fix a bug with getopt() where the last option in the list of options that don't take an argument was being ignored.
  4. The patch does not explicitly block IPv6 forwarding across the tunnel. So connecting to an automatically generated link-local address at the other end, i.e. fe80::x should work. This allows users to install iodine regardless of underlying OS IPv6 support and not break anything (as long as -S is not specified on the command line).
  5. Tested on both LINUX and DARWIN. Not tested on WINDOWS32.

@chris-hellberg
Copy link
Author

One further comment. One of the commits was to increment the version number to 0.8.0. I reverted that change so CHANGELOG text is the same as the HEAD version of the file.

@yarrick
Copy link
Owner

yarrick commented Jan 5, 2022

Thanks, I will take a closer look at this. Some early comments:

I think it can be worth supporting any IPv6 address - it could either be site local or maybe even a global one for easy routing? We dont want to require NAT for v6.

Does the client really need to control the v6 forwarding? Are there drawbacks in letting the server control it? Or maybe default the client to support it in case the server supplies an address.

The commit history is kind of messy, I would prefer having it cleaner (fixing this can wait until it is closer to merging)

It looks like this is not changing the protocol - just inspecting the packet header to handle the packet types. That makes sense for data packets since there are no unused bits in the current headers. Allowing any /64 network would require some changes in setup though - and that should be documented (and probably tag a new version).

I prefer having a util method for checking ip version instead of comparing with 64 in different places.

The unrelated getopt fixes would fit better as a separate change.

@chris-hellberg
Copy link
Author

chris-hellberg commented Jan 6, 2022

Thanks for your initial review. Comments:

I think it can be worth supporting any IPv6 address - it could either be site local or maybe even a global one for easy routing? We dont want to require NAT for v6.

Yes, this is a good idea. I'll add this in to allow specification of a V6 address on the server to dish out to clients.

Does the client really need to control the v6 forwarding? Are there drawbacks in letting the server control it? Or maybe default the client to support it in case the server supplies an address.

No, not really. I had thought of this as graceful in case a client doesn't have V6 enabled on an interface but for ease of use we can just implicitly check for V6 and enable it if something like the OS successfully (i.e. no need to wait for DAD to complete in case of duplicate link-local address for auto-assignment) tries to start IPv6 on the interface.

The commit history is kind of messy, I would prefer having it cleaner (fixing this can wait until it is closer to merging)

I am not very git fluent so I wasn't sure how to make a pull request with a single commit to your repo. I have a couple of private repos that I commit updates from, hence the messy history.

It looks like this is not changing the protocol - just inspecting the packet header to handle the packet types. That makes sense for data packets since there are no unused bits in the current headers. Allowing any /64 network would require some changes in setup though - and that should be documented (and probably tag a new version).

Question for you: On the client, how do you currently handle unsupported options sent from the server? Say the server assigns an IPv6 address to a client, which is not yet updated for V6 support, does that get silently dropped and life carries on? I remember reading that having mismatching iodine protocol versions is not supported so I suppose we would insist that both client and server must be V6-capable for V6-assignment to work.

Answering your question about /64 masks, upon further reflection, I think it doesn't matter what mask is specified, neither on the server nor the client. We would set the mask as /128 on the client tun interface since I think it's a /32 mask currently on the IPv4 world and leave the routing up to the user. On the server we would assign whatever mask the user specifies and let iodine do the multiplexing to the clients.

I prefer having a util method for checking ip version instead of comparing with 64 in different places.

Good point. I will add. The alternative is to check the ether-type in the wrapper but I think this might introduce more issues than it's worth.

The unrelated getopt fixes would fit better as a separate change.

No problem. Will revert.

@yarrick
Copy link
Owner

yarrick commented Jan 11, 2022

Question for you: On the client, how do you currently handle unsupported options sent from the server? Say the server assigns an IPv6 address to a client, which is not yet updated for V6 support, does that get silently dropped and life carries on? I remember reading that having mismatching iodine protocol versions is not supported so I suppose we would insist that both client and server must be V6-capable for V6-assignment to work.

Most new features have been initiated from the client so we have been able to avoid such issues. If backwards compatibility is tricky then bumping the protocol version is the best way (it will require the client to update before connection is accepted).

Maybe we can signal the server if the client did set up IPv6 so that it would only send such packets if they would be handled correctly on the other end.

Answering your question about /64 masks, upon further reflection, I think it doesn't matter what mask is specified, neither on the server nor the client. We would set the mask as /128 on the client tun interface since I think it's a /32 mask currently on the IPv4 world and leave the routing up to the user. On the server we would assign whatever mask the user specifies and let iodine do the multiplexing to the clients.

I think the normal setup is /27 to fit all the clients in the same network - clients should be able to communicate if logged in at the same time. I think hardcoding a /64 mask for IPv6 is reasonable, any packets would still get sent to the server since it is a point-to-point connection.

@yarrick
Copy link
Owner

yarrick commented Jan 11, 2022

I saw the comment about MTU, that is an interesting limitation. It is quite common to have MTU sizes just around or below the IPv6 minimum. Maybe iodine must do fragmentation of the downstream data for this to work well.

@chris-hellberg
Copy link
Author

chris-hellberg commented Jan 12, 2022

I saw the comment about MTU, that is an interesting limitation. It is quite common to have MTU sizes just around or below the IPv6 minimum. Maybe iodine must do fragmentation of the downstream data for this to work well.

On Linux, it is a (unhelpfully) silent condition if the interface MTU is set < MIN_IPV6_MTU of 1280 bytes. All traces of IPv6 from the interface disappear. I would expect Linux to drop packets when trying to select an outgoing interface whose MTU is below the minimum MTU since there is no longer any IPv6 forwarding enabled there. I can understand where fragmentation would happen if the packet is larger than an outgoing interface whose MTU is 1280 < x < PACKET_SIZE bytes. Maybe other OSs are more graceful - I'll check my darwin environment.

If we really have to forward packets smaller than 1280 bytes, we could look at a scheme whereby the tap interface does Ethernet encapsulation rather than IP (i.e. use tap instead of tun on linux) and do some kind of unnumbering to a loopback interface (think of how the bridge interfaces work), which has the IPv4 and IPv6 addresses. But is there a use case for such small MTUs? I can understand small packets but small MTUs?

@yarrick
Copy link
Owner

yarrick commented Jan 12, 2022

I looked a bit more at the code. The tunnel MTU is decided by the server, and is 1130 by default. We can increase the MTU to 1280 when using IPv6.

The downstream fragment size is then probed on login, and splitting larger packets is already implemented (max 16 slices). So the MTU issue should be fine, and is not related to the downstream fragment size.

@chris-hellberg
Copy link
Author

I've made several updates to the patch but I'm not so good at driving git. In any case, here are the changes:

  • updated the README to include IPv6
  • allowed someone to assign an IPv6 prefix on the server whose covering IPv6 subnet that will be allocated to clients
  • enabled the client to silently probe IPv6 capability on the server and if successful, assign the allocated IPv6 address to the tunnel interface

I've tested on freebsd, openbsd, netbsd, macos and linux (arm).

@chris-hellberg
Copy link
Author

Any thoughts on the submitted patches? Thanks

@yarrick
Copy link
Owner

yarrick commented Jun 4, 2022

Since IPv6 wont accept smaller MTUs (minimum 1280), doesn't this mean that the tunnel MTU often has to be higher than the actual one (what fits in a raw NULL response) so that both IPv4 and IPv6 need fragmenting and become much slower than when just IPv4 is used?

@chris-hellberg
Copy link
Author

chris-hellberg commented Jun 4, 2022 via email

@@ -141,6 +141,14 @@ to your DNS setup. Extending the example above would look like this:
t1ns IN A 10.15.213.99
t1ns IN AAAA 2001:db8::1001:99

On the server, specify -S followed by an IPv6 address that will be the server end
Copy link
Owner

Choose a reason for hiding this comment

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

flag changes should be reflected in manpages also

@@ -101,6 +103,7 @@ static time_t lastdownstreamtime;
static long send_query_sendcnt = -1;
static long send_query_recvcnt = 0;
static int hostname_maxlen = 0xFF;
static bool use_v6 = false;
Copy link
Owner

Choose a reason for hiding this comment

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

use int instead (also in other changes)

@@ -13,6 +13,8 @@
* WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
* ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*
Copy link
Owner

Choose a reason for hiding this comment

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

undo

@@ -37,5 +37,5 @@ void client_set_hostname_maxlen(int i);
int client_handshake(int dns_fd, int raw_mode, int autodetect_frag_size,
int fragsize);
int client_tunnel(int tun_fd, int dns_fd);

int handshake_check_v6(int tun_fd);
Copy link
Owner

Choose a reason for hiding this comment

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

this is an internal function, can you move it so it doesn't need to be declared here?

@@ -557,3 +559,17 @@ fd_set_close_on_exec(int fd)
}
#endif

bool
isV6AddrSet(struct in6_addr *ip6)
Copy link
Owner

Choose a reason for hiding this comment

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

follow naming convention. also, can this replaced by using the comparison function vs a constant?

#ifdef WINDOWS32 /* WINDOWS32 */

/* Set device as connected */
fprintf(stderr, "Enabling interface '%s'\n", if_name);
Copy link
Owner

Choose a reason for hiding this comment

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

Is this really needed? Already done when setting IPv4

Looks like a straight copy from tun_setip?


int v;

v = first_byte & 0xf0;
Copy link
Owner

Choose a reason for hiding this comment

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

Shift right by 4, and just return that?

int i;
char v6AddrOut[32];

inet_ntop(AF_INET6, v6Addr, v6AddrOut, INET6_ADDRSTRLEN);
Copy link
Owner

Choose a reason for hiding this comment

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

is this used?

ip6_netmask[i6] |= 0xFF;
}

ip6_netmask[netbits6 / 8] = pow(2, (netbits6 % 8 )) - 1;
Copy link
Owner

Choose a reason for hiding this comment

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

this seems complicated, can it be made simpler?

{
int i;

for (i = 0; i < 16; i++) {
Copy link
Owner

Choose a reason for hiding this comment

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

Use memcmp instead? Return int instead of bool

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.

2 participants