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

fix detecting new connection bug when ppp connections are existed. #90

Merged
merged 1 commit into from
Jan 28, 2017

Conversation

Mabin-J
Copy link
Contributor

@Mabin-J Mabin-J commented Jan 17, 2017

When ppp connections are existed before connect with openfortivpn, openfortivpn use only 'ppp0'.
So, after connect vpn with openfortivpn that situation, routing table get tangled.

There is also same situation when connecting one more vpn with only openfortivpn.

So, I changed checking new connection logic.

@Mabin-J Mabin-J force-pushed the fix-multi-connect branch 2 times, most recently from a1d5b3b to 37a2db4 Compare January 17, 2017 04:36
mrbaseman added a commit to mrbaseman/openfortivpn that referenced this pull request Jan 18, 2017
@mrbaseman
Copy link
Collaborator

I have created a testing branch and at a first glance this doesn't break things in my setup. I'm also interested in running openfortivpn over an LTE modem connection which itself uses ppp0 already. However, I haven't tested that, yet.

@Mabin-J
Copy link
Contributor Author

Mabin-J commented Jan 19, 2017

I fix 'ppp_interface_is_up' with my code base for get interface's address type.
(because original logic return earlier than my logic)

int ppp_interface_is_up(struct tunnel *tunnel)
{
        struct ifaddrs *ifap, *ifa;

        log_debug("Got Address: %s\n", inet_ntoa(tunnel->ipv4.ip_addr));

        if (getifaddrs(&ifap)) {
                log_error("getifaddrs: %s\n", strerror(errno));
                return 0;
        }

        for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
                log_debug("Interface Name: %s\n", ifa->ifa_name);
                if (&(ifa->ifa_addr->sa_family) != NULL){
                        log_debug("Interface's Address Type: %d\n", ifa->ifa_addr->sa_family);
                } else {
                        log_debug("Interface's Address Type: NULL\n");
                }

                if (strstr(ifa->ifa_name, "ppp") != NULL
                    && ifa->ifa_flags & IFF_UP) {
                        if (&(ifa->ifa_addr->sa_family) != NULL
                            && ifa->ifa_addr->sa_family == AF_INET) {
                                struct in_addr if_ip_addr =
                                        ((struct sockaddr_in *) ifa->ifa_addr)->sin_addr;

                                log_debug("Interface Addr: %s\n", inet_ntoa(if_ip_addr));

                                if (tunnel->ipv4.ip_addr.s_addr == if_ip_addr.s_addr) {
                                        strncpy(tunnel->ppp_iface, ifa->ifa_name,
                                                ROUTE_IFACE_LEN - 1);
                                        freeifaddrs(ifap);
                                        return 1;
                                }
                        }
                }
        }
        freeifaddrs(ifap);

        return 0;
}

and I connected one fortivpn and try to connect one more fortivpn.
This is debug message of second one.

DEBUG:  Got Address: XXX.XXX.XXX.XX1
DEBUG:  Interface Name: lo
DEBUG:  Interface's Address Type: 17
DEBUG:  Interface Name: enp4s0f0
DEBUG:  Interface's Address Type: 17
DEBUG:  Interface Name: wlp3s0
DEBUG:  Interface's Address Type: 17
DEBUG:  Interface Name: wlx10feed2529d9
DEBUG:  Interface's Address Type: 17
DEBUG:  Interface Name: br0
DEBUG:  Interface's Address Type: 17
DEBUG:  Interface Name: vboxnet0
DEBUG:  Interface's Address Type: 17
DEBUG:  Interface Name: ppp0
DEBUG:  Interface's Address Type: NULL
DEBUG:  Interface Name: ppp1
DEBUG:  Interface's Address Type: NULL
DEBUG:  Interface Name: lo
DEBUG:  Interface's Address Type: 2
DEBUG:  Interface Name: br0
DEBUG:  Interface's Address Type: 2
DEBUG:  Interface Name: ppp0
DEBUG:  Interface's Address Type: 2
DEBUG:  Interface Addr: XXX.XXX.XXX.XX0
DEBUG:  Interface Name: ppp1
DEBUG:  Interface's Address Type: 2
DEBUG:  Interface Addr: XXX.XXX.XXX.XX1
INFO:   Interface ppp1 is UP.

If use original code, 'ppp_interface_is_up' select 'ppp0' because check only interface name with 'ppp' this point.

DEBUG:  Interface Name: ppp0
DEBUG:  Interface's Address Type: NULL

I think that order of interface list of 'getifaddrs' function's return value can be different each machine. so I think that checking address is more correct than interface name.

@Mabin-J
Copy link
Contributor Author

Mabin-J commented Jan 19, 2017

@mrbaseman
Sorry. I thought you said that original logic is run normally caused my low english skill...
^^;;;;;

@mrbaseman
Copy link
Collaborator

On the weekend I found the time to test this with my lte modem. Without your patch openfortivpn uses ppp0 for both, the modem traffic uplink and the vpn channel, which of course does not work.
With your patch it correctly allocates a ppp1 device but fails to set up routes:

INFO: Connected to gateway.
INFO: Authenticated.
INFO: Remote gateway has allocated a VPN.
INFO: Got addresses: [10.212.XXX.YYY], ns [192.168.XXX.YYY, 8.8.8.8]
INFO: Got addresses: [10.212.XXX.YYY], ns [192.168.XXX.YYY, 8.8.8.8]
INFO: Got addresses: [10.212.XXX.YYY], ns [192.168.XXX.YYY, 8.8.8.8]
INFO: Interface ppp1 is UP.
INFO: Setting new routes...
WARN: Could not set route to tunnel gateway (Invalid argument).
INFO: Adding VPN nameservers...
INFO: Tunnel is up and running.
ERROR: read: Input/output error
INFO: Cancelling threads...
INFO: Setting ppp interface down.
INFO: Restoring routes...
WARN: Could not delete route through tunnel (No such device).
WARN: Could not delete route to gateway (No such process).
INFO: Removing VPN nameservers...
INFO: Terminated pppd.
INFO: Closed connection to gateway.
INFO: Logged out.

it might be due to the fact that my Fortigate pushes a default route throgh the vpn channel (to ensure that the vpn also encrypts my surf traffic when I'm on an insecure network) - or I might have missed the latest commit (I'm not sure anymore, when I have pulled the source that I have used for testing). I'll re-test and give you an update.

@mrbaseman
Copy link
Collaborator

ah, I just noticed that I have probably merged PR #88 into the code I was using for testing and I might have hit a problem with that code or some interference between the two pull requests. I definitely have to re-test.

@adrienverge
Copy link
Owner

Hi, this looks good!

I'd be happy to merge once it is reported to work.

@mrbaseman
Copy link
Collaborator

I don't have split routes configured on my fortigate, That means all traffic goes through the vpn connection. When I manually add a host route over ppp0 for my fortigate before starting openfortivpn this works for me. The traffic of the vpn channel goes over ppp0 and everything else is routed over the new ppp1. When split routes is configured this patch should work out of the box but I haven't tested that yet. It would be nice if the host route could be added automatically when starting openfortivpn.

@mrbaseman
Copy link
Collaborator

I have just verified that when no default route is pushed by the fortigate this patch works fine. Adding a host route over ppp0 in the other case could be added in a separate commit.

When ppp connections are existed before connect with openfortivpn,
openfortivpn use only 'ppp0'.  So, after connect vpn with openfortivpn
that situation, routing table get tangled.

There is also same situation when connecting one more vpn with only
openfortivpn.

So, I changed checking new connection logic.
@adrienverge
Copy link
Owner

Thanks a lot @Mabin-J and @mrbaseman for testing!

@adrienverge adrienverge merged commit c8b2180 into adrienverge:master Jan 28, 2017
mrbaseman added a commit to mrbaseman/openfortivpn that referenced this pull request Jan 31, 2017
mrbaseman added a commit to mrbaseman/openfortivpn that referenced this pull request Feb 7, 2017
This issue has been discussed in adrienverge#94 (set up routes correctly when ppp0 interface is already there) as a follow-up for adrienverge#90 which implemented that a new ppp device is used when ppp0 is already there.
- in ipv4_set_split_routes: add a host route to the gateway (basically copy&paste code from ipv4_set_default_routes)
- in ipv4_set_split_routes: merge in the two fixes for issue adrienverge#25 from adrienverge#88:
   1) add the gateway flag to the routes received from the remote side. Without the gateway flag the destination is directly connected (over the vpn tunnel), which is not always the case. There might be more hops behind the remote gateway to reach the network to which this route points.
   2) correct the test if the route has already a gateway set or not: -1 casted to an unsigned int would result in UINT_MAX which is 255.255.255.255 in IP notation. Routes without a gateway however carry the gateway IP 0.0.0.0 which is 0 written as uint.
- in ipv4_set_default_routes: change the route flag to host instead of gateway for the route to the Fortigate. This is important when the existing default route is over a ppp device, because in that case the gateway flag can not be set on linux. Moreover, the gateway flag is not needed here at all, because the vpn traffic is not routed via this ip as a gateway in the sense of a routing instance. The traffic is sent through the vpn channel and the encrypted packages are sent to the remote IP which can be seen as a single host that unpacks the encrypted traffic afterwards.
- in ipv4_restore_routes: remove the host route to the gateway which is now added in gateway mode, and properly free resources
@Mabin-J Mabin-J deleted the fix-multi-connect branch February 15, 2017 08:44
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