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

Adding --pppd-keepalive option. #802

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

Conversation

FinboySlick
Copy link

In reference to #92 where lcp-echo-interval would still be useful. Figured if I'm going to ask for a feature, I might as well do some of the work.

@DimitriPapadopoulos
Copy link
Collaborator

Would we need to consider other LCP parameters? This works with pppd on Linux but I cannot find anything similar in ppp on FreeBSD. Do you have any clue?

@FinboySlick
Copy link
Author

Honestly, my initial approach would have been to enable any pppd option via a generic --pppd-opt= type of parameter, but I wasn't familiar enough with the code/intent to take into account the design and security implications (and it was very late).

As such, this patch is just to solve an immediate problem. That being said, I think there would be value in making openfortivpn 'independent' of other config files. Since the only way to do that as far as pppd is concerned is the command line, a '--pppd-opt=' type of solution is worth considering.

@luzik
Copy link

luzik commented Apr 20, 2022

Can we move such a keepalive packet into upper layer ? ICMP broadcast into new interface or something similar ? I believe that tunnel just need any packet to keep tunnel alive.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Apr 20, 2022

I think FortiClient handles dead peer detection that at the PPP level, see for example https://gitlab.com/openconnect/openconnect/-/commit/e1eac267 - but perhaps in upper layers too.

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.

4 participants