[Openvpn-devel] [PATCH applied] Re: Refuse connection if server pushes an option contradicting allow-compress
Acked-by: Gert Doering
We had a long and heated discussion about this... I wanted a 3-liner that
just does the "if (DCO && compression) { explode(); }" bit, but this is
indeed making the code more readable - and my fix might have interfered
with server / ccd/ option handling anyway.
This patch in itself does not change any behaviour yet (= a client
without any compression option in its config will accept "comp-lzo no",
going to stub mode, and will not accept "compress $nonstub").
Tested over dinner on the client/server testbeds, with and without
compression. This all passed.
I have also tested pushing a compression option to an "unsuspecting
client" ("comp-lzo no" works, "compress lz4" is refused) and also having
something in ccd/ on an "unsuspecting DCO server without compression"
(will reject the client, even for "comp-lzo no" or "stub-v2").
"compress migrate" plus a client with "comp-lzo" will also still do
the right thing and push "compress stub-v2".
Your patch has been applied to the master and release/2.6 branch.
commit e86bc8b2967484afdb1e96efddb8d91185c4cc2c (master)
commit e950ca1b9fca58e97aacedc5c0229856aa1e4e86 (release/2.6)
Author: Arne Schwabe
Date: Thu Mar 23 18:05:59 2023 +0100
Refuse connection if server pushes an option contradicting allow-compress
Signed-off-by: Arne Schwabe
Acked-by: Gert Doering
Message-Id: <20230323170601.1256132-2-a...@rfc2549.org>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26503.html
Signed-off-by: Gert Doering
--
kind regards,
Gert Doering
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Simplify --compress parsing in options.c
Acked-by: Gert Doering
"git show -w" shows that this is mostly whitespace changes and
streq()'ing alg instead of p[1] - with alg defaulting to "stub"
now (instead of having an else{} clause for "no option" that does
the same).
There is a minor difference, as "compress " would set
COMP_F_SWAP beforehand, and now adds COMP_F_ADVERTISE_STUBS_ONLY,
which might warrant an update to the documentation (it says "Additionally,
'stub' and 'stub-v2' will disable announcing lzo and lz4 compression",
so there is a documented difference to 'empty').
Client-side tested on Linux / t_client with and without compression.
Your patch has been applied to the master and release/2.6 branch.
commit bfc00a01c10bbdd9683aab5db2c2e7dcbb2f7378 (master)
commit 5fed4be1bf4b2c6e4ff0117bceb9613fa68b412d (release/2.6)
Author: Arne Schwabe
Date: Thu Mar 23 18:05:58 2023 +0100
Simplify --compress parsing in options.c
Signed-off-by: Arne Schwabe
Acked-by: Gert Doering
Message-Id: <20230323170601.1256132-1-a...@rfc2549.org>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26501.html
Signed-off-by: Gert Doering
--
kind regards,
Gert Doering
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v3 4/4] Parse compression options and bail out when compression is disabled
This change keeps the option parsing of compression options even when
compression is disabled. This allows OpenVPN to also refuse/reject connections
that try to use compression when compression is completely disabled.
Change-Id: I9d7afd8f1d67d2455b4ec6bc12f4dcde80140c4f
Signed-off-by: Arne Schwabe
---
src/openvpn/comp.c| 14 ---
src/openvpn/comp.h| 85 ++-
src/openvpn/init.c| 2 -
src/openvpn/multi.c | 2 -
src/openvpn/options.c | 12 +-
src/openvpn/options.h | 4 --
6 files changed, 54 insertions(+), 65 deletions(-)
diff --git a/src/openvpn/comp.c b/src/openvpn/comp.c
index c7a562f5a..27b640ce0 100644
--- a/src/openvpn/comp.c
+++ b/src/openvpn/comp.c
@@ -29,10 +29,11 @@
#include "syshead.h"
-#ifdef USE_COMP
-
#include "comp.h"
#include "error.h"
+
+#ifdef USE_COMP
+
#include "otime.h"
#include "memdbg.h"
@@ -158,6 +159,7 @@ comp_generate_peer_info_string(const struct
compress_options *opt, struct buffer
buf_printf(out, "IV_COMP_STUB=1\n");
buf_printf(out, "IV_COMP_STUBv2=1\n");
}
+#endif /* USE_COMP */
bool
check_compression_settings_valid(struct compress_options *info, int msglevel)
@@ -170,8 +172,13 @@ check_compression_settings_valid(struct compress_options
*info, int msglevel)
if (info->alg != COMP_ALGV2_UNCOMPRESSED && info->alg != COMP_ALG_UNDEF
&& (info->flags & COMP_F_ALLOW_NOCOMP_ONLY))
{
+#ifdef USE_COMP
msg(msglevel, "Compression or compression stub framing is not allowed "
"since data-channel offloading is enabled.");
+#else
+msg(msglevel, "Compression or compression stub framing is not allowed "
+"since OpenVPN was built without compression support.");
+#endif
return false;
}
@@ -199,6 +206,3 @@ check_compression_settings_valid(struct compress_options
*info, int msglevel)
#endif
return true;
}
-
-
-#endif /* USE_COMP */
diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h
index 71b350d09..89940cf3a 100644
--- a/src/openvpn/comp.h
+++ b/src/openvpn/comp.h
@@ -28,12 +28,19 @@
#ifndef OPENVPN_COMP_H
#define OPENVPN_COMP_H
-#ifdef USE_COMP
+/* We always parse all compression options, so we include these defines/struct
+ * outside of the USE_COMP define */
-#include "buffer.h"
-#include "mtu.h"
-#include "common.h"
-#include "status.h"
+/* Compression flags */
+#define COMP_F_ADAPTIVE (1<<0) /* COMP_ALG_LZO only */
+#define COMP_F_ALLOW_COMPRESS (1<<1) /* not only downlink is compressed
but also uplink */
+#define COMP_F_SWAP (1<<2) /* initial command byte is swapped
with last byte in buffer to preserve payload alignment */
+#define COMP_F_ADVERTISE_STUBS_ONLY (1<<3) /* tell server that we only support
compression stubs */
+#define COMP_F_ALLOW_STUB_ONLY (1<<4) /* Only accept stub compression,
even with COMP_F_ADVERTISE_STUBS_ONLY
+* we still accept other
compressions to be pushed */
+#define COMP_F_MIGRATE (1<<5) /* push stub-v2 or comp-lzo no when
we see a client with comp-lzo in occ */
+#define COMP_F_ALLOW_ASYM (1<<6) /* Compression was explicitly set
to allow asymetric compression */
+#define COMP_F_ALLOW_NOCOMP_ONLY(1<<7) /* Do not allow compression framing
(breaks DCO) */
/* algorithms */
#define COMP_ALG_UNDEF 0
@@ -51,16 +58,37 @@
#define COMP_ALGV2_SNAPPY 13
*/
-/* Compression flags */
-#define COMP_F_ADAPTIVE (1<<0) /* COMP_ALG_LZO only */
-#define COMP_F_ALLOW_COMPRESS (1<<1) /* not only downlink is compressed
but also uplink */
-#define COMP_F_SWAP (1<<2) /* initial command byte is swapped
with last byte in buffer to preserve payload alignment */
-#define COMP_F_ADVERTISE_STUBS_ONLY (1<<3) /* tell server that we only support
compression stubs */
-#define COMP_F_ALLOW_STUB_ONLY (1<<4) /* Only accept stub compression,
even with COMP_F_ADVERTISE_STUBS_ONLY
-* we still accept other
compressions to be pushed */
-#define COMP_F_MIGRATE (1<<5) /* push stub-v2 or comp-lzo no when
we see a client with comp-lzo in occ */
-#define COMP_F_ALLOW_ASYM (1<<6) /* Compression was explicitly set
to allow asymetric compression */
-#define COMP_F_ALLOW_NOCOMP_ONLY(1<<7) /* Do not allow compression framing
(breaks DCO) */
+/*
+ * Information that basically identifies a compression
+ * algorithm and related flags.
+ */
+struct compress_options
+{
+int alg;
+unsigned int flags;
+};
+
+static inline bool
+comp_non_stub_enabled(const struct compress_options *info)
+{
+return info->alg != COMP_ALGV2_UNCOMPRESSED
+ && info->alg != COMP_ALG_STUB
+ && info->alg != COMP_ALG_UNDEF;
+}
+
+/**
+ * Checks if the compression settings are valid. Takes into account the
+ * flags of allow-compression and also the whether algorithms are compiled
+ * in
+ */
+bool
[Openvpn-devel] [PATCH v3 2/4] Refuse connection if server pushes an option contradicting allow-compress
This removes also the checks in options.c itself as they we now bail out
later and no longer need to ignore them during parsing.
Change-Id: I872c06f402c35112194ba77c3d6aee78e22547cb
Signed-off-by: Arne Schwabe
---
Changes.rst | 4
src/openvpn/comp.c| 29 +
src/openvpn/comp.h| 8
src/openvpn/init.c| 8
src/openvpn/multi.c | 8
src/openvpn/options.c | 27 ---
6 files changed, 61 insertions(+), 23 deletions(-)
diff --git a/Changes.rst b/Changes.rst
index dedb97ee4..77bcef266 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -232,6 +232,10 @@ User-visible Changes
- The ``client-pending-auth`` management command now requires also the
key id. The management version has been changed to 5 to indicate this change.
+- (OpenVPN 2.6.2) A client will now refuse a connection if pushed compression
+ settings will contradict the setting of allow-compression as this almost
+ always results in a non-working connection.
+
Common errors with OpenSSL 3.0 and OpenVPN 2.6
--
Both OpenVPN 2.6 and OpenSSL 3.0 tighten the security considerable, so some
diff --git a/src/openvpn/comp.c b/src/openvpn/comp.c
index 3b8d78996..d6d8029da 100644
--- a/src/openvpn/comp.c
+++ b/src/openvpn/comp.c
@@ -157,4 +157,33 @@ comp_generate_peer_info_string(const struct
compress_options *opt, struct buffer
}
}
+bool
+check_compression_settings_valid(struct compress_options *info, int msglevel)
+{
+if ((info->flags & COMP_F_ALLOW_STUB_ONLY) && comp_non_stub_enabled(info))
+{
+msg(msglevel, "Compression is not allowed since allow-compression is "
+"set to 'no'");
+return false;
+}
+#ifndef ENABLE_LZ4
+if (info->alg == COMP_ALGV2_LZ4 || info->alg == COMP_ALG_LZ4)
+{
+msg(msglevel, "OpenVPN is compiled without LZ4 support. Requested "
+"compression cannot be enabled.");
+return false;
+}
+#endif
+#ifndef ENABLE_LZO
+if (info->alg == COMP_ALG_LZO || info->alg == COMP_ALG_LZ4)
+{
+msg(msglevel, "OpenVPN is compiled without LZO support. Requested "
+"compression cannot be enabled.");
+return false;
+}
+#endif
+return true;
+}
+
+
#endif /* USE_COMP */
diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h
index 685f40391..8636727ab 100644
--- a/src/openvpn/comp.h
+++ b/src/openvpn/comp.h
@@ -196,5 +196,13 @@ comp_non_stub_enabled(const struct compress_options *info)
&& info->alg != COMP_ALG_UNDEF;
}
+/**
+ * Checks if the compression settings are valid. Takes into account the
+ * flags of allow-compression and also the whether algorithms are compiled
+ * in
+ */
+bool
+check_compression_settings_valid(struct compress_options *info, int msglevel);
+
#endif /* USE_COMP */
#endif /* ifndef OPENVPN_COMP_H */
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 3a6f624fd..14859499d 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2637,6 +2637,14 @@ do_deferred_options(struct context *c, const unsigned
int found)
#ifdef USE_COMP
if (found & OPT_P_COMP)
{
+if (!check_compression_settings_valid(&c->options.comp, D_PUSH_ERRORS))
+{
+msg(D_PUSH_ERRORS, "OPTIONS ERROR: server pushed compression "
+"settings that are not allowed and will result "
+"in a non-working connection. "
+"See also allow-compression in the manual.");
+return false;
+}
msg(D_PUSH_DEBUG, "OPTIONS IMPORT: compression parms modified");
comp_uninit(c->c2.comp_context);
c->c2.comp_context = comp_init(&c->options.comp);
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 1480bf477..ac090ef5a 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2766,6 +2766,14 @@ multi_connection_established(struct multi_context *m,
struct multi_instance *mi)
cc_succeeded = false;
}
+#ifdef USE_COMP
+if (!check_compression_settings_valid(&mi->context.options.comp,
D_MULTI_ERRORS))
+{
+msg(D_MULTI_ERRORS, "MULTI: client has been rejected due to invalid
compression options");
+cc_succeeded = false;
+}
+#endif
+
if (cc_succeeded)
{
multi_client_connect_late_setup(m, mi, *option_types_found);
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 2bed4ce99..435e1ca9e 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3779,6 +3779,9 @@ options_postprocess_mutate(struct options *o, struct
env_set *es)
/* this depends on o->windows_driver, which is set above */
options_postprocess_mutate_invariant(o);
+/* check that compression settings in the options are okay */
+check_compression_settings_valid(&o->comp, M_USAGE);
+
/*
* Save certain parms before modifying options during connect, especially
* when using --pull
@@ -8405
[Openvpn-devel] [PATCH v3 1/4] Simplify --compress parsing in options.c
This removes a level of identation and make the "stub" condition
easier to see.
Change-Id: Iae47b191f522625f81eedd3a237b272cb7374d90
Signed-off-by: Arne Schwabe
---
src/openvpn/options.c | 87 +--
1 file changed, 43 insertions(+), 44 deletions(-)
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 64a8250b2..2bed4ce99 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8458,60 +8458,59 @@ add_option(struct options *options,
else if (streq(p[0], "compress") && !p[2])
{
VERIFY_PERMISSION(OPT_P_COMP);
+const char *alg = "stub";
if (p[1])
{
-if (streq(p[1], "stub"))
-{
-options->comp.alg = COMP_ALG_STUB;
-options->comp.flags |=
(COMP_F_SWAP|COMP_F_ADVERTISE_STUBS_ONLY);
-}
-else if (streq(p[1], "stub-v2"))
-{
-options->comp.alg = COMP_ALGV2_UNCOMPRESSED;
-options->comp.flags |= COMP_F_ADVERTISE_STUBS_ONLY;
-}
-else if (streq(p[1], "migrate"))
-{
-options->comp.alg = COMP_ALG_UNDEF;
-options->comp.flags = COMP_F_MIGRATE;
+alg = p[1];
+}
-}
-else if (options->comp.flags & COMP_F_ALLOW_STUB_ONLY)
-{
-/* Also printed on a push to hint at configuration problems */
-msg(msglevel, "Cannot set compress to '%s', "
-"allow-compression is set to 'no'", p[1]);
-goto err;
-}
+if (streq(alg, "stub"))
+{
+options->comp.alg = COMP_ALG_STUB;
+options->comp.flags |= (COMP_F_SWAP|COMP_F_ADVERTISE_STUBS_ONLY);
+}
+else if (streq(alg, "stub-v2"))
+{
+options->comp.alg = COMP_ALGV2_UNCOMPRESSED;
+options->comp.flags |= COMP_F_ADVERTISE_STUBS_ONLY;
+}
+else if (streq(alg, "migrate"))
+{
+options->comp.alg = COMP_ALG_UNDEF;
+options->comp.flags = COMP_F_MIGRATE;
+
+}
+else if (options->comp.flags & COMP_F_ALLOW_STUB_ONLY)
+{
+/* Also printed on a push to hint at configuration problems */
+msg(msglevel, "Cannot set compress to '%s', "
+"allow-compression is set to 'no'", alg);
+goto err;
+}
#if defined(ENABLE_LZO)
-else if (streq(p[1], "lzo"))
-{
-options->comp.alg = COMP_ALG_LZO;
-options->comp.flags &= ~(COMP_F_ADAPTIVE | COMP_F_SWAP);
-}
+else if (streq(alg, "lzo"))
+{
+options->comp.alg = COMP_ALG_LZO;
+options->comp.flags &= ~(COMP_F_ADAPTIVE | COMP_F_SWAP);
+}
#endif
#if defined(ENABLE_LZ4)
-else if (streq(p[1], "lz4"))
-{
-options->comp.alg = COMP_ALG_LZ4;
-options->comp.flags |= COMP_F_SWAP;
-}
-else if (streq(p[1], "lz4-v2"))
-{
-options->comp.alg = COMP_ALGV2_LZ4;
-}
-#endif
-else
-{
-msg(msglevel, "bad comp option: %s", p[1]);
-goto err;
-}
+else if (streq(alg, "lz4"))
+{
+options->comp.alg = COMP_ALG_LZ4;
+options->comp.flags |= COMP_F_SWAP;
+}
+else if (streq(alg, "lz4-v2"))
+{
+options->comp.alg = COMP_ALGV2_LZ4;
}
+#endif
else
{
-options->comp.alg = COMP_ALG_STUB;
-options->comp.flags |= COMP_F_SWAP;
+msg(msglevel, "bad comp option: %s", alg);
+goto err;
}
+
show_compression_warning(&options->comp);
}
#endif /* USE_COMP */
--
2.37.1 (Apple Git-137.1)
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v3 3/4] Add 'allow-compression stub-only' internally for DCO
This changes the "no" setting of allow-compression to also refuse framing.
This is important for our DCO implementation as these do not implement framing.
This behaviour surfaced when a commercial VPN provider was pushing
"comp-lzo no" to a client with DCO. While we are technically at fault here
for announcing comp-lzo no support by announcing IV_LZO_STUB=1, the
VPN provider continues to push "comp-lzo no" even in absense of that
flag.
As the new default we default to allow-compression stub-only if a stub option
is found in the config and to allow-compression no otherwise. This ensures
that we only enable DCO when no compression framing is used.
This will now also bail out if the server pushes a compression setting that
we do not support as mismatching compression is almost never a working
connection. In my lz4-v2 and lzo-v2 you might have a connection that works
mostly but some packets will be dropped since they compressed which is not
desirable either since it becomes very hard to debug.
Patch v2: bail out if server pushes an unsupported method. Also include this
bail out logic when OpenVPN is compiled without compression support.
Patch v3: always parse all compression option and move logic to check method
Change-Id: Ibd0c77af24e2214b3055d585dc23a4b06dccd414
Signed-off-by: Arne Schwabe
---
doc/man-sections/protocol-options.rst | 4 ++-
src/openvpn/comp.c| 47 ++-
src/openvpn/comp.h| 2 +-
src/openvpn/options.c | 18 --
4 files changed, 50 insertions(+), 21 deletions(-)
diff --git a/doc/man-sections/protocol-options.rst
b/doc/man-sections/protocol-options.rst
index 248f65cfd..055d08f95 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -25,7 +25,9 @@ configured in a compatible way between both the local and
remote side.
compression at the same time is not a feasible option.
:code:`no` (default)
- OpenVPN will refuse any non-stub compression.
+ OpenVPN will refuse any compression. If data-channel offloading
+ is enabled. OpenVPN will additionally also refuse compression
+ framing (stub).
:code:`yes`
OpenVPN will send and receive compressed packets.
diff --git a/src/openvpn/comp.c b/src/openvpn/comp.c
index d6d8029da..c7a562f5a 100644
--- a/src/openvpn/comp.c
+++ b/src/openvpn/comp.c
@@ -134,36 +134,51 @@ comp_print_stats(const struct compress_context *compctx,
struct status_output *s
void
comp_generate_peer_info_string(const struct compress_options *opt, struct
buffer *out)
{
-if (opt)
+if (!opt || opt->flags & COMP_F_ALLOW_NOCOMP_ONLY)
+{
+return;
+}
+
+bool lzo_avail = false;
+if (!(opt->flags & COMP_F_ADVERTISE_STUBS_ONLY))
{
-bool lzo_avail = false;
-if (!(opt->flags & COMP_F_ADVERTISE_STUBS_ONLY))
-{
#if defined(ENABLE_LZ4)
-buf_printf(out, "IV_LZ4=1\n");
-buf_printf(out, "IV_LZ4v2=1\n");
+buf_printf(out, "IV_LZ4=1\n");
+buf_printf(out, "IV_LZ4v2=1\n");
#endif
#if defined(ENABLE_LZO)
-buf_printf(out, "IV_LZO=1\n");
-lzo_avail = true;
+buf_printf(out, "IV_LZO=1\n");
+lzo_avail = true;
#endif
-}
-if (!lzo_avail)
-{
-buf_printf(out, "IV_LZO_STUB=1\n");
-}
-buf_printf(out, "IV_COMP_STUB=1\n");
-buf_printf(out, "IV_COMP_STUBv2=1\n");
}
+if (!lzo_avail)
+{
+buf_printf(out, "IV_LZO_STUB=1\n");
+}
+buf_printf(out, "IV_COMP_STUB=1\n");
+buf_printf(out, "IV_COMP_STUBv2=1\n");
}
bool
check_compression_settings_valid(struct compress_options *info, int msglevel)
{
+/*
+ * We also allow comp-stub-v2 here as it technically allows escaping of
+ * weird mac address and IPv5 protocol but practically always is used
+ * as an way to disable all framing.
+ */
+if (info->alg != COMP_ALGV2_UNCOMPRESSED && info->alg != COMP_ALG_UNDEF
+&& (info->flags & COMP_F_ALLOW_NOCOMP_ONLY))
+{
+msg(msglevel, "Compression or compression stub framing is not allowed "
+"since data-channel offloading is enabled.");
+return false;
+}
+
if ((info->flags & COMP_F_ALLOW_STUB_ONLY) && comp_non_stub_enabled(info))
{
msg(msglevel, "Compression is not allowed since allow-compression is "
-"set to 'no'");
+"set to 'stub-only'");
return false;
}
#ifndef ENABLE_LZ4
diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h
index 8636727ab..71b350d09 100644
--- a/src/openvpn/comp.h
+++ b/src/openvpn/comp.h
@@ -60,7 +60,7 @@
* we still accept other
compressions to be pushed */
#define COMP_F_MIGRATE (1<<5) /* push stub-v2 or comp-lzo no when
we see a client with comp-lzo in occ */
#define COMP_F_ALLOW_ASYM (1<<6) /* Comp
[Openvpn-devel] [PATCH] unit_tests: make cert_data.h compile with MSVC
Currently this is not obvious since we never build the UTs with MSVC, but it doesn't like the initializers with "const" variables. They cause error C2099: initializer is not a constant when used in an initializer. So change all of them to preprocessor defines instead. It also doesn't like the empty initializer. error C2059: syntax error: '}' CC: Selva Nair Signed-off-by: Frank Lichtenheld --- tests/unit_tests/openvpn/cert_data.h | 240 +++--- tests/unit_tests/openvpn/test_cryptoapi.c | 10 +- tests/unit_tests/openvpn/test_pkcs11.c| 10 +- 3 files changed, 127 insertions(+), 133 deletions(-) Notes: * this is an prereq for my CMake patch but is otherwise independent, so submitting separately * this is on top of Selva's "Unit tests: Test for PKCS#11 using a softhsm2 token" diff --git a/tests/unit_tests/openvpn/cert_data.h b/tests/unit_tests/openvpn/cert_data.h index 33de35ec..359718bb 100644 --- a/tests/unit_tests/openvpn/cert_data.h +++ b/tests/unit_tests/openvpn/cert_data.h @@ -36,131 +36,125 @@ */ /* sample-ec.crt */ -static const char *const cert1 = -"-BEGIN CERTIFICATE-\n" -"MIIClzCCAX+gAwIBAgIRAIJr3cy95V63CPEtaAA8JN4wDQYJKoZIhvcNAQELBQAw\n" -"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMTAgFw0yMzAzMTMxNjExMjhaGA8yMTIz\n" -"MDIxNzE2MTEyOFowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMTBZMBMGByqGSM49\n" -"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n" -"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n" -"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAU3MLD\n" -"NDOK13DqflQ8ra7FeGBXK06hHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTGC\n" -"FD55ErHXpK2JXS3WkfBm0NB1r3vKMBMGA1UdJQQMMAoGCCsGAQUFBwMCMAsGA1Ud\n" -"DwQEAwIHgDANBgkqhkiG9w0BAQsFAAOCAQEAhH/wOFqP4R+FK5QvU+oW/XacFMku\n" -"+qT8lL9J7BG28WhZ0ZcAy/AmtnyynkDyuZSwnlzGgJ5m4L/RfwTzJKhEHiSU3BvB\n" -"5C1Z1Q8k67MHSfb565iCn8GzPUQLK4zsILCoTkJPvimv2bJ/RZmNaD+D4LWiySD4\n" -"tuOEdHKrxIrbJ5eAaN0WxRrvDdwGlyPvbMFvfhXzd/tbkP4R2xvlm7S2DPeSTJ8s\n" -"srXMaPe0lAea4etMSZsjIRPwGRMXBrwbRmb6iN2Cq40867HdaJoAryYig7IiDwSX\n" -"htCbOA6sX+60+FEOYDEx5cmkogl633Pw7LJ3ICkyzIrUSEt6BOT1Gsc1eQ==\n" -"-END CERTIFICATE-\n"; -static const char *const key1 = -"-BEGIN PRIVATE KEY-\n" -"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg5Xpw/lLvBrWjAWDq\n" -"L6dm/4a1or6AQ6O3yXYgw78B23ihRANCAAR4SRvnSuGdJmPitKbqcFbcgyzsMBlh\n" -"4wWOrty4I0ZlIXxY2qEnyb3YKz4OdMGzpK7FLfQZehHg6LGblcLs4EW7\n" -"-END PRIVATE KEY-\n"; -static const char *const hash1 = "A4B74F1D68AF50691F62CBD675E24C8655369567"; -static const char *const cname1 = "ovpn-test-ec1"; +#define cd_cert1 "-BEGIN CERTIFICATE-\n" \ +"MIIClzCCAX+gAwIBAgIRAIJr3cy95V63CPEtaAA8JN4wDQYJKoZIhvcNAQELBQAw\n" \ +"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMTAgFw0yMzAzMTMxNjExMjhaGA8yMTIz\n" \ +"MDIxNzE2MTEyOFowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMTBZMBMGByqGSM49\n" \ +"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n" \ +"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n" \ +"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAU3MLD\n" \ +"NDOK13DqflQ8ra7FeGBXK06hHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTGC\n" \ +"FD55ErHXpK2JXS3WkfBm0NB1r3vKMBMGA1UdJQQMMAoGCCsGAQUFBwMCMAsGA1Ud\n" \ +"DwQEAwIHgDANBgkqhkiG9w0BAQsFAAOCAQEAhH/wOFqP4R+FK5QvU+oW/XacFMku\n" \ +"+qT8lL9J7BG28WhZ0ZcAy/AmtnyynkDyuZSwnlzGgJ5m4L/RfwTzJKhEHiSU3BvB\n" \ +"5C1Z1Q8k67MHSfb565iCn8GzPUQLK4zsILCoTkJPvimv2bJ/RZmNaD+D4LWiySD4\n" \ +"tuOEdHKrxIrbJ5eAaN0WxRrvDdwGlyPvbMFvfhXzd/tbkP4R2xvlm7S2DPeSTJ8s\n" \ +"srXMaPe0lAea4etMSZsjIRPwGRMXBrwbRmb6iN2Cq40867HdaJoAryYig7IiDwSX\n" \ +"htCbOA6sX+60+FEOYDEx5cmkogl633Pw7LJ3ICkyzIrUSEt6BOT1Gsc1eQ==\n" \ +"-END CERTIFICATE-\n" +#define cd_key1 "-BEGIN PRIVATE KEY-\n" \ +"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg5Xpw/lLvBrWjAWDq\n" \ +"L6dm/4a1or6AQ6O3yXYgw78B23ihRANCAAR4SRvnSuGdJmPitKbqcFbcgyzsMBlh\n" \ +"4wWOrty4I0ZlIXxY2qEnyb3YKz4OdMGzpK7FLfQZehHg6LGblcLs4EW7\n" \ +"-END PRIVATE KEY-\n" +#define cd_hash1 "A4B74F1D68AF50691F62CBD675E24C8655369567" +#define cd_cname1 "ovpn-test-ec1" -static const char *const cert2 = -"-BEGIN CERTIFICATE-\n" -"MIIClzCCAX+gAwIBAgIRAN9fIkTDOjX0Bd9adHVcLx8wDQYJKoZIhvcNAQELBQAw\n" -"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMjAgFw0yMzAzMTMxODAzMzFaGA8yMTIz\n" -"MDIxNzE4MDMzMVowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMjBZMBMGByqGSM49\n" -"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n" -"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n" -"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAUyX3c\n" -"tpRP5cKlESsG80rOGhEphsGhHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTKC\n" -"FBc8ra53hwYrlIkdY3Ay1WCrrHJ8MBMGA1UdJQQMMAoGCCsGAQUFBwMCMAsGA1Ud\n" -"DwQEAwIHgDANBgkqhkiG9w0BAQsFAAOCAQEAWmA40BvEgBbKb1ReKlKzk64xi2ak\n" -"4tyr3sW9wIYQ2N1zkSomwEV6wGEawLqPADRbXiY
[Openvpn-devel] Fwd: [PATCH] Print DCO client stats on SIGUSR2
For reference, my comments.
-- Forwarded message -
From: Lev Stipakov
Date: Thu, Mar 23, 2023 at 9:39 AM
Subject: Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2
To: Selva Nair
I see the point - we now have driver-specific code in functions
{multi}_print_status which are supposed to be driver-agnostic and it
doesn't scale well when/if we add another driver. Also in general it
feels wrong that
print_ functions do print-unrelated things, like calls to the network driver.
I think in the long run we might want to get rid of
dco_read/write_bytes and just store dco stats into
link_read/write_bytes. I remember that in Linux (and likely in
FreeBSD?) we do initial handshake in userspace and
then switch to dco - probably that was the rationale of keeping link
and dco stats separately. Not sure if stats granularity justifies
complexity of adding driver-specific counters.
Your reply wasn't sent to -devel, not sure if it was intentional.
-Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] Fwd: [PATCH] Print DCO client stats on SIGUSR2
I didn't realize it until Lev pointed out that this reply yesterday didn't go to the list. FTR, copying to the list. -- Forwarded message - From: Selva Nair Date: Wed, Mar 22, 2023 at 9:42 AM Subject: Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2 To: Lev Stipakov > > > >> +status_printf(so, "TCP/UDP read bytes," counter_format, > c->c2.link_read_bytes + c->c2.dco_read_bytes); > >> +status_printf(so, "TCP/UDP write bytes," counter_format, > c->c2.link_write_bytes + c->c2.dco_write_bytes); > > > > Same here -- have a place to read the total count from independent of > dco. > > But what is wrong in fetching dco stats when we need them? What is the > advantage of timer? > What happens when we have three other drivers like DCO? Not saying that will ever happen, but using that as an example to clarify why I think this is "wrong" style. That said, I agree why a timer may not be a good idea either. If an approach that can keep internal details "private" is going to be too complex, we can live with this. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: dco-linux: implement dco_get_peer_stats{, multi} API
Acked-by: Gert Doering
This only touches linux only files, so only tested on Linux (builds with
and without DCO). The patch looks larger than it is because of a new
argument to ovpn_nl_msg_send(), but for the "existing code" this is
unused (extra argument to the callback function), so no change. The
actual new code looks quite reasonable.
Testing confirms that we have counters now, for both server (status file)
and client (SIGUSR2) use.
There is a catch to it - the userland side has support for "link bytes"
and "tun bytes" (which end up in dco*bytes), but the "recommended kernel
version" from ChangeLog does not have the link counters yet - so all
queries end up with output like this:
tun-udp-p2mp[492454]: dco_update_peer_stat: no link RX bytes provided in reply
for peer 0
tun-udp-p2mp[492454]: dco_update_peer_stat: no link TX bytes provided in reply
for peer 0
tun-udp-p2mp[492454]: dco_update_peer_stat / tun_read_bytes: 5600
tun-udp-p2mp[492454]: dco_update_peer_stat / tun_write_bytes: 3592
upgrading kernel commit to commit 726fdfe0f brings both counters (and
the values correlate to "which peer did what?").
tun-udp-p2mp[493935]: dco_update_peer_stat / dco_read_bytes: 132176
tun-udp-p2mp[493935]: dco_update_peer_stat / dco_write_bytes: 131600
tun-udp-p2mp[493935]: dco_update_peer_stat / tun_read_bytes: 128096
tun-udp-p2mp[493935]: dco_update_peer_stat / tun_write_bytes: 127640
Your patch has been applied to the master and release/2.6 branch.
commit 5a8fb55ac8cf4019afee884d3be545ddf87435a4 (master)
commit 1fd69b9085f4c21542ec506d101eb73b3cd0abc4 (release/2.6)
Author: Antonio Quartulli
Date: Wed Mar 22 20:27:57 2023 +0100
dco-linux: implement dco_get_peer_stats{, multi} API
Signed-off-by: Antonio Quartulli
Acked-by: Gert Doering
Message-Id: <20230322192757.20767-...@unstable.cc>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26481.html
Signed-off-by: Gert Doering
--
kind regards,
Gert Doering
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4] dco-linux: implement dco_get_peer_stats{, multi} API
Spot on and sorry for forgetting to mentioning it: You need ovpn-dco at this commit: commit 726fdfe0fa21aa4e87c5a60294ea0365ce7b6809 (HEAD -> master, origin/master) Author: Antonio Quartulli Date: Mon Mar 20 23:50:52 2023 +0100 ovpn-dco: store and report transport rx/tx stats as well Signed-off-by: Antonio Quartulli Which comes right after the one you are using. It is not a breaking change (as you see there is no real *failure*, just missing information from the kernel side). Regards, On 23/03/2023 13:03, Gert Doering wrote: Hi, On Wed, Mar 22, 2023 at 08:27:57PM +0100, Antonio Quartulli wrote: With this API it is possible to retrieve the stats for a specific peer or for all peers and then update the userspace counters with the value reported by DCO. Change-Id: Ia3990b86b1be7ca844fb1674b39ce0d60528ccff Signed-off-by: Antonio Quartulli This *looks* very reasonable, but only half of it works for me. ... tun-udp-p2mp[492453]: OpenVPN 2.7_git [git:vw/master/5a8fb55ac8cf4019] x86_64-pc-linux-gnu [SSL (mbed TLS)] [LZO] [LZ4] [EPOLL] [MH/PKTINFO] [AEAD] [DCO] built on Mar 23 2023 tun-udp-p2mp[492453]: DCO version: 0.2.20230313 ... tun-udp-p2mp[492454]: dco_get_peer_stats_multi tun-udp-p2mp[492454]: dco_parse_peer_multi: parsing message... tun-udp-p2mp[492454]: dco_update_peer_stat: no link RX bytes provided in reply for peer 0 tun-udp-p2mp[492454]: dco_update_peer_stat: no link TX bytes provided in reply for peer 0 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_read_bytes: 240 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_write_bytes: 96 tun-udp-p2mp[492454]: dco_parse_peer_multi: parsing message... tun-udp-p2mp[492454]: dco_update_peer_stat: no link RX bytes provided in reply for peer 1 tun-udp-p2mp[492454]: dco_update_peer_stat: no link TX bytes provided in reply for peer 1 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_read_bytes: 776 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_write_bytes: 80 So neither for p2p tls nor for --server setups, I am receiving "link bytes", only tun*bytes. I have not checked the kernel side if I *should* receive anything, but this looks incomplete - either the kernel side is not yet ready (not a showstopper) or there is a desync in the tags used, which might or might not prevent merging... Antonio, any ideas? gert -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4] dco-linux: implement dco_get_peer_stats{, multi} API
Hi, On Wed, Mar 22, 2023 at 08:27:57PM +0100, Antonio Quartulli wrote: > With this API it is possible to retrieve the stats for a specific peer > or for all peers and then update the userspace counters with the value > reported by DCO. > > Change-Id: Ia3990b86b1be7ca844fb1674b39ce0d60528ccff > Signed-off-by: Antonio Quartulli This *looks* very reasonable, but only half of it works for me. ... tun-udp-p2mp[492453]: OpenVPN 2.7_git [git:vw/master/5a8fb55ac8cf4019] x86_64-pc-linux-gnu [SSL (mbed TLS)] [LZO] [LZ4] [EPOLL] [MH/PKTINFO] [AEAD] [DCO] built on Mar 23 2023 tun-udp-p2mp[492453]: DCO version: 0.2.20230313 ... tun-udp-p2mp[492454]: dco_get_peer_stats_multi tun-udp-p2mp[492454]: dco_parse_peer_multi: parsing message... tun-udp-p2mp[492454]: dco_update_peer_stat: no link RX bytes provided in reply for peer 0 tun-udp-p2mp[492454]: dco_update_peer_stat: no link TX bytes provided in reply for peer 0 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_read_bytes: 240 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_write_bytes: 96 tun-udp-p2mp[492454]: dco_parse_peer_multi: parsing message... tun-udp-p2mp[492454]: dco_update_peer_stat: no link RX bytes provided in reply for peer 1 tun-udp-p2mp[492454]: dco_update_peer_stat: no link TX bytes provided in reply for peer 1 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_read_bytes: 776 tun-udp-p2mp[492454]: dco_update_peer_stat / tun_write_bytes: 80 So neither for p2p tls nor for --server setups, I am receiving "link bytes", only tun*bytes. I have not checked the kernel side if I *should* receive anything, but this looks incomplete - either the kernel side is not yet ready (not a showstopper) or there is a desync in the tags used, which might or might not prevent merging... Antonio, any ideas? gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Print DCO client stats on SIGUSR2
Acked-by: Gert Doering I have listened to the discussion, and I think we all agree that we need to revisit this "DCO counter" business: - definition of c2 structure elements - do we need extra fields for "dco counters"? - do we need more counters? Windows currently has "TransportBytes*" and "TunBytes*", while FreeBSD only has "bytes" (which are what, exactly?) -> *I* definitely need a better understanding of these - when and where do we query the kernel for counters (timer, signal, ...?) This said, we want the functionality now :-) - so, I have tested this (on FreeBSD), and it does what it says, and the code looks reasonable according to "it will not access undefined pointers, will not leak memory" etc. Your patch has been applied to the master and release/2.6 branch. commit d5238627e4fab93a6c09816c60eb90e237b626c3 (master) commit d598871ca9fb7b4814ee8d8edfb26d20479bb6ed (release/2.6) Author: Lev Stipakov Date: Wed Mar 22 13:32:49 2023 +0200 Print DCO client stats on SIGUSR2 Signed-off-by: Lev Stipakov Acked-by: Gert Doering Message-Id: <20230322113249.2039-1-lstipa...@gmail.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26471.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] Enable pkcs11 an dtest_pkcs11 in github actions
On Wed, Mar 22, 2023 at 06:14:56PM -0400, selva.n...@gmail.com wrote: > From: Selva Nair > > - Enabled for the Ubuntu 22.04 build (OpenSSL 3) and one of the > Ubuntu 20.04 builds (OpenSSL 1.1.1). > > Signed-off-by: Selva Nair > --- > .github/workflows/build.yaml | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Worked in the PR, so Acked-By: Frank Lichtenheld -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/3] Unit tests: Test for PKCS#11 using a softhsm2 token
On Wed, Mar 22, 2023 at 06:14:55PM -0400, selva.n...@gmail.com wrote: > From: Selva Nair > > - Load some test certificate/key pairs into a temporary softhsm2 token > and enumerate available objects through pkcs11-helper interface > > - For each object, load it into SSL_CTX and test sign (if using OpenSSL 3) > or check the certificate and public-key match (if using OpenSSl 1.1.1.). > The pkcs11-id for each object is specified directly or > through a mocked management callback to test pkcs11-id-management > > Limitations: > Depends on libsofthsm2.so and p11tool (install softhsm2 and gnutls-bin > packages). Mbed-TLS/pkcs11-helper combination is not tested. > > If locations of these binaries are not auto-detected or need to be > overridden, use -DSOFTHSM2_UTIL= -DP11TOOL= to configure. > Location of SOFTHSM2_MODULE is not auto-detected and defaults to > /usr/lib/softhsm/libsofthsm2.so. It may be changed by passing > -DSOFTHSM2_MODULE=/some-path/libsofthsm2.so to configure. > Also see "configure --help". > > The test is enabled only if --enable-pkcs11 is in use, and SOFTHSM2_UTIL > & P11TOOL are found in path or manually defined during configuring. > > Changes relative to github PR > - Explicitly disable building the test on Windows: need to port mkstemp, > mkdtemp, setenv etc., before enabling this on Windows. > > Signed-off-by: Selva Nair I reviewed this on Github, with a focus on the configuration part. Approved it there, so Acked-By: Frank Lichtenheld -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/3] Move digest_sign_verify out of test_cryptoapi.c
On Wed, Mar 22, 2023 at 06:14:54PM -0400, selva.n...@gmail.com wrote: > From: Selva Nair > > - This function will be reused for testing pkcs11 > > Signed-off-by: Selva Nair This just moves code around. Acked-By: Frank Lichtenheld -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: dco-freebsd: use m->instances[] instead of m->hash
As this patch has a bit of mixed history "who wrote it, who ACKed it, what happened afterwards" I decided to record the ACK from Arne and Kristof. v4 has been tested on FreeBSD with DCO enabled, p2mp udp server, one client being connected all the time and the other client reconnecting (moving between peer-id 1 and 2), packets going back and forth, looking at the resulting counters. Everything looked as expected. (This is basically an optimization that was discussed in November already but nobody followed up on it - and with the upcoming Linux DCO counter patches, the topic "why use iterator when you have an array?" resurfaced - thanks, Antonio, for covering FreeBSD too) Your patch has been applied to the master and release/2.6 branch. commit 03145f223236df90b35d1db444319fd3f785792b (master) commit 5acefd944c6a8a4338b5105ffe6b9ffce6bad330 (release/2.6) Author: Antonio Quartulli Date: Thu Mar 23 09:03:41 2023 +0100 dco-freebsd: use m->instances[] instead of m->hash Signed-off-by: Antonio Quartulli Acked-by: Arne Schwabe Acked-by: Kristof Provost Message-Id: <20230323080341.51624-1-g...@greenie.muc.de> URL: https://www.mail-archive.com/search?l=mid&q=20230323080341.51624-1-g...@greenie.muc.de Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4] dco-freebsd: use m->instances[] instead of m->hash
Am 23.03.23 um 09:03 schrieb Gert Doering:
From: Antonio Quartulli
When retrieving the multi_instance of a specific peer,
there is no need to peform a linear search across the
whole m->hash list. We can directly access the needed
object via m->instances[peer-id] in constant time (and
just one line of code).
Adapt the dco-freebsd code to do so.
v4: use "peerid" everywhere as that's what FreeBSD does, change message text
Cc: Kristof Provost
Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
Signed-off-by: Antonio Quartulli
---
src/openvpn/dco_freebsd.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 225b3cf8..a334d5d2 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -674,27 +674,17 @@ dco_event_set(dco_context_t *dco, struct event_set *es,
void *arg)
static void
dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t
*nvl)
{
-struct hash_element *he;
-struct hash_iterator hi;
-hash_iterator_init(m->hash, &hi);
-
-while ((he = hash_iterator_next(&hi)))
+if (peerid >= m->max_clients || !m->instances[peerid])
{
-struct multi_instance *mi = (struct multi_instance *) he->value;
-
-if (mi->context.c2.tls_multi->peer_id != peerid)
-{
-continue;
-}
-
-mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
-mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
-
+msg(M_WARN, "dco_update_peer_stat: invalid peer ID %d returned by
kernel", peerid);
return;
}
-msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);
+struct multi_instance *mi = m->instances[peerid];
+
+mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
+mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
}
int
Acked-By: Arne Schwabe
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v4] dco-freebsd: use m->instances[] instead of m->hash
Hi,
On 23/03/2023 09:03, Gert Doering wrote:
From: Antonio Quartulli
When retrieving the multi_instance of a specific peer,
there is no need to peform a linear search across the
whole m->hash list. We can directly access the needed
object via m->instances[peer-id] in constant time (and
just one line of code).
Adapt the dco-freebsd code to do so.
v4: use "peerid" everywhere as that's what FreeBSD does, change message text
Cc: Kristof Provost
Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
Signed-off-by: Antonio Quartulli
---
src/openvpn/dco_freebsd.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 225b3cf8..a334d5d2 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -674,27 +674,17 @@ dco_event_set(dco_context_t *dco, struct event_set *es,
void *arg)
static void
dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t
*nvl)
{
-struct hash_element *he;
-struct hash_iterator hi;
-hash_iterator_init(m->hash, &hi);
-
-while ((he = hash_iterator_next(&hi)))
+if (peerid >= m->max_clients || !m->instances[peerid])
{
-struct multi_instance *mi = (struct multi_instance *) he->value;
-
-if (mi->context.c2.tls_multi->peer_id != peerid)
-{
-continue;
-}
-
-mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
-mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
-
+msg(M_WARN, "dco_update_peer_stat: invalid peer ID %d returned by
kernel", peerid);
I'd prefer to use __func__ here, but it is noted that dco_freebsd.c
seldomly uses this approach.
For this reason I think hardcoding the function name is fine for now.
Later on we can do a full cleanup.
return;
}
-msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);
+struct multi_instance *mi = m->instances[peerid];
+
+mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
+mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
}
int
This patch is basically my v3 with the peerid fixed and the message changed.
I can't ACK since this patch bears my own signature, but I am fine with
the applied modification.
Cheers,
--
Antonio Quartulli
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v4] dco-freebsd: use m->instances[] instead of m->hash
From: Antonio Quartulli
When retrieving the multi_instance of a specific peer,
there is no need to peform a linear search across the
whole m->hash list. We can directly access the needed
object via m->instances[peer-id] in constant time (and
just one line of code).
Adapt the dco-freebsd code to do so.
v4: use "peerid" everywhere as that's what FreeBSD does, change message text
Cc: Kristof Provost
Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
Signed-off-by: Antonio Quartulli
---
src/openvpn/dco_freebsd.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 225b3cf8..a334d5d2 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -674,27 +674,17 @@ dco_event_set(dco_context_t *dco, struct event_set *es,
void *arg)
static void
dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t
*nvl)
{
-struct hash_element *he;
-struct hash_iterator hi;
-hash_iterator_init(m->hash, &hi);
-
-while ((he = hash_iterator_next(&hi)))
+if (peerid >= m->max_clients || !m->instances[peerid])
{
-struct multi_instance *mi = (struct multi_instance *) he->value;
-
-if (mi->context.c2.tls_multi->peer_id != peerid)
-{
-continue;
-}
-
-mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
-mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
-
+msg(M_WARN, "dco_update_peer_stat: invalid peer ID %d returned by
kernel", peerid);
return;
}
-msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);
+struct multi_instance *mi = m->instances[peerid];
+
+mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
+mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
}
int
--
2.39.2
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
