[Openvpn-devel] [PATCH applied] Re: Disable DCO when TLS mode is not used
Acked-by: Gert Doering Yeah, thanks :-) (tested on the "p2p --secret" server, still does the right thing. Have no "no secrets at all" setup, but from stare-at-code I see no reason why this wouldn't work as well) Dec 12 09:29:15 ubuntu2004 tun-udp-p2p[1272956]: No tls-client or tls-server option in configuration detected. Disabling data channel offload. Dec 12 09:29:15 ubuntu2004 tun-udp-p2p[1272956]: DEPRECATION: No tls-client or tls-server option in configuration detected. OpenVPN 2.7 will remove the functionality to run a VPN without TLS. See the examples section in the manual page for examples of a similar quick setup with peer-fingerprint. .. Dec 12 09:29:15 ubuntu2004 tun-udp-p2p[1272957]: TUN/TAP device tun5 opened Your patch has been applied to the master and release/2.6 branch. commit a68f064c7ff57cdebb3afceb72e1263a3ba9 (master) commit 9b277f426c7d295c8f354496e8e226fc26ff7b1c (release/2.6) Author: Arne Schwabe Date: Sat Dec 10 14:44:27 2022 +0100 Disable DCO when TLS mode is not used Signed-off-by: Arne Schwabe Acked-by: Gert Doering Message-Id: <20221210134427.1433419-1-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25641.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 1/2] management: refactor bytecount routines
From: Lev Stipakov
In preparation of DCO stats support, simplify
call chains of bytecount routines. No functional changes.
Signed-off-by: Lev Stipakov
---
src/openvpn/forward.c | 4 +--
src/openvpn/manage.c | 64 -
src/openvpn/manage.h | 66 ---
3 files changed, 52 insertions(+), 82 deletions(-)
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 5cd7eaa6..1cd20a0b 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -948,7 +948,7 @@ process_incoming_link_part1(struct context *c, struct
link_socket_info *lsi, boo
#ifdef ENABLE_MANAGEMENT
if (management)
{
-management_bytes_in(management, c->c2.buf.len);
+management_bytes_client(management, c->c2.buf.len, 0);
management_bytes_server(management, >c2.link_read_bytes,
>c2.link_write_bytes, >c2.mda_context);
}
#endif
@@ -1788,7 +1788,7 @@ process_outgoing_link(struct context *c)
#ifdef ENABLE_MANAGEMENT
if (management)
{
-management_bytes_out(management, size);
+management_bytes_client(management, 0, size);
management_bytes_server(management,
>c2.link_read_bytes, >c2.link_write_bytes, >c2.mda_context);
}
#endif
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index 5b288eab..1f76a23d 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -471,31 +471,55 @@ man_bytecount(struct management *man, const int
update_seconds)
msg(M_CLIENT, "SUCCESS: bytecount interval changed");
}
-void
+static void
man_bytecount_output_client(struct management *man)
{
-char in[32];
-char out[32];
-/* do in a roundabout way to work around possible mingw or mingw-glibc bug
*/
-openvpn_snprintf(in, sizeof(in), counter_format, man->persist.bytes_in);
-openvpn_snprintf(out, sizeof(out), counter_format, man->persist.bytes_out);
-msg(M_CLIENT, ">BYTECOUNT:%s,%s", in, out);
-man->connection.bytecount_last_update = now;
+if (man->connection.bytecount_update_seconds > 0
+&& now >= man->connection.bytecount_last_update
++ man->connection.bytecount_update_seconds)
+{
+char in[32];
+char out[32];
+
+/* do in a roundabout way to work around possible mingw or mingw-glibc
bug */
+openvpn_snprintf(in, sizeof(in), counter_format,
man->persist.bytes_in);
+openvpn_snprintf(out, sizeof(out), counter_format,
man->persist.bytes_out);
+msg(M_CLIENT, ">BYTECOUNT:%s,%s", in, out);
+man->connection.bytecount_last_update = now;
+}
+}
+
+void
+management_bytes_client(struct management *man,
+const int size_in,
+const int size_out)
+{
+if (!(man->persist.callback.flags & MCF_SERVER))
+{
+man->persist.bytes_in += size_in;
+man->persist.bytes_out += size_out;
+man_bytecount_output_client(man);
+}
}
void
-man_bytecount_output_server(struct management *man,
-const counter_type *bytes_in_total,
-const counter_type *bytes_out_total,
-struct man_def_auth_context *mdac)
-{
-char in[32];
-char out[32];
-/* do in a roundabout way to work around possible mingw or mingw-glibc bug
*/
-openvpn_snprintf(in, sizeof(in), counter_format, *bytes_in_total);
-openvpn_snprintf(out, sizeof(out), counter_format, *bytes_out_total);
-msg(M_CLIENT, ">BYTECOUNT_CLI:%lu,%s,%s", mdac->cid, in, out);
-mdac->bytecount_last_update = now;
+management_bytes_server(struct management *man,
+const counter_type *bytes_in_total,
+const counter_type *bytes_out_total,
+struct man_def_auth_context *mdac)
+{
+if (man->connection.bytecount_update_seconds > 0
+&& now >= mdac->bytecount_last_update +
man->connection.bytecount_update_seconds
+&& (mdac->flags & (DAF_CONNECTION_ESTABLISHED |
DAF_CONNECTION_CLOSED)) == DAF_CONNECTION_ESTABLISHED)
+{
+char in[32];
+char out[32];
+/* do in a roundabout way to work around possible mingw or mingw-glibc
bug */
+openvpn_snprintf(in, sizeof(in), counter_format, *bytes_in_total);
+openvpn_snprintf(out, sizeof(out), counter_format, *bytes_out_total);
+msg(M_CLIENT, ">BYTECOUNT_CLI:%lu,%s,%s", mdac->cid, in, out);
+mdac->bytecount_last_update = now;
+}
}
static void
diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
index f46274e6..621440be 100644
--- a/src/openvpn/manage.h
+++ b/src/openvpn/manage.h
@@ -511,70 +511,16 @@ void management_auth_token(struct management *man, const
char *token);
/*
* These functions drive the bytecount in/out counters.
*/
+void
+management_bytes_client(struct management *man,
+
Re: [Openvpn-devel] [PATCH 2/2] Fix message for too long tls-crypt-v2 metadata
Hi, On Mon, Dec 12, 2022 at 12:24:10PM +, Maximilian Fillinger wrote: > Right now, openvpn just checks that we have at most 980 base64 characters > and then tries to decode them into a 733 byte buffer. But 980 characters > of base64 can encode up to 735 bytes. In that case, openvpn gives a fatal > error about being unable to decode the base64 which I find misleading. > > My patch always allocates a large enough buffer to decode the base64 and > checks that the decoded length is <= 733. An alternative would be to check > the base64 length, decode to a 735 bytes buffer, then check the decoded > length. I thought it's cleaner to have one length check, but I don't have > a strong opinion about this. Thanks (to you and Arne, your mails arrived here about the same time) - I was indeed confused about pre-decode / post-decode buffer size, and padding. Will proceed with merging as soon as I have the EEN patch out of the way. 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 2/2] management: add timer to push BYTECOUNT
From: Lev Stipakov
At the moment BYTECOUNT in,out is pushed if there is traffic.
With DCO, userspace process doesn't see the traffic, so we need
to add a timer which periodically fetches stats from DCO and
pushes to management client. The timer interval is set by existing
"bytecount n" management command.
This commit only adds a timer, stats fetching from DCO will be added
later.
Signed-off-by: Lev Stipakov
---
src/openvpn/forward.c | 11 --
src/openvpn/manage.c | 51 ---
src/openvpn/manage.h | 10 +++--
3 files changed, 60 insertions(+), 12 deletions(-)
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 1cd20a0b..830843c0 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -766,6 +766,13 @@ process_coarse_timers(struct context *c)
/* Should we ping the remote? */
check_ping_send(c);
+
+#ifdef ENABLE_MANAGEMENT
+if (management)
+{
+management_check_bytecount(c, management, >c2.timeval);
+}
+#endif /* ENABLE_MANAGEMENT */
}
static void
@@ -949,7 +956,7 @@ process_incoming_link_part1(struct context *c, struct
link_socket_info *lsi, boo
if (management)
{
management_bytes_client(management, c->c2.buf.len, 0);
-management_bytes_server(management, >c2.link_read_bytes,
>c2.link_write_bytes, >c2.mda_context);
+management_bytes_server(management, c->c2.link_read_bytes,
c->c2.link_write_bytes, >c2.mda_context);
}
#endif
}
@@ -1789,7 +1796,7 @@ process_outgoing_link(struct context *c)
if (management)
{
management_bytes_client(management, 0, size);
-management_bytes_server(management,
>c2.link_read_bytes, >c2.link_write_bytes, >c2.mda_context);
+management_bytes_server(management, c->c2.link_read_bytes,
c->c2.link_write_bytes, >c2.mda_context);
}
#endif
}
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index 1f76a23d..ce952d52 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -42,6 +42,7 @@
#include "ssl.h"
#include "common.h"
#include "manage.h"
+#include "openvpn.h"
#include "memdbg.h"
@@ -463,16 +464,22 @@ man_bytecount(struct management *man, const int
update_seconds)
if (update_seconds >= 0)
{
man->connection.bytecount_update_seconds = update_seconds;
+event_timeout_init(>connection.bytecount_update_interval,
+ man->connection.bytecount_update_seconds,
+ now);
}
else
{
man->connection.bytecount_update_seconds = 0;
+event_timeout_clear(>connection.bytecount_update_interval);
}
msg(M_CLIENT, "SUCCESS: bytecount interval changed");
}
static void
-man_bytecount_output_client(struct management *man)
+man_bytecount_output_client(struct management *man,
+counter_type dco_read_bytes,
+counter_type dco_write_bytes)
{
if (man->connection.bytecount_update_seconds > 0
&& now >= man->connection.bytecount_last_update
@@ -482,8 +489,8 @@ man_bytecount_output_client(struct management *man)
char out[32];
/* do in a roundabout way to work around possible mingw or mingw-glibc
bug */
-openvpn_snprintf(in, sizeof(in), counter_format,
man->persist.bytes_in);
-openvpn_snprintf(out, sizeof(out), counter_format,
man->persist.bytes_out);
+openvpn_snprintf(in, sizeof(in), counter_format, man->persist.bytes_in
+ dco_read_bytes);
+openvpn_snprintf(out, sizeof(out), counter_format,
man->persist.bytes_out + dco_write_bytes);
msg(M_CLIENT, ">BYTECOUNT:%s,%s", in, out);
man->connection.bytecount_last_update = now;
}
@@ -498,14 +505,14 @@ management_bytes_client(struct management *man,
{
man->persist.bytes_in += size_in;
man->persist.bytes_out += size_out;
-man_bytecount_output_client(man);
+man_bytecount_output_client(man, 0, 0);
}
}
void
management_bytes_server(struct management *man,
-const counter_type *bytes_in_total,
-const counter_type *bytes_out_total,
+const counter_type bytes_in_total,
+const counter_type bytes_out_total,
struct man_def_auth_context *mdac)
{
if (man->connection.bytecount_update_seconds > 0
@@ -515,8 +522,8 @@ management_bytes_server(struct management *man,
char in[32];
char out[32];
/* do in a roundabout way to work around possible mingw or mingw-glibc
bug */
-openvpn_snprintf(in, sizeof(in), counter_format, *bytes_in_total);
-openvpn_snprintf(out, sizeof(out), counter_format, *bytes_out_total);
+openvpn_snprintf(in, sizeof(in), counter_format, bytes_in_total);
+
Re: [Openvpn-devel] [PATCH 2/2] Fix message for too long tls-crypt-v2 metadata
Hi, On Sat, Nov 26, 2022 at 05:26:48PM +0100, Max Fillinger wrote: > The current code only checks if the base64-encoded metadata is at most > 980 characters. However, that can encode up to 735 bytes of data, while > only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn > prints a misleading error message saying that the base64 cannot be > decoded. > > This patch checks the decoded length to show an accurate error message. This patch looks overly complex to me for "change 735 to 733" - could you (or Arne) explain the change a bit better? (I see that the patch has an ACK, so this is not a showstopper, just be being confused) 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
Re: [Openvpn-devel] [PATCH 1/2] Correct tls-crypt-v2 metadata length in man page
Am 26.11.22 um 17:26 schrieb Max Fillinger: The manual page claims that the client metadata can be up to 735 bytes (encoded as upt to 980 characters base64), but the actual maximum length is 733 bytes which is also encoded as 980 characters in base64. Signed-off-by: Max Fillinger 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 2/2] Fix message for too long tls-crypt-v2 metadata
Am 12.12.22 um 13:03 schrieb Gert Doering: Hi, On Sat, Nov 26, 2022 at 05:26:48PM +0100, Max Fillinger wrote: The current code only checks if the base64-encoded metadata is at most 980 characters. However, that can encode up to 735 bytes of data, while only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn prints a misleading error message saying that the base64 cannot be decoded. This patch checks the decoded length to show an accurate error message. This patch looks overly complex to me for "change 735 to 733" - could you (or Arne) explain the change a bit better? (I see that the patch has an ACK, so this is not a showstopper, just be being confused) Problem is that base64 does padding. So e.g. a => YQ== ab => YWI= abc => YWJj So checking the length of the base64 gives you only something that is rounded up to the next 3 bytes (base64 encodes 3 bytes to 4 bytes). So if you have a limit like 733, you need to actually decode the base64 to check if it is short enough. The alternative would be to only allow 732 bytes, so we could check the base64 length again or use 735 bytes and use a maximum tls-crypt wrapped key size of 1026 bytes (which sounds a bit weird) Arne ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] Fix message for too long tls-crypt-v2 metadata
Hi! > -Original Message- > From: Gert Doering [mailto:g...@greenie.muc.de] > Sent: maandag 12 december 2022 13:03 > To: Maximilian Fillinger > Cc: openvpn-devel@lists.sourceforge.net > Subject: Re: [Openvpn-devel] [PATCH 2/2] Fix message for too long tls- > crypt-v2 metadata > > Hi, > > On Sat, Nov 26, 2022 at 05:26:48PM +0100, Max Fillinger wrote: > > The current code only checks if the base64-encoded metadata is at most > > 980 characters. However, that can encode up to 735 bytes of data, > while > > only up to 733 bytes are allowed. When passing 734 or 735 bytes, > openvpn > > prints a misleading error message saying that the base64 cannot be > > decoded. > > > > This patch checks the decoded length to show an accurate error > message. > > This patch looks overly complex to me for "change 735 to 733" - could > you (or Arne) explain the change a bit better? What's going on is that the tls-crypt-v2 metadata can be at most 733 bytes. But the metadata is supplied in base64. If you want to encode 733 bytes in base64, you need 980 characters. Right now, openvpn just checks that we have at most 980 base64 characters and then tries to decode them into a 733 byte buffer. But 980 characters of base64 can encode up to 735 bytes. In that case, openvpn gives a fatal error about being unable to decode the base64 which I find misleading. My patch always allocates a large enough buffer to decode the base64 and checks that the decoded length is <= 733. An alternative would be to check the base64 length, decode to a 735 bytes buffer, then check the decoded length. I thought it's cleaner to have one length check, but I don't have a strong opinion about this. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/4] Read DCO traffic stats from the kernel
Hi,
On Mon, Dec 12, 2022 at 01:50:51PM +0100, Arne Schwabe wrote:
> > I am not too much into FreeBSD parts, but
> >
> >> +hash_iterator_init(m->hash, );
> >> +
> >> +while ((he = hash_iterator_next()))
> >> +{
> >> +struct multi_instance *mi = (struct multi_instance *) he->value;
> >> +
> >> +if (mi->context.c2.tls_multi->peer_id != peerid)
> >> +continue;
> >
> > Shouldn't we use m->instances[peerid] instead of iterating over m->hash?
>
> Yes and a check that kernel does not give something > max_peer
I agree, doing the iterator here for every single active peer when we
already have the array. But then, of course, p2p peer-id will be
"something random", not constrained by max-clients.
Anyway, we have a procedural problem here - kp@ is on vacation for a
few weeks now, so we won't see a v2 of this patch any time soon.
Do you think the patch is ready to be merged, "as is", with not-so-good
efficiency? One of you could then send a followup patch changing this
to use the array accessor.
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
Re: [Openvpn-devel] [PATCH 1/4] Read DCO traffic stats from the kernel
Am 12.12.22 um 14:30 schrieb Gert Doering:
Hi,
On Mon, Dec 12, 2022 at 01:50:51PM +0100, Arne Schwabe wrote:
I am not too much into FreeBSD parts, but
+hash_iterator_init(m->hash, );
+
+while ((he = hash_iterator_next()))
+{
+struct multi_instance *mi = (struct multi_instance *) he->value;
+
+if (mi->context.c2.tls_multi->peer_id != peerid)
+continue;
Shouldn't we use m->instances[peerid] instead of iterating over m->hash?
Yes and a check that kernel does not give something > max_peer
I agree, doing the iterator here for every single active peer when we
already have the array. But then, of course, p2p peer-id will be
"something random", not constrained by max-clients.
on p2mp code, it will be contrainst by max-clients. The random peerid
only happens in p2p mode where do not have multi_instance anyway.
Anyway, we have a procedural problem here - kp@ is on vacation for a
few weeks now, so we won't see a v2 of this patch any time soon.
Do you think the patch is ready to be merged, "as is", with not-so-good
efficiency? One of you could then send a followup patch changing this
to use the array accessor.
Fine by me.
Arne
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/2] management: refactor bytecount routines
Am 12.12.22 um 10:58 schrieb Lev Stipakov:
From: Lev Stipakov
In preparation of DCO stats support, simplify
call chains of bytecount routines. No functional changes.
It would be nice to be a bit more verbose what you are actually
simplyfing in the commit message. E.g. inlining all the various function
that are used only once.
+
+void
+management_bytes_client(struct management *man,
+const int size_in,
+const int size_out)
+{
+if (!(man->persist.callback.flags & MCF_SERVER))
+{
+man->persist.bytes_in += size_in;
+man->persist.bytes_out += size_out;
+man_bytecount_output_client(man);
+}
}
I don't like this part of the change. We are here moving a inline
function from the critical path of data channel packets that is called
on every single packet into an non-inline function.
The old code was taking care that everything that was in that critical
path can be inlined. The new code breaks that promise.
Arne
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Correct tls-crypt-v2 metadata length in man page
Your patch has been applied to the master and release/2.6 branch. commit 0bd2fa38fb70ad9022c05ffa67b2bd8751ca5a5b (master) commit acc7ecc2721adf3628b1bf8eca4365663259844c (release/2.6) Author: Max Fillinger Date: Sat Nov 26 17:26:47 2022 +0100 Correct tls-crypt-v2 metadata length in man page Signed-off-by: Max Fillinger Acked-by: Arne Schwabe Message-Id: <20221126162648.150678-1-maximilian.fillin...@foxcrypto.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25546.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 1/4] Read DCO traffic stats from the kernel
Hi,
This is good - I need an API to get stats to make openvpn-gui show
those (via the management interface).
I am not too much into FreeBSD parts, but
> +hash_iterator_init(m->hash, );
> +
> +while ((he = hash_iterator_next()))
> +{
> +struct multi_instance *mi = (struct multi_instance *) he->value;
> +
> +if (mi->context.c2.tls_multi->peer_id != peerid)
> +continue;
Shouldn't we use m->instances[peerid] instead of iterating over m->hash?
from multi.h:
struct multi_context {
struct multi_instance **instances; /**< Array of multi_instances.
An instance can be
* accessed using peer-id as
an index. */
--
-Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] management: add timer to push BYTECOUNT
Am 12.12.22 um 12:56 schrieb Lev Stipakov: From: Lev Stipakov At the moment BYTECOUNT in,out is pushed if there is traffic. With DCO, userspace process doesn't see the traffic, so we need to add a timer which periodically fetches stats from DCO and pushes to management client. The timer interval is set by existing "bytecount n" management command. This commit only adds a timer, stats fetching from DCO will be added later. If you do the timers anyway why keep the other logic that triggers on updating the byte count? That feels duplicating and unnecessary complexity. So if we switch to a timer, I would strongly advocate to remove the other code path to avoid having two code paths that only add complexity but do not offer any significant (none?) benefit vs only keeping the timer. Arne ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] Fix message for too long tls-crypt-v2 metadata
Am 26.11.22 um 17:26 schrieb Max Fillinger: The current code only checks if the base64-encoded metadata is at most 980 characters. However, that can encode up to 735 bytes of data, while only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn prints a misleading error message saying that the base64 cannot be decoded. This patch checks the decoded length to show an accurate error message. 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 1/4] Read DCO traffic stats from the kernel
Am 12.12.22 um 13:34 schrieb Lev Stipakov:
Hi,
This is good - I need an API to get stats to make openvpn-gui show
those (via the management interface).
I am not too much into FreeBSD parts, but
+hash_iterator_init(m->hash, );
+
+while ((he = hash_iterator_next()))
+{
+struct multi_instance *mi = (struct multi_instance *) he->value;
+
+if (mi->context.c2.tls_multi->peer_id != peerid)
+continue;
Shouldn't we use m->instances[peerid] instead of iterating over m->hash?
Yes and a check that kernel does not give something > max_peer
Arne
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v4] Introduce dynamic tls-crypt for secure soft_reset/session renegotiation
Currently we have only one slot for renegotiation of the session/keys. If a replayed/faked packet is inserted by a malicous attacker, the legimate peer cannot renegotiate anymore. This commit introduces dynamic tls-crypt. When both peer support this feature, both peer create a dynamic tls-crypt key using TLS EKM (export key material) and will enforce using that key and tls-crypt for all renegotiations. This also add an additional protection layer for renegotiations to be taken over by an illegimate client, binding the renegotiations tightly to the original session. Especially when 2FA, webauth or similar authentication is used, many third party setup ignore the need to secure renegotiation with an auth-token. Since one of tls-crypt/tls-crypt-v2 purposes is to provide poor man's post quantum crypto guarantees, we have to ensure that the dynamic key tls-crypt key that replace the original tls-crypt key is as strong as the orginal key to avoid problems if there is a weak RNG or TLS EKM produces weak keys. We ensure this but XORing the original key with the key from TLS EKM. If tls-crypt/tls-cryptv2 is not active, we use just the key generated by TLS EKM. We also do not use hashing or anything else on the original key before XOR to avoid any potential of a structure in the key or something else that might weaken post-quantum use cases. OpenVPN 2.x reserves the TM_ACTIVE session for renegotians. When a SOFT_RESET_V1 packet is received, the active TLS session is moved from KS_PRIMARY to KS_SECONDARY. Here an attacker could theorectically send a faked/replayed SOFT_RESET_V1 and firste packet containing the TLS client hello. If this happens, the session is blocked until the TLS renegotiation attempt times out, blocking the legimitate client. Using a dynamic tls-crypt key here block any SOFT_RESET_V1 (and following packets) as replay and fake packets will not have a matching authentication/encryption are discarded. HARD_RESET packets that are from a reconnecting peer are instead put in the TM_UNTRUSTED/KS_PRIMARY slot until they are sufficiently verified, so the dyanmic tls-crypt key is not used here. Replay/fake packets also do not block the legimiate client. This commit delays the purging of the original tls-crypt key data from directly after passing it to crypto library to tls_wrap_free. We do this to allow us mixing the new exported key with the original key. Since need to be able to generate the secure renegotiation key, deleting the key after generating a secure renegotiation key is not an option. Even when the client does not support secure renegotiation, deleting the key is not a good as the reconnecting client or (especially in p2p mode with float) another client does the reconnect, we might need to generate a secure renegotiation key. Delaying the deletion of the key has also little effect as the key is still present in the OpenSSL/mbed TLS structures in the tls_wrap structure, so only the number the keys is in memory would be reduced. Patch v2: fix spellings of reneg and renegotiations. Patch v3: expand comment to original_tlscrypt_keydata and commit message, add Changes.rst Patch v4: improve commit message, Changes.rst Signed-off-by: Arne Schwabe --- Changes.rst | 6 ++ src/openvpn/auth_token.h | 2 +- src/openvpn/crypto.c | 7 +- src/openvpn/crypto.h | 16 +++- src/openvpn/init.c| 8 +- src/openvpn/multi.c | 4 + src/openvpn/openvpn.h | 2 + src/openvpn/options.c | 4 + src/openvpn/push.c| 5 ++ src/openvpn/ssl.c | 25 -- src/openvpn/ssl.h | 5 ++ src/openvpn/ssl_backend.h | 1 + src/openvpn/ssl_common.h | 13 src/openvpn/ssl_ncp.c | 4 + src/openvpn/ssl_pkt.c | 2 +- src/openvpn/ssl_pkt.h | 21 + src/openvpn/tls_crypt.c | 93 --- src/openvpn/tls_crypt.h | 19 - tests/unit_tests/openvpn/test_pkt.c | 17 - tests/unit_tests/openvpn/test_tls_crypt.c | 85 + 20 files changed, 310 insertions(+), 29 deletions(-) diff --git a/Changes.rst b/Changes.rst index fe91ece2e..d0b8ac21a 100644 --- a/Changes.rst +++ b/Changes.rst @@ -111,6 +111,12 @@ Tun MTU can be pushed directive ``--tun-mtu-max`` has been introduced to increase the maximum pushable MTU size (defaults to 1600). +Secure renegotiation +When both peer are OpenVPN 2.6.0+, OpenVPN will use secure renegotiation +using a dynamically created tls-crypt key. This ensure that only the +previously authenticated peer can do trigger renegotiation and complete +renegotiations. + Improved control channel packet size control (``max-packet-size``) The size of control
[Openvpn-devel] [PATCH applied] Re: Ignore connection attempts while server is shutting down
Acked-by: Gert Doering Stare-at-code looks good, and testing confirms that it does fix over-eager clients reconnecting too fast: ^C2022-12-12 14:03:11 us=841186 event_wait : Interrupted system call (fd=-1,code=4) 2022-12-12 14:03:11 us=841294 SENT CONTROL [cron2-freebsd-tc-amd64]: 'RESTART' (status=1) 2022-12-12 14:03:11 us=841346 SENT CONTROL [freebsd-74-amd64]: 'RESTART' (status=1) 2022-12-12 14:03:11 us=841391 SENT CONTROL [freebsd-11-amd64]: 'RESTART' (status=1) 2022-12-12 14:03:12 us=893434 MULTI: Connection attempt from 194.97.140.21:13788 ignored while server is shutting down and on the client: 2022-12-12 14:03:11 SIGUSR1[soft,server-pushed-connection-reset] received, process restarting 2022-12-12 14:03:11 Restart pause, 1 second(s) 2022-12-12 14:03:12 TCP/UDP: Preserving recently used remote address: [AF_INET]194.97.140.11:51196 [..] 2022-12-12 14:04:12 TLS Error: TLS key negotiation failed to occur within 60 seconds (check your network connectivity) 2022-12-12 14:04:12 TLS Error: TLS handshake failed so, no more "it reconnects right away and then the server disappears" (which I could reproduce without the patch - the client would then show "Initialization Sequence Completed" and wait for --ping-timeout). For 2.7, we might actually consider reworking the server-exit behaviour to "exit as soon as all outstanding control messages have been ACKed" or even "fire-and-forget on the CC EENs"? But this is a bigger thing than "fix goes into 2.6_beta2", I'm afraid. Your patch has been applied to the master and release/2.6 branch. commit 7d0a90335fe79a352456f262ce42ea501796ae87 (master) commit f8bfe1a5fb785d2f26f3a38d597604031f081018 (release/2.6) Author: Arne Schwabe Date: Thu Dec 8 16:31:29 2022 +0100 Ignore connection attempts while server is shutting down Signed-off-by: Arne Schwabe Acked-by: Gert Doering Message-Id: <20221208153129.1207228-1-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25638.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 2/2] Fix message for too long tls-crypt-v2 metadata
Am 26.11.22 um 17:26 schrieb Max Fillinger:
The current code only checks if the base64-encoded metadata is at most
980 characters. However, that can encode up to 735 bytes of data, while
only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn
prints a misleading error message saying that the base64 cannot be
decoded.
This patch checks the decoded length to show an accurate error message.
Gert looked at this again and we found some issues now:
This patch leaves an unused define TLS_CRYPT_V2_MAX_B64_METADATA_LEN
that is no longer used.
TLS_CRYPT_V2_MAX_METADATA_LEN is actually 734 sice it includes the type
byte for the type of metadata, which gives us the 733 bytes of metadata
decoded from base64.
BCAP());
@@ -644,10 +640,18 @@ tls_crypt_v2_write_client_key_file(const char *filename,
msg(M_FATAL, "ERROR: failed to base64 decode provided metadata");
goto cleanup;
}
+if (decoded_len > TLS_CRYPT_V2_MAX_METADATA_LEN)
+{
+msg(M_FATAL,
+"ERROR: metadata too long (%d bytes, max %u bytes)",
+decoded_len, TLS_CRYPT_V2_MAX_METADATA_LEN - 1);
+goto cleanup;
+}
The error message correctly uses 733 as length but the check should also
check for TLS_CRYPT_V2_MAX_METADATA_LEN -1 or use >=
Maybe we can add a unit test for this
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] Fix message for too long tls-crypt-v2 metadata
> So if you have a limit like 733, you need to actually decode the base64 > to check if it is short enough. The alternative would be to only allow > 732 bytes, so we could check the base64 length again or use 735 bytes > and use a maximum tls-crypt wrapped key size of 1026 bytes (which sounds > a bit weird) Allowing 732 bytes/979 characters actually seems like the cleanest solution to me. It somehow just didn't occur to me. Well, now that my solution is acked, we can just go with it. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] Fix message for too long tls-crypt-v2 metadata
Hi, On Mon, Dec 12, 2022 at 05:06:47PM +, Maximilian Fillinger wrote: > Well, now that my solution is acked, we can just go with it. It got an after-NAK, as there is an off-by-one... so feel free to send a v2 either way :-) 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
Re: [Openvpn-devel] [PATCH applied] Re: Disable DCO when TLS mode is not used
Hi, On 12/12/2022 09:32, Gert Doering wrote: Acked-by: Gert Doering Yeah, thanks :-) (tested on the "p2p --secret" server, still does the right thing. Have no "no secrets at all" setup, but from stare-at-code I see no reason why this wouldn't work as well) I know I am late to the party - but still wanted to give my virtual ACK Acked-by: Antonio Quartulli Thanks for cleaning after my half baked fix! Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/4] Read DCO traffic stats from the kernel
Hi,
On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote:
[cut]
+
+int
+dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
+{
+
+struct ifdrv drv;
+uint8_t buf[4096];
+nvlist_t *nvl;
+const nvlist_t *const *nvpeers;
+size_t npeers;
+int ret;
+
+if (!dco || !dco->open)
+{
+return 0;
+}
+
+CLEAR(drv);
+snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
+drv.ifd_cmd = OVPN_GET_PEER_STATS;
What speaks against having a simple GET_PEER here which returns the full
peer object with all possible attributes?
This way, if we want to retrieve another attribute in the future, this
attribute will already be delivered by the same API, without the need to
implement a new command each time.
Cheers,
--
Antonio Quartulli
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/4] Read DCO traffic stats from the kernel
Hi,
On Mon, Dec 12, 2022 at 09:53:36PM +0100, Antonio Quartulli wrote:
> On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote:
> [cut]
> > +int
> > +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
> > +{
> > +
> > +struct ifdrv drv;
> > +uint8_t buf[4096];
> > +nvlist_t *nvl;
> > +const nvlist_t *const *nvpeers;
> > +size_t npeers;
> > +int ret;
> > +
> > +if (!dco || !dco->open)
> > +{
> > +return 0;
> > +}
> > +
> > +CLEAR(drv);
> > +snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
> > +drv.ifd_cmd = OVPN_GET_PEER_STATS;
>
> What speaks against having a simple GET_PEER here which returns the full
> peer object with all possible attributes?
>
> This way, if we want to retrieve another attribute in the future, this
> attribute will already be delivered by the same API, without the need to
> implement a new command each time.
One counter-argument would be "the statistics are polled more often than
'all information about a peer object', so it should be less costly"
(also, when do we ever poll 'all attributes'? Since OpenVPN *sets*
these, we should know, except for the counters...)
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
