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

set up routes correctly when ppp0 interface is already there #94

Closed
mrbaseman opened this issue Jan 30, 2017 · 6 comments
Closed

set up routes correctly when ppp0 interface is already there #94

mrbaseman opened this issue Jan 30, 2017 · 6 comments

Comments

@mrbaseman
Copy link
Collaborator

as discussed in pull request #90 which implemented that a new ppp device is used when ppp0 is already there, I have noticed a problem when the vpn pushes a new default route over the new ppp1 interface.
In that particular case adding the gateway route fails in ipv4_set_default_routes with the following debug output:

DEBUG: ip route add to A.B.C.D/255.255.255.255 via 0.0.0.0 dev ppp0
WARN: Could not set route to tunnel gateway (Invalid argument).

Adding a host route over ppp0 before starting openvpn makes the tunnel work. The above warning still appears then, but the tunnel works. Comparing the routing entries I found that in this case the flags are sligtly different. Instead of
gtw_rt->rt_flags |= RTF_GATEWAY;
one would have to put:
gtw_rt->rt_flags |= RTF_HOST;
However, when I try to open a vpn connection to another gateway, which does not push a default route, the code is needed as it is. This is in the early stage, before the routes etc. are received, so I'm not sure how to handle this decision. Maybe we should add an option to the config file (something like is-gateway or host-route or additional-flags...) which is used here and allows us to control the setup of the routes. Any suggestions? I'm willing to implement and test this, but @adrienverge, I would like to have your opinion which path I should follow.

@Mabin-J
Copy link
Contributor

Mabin-J commented Jan 31, 2017

I think that changing 321st line of ipv4.c is enough for fix issue of warning message because 'ipv4_set_default_routes' function is only called when it is set 'default gateway mode(?)'.

Apart from this issue, I think that adding a route for VPN server is needed when it is set not only 'default gateway mode' but also 'split routes mode(?).'

When I connect 'A.B.C.D' IP of VPN Server that contains 'A.B.C.0/24' route in 'split routing table,' Packets cannot be passed through VPN because 'A.B.C.0/24' route.

So, I have a plan that I'll insert logic that adding a default route for server's IP.

(Maybe my issue is like #25)

@mrbaseman
Copy link
Collaborator Author

@Mabin-J: Thanks for your input. You are right: 'ipv4_set_default_routes' function is only called when the config on the Fortigate contains
set split-tunneling disable
which you call 'default gateway mode'. And you are also right, we have received the configuration already from the Fortigate at this moment, it's just not yet fully processed. So, I agree that changing line 321 to
gtw_rt->rt_flags |= RTF_HOST;
should solve this issue.

I have to re-test. I thought that this would break the 'split routes mode', but you are right, in that case this line is not executed at all. Maybe I had messed up my routing table during my tests yesterday.

I believe we don't need the gateway flag here. Traffic is not routed to the public IP address of the Fortigate, but it is sent through the tunnel.

You have seen Pull request #88 which shall fix the split routes mode? Now that I have a deeper understanding I'll comment on that also.

@adrienverge: Now I'm convinced that we don't need an extra option in the config file.

@mrbaseman
Copy link
Collaborator Author

I forgot to mention two more thoughts:

I agree that there should be a host route to the Fortigate in 'split routes mode' just for the case that it pushes a route that contains its own public IP. If that's not the case, the host route doesn't hurt (except that it increases the size of the routing table by a single entry).

And I wanted to mention that I have already tried
gtw_rt->rt_flags |= ( RTF_GATEWAY | RTF_HOST);
which didn't work when my existing default gateway is ppp0 and the vpn tunnel on ppp1. Somehow it does not like the gateway flag in this case, perhaps because we have copied the whole routing entry from a route to a ppp device already, which due to its point to point nature might behave a bit different than routing entries over other network devices (I haven't examined the other fields of rentry in detail, just noticed that it still throws the invalid argument warning whenever the gateway flag is set).

mrbaseman added a commit to mrbaseman/openfortivpn that referenced this issue Jan 31, 2017
@mrbaseman
Copy link
Collaborator Author

I think I have a working solution now in my ppp-routes branch - I have merged in the related pull requests as well and have successfuly tested with and without split routes over my lte modem (vpn on ppp1) and with split routes over wlan (vpn on ppp0). I hope that I can test default gateway mode over lan (vpn on ppp0) tomorrow.

@mrbaseman
Copy link
Collaborator Author

I have tested my ppp-routes branch again, this time with direct connection over lan and vpn on ppp0, both split routes and gateway mode. It works for all cases I could test and in #88 @Valantin has also reported that it fixes #25 for him. So, I'll open a pull request now and reference the current issue there.

mrbaseman added a commit to mrbaseman/openfortivpn that referenced this issue 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
adrienverge added a commit that referenced this issue Feb 7, 2017
set up routes correctly (pull request for #94)
@mrbaseman
Copy link
Collaborator Author

#95 has been merged. Thanks to @adrienverge

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

No branches or pull requests

2 participants