| Message ID | CAEeEfvzkhp_FQ8cnOZcASiVbDN8=+Gw_2sV5SNonL5x7OYb1jA@mail.gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Sommerseth |
| Headers | show |
| Series | [Openvpn-devel,OpenVPN3] Add 'pull' to ignored options | expand |
Am 27.07.23 um 10:52 schrieb Merten Fermont: > Fixes error that --pull is an unknown option in client config. > --- > openvpn/client/cliopt.hpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp > index f7be44a8..431791f3 100644 > --- a/openvpn/client/cliopt.hpp > +++ b/openvpn/client/cliopt.hpp > @@ -797,6 +797,7 @@ class ClientOptions : public RC<thread_unsafe_refcount> > "mute-replay-warnings", > "nobind", /* only behaviour in v3 client anyway */ > "prng", > + "pull", /* option is implied by 'client' */ > "rcvbuf", /* present in many configs */ > "replay-persist", /* Makes little sense in TLS mode */ > "script-security", While that will work, it would be better to not ignore that option but handle it in a similar way to --client. In general we should actually throw an error if neither client or tls-client+pull are present as OpenVPN3 cannot operate without these in p2p mode. Arne
Hi Arne,
I changed my patch to check the client and client+pull options.
Giving an error when neither options are declared.
This however may break current implementations that depend on 'client'
not being a required option?
Greetings,
Merten
Subject: [PATCH] Check for client options
Require 'client' or 'tls-client'+'pull' to be declared in the config.
To prevent other errors, 'client' option is added when 'tls-client'
and 'pull' are both declared.
Fixes error that --pull is an unknown option.
---
openvpn/client/cliopt.hpp | 1 -
openvpn/client/cliopthelper.hpp | 11 ++++++++++-
openvpn/common/options.hpp | 4 ++--
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp
index f7be44a8..8c52a5c0 100644
--- a/openvpn/client/cliopt.hpp
+++ b/openvpn/client/cliopt.hpp
@@ -801,7 +801,6 @@ class ClientOptions : public RC<thread_unsafe_refcount>
"replay-persist", /* Makes little sense in TLS mode */
"script-security",
"sndbuf",
- "tls-client", /* Always enabled */
"tmp-dir",
"tun-ipv6", /* ignored in v2 as well */
"txqueuelen", /* so platforms evaluate that in tun, some do
not, do not warn about that */
diff --git a/openvpn/client/cliopthelper.hpp b/openvpn/client/cliopthelper.hpp
index 95aa6664..ad3b4445 100644
--- a/openvpn/client/cliopthelper.hpp
+++ b/openvpn/client/cliopthelper.hpp
@@ -367,13 +367,22 @@ class ParseClientConfig
bool added = false;
// client
- if (!options.exists("client"))
+ if (options.exists("client"))
+ {
+ options.touch("tls-client", true);
+ options.touch("pull", true);
+ }
+ else if (options.exists("tls-client") && options.exists("pull"))
{
Option opt;
opt.push_back("client");
options.push_back(std::move(opt));
added = true;
}
+ else
+ {
+ throw option_error("No 'client' or
'tls-client'+'pull' directive declared. Other roles are not
supported.");
+ }
// dev
if (!options.exists("dev"))
diff --git a/openvpn/common/options.hpp b/openvpn/common/options.hpp
index d594c41a..a813647e 100644
--- a/openvpn/common/options.hpp
+++ b/openvpn/common/options.hpp
@@ -1460,11 +1460,11 @@ class OptionList : public std::vector<Option>,
public RCCopyable<thread_unsafe_r
}
// Touch an option, if it exists.
- void touch(const std::string &name) const
+ void touch(const std::string &name, bool lightly = false) const
{
const Option *o = get_ptr(name);
if (o)
- o->touch();
+ o->touch(lightly);
}
// Render object as a string.
--
2.41.0
On Thu, 27 Jul 2023 at 11:14, Arne Schwabe <arne@rfc2549.org> wrote:
>
> Am 27.07.23 um 10:52 schrieb Merten Fermont:
> > Fixes error that --pull is an unknown option in client config.
> > ---
> > openvpn/client/cliopt.hpp | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp
> > index f7be44a8..431791f3 100644
> > --- a/openvpn/client/cliopt.hpp
> > +++ b/openvpn/client/cliopt.hpp
> > @@ -797,6 +797,7 @@ class ClientOptions : public RC<thread_unsafe_refcount>
> > "mute-replay-warnings",
> > "nobind", /* only behaviour in v3 client anyway */
> > "prng",
> > + "pull", /* option is implied by 'client' */
> > "rcvbuf", /* present in many configs */
> > "replay-persist", /* Makes little sense in TLS mode */
> > "script-security",
>
>
> While that will work, it would be better to not ignore that option but
> handle it in a similar way to --client. In general we should actually
> throw an error if neither client or tls-client+pull are present as
> OpenVPN3 cannot operate without these in p2p mode.
>
> Arne
>
Am 27.07.23 um 14:21 schrieb Merten Fermont: > Hi Arne, > > I changed my patch to check the client and client+pull options. > Giving an error when neither options are declared. > > This however may break current implementations that depend on 'client' > not being a required option? > Acked-By: Arne Schwabe <arne@rfc2549.org> Yes. It might. But then again there are people that run into strange problems because these configuration are not really working when switching to an OpenVPN 2.x client. We rather have consistency between clients here. Arne
On 27/07/2023 14:21, Merten Fermont wrote: > Hi Arne, > > I changed my patch to check the client and client+pull options. > Giving an error when neither options are declared. > > This however may break current implementations that depend on 'client' > not being a required option? > > Greetings, > Merten > > Subject: [PATCH] Check for client options > Hi, Thanks for your patch. Changes looks reasonable to me as well, and I wanted to pull it in. But it turns out that the patch has been mangled somehow. Even good old 'patch' refuses to apply anything. This is not an unknown issue with gmail.com; which is why we generally recommend to use 'git send-mail' [1]. In this specific case, resending the patch as an attachment can also work. [1] <https://git-scm.com/docs/git-send-email>
On Tue, Aug 01, 2023 at 08:22:24PM +0200, David Sommerseth wrote: > On 27/07/2023 14:21, Merten Fermont wrote: > > Hi Arne, > > > > I changed my patch to check the client and client+pull options. > > Giving an error when neither options are declared. > > > > This however may break current implementations that depend on 'client' > > not being a required option? > > > > Greetings, > > Merten > > > > Subject: [PATCH] Check for client options > > > > Hi, > > Thanks for your patch. Changes looks reasonable to me as well, and I wanted > to pull it in. But it turns out that the patch has been mangled somehow. > Even good old 'patch' refuses to apply anything. This is not an unknown > issue with gmail.com; which is why we generally recommend to use 'git > send-mail' [1]. > > In this specific case, resending the patch as an attachment can also work. Since I was confused about the state of this patch: It has been superseded by a patch from Arne, see commit https://github.com/OpenVPN/openvpn3/commit/53614a0cce7775ba0ae4a43887ee03aa2fa098cc Also marked it in Patchwork correctly. Regards,
On Fri, Mar 08, 2024 at 10:47:27AM +0100, Merten Fermont wrote: > Hi Frank, > > Arne has indeed applied a fix based on this patch. > > However, it looks like the case from this issue is not completely solved: > https://github.com/OpenVPN/openvpn3/issues/277 > If you have options "client" and "pull" but no "tls-client" in the config, > the "pull" option will not be touched. True, due to short-circuit logic. I will prepare a fix. Regards,
diff --git a/openvpn/client/cliopt.hpp b/openvpn/client/cliopt.hpp index f7be44a8..431791f3 100644 --- a/openvpn/client/cliopt.hpp +++ b/openvpn/client/cliopt.hpp @@ -797,6 +797,7 @@ class ClientOptions : public RC<thread_unsafe_refcount> "mute-replay-warnings", "nobind", /* only behaviour in v3 client anyway */ "prng", + "pull", /* option is implied by 'client' */ "rcvbuf", /* present in many configs */ "replay-persist", /* Makes little sense in TLS mode */ "script-security",