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

openvpn: add missing script event options to be used in the openvpn config file #21758

Closed
wants to merge 3 commits into from

Conversation

egc112
Copy link
Contributor

@egc112 egc112 commented Aug 8, 2023

Maintainer: @mkrkn @neheb
Compile tested: aarch64, cortex-a53, OpenWRT 23.05
Run tested: Dynalink DL-WRX36

Description:
A previous commit has added more script event options.
However it looked like that commit was not complete as it stops the use of the script events route-up, route-pre-down, and ipchange when those are placed in the openvpn config file.

This PR fixes a regression that makes it problematic to specify certain event options in the OpenVPN configuration file.

Discussion in this thread and here

Please have a look and consider implementing or make it possible to use all script event options in the openvpn config file in another way.

Pull request has been discussed and improved with the help of @AuthorReflex, see: #21732

Signed-off-by: Erik Conijn egc112@msn.com

@egc112
Copy link
Contributor Author

egc112 commented Aug 8, 2023

I am not allowed to put a label on it (understandably) but can this also be backported to 23.05?

@@ -159,6 +162,9 @@ openvpn_add_instance() {
${client:+--ipchange "/usr/libexec/openvpn-hotplug ipchange $name"} \
Copy link
Contributor

Choose a reason for hiding this comment

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

this line not needed if you redefined new behavior below

Copy link
Contributor Author

@egc112 egc112 Aug 9, 2023

Choose a reason for hiding this comment

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

If we remove it the ipchange action in the hotplug script (/etc/hotplug.d/openvpn/01-user) is never triggered.

The beauty of the solution is that it combines OpenWRT's hotplug actions via the hotplug script but makes it also compatible with the old way of placing script events in the openvpn config file by parsing the openvpn config file and add those scripts to the hotplug actions by setting it as an environmental variable e.g. $user_ipchange.

By removing ${client:+--ipchange "/usr/libexec/openvpn-hotplug ipchange $name"} \ we are breaking this.

Sure the ipchange script in the openvpn config file is executed but by openvpn and not by the hotplug action as it is intended to be.

Copy link
Contributor

@AuthorReflex AuthorReflex Aug 9, 2023

Choose a reason for hiding this comment

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

If parameter specified more once, only last occurence will be accepted to execute. So, even without removing, hotplug event do not executed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we focus on ipchange then we place the --ipchange option on the command line which executes on the ipchange event.
Furthermore we place --setenv on the command line to set an environmental variable with the name of the ipchange script as parsed form the openvpn config.
This ipchange script as set in the openvpn config is not executed by OpenVPN and that is what OpenVPN complains about it actually sees two ipchange script options, the one on the command line and the one placed in the config file.

OpenVPN will only execute the last ipchange option which is the one on the command line.
This one on the command line is setup to use the hotplug script with hotplug actions.
these actions correspond with the script event placed on the command line e.g. --up, --down, --route-up, --route-pre-down, --ipchange, but --ipchange only for client and not for server.

The OpenVPN hotplug script has already an action predefined and that is executing the script as set by the --setenv user_ipchange which contains the name of the ipchange script as parsed from the config file.

So the hotplug script on ipchange action can execute other actions/scripts you place there but it will also automatically execute the script as parsed from the config file so making it compatible with the old way of placing the script events and scripts in the config file.

I have run tests which confirms that it works this way and if I am right we need the ${client:+--ipchange "/usr/libexec/openvpn-hotplug ipchange $name"} \ otherwise it will not trigger the ipchange event in the hotplug script

I make my own builds and have it compiled in so I am happy as it is and if someone thinks it is useful than add it otherwise throw it away.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current code already overrides the options from the config file with command-line options, so OpenVPN ignores the route-up, route-pre-down, and ipchange options in the config file.

The proposed code exports the relevant options as environment variables which are used in the hotplug script to call the action-specific scripts and make the options from the config file work again.

@BKPepe
Copy link
Member

BKPepe commented Aug 8, 2023

Lets ping @miska about this since he introduced those options as you refenced.

Signed-off-by: Erik Conijn <egc112@msn.com>
@ThomasCr
Copy link

ThomasCr commented Oct 20, 2023

I also miss the route-up and route-pre-down option, because I setup extra routing table/rules based on a list of hostnames I translate at runtime and for a specific client on my network. Now I am a little bit lost, that my setup is broken.
Is there any chance, that this get fixed in the next months?

@supersebbo
Copy link

This behaviour introduced in 23.05 has completely broken my VPN routes, when will this fix be merged?

@zorxd
Copy link
Contributor

zorxd commented Nov 4, 2023

A possible workaround is to add the following to /etc/openvpn.user

if [ "$ACTION" == "route-up" ]
then
/sbin/ip route whatever argument here
fi

Maintainer: @mkrkn  @neheb
Compile tested: aarch64, cortex-a53, OpenWRT 23.05
Run tested: Dynalink DL-WRX36

Description:
[A previous commit](f8a8b71) has added more script event options.
However it looked like that commit was not complete as it stops the use of the script events route-up, route-pre-down, and ipchange when those are placed in the openvpn config file.

This PR fixes a regression that makes it problematic to specify certain event options in the OpenVPN configuration file.

Discussion in [this thread](https://forum.openwrt.org/t/openvpn-custom-route-up-script-in-23-05-rc2/167105/13) and [here](https://forum.openwrt.org/t/openvpn-route-up-and-route-pre-down-broken-in-23-05/176568)

Signed-off-by: Erik Conijn <63402314+egc112@users.noreply.github.com>
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.

None yet

7 participants