connman.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/2] Improve OpenVPN configuration value processing
@ 2021-11-16 15:14 Jussi Laakkonen
  2021-11-16 15:14 ` [PATCH v1 1/2] vpn-provider: Support checking if provider setting key exists Jussi Laakkonen
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jussi Laakkonen @ 2021-11-16 15:14 UTC (permalink / raw)
  To: connman

Rework how OpenVPN processes the configuration values by introducing a
more clear way to classify different types of options. This makes it possible
to add options that may have a value or can be used without a value thus,
resorting to default value set for the option. Also the --auth-user-pass
and alike options having a special value are handled in cleaner way.

Motivation for this was that the --comp-lzo option was not processed
correctly with its value, and even though being listed as deprecated some
OpenVPN providers use the value. Omitting this option even though it was
required caused issues with both UDP and TCP VPNs:
  write to TUN/TAP : Invalid argument (code=22)
after which OpenVPN silently restarts itself.

Jussi Laakkonen (2):
  vpn-provider: Support checking if provider setting key exists.
  openvpn: Improve configuration value processing

 vpn/plugins/openvpn.c | 119 ++++++++++++++++++++++++++----------------
 vpn/vpn-provider.c    |   6 +++
 vpn/vpn-provider.h    |   2 +
 3 files changed, 83 insertions(+), 44 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v1 1/2] vpn-provider: Support checking if provider setting key exists.
  2021-11-16 15:14 [PATCH v1 0/2] Improve OpenVPN configuration value processing Jussi Laakkonen
@ 2021-11-16 15:14 ` Jussi Laakkonen
  2021-11-16 15:14 ` [PATCH v1 2/2] openvpn: Improve configuration value processing Jussi Laakkonen
  2021-11-18 21:02 ` [PATCH v1 0/2] Improve OpenVPN " Daniel Wagner
  2 siblings, 0 replies; 6+ messages in thread
From: Jussi Laakkonen @ 2021-11-16 15:14 UTC (permalink / raw)
  To: connman

Add a function to check if the setting key exists in the setting hash
table.
---
 vpn/vpn-provider.c | 6 ++++++
 vpn/vpn-provider.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/vpn/vpn-provider.c b/vpn/vpn-provider.c
index 4bb6c24e..cc325967 100644
--- a/vpn/vpn-provider.c
+++ b/vpn/vpn-provider.c
@@ -2937,6 +2937,12 @@ bool vpn_provider_get_string_immutable(struct vpn_provider *provider,
 	return setting->immutable;
 }
 
+bool vpn_provider_setting_key_exists(struct vpn_provider *provider,
+							const char *key)
+{
+	return g_hash_table_contains(provider->setting_strings, key);
+}
+
 void vpn_provider_set_auth_error_limit(struct vpn_provider *provider,
 							unsigned int limit)
 {
diff --git a/vpn/vpn-provider.h b/vpn/vpn-provider.h
index ee09e8ef..5d1455da 100644
--- a/vpn/vpn-provider.h
+++ b/vpn/vpn-provider.h
@@ -88,6 +88,8 @@ int vpn_provider_set_boolean(struct vpn_provider *provider, const char *key,
 							bool force_change);
 bool vpn_provider_get_boolean(struct vpn_provider *provider, const char *key,
 							bool default_value);
+bool vpn_provider_setting_key_exists(struct vpn_provider *provider,
+							const char *key);
 void vpn_provider_set_auth_error_limit(struct vpn_provider *provider,
 							unsigned int limit);
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v1 2/2] openvpn: Improve configuration value processing
  2021-11-16 15:14 [PATCH v1 0/2] Improve OpenVPN configuration value processing Jussi Laakkonen
  2021-11-16 15:14 ` [PATCH v1 1/2] vpn-provider: Support checking if provider setting key exists Jussi Laakkonen
@ 2021-11-16 15:14 ` Jussi Laakkonen
  2021-11-18 21:02 ` [PATCH v1 0/2] Improve OpenVPN " Daniel Wagner
  2 siblings, 0 replies; 6+ messages in thread
From: Jussi Laakkonen @ 2021-11-16 15:14 UTC (permalink / raw)
  To: connman

Improve how the configuration options are processed in OpenVPN. Add a
config option type that is providing better granularity in how the
options should be processed when adding to the task, which eventually
sets them to the binary. Drop the has_value option as unnecessary.

This now allows to add string variables that have a default value when
they are used alone by setting the "opt_type" to OPT_STRING. And adding
the option without a value.

Furthermore, support also setting an option to null for a string option
if the option string value matches the special value set in the array.
This is the case for example with --auth-user-pass where the value "-"
for OpenVPN.AuthUserPass indicates that the password should be queried
from VPN agent using the management interface.
---
 vpn/plugins/openvpn.c | 119 ++++++++++++++++++++++++++----------------
 1 file changed, 75 insertions(+), 44 deletions(-)

diff --git a/vpn/plugins/openvpn.c b/vpn/plugins/openvpn.c
index e1962942..761474de 100644
--- a/vpn/plugins/openvpn.c
+++ b/vpn/plugins/openvpn.c
@@ -56,36 +56,48 @@
 
 static DBusConnection *connection;
 
+enum opt_type {
+	OPT_NONE = 0,
+	OPT_STRING = 1,
+	OPT_BOOL = 2,
+};
+
 struct {
-	const char *cm_opt;
-	const char *ov_opt;
-	char       has_value;
+	const char	*cm_opt;
+	const char	*ov_opt;
+	const char	*ov_opt_to_null;
+	enum opt_type	opt_type;
 } ov_options[] = {
-	{ "Host", "--remote", 1 },
-	{ "OpenVPN.CACert", "--ca", 1 },
-	{ "OpenVPN.Cert", "--cert", 1 },
-	{ "OpenVPN.Key", "--key", 1 },
-	{ "OpenVPN.MTU", "--tun-mtu", 1 },
-	{ "OpenVPN.NSCertType", "--ns-cert-type", 1 },
-	{ "OpenVPN.Proto", "--proto", 1 },
-	{ "OpenVPN.Port", "--port", 1 },
-	{ "OpenVPN.AuthUserPass", "--auth-user-pass", 1 },
-	{ "OpenVPN.AskPass", "--askpass", 1 },
-	{ "OpenVPN.AuthNoCache", "--auth-nocache", 0 },
-	{ "OpenVPN.TLSRemote", "--tls-remote", 1 },
-	{ "OpenVPN.TLSAuth", NULL, 1 },
-	{ "OpenVPN.TLSAuthDir", NULL, 1 },
-	{ "OpenVPN.TLSCipher", "--tls-cipher", 1},
-	{ "OpenVPN.Cipher", "--cipher", 1 },
-	{ "OpenVPN.Auth", "--auth", 1 },
-	{ "OpenVPN.CompLZO", "--comp-lzo", 0 },
-	{ "OpenVPN.RemoteCertTls", "--remote-cert-tls", 1 },
-	{ "OpenVPN.ConfigFile", "--config", 1 },
-	{ "OpenVPN.DeviceType", NULL, 1 },
-	{ "OpenVPN.Verb", "--verb", 1 },
-	{ "OpenVPN.Ping", "--ping", 1},
-	{ "OpenVPN.PingExit", "--ping-exit", 1},
-	{ "OpenVPN.RemapUsr1", "--remap-usr1", 1},
+	{ "Host", "--remote", NULL, OPT_STRING},
+	{ "OpenVPN.CACert", "--ca", NULL, OPT_STRING},
+	{ "OpenVPN.Cert", "--cert", NULL, OPT_STRING},
+	{ "OpenVPN.Key", "--key", NULL, OPT_STRING},
+	{ "OpenVPN.MTU", "--tun-mtu", NULL, OPT_STRING},
+	{ "OpenVPN.NSCertType", "--ns-cert-type", NULL, OPT_STRING},
+	{ "OpenVPN.Proto", "--proto", NULL, OPT_STRING},
+	{ "OpenVPN.Port", "--port", NULL, OPT_STRING},
+	/*
+	 * If the AuthUserPass option is "-", provide the input  via management
+	 * interface. To facilitate this set the option as NULL.
+	 */
+	{ "OpenVPN.AuthUserPass", "--auth-user-pass", "-", OPT_STRING},
+	{ "OpenVPN.AskPass", "--askpass", NULL, OPT_STRING},
+	{ "OpenVPN.AuthNoCache", "--auth-nocache", NULL, OPT_BOOL},
+	{ "OpenVPN.TLSRemote", "--tls-remote", NULL, OPT_STRING},
+	{ "OpenVPN.TLSAuth", NULL, NULL, OPT_NONE},
+	{ "OpenVPN.TLSCipher", "--tls-cipher", NULL, OPT_STRING},
+	{ "OpenVPN.TLSAuthDir", NULL, NULL, OPT_NONE},
+	{ "OpenVPN.Cipher", "--cipher", NULL, OPT_STRING},
+	{ "OpenVPN.Auth", "--auth", NULL, OPT_STRING},
+	/* Is set to adaptive by default if value is omitted */
+	{ "OpenVPN.CompLZO", "--comp-lzo", NULL, OPT_STRING},
+	{ "OpenVPN.RemoteCertTls", "--remote-cert-tls", NULL, OPT_STRING},
+	{ "OpenVPN.ConfigFile", "--config", NULL, OPT_STRING},
+	{ "OpenVPN.DeviceType", NULL, NULL, OPT_NONE},
+	{ "OpenVPN.Verb", "--verb", NULL, OPT_STRING},
+	{ "OpenVPN.Ping", "--ping", NULL, OPT_STRING},
+	{ "OpenVPN.PingExit", "--ping-exit", NULL, OPT_STRING},
+	{ "OpenVPN.RemapUsr1", "--remap-usr1", NULL, OPT_STRING},
 };
 
 struct ov_private_data {
@@ -355,29 +367,48 @@ static int ov_save(struct vpn_provider *provider, GKeyFile *keyfile)
 static int task_append_config_data(struct vpn_provider *provider,
 					struct connman_task *task)
 {
-	const char *option;
 	int i;
 
 	for (i = 0; i < (int)ARRAY_SIZE(ov_options); i++) {
-		if (!ov_options[i].ov_opt)
-			continue;
+		const char *ov_opt = ov_options[i].ov_opt;
+		const char *cm_opt = ov_options[i].cm_opt;
+		const char *option = NULL;
 
-		option = vpn_provider_get_string(provider,
-					ov_options[i].cm_opt);
-		if (!option)
+		switch (ov_options[i].opt_type) {
+		case OPT_NONE:
 			continue;
 
-		/*
-		 * If the AuthUserPass option is "-", provide the input
-		 * via management interface
-		 */
-		if (!strcmp(ov_options[i].cm_opt, "OpenVPN.AuthUserPass") &&
-						!strcmp(option, "-"))
-			option = NULL;
+		case OPT_STRING:
+			if (!ov_opt)
+				continue;
+
+			option = vpn_provider_get_string(provider, cm_opt);
+			/*
+			 * A string option may be used alone without a value
+			 * in which case the default value is used by OpenVPN.
+			 */
+			if (!option && !vpn_provider_setting_key_exists(
+							provider, ov_opt))
+				continue;
+
+			const char *opt_to_null = ov_options[i].ov_opt_to_null;
+			if (opt_to_null && !g_strcmp0(option, opt_to_null))
+				option = NULL;
+
+			break;
+
+		case OPT_BOOL:
+			if (!ov_opt)
+				continue;
+
+			/* Ignore the boolean toggle if option is disabled. */
+			if (!vpn_provider_get_boolean(provider, cm_opt, false))
+				continue;
+
+			break;
+		}
 
-		if (connman_task_add_argument(task,
-				ov_options[i].ov_opt,
-				ov_options[i].has_value ? option : NULL) < 0)
+		if (connman_task_add_argument(task, ov_opt, option))
 			return -EIO;
 
 	}
-- 
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 0/2] Improve OpenVPN configuration value processing
  2021-11-16 15:14 [PATCH v1 0/2] Improve OpenVPN configuration value processing Jussi Laakkonen
  2021-11-16 15:14 ` [PATCH v1 1/2] vpn-provider: Support checking if provider setting key exists Jussi Laakkonen
  2021-11-16 15:14 ` [PATCH v1 2/2] openvpn: Improve configuration value processing Jussi Laakkonen
@ 2021-11-18 21:02 ` Daniel Wagner
  2021-11-19  8:09   ` Jussi Laakkonen
  2 siblings, 1 reply; 6+ messages in thread
From: Daniel Wagner @ 2021-11-18 21:02 UTC (permalink / raw)
  To: Jussi Laakkonen; +Cc: connman

Hi Jussi,

On Tue, Nov 16, 2021 at 05:14:15PM +0200, Jussi Laakkonen wrote:
> Rework how OpenVPN processes the configuration values by introducing a
> more clear way to classify different types of options. This makes it possible
> to add options that may have a value or can be used without a value thus,
> resorting to default value set for the option. Also the --auth-user-pass
> and alike options having a special value are handled in cleaner way.
> 
> Motivation for this was that the --comp-lzo option was not processed
> correctly with its value, and even though being listed as deprecated some
> OpenVPN providers use the value. Omitting this option even though it was
> required caused issues with both UDP and TCP VPNs:
>   write to TUN/TAP : Invalid argument (code=22)
> after which OpenVPN silently restarts itself.

Both patches applied after fixing up a warning:

vpn/plugins/openvpn.c:394:25: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]                              
  394 |                         const char *opt_to_null = ov_options[i].ov_opt_to_null;                                                             
      |                         ^~~~~                                                                                                               

Daniel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 0/2] Improve OpenVPN configuration value processing
  2021-11-18 21:02 ` [PATCH v1 0/2] Improve OpenVPN " Daniel Wagner
@ 2021-11-19  8:09   ` Jussi Laakkonen
  2021-11-19  8:30     ` Daniel Wagner
  0 siblings, 1 reply; 6+ messages in thread
From: Jussi Laakkonen @ 2021-11-19  8:09 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: connman

Hi Daniel,

On 11/18/21 11:02 PM, Daniel Wagner wrote:
> Hi Jussi,
> 
> On Tue, Nov 16, 2021 at 05:14:15PM +0200, Jussi Laakkonen wrote:
>> Rework how OpenVPN processes the configuration values by introducing a
>> more clear way to classify different types of options. This makes it possible
>> to add options that may have a value or can be used without a value thus,
>> resorting to default value set for the option. Also the --auth-user-pass
>> and alike options having a special value are handled in cleaner way.
>>
>> Motivation for this was that the --comp-lzo option was not processed
>> correctly with its value, and even though being listed as deprecated some
>> OpenVPN providers use the value. Omitting this option even though it was
>> required caused issues with both UDP and TCP VPNs:
>>    write to TUN/TAP : Invalid argument (code=22)
>> after which OpenVPN silently restarts itself.
> 
> Both patches applied after fixing up a warning:
> 

Thanks.

> vpn/plugins/openvpn.c:394:25: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
>    394 |                         const char *opt_to_null = ov_options[i].ov_opt_to_null;
>        |                         ^~~~~
> 

Oops, sorry. My build env did not complain about that so I guess I 
overlooked that bit.

Cheers,
  Jussi

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 0/2] Improve OpenVPN configuration value processing
  2021-11-19  8:09   ` Jussi Laakkonen
@ 2021-11-19  8:30     ` Daniel Wagner
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Wagner @ 2021-11-19  8:30 UTC (permalink / raw)
  To: Jussi Laakkonen; +Cc: connman

Hi Jussi,

On Fri, Nov 19, 2021 at 10:09:27AM +0200, Jussi Laakkonen wrote:
> Oops, sorry. My build env did not complain about that so I guess I
> overlooked that bit.

No worries. My normal build didn't catch it either. I have a commit hook
which builds any patch I add. This one triggered.

Cheers,
Daniel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-11-19  8:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-16 15:14 [PATCH v1 0/2] Improve OpenVPN configuration value processing Jussi Laakkonen
2021-11-16 15:14 ` [PATCH v1 1/2] vpn-provider: Support checking if provider setting key exists Jussi Laakkonen
2021-11-16 15:14 ` [PATCH v1 2/2] openvpn: Improve configuration value processing Jussi Laakkonen
2021-11-18 21:02 ` [PATCH v1 0/2] Improve OpenVPN " Daniel Wagner
2021-11-19  8:09   ` Jussi Laakkonen
2021-11-19  8:30     ` Daniel Wagner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).