Re: [Openvpn-devel] [PATCH] Allow "setenv opt" to be pushed from server to client

[Selva Nair]

Hi,

On Sat, Oct 29, 2016 at 9:27 AM, Jan Just Keijser  wrote:

> That apart, NAK to this patch as its not correct
>
> I'll see if I can refactor the patch - but your suggestion to put it at
> the top of add_option did not work for me initially.


Its not about refactoring. The patch simply causes definition of an env
variable named "opt" and does not treat what follows "setenv opt" as an
option. To handle the option correctly, setenv and opt should be stripped
before the parsing starts as is done when processed from config file.

Regards,

Selva
--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Allow "setenv opt" to be pushed from server to client

[Jan Just Keijser]

Hi,

On 28/10/16 19:50, Selva Nair wrote:

Hi,

On Fri, Oct 28, 2016 at 6:27 AM, Jan Just Keijser > wrote:


---
 src/openvpn/options.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)


As Arne pointed out the utility of allowing setenv opt in push is 
questionable. "setenv opt bad-option" in config file logs a warning 
and continues. "push bad-option" logs an error and continues. The end 
result is that both avoids fatal errors with unrecognized/unknown 
options.




the desired effect of the patch is to not print an ERROR but a warning 
instead. For a sysadmin of a large scale deployment *any* method to 
reduce the number of false errors/warnings is an advantage.
Also, I see no reason to NOT add this patch, as it does not introduce 
any vulnerabilities, but it only makes the config file behaviour versus 
"pushed" behaviour more consistent. Abusing "setenv" to add an optional 
option to a config file is a kludge at any rate - might as well make the 
kludge pushable ;)




That apart, NAK to this patch as its not correct

I'll see if I can refactor the patch - but your suggestion to put it at 
the top of add_option did not work for me initially.


JJK
--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Allow "setenv opt" to be pushed from server to client

[Selva Nair]

Hi,

On Fri, Oct 28, 2016 at 6:27 AM, Jan Just Keijser  wrote:

> ---
>  src/openvpn/options.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
>
As Arne pointed out the utility of allowing setenv opt in push is
questionable. "setenv opt bad-option" in config file logs a warning and
continues. "push bad-option" logs an error and continues. The end result is
that both avoids fatal errors with unrecognized/unknown options.

That apart, NAK to this patch as its not correct


> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 281ef0b..dbb926d 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -5616,7 +5616,14 @@ add_option (struct options *options,
>  }
>else if (streq (p[0], "setenv") && p[1] && !p[3])
>  {
> -  VERIFY_PERMISSION (OPT_P_GENERAL);
>

First, "setenv opt" has nothing to do with setenv, so its better not
handled here.


> +  if (streq (p[1], "opt"))
> +   {
> +  VERIFY_PERMISSION (OPT_P_SETENV);
>

This will only result in defining/redefining an env variable "opt = xxx"
which is not what is desired.

+   }
> +  else
> +   {
> +  VERIFY_PERMISSION (OPT_P_GENERAL);
> +   }
>if (streq (p[1], "REMOTE_RANDOM_HOSTNAME") && !p[2])
> {
>   options->sockflags |= SF_HOST_RANDOMIZE;
>

If we really want to allow push "setenv opt ..", the way to do this would
be at the top of the function add_option(), where the words "setenv" and
"opt" are removed by "p += 2" so that the rest gets parsed as a normal
option at a non-fatal msglevel. Make that apply even when pull_mode is
true. Also the man page needs update/correction: remove "--setenv" from
allowed push options list and add "--setenv opt".

Even then its no better than a simple push unless msglevel_fc is changed to
a high verbosity level than M_WARN as well.

Selva
--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Allow "setenv opt" to be pushed from server to client

[Jan Just Keijser]

Hi Arne,

On 28/10/16 13:08, Arne Schwabe wrote:
> Hm,
>
>
> I would like to see a rationale why this is needed. The client will
> already only warn on unsupported options. Your patch would make push
> "setenv opt unsupported" similar to "push unsupported".
the rationale behind this is based on an email thread on openvpn-users:

if I can set
   setenv opt UNSUPPORTED=1
in a client config, then why can't I push that from the server? it would allow 
an admin to do stuff like
   push "setenv opt register-dns"
which would apply only to the clients that support it (Windows) whereas no 
warnings are logged on other platforms. So it's more 
a way to suppress a warning message than anything else.  In theory you could 
also use "pull-filter" for this but that would 
require a change on all clients (and it won't work in 2.3 clients).

cheers,

JJK


--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Allow "setenv opt" to be pushed from server to client

[Arne Schwabe]

Hm,


I would like to see a rationale why this is needed. The client will
already only warn on unsupported options. Your patch would make push
"setenv opt unsupported" similar to "push unsupported".


Arne

--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel