References: <1477650469-32431-1-git-send-email-janjust@...1147...> From: Jan Just Keijser Message-ID: Date: Sat, 29 Oct 2016 15:27:57 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------167332F46A5467F057BA660B" Subject: Re: [Openvpn-devel] [PATCH] Allow "setenv opt" to be pushed from server to client List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Selva Nair Cc: "openvpn-devel@lists.sourceforge.net" This is a multi-part message in MIME format. --------------167332F46A5467F057BA660B Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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 --------------167332F46A5467F057BA660B X-MIME-Autoconverted: from 8bit to quoted-printable by outgoing1-ams.mf.surf.net id u9TDS3c7006322 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable

Hi,

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

Hi,

On Fri, Oct 28, 2016 at 6:27 AM, Jan Just Keijser <janjus= t@...1147...> wrote:

---
=C2=A0src/openvpn/options.c | 9 ++++++++-
=C2=A01 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.=C2=A0

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
--------------167332F46A5467F057BA660B--