Page MenuHomeVyOS Platform
Create Task
Maniphest T3350

OpenVPN config file generation broken
Closed, ResolvedPublicBUG
Actions

Description

Following this change https://phabricator.vyos.net/T2381 the OpenVPN config file is now broken and the process crash if the option string contains '--'.

Here is an example of a config directive we use:

openvpn-option "plugin /config/openvpn_cc_script.so "/config/scripts/script.sh --cache-dir /tmp/cache check --gateway \\"Gateway 1\\" /dev/null""

In my opinion no parsing should be made on this string and must remain raw.
Also the leading '--' is unnecessary in the config file.

Details

Difficulty level
Hard (possibly days)
Version
1.3-beta
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Config syntax change (non-migratable)
Issue type
Bug (incorrect behavior)

Related Objects

Event Timeline

remfalc created this task.Feb 24 2021, 2:19 PM
remfalc updated the task description. (Show Details)Feb 24 2021, 2:22 PM
pasik added a subscriber: pasik.Feb 24 2021, 2:25 PM
Viacheslav triaged this task as Normal priority.Mar 17 2021, 1:40 PM
Viacheslav changed Difficulty level from Easy (less than an hour) to Normal (likely a few hours).
Viacheslav added a subscriber: Viacheslav.EditedMar 17 2021, 5:18 PM

Proposed changes
Add hook/magic word string and write values in one string if word string in options is present

diff --git a/data/templates/openvpn/server.conf.tmpl b/data/templates/openvpn/server.conf.tmpl
index 79288e40..42b5b6ad 100644
--- a/data/templates/openvpn/server.conf.tmpl
+++ b/data/templates/openvpn/server.conf.tmpl
@@ -281,10 +281,14 @@ compat-names
 # Custom options added by user (not validated)
 #
 {%   for option in openvpn_option %}
+{%     if 'string ' in option %}
+{{ option | replace('string ', '') }}
+{%     else %}
 {%     for argument in option.split('--') %}
 {%       if argument is defined and argument != '' %}
 --{{ argument }}
 {%       endif %}
 {%     endfor %}
+{%     endif %}
 {%   endfor %}
 {% endif %}

Command

set interfaces openvpn vtun10 openvpn-option 'string plugin /config/openvpn_cc_script.so "/config/scripts/script.sh --cache-dir /tmp/cache check --gateway \\"Gateway 1\\" /dev/null"'

Config

vyos@r5# cat /run/openvpn/vtun10.conf | tail -n4
#
# Custom options added by user (not validated)
#
plugin /config/openvpn_cc_script.so "/config/scripts/script.sh --cache-dir /tmp/cache check --gateway \"Gateway 1\" /dev/null"
[edit]
vyos@r5#
remfalc added a comment.Mar 17 2021, 8:06 PM

I am not sure why these extra options are parsed to begin with. My view is Vyos gives the possibility to write extra config file elements because it is not yet supported by the Vyos system configuration. Therefore it should be up to the user to write the correct data as it was in the past.
There should be absolutely no parsing whatsoever and options must be passed through the file unchanged.

Can you explain the reason you want to keep this parsing logic into the template? It really doesn't make sense to me.

Viacheslav added a comment.Mar 22 2021, 2:50 PM

Another solution it include "user" defined file for options
An example CLI

set interfaces openvpn vtun10 openvpn-option-include '/config/openvpn/included.conf'
diff --git a/data/templates/openvpn/server.conf.tmpl b/data/templates/openvpn/server.conf.tmpl
index 79288e40..bcc88c09 100644
--- a/data/templates/openvpn/server.conf.tmpl
+++ b/data/templates/openvpn/server.conf.tmpl
@@ -288,3 +288,8 @@ compat-names
 {%     endfor %}
 {%   endfor %}
 {% endif %}
+
+# Include file for configuration options
+{% if openvpn_option_include is defined and openvpn_option_include is not none %}
+config {{ openvpn_option_include }}
+{% endif %}
diff --git a/interface-definitions/interfaces-openvpn.xml.in b/interface-definitions/interfaces-openvpn.xml.in
index effbdd67..2cba59af 100644
--- a/interface-definitions/interfaces-openvpn.xml.in
+++ b/interface-definitions/interfaces-openvpn.xml.in
@@ -314,6 +314,14 @@
               <multi/>
             </properties>
           </leafNode>
+          <leafNode name="openvpn-option-include">
+            <properties>
+              <help>Additional OpenVPN file configuration. You must
+                use the syntax of openvpn.conf in this file. Using this
+                without proper knowledge may result in a crashed OpenVPN server.
+                Check system log to look for errors.</help>
+            </properties>
+          </leafNode>
           <leafNode name="persistent-tunnel">
Viacheslav changed Difficulty level from Normal (likely a few hours) to Hard (possibly days).Mar 22 2021, 6:44 PM
zsdc changed the task status from Open to Confirmed.Mar 22 2021, 11:24 PM
zsdc added a subscriber: zsdc.

The root of the problem here is changed place for custom options and the ability to configure options that should be applied differently, depending on the place. In other words, "Additional OpenVPN options" becomes "Additional OpenVPN options. You must use the syntax of openvpn.conf in this text-field", but actually these variants are not fully equal and cannot be converted directly.

Except single value options in the openvpn-option can be configured multiple options inside the single line:

openvpn-option '--tun-mtu 1500 --fragment 1300 --mssfix'

They must be converted to multiple lines inside a config file:

tun-mtu 1500
fragment 1300
mssfix

And very rarely we can see also options like in this task, that must be passed without any modifications.

Unfortunately, it is not possible to confidently detect what type of options is used. Therefore, we must force users to configure options in a way that will not allow multiple approaches.

I see only the next two ways how to get out of this impasse.

  1. Switch back to passing openvpn-option items to the command line. In this case, everything stays as it is and all options will be valid.

This way is ugly because it requires changing the CLI definition again and split config for daemon into two different places.

  1. Use configuration migration to convert all the options into the common format.

This openvpn-option '--tun-mtu 1500 --fragment 1300 --mssfix' will be converted to:

openvpn-option 'tun-mtu 1500'
openvpn-option 'fragment 1300'
openvpn-option 'mssfix'

And switch off any parsing of openvpn-option inside the template, passing them all as is.
In this case, unfortunately, options, like provided in this ticket, will be split improperly and need to be reconfigured again to make them work. But this will be only one-time action. Given that they do not work now at all, this is not perfect but may be acceptable.

zsdc changed Is it a breaking change? from Unspecified (possibly destroys the router) to Config syntax change (non-migratable).Mar 22 2021, 11:41 PM
remfalc added a comment.Mar 23 2021, 4:09 PM
In T3350#90321, @Viacheslav wrote:

Another solution it include "user" defined file for options
An example CLI

set interfaces openvpn vtun10 openvpn-option-include '/config/openvpn/included.conf'
diff --git a/data/templates/openvpn/server.conf.tmpl b/data/templates/openvpn/server.conf.tmpl
index 79288e40..bcc88c09 100644
--- a/data/templates/openvpn/server.conf.tmpl
+++ b/data/templates/openvpn/server.conf.tmpl
@@ -288,3 +288,8 @@ compat-names
 {%     endfor %}
 {%   endfor %}
 {% endif %}
+
+# Include file for configuration options
+{% if openvpn_option_include is defined and openvpn_option_include is not none %}
+config {{ openvpn_option_include }}
+{% endif %}
diff --git a/interface-definitions/interfaces-openvpn.xml.in b/interface-definitions/interfaces-openvpn.xml.in
index effbdd67..2cba59af 100644
--- a/interface-definitions/interfaces-openvpn.xml.in
+++ b/interface-definitions/interfaces-openvpn.xml.in
@@ -314,6 +314,14 @@
               <multi/>
             </properties>
           </leafNode>
+          <leafNode name="openvpn-option-include">
+            <properties>
+              <help>Additional OpenVPN file configuration. You must
+                use the syntax of openvpn.conf in this file. Using this
+                without proper knowledge may result in a crashed OpenVPN server.
+                Check system log to look for errors.</help>
+            </properties>
+          </leafNode>
           <leafNode name="persistent-tunnel">

This approach is very elegant and leave freedom to the user for the most complex customization.

If not possible to do that, I vote for the approach of switching off any parsing for the openvpn-option config directive.

Viacheslav added a comment.Apr 19 2021, 10:40 AM

Workaround set raw option "config /path/to/config/file"

set interfaces openvpn vtun10 openvpn-option 'config /config/openvpn/included.conf'

And declare such raw options in the file "included.conf".

dmbaturin added a project: test.Jul 29 2021, 8:59 AM
Viacheslav mentioned this in T3737: openvpn-option needs to be able to support quotes as since openvpn 2.4..Aug 11 2021, 10:14 AM
syncer edited projects, added VyOS 1.3 Equuleus (1.3.0-epa1); removed test, VyOS 1.3 Equuleus.Sep 5 2021, 7:06 AM
zsdc raised the priority of this task from Normal to Unbreak Now!.Sep 27 2021, 7:14 AM
zsdc set Issue type to Bug (incorrect behavior).
Viacheslav added a comment.Sep 27 2021, 8:31 AM
set interfaces openvpn vtun20 openvpn-option '--push dhcp-option DNS 203.0.113.1'

generated config:

--push dhcp-option DNS 203.0.113.1

expected configuration:

push dhcp-option "DNS 203.0.113.1"
syncer edited projects, added VyOS 1.3 Equuleus (1.3.0); removed VyOS 1.3 Equuleus (1.3.0-epa1).Nov 1 2021, 10:09 PM
c-po added a project: VyOS 1.4 Sagitta.Nov 15 2021, 8:08 PM
c-po closed this task as Resolved.Nov 17 2021, 7:42 PM
c-po assigned this task to zsdc.
c-po moved this task from Need Triage to Finished on the VyOS 1.3 Equuleus (1.3.0) board.
c-po moved this task from Need Triage to Finished on the VyOS 1.4 Sagitta board.