search

openwrt / packages

Community maintained packages for OpenWrt. Documentation for submitting pull requests is in CONTRIBUTING.md
GNU General Public License v2.0
3.94k stars 3.45k forks source link

luci-app-openvpn: data_ciphers / data-ciphers option does not work correctly #23876

Open tievolu opened 5 months ago

tievolu commented 5 months ago

Maintainer: @systemcrash Environment: OpenWrt 23.05.3 x64

Description:

The data_ciphers / data-ciphers option added in this commit doesn't seem to work correctly. After adding this option in LuCI and saving the changes, the data_ciphers option gets added to /etc/config/openvpn, but this doesn't result in the data-ciphers option being added to the openvpn .conf file that's actually used to create the server.

I think this is because the openvpn package expects data_ciphers to be specified as a list, not an option:

https://github.com/openwrt/packages/blob/020d925f66739f242bf6043d59aef712ac7584b3/net/openvpn/files/openvpn.config#L264-L268

systemcrash commented 5 months ago

OK. What does it result in?

tievolu commented 5 months ago

Adding the option in LuCI causes an option to appear in /etc/config/ciphers like this:

option data_ciphers 'AES-128-GCM:AES-128-CBC'

But nothing at all is added to the OpenVPN conf file that's used to launch the server (/tmp/etc/openvpn-[instancename].conf). I assume that's because the OpenVPN OpenWrt package doesn't understand the option in the form above. I think it's expecting something like this instead:

list data_ciphers 'AES-128-GCM'
list data_ciphers 'AES-128-CBC'
systemcrash commented 5 months ago

Weird. https://github.com/OpenVPN/openvpn/blob/32e6586687a548174b88b64fe54bfae6c74d4c19/sample/sample-config-files/server.conf#L99

@egc112 any ideas? Perhaps change to a List or DynamicList and split those up?

tievolu commented 5 months ago

FYI I just confirmed that manually adding the following to /etc/config/openvpn works:

list data_ciphers 'AES-128-GCM'
list data_ciphers 'AES-128-CBC'

When I restart the openvpn service, the data-ciphers option has been added to the .conf file:

data-ciphers AES-128-GCM:AES-128-CBC
egc112 commented 5 months ago

Weird. https://github.com/OpenVPN/openvpn/blob/32e6586687a548174b88b64fe54bfae6c74d4c19/sample/sample-config-files/server.conf#L99

@egc112 any ideas? Perhaps change to a List or DynamicList and split those up?

No idea at this moment but not a Luci guru but will take a look tomorrow

egc112 commented 5 months ago

Ok had some time to look into it and can see what the problem is (and possibly the solution) Full report will follow

egc112 commented 5 months ago

Here is my take: The data_ciphers addition was based on the ncp_ciphers (which is actually obsolete) but it appears there was already a bug in the ncp_ciphers. Both ncp_ciphers and data_ciphers are Values, but openvpn.options (which describes the options ) expects a dynamic list.

We can either format ncp-ciphers and data-ciphers as a dynamic list or change the options to be a Value. I favour setting both as a Value as it is good practice to use a list of available ciphers in order of preference, which the standard Value does.

@tievolu can you run some tests for me to see if this solves this?

Before you do make a backup of your router, make sure you have a working sysupgrade file and in your openvpn configs delete existing data-ciphers and ncp-ciphers.

Attached is a new openvpn.options download the file and remove the .txt extension. openvpn.options.txt

openvpn.options can be found on the router in /usr/share/openvpn/ rename the existing file to openvpn.options.org and/or copy to your PC Copy the downloaded new openvpn.options to /usr/share/openvpn/ and check if it indeed has data_ciphers and ncp_ciphers listed under OPENVPN_PARAMS='

If this is correct reboot the router and check if you can get it working

tievolu commented 5 months ago

@egc112 - The test fix described in the previous comment works for me.

egc112 commented 5 months ago

@tievolu thanks for reporting back

@systemcrash I can make a pull request, I have just compile tested on my DL-WRX36 and it also works Attached the patch. Any comment or recommended course of action? openvpn-options-3.patch

egc112 commented 5 months ago

On second thoughts, it might brake existing configurations. Alternative is to just format ncp- and data-ciphers as a dynamic list in luci. See attached patch luci-openvpn-data-ciphers-dynamic-list-1.patch

systemcrash commented 5 months ago

Sure - looks acceptable. Bump poly to the first position.

If you're feeling adventurous, maybe you could convert this old rust-bucket to JS? In the meantime, submit a PR with those changes.

egc112 commented 5 months ago

Sure - looks acceptable. Bump poly to the first position.

If you're feeling adventurous, maybe you could convert this old rust-bucket to JS? In the meantime, submit a PR with those changes.

I actually had poly in the first position , but I changed that. My line of thought was that users might just add one cipher (the first) and Poly is not the most used yet, although it is arguably the best. Will put poly first again and will make a PR.

This is a rust-bucket indeed, I do not know many people using it, just uploading your own config works way easier. JS is not my field of expertise but if I have more time later in the year I might take a stab at it.

@tievolu, thanks for reporting, @systemcrash thanks for helping out

systemcrash commented 4 months ago

should be fixed by https://github.com/openwrt/luci/commit/f6301561e709433b8602264fa00495c4aeb70ad3

So this can be closed.