[Openvpn-devel] question on use of "push" on "non --server" peers
Hi, Arne and I discussed PUSH_REQUEST/PUSH_REPLY handling in OpenVPN a lot recently, and found something interesting. Naively, one would assume that only "--client" (or "--pull") instances would send PULL_REQUEST messages, and only "--server" (or "--mode server") instances would reply to them, and behaviour in other cases is "somewhat unclear" - a non-server would not have information about client IPs to push, for example. After looking at my test rig, it turns out that my --inetd test case actually uses such a combination... - the server side has --inetd --tls-server (because --server is not permitted in --inetd mode), plus a few "push route ..." statements in the config - the client has "--ifconfig" and "--client" in its config - so, it does not rely on server-pushed IP config, but it *does* send PUSH_REQUEST and the server *does* send PUSH_REPLY with the needed routes. Breaking this "interesting combination of features" by permitting replies to PUSH_REQUEST only in "--mode server" configs would not hurt *me* much (we want to get rid of --inetd mode anyway), but it brought up the question "are people using this?". So: if you make use of PUSH_REPLY coming back from a "server" that is not configured with "--server" or "--mode server" (so, technically, the remote end of a p2p instance, not p2mp), please let us know what you do and why :-) Genuinely curious... gert PS: this is the reason why https://patchwork.openvpn.net/patch/1338/ was withdrawn, for now. -- "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: Avoid sending push request after receving push reply
Acked-by: Gert Doering
This is magic :-) - it's just a one liner (plus lots of documentation)
but the description makes sense, staring at the code for a while also
makes sense. Plus, it works :-)
I have not tested the server side, as (to my understanding) this is
basically "client side only" code. I *have* tested p2p with --tls-client
(now! part of my t_client rig :-) ) and that still works - relevant, as
the code flows along the "established? send pull?" path here.
The new code also works correctly with pre-fast-pull servers (the
"reference server" is - because I'm lazy - still running an older
version).
I have merged the typo fixes from Richard as well (note: ommitted has
only one m! I asked google!), and done a bit more bragging in the
Changes.rst wording ("significantly! improves connection time!").
Your patch has been applied to the master branch.
commit a3b21a76b87fedf045c409481f55c34486d8cd27
Author: Arne Schwabe
Date: Sun Jul 26 01:48:03 2020 +0200
Avoid sending push request after receving push reply
Signed-off-by: Arne Schwabe
Acked-by: Gert Doering
Message-Id: <20200725234803.22058-2-a...@rfc2549.org>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20589.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] Refuse PUSH_REQUEST as client/refactor process_incoming_push_request
Am 26.07.20 um 01:51 schrieb Arne Schwabe: > When a server sends a client a push request, the client will reply > with a push reply. The reply is bogus and almost empty since almost > all the options that are normally set (remote ip etc) are unset. > > I checked 2.4 and master and this does not have any security implications > or other bugs but it is still better to refuse this. > > This code also refactors the method to get rid of the ret variable to > make the code flow easier to understand. On further discussion on IRC, retract this patch. The tls-server/tls-client pair as a p2p pair with one side (does not even need to be the one with tls-server) can have multiple "push xy" in the config. Arne signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Simplify calling logic of check_connection_established_dowork
Acked-by: Gert Doering The change to "check_connection_established(c)" is really trivial and straight-forward, and best viewed with "git show -w". Taking away an effective no-op wrapper function and getting rid of one indentation level is always good. Nevertheless I've forced this through the full client/server tests, and it succeeded nicely :-) Your patch has been applied to the master branch. commit 7cadbe24b6e5b583b4e763cd7a3b30d540b1c7a0 Author: Arne Schwabe Date: Sun Jul 26 01:48:02 2020 +0200 Simplify calling logic of check_connection_established_dowork Signed-off-by: Arne Schwabe Acked-by: Gert Doering Message-Id: <20200725234803.22058-1-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20588.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 applied] Re: Include utun device number in utun error messages
Hi, On Sun, Jul 26, 2020 at 04:19:21PM +0200, Arne Schwabe wrote: > Am 26.07.20 um 15:40 schrieb Gert Doering: > > I wonder if we could just skip print the error message for this > > particular call (connect()) if the return is EBUSY? Or maybe > > "skip if utunnumber < 10 && EBUSY"? > > since you can also specify that you want utun9 that would break that. Only for the "loop from 0 to find the first working one", of course :) > More realistically we should just skip the interfaces that are already > present in the system. Baiscally if 'does interface utunX exist?' => > skip on trying the first working one. Yes that was the idea. 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: Include utun device number in utun error messages
Am 26.07.20 um 15:40 schrieb Gert Doering: > Acked-by: Gert Doering > > I've left out the stray uncrustify fixes - not that I disagree with > them, but they should go to their own patch. Sorry for not cleaning > that up while merging the corresponding patch. > > Code change looks good. Not tested anything (only test compiled on MacOS). > > I wonder if we could just skip print the error message for this > particular call (connect()) if the return is EBUSY? Or maybe > "skip if utunnumber < 10 && EBUSY"? > since you can also specify that you want utun9 that would break that. More realistically we should just skip the interfaces that are already present in the system. Baiscally if 'does interface utunX exist?' => skip on trying the first working one. Arne signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Include utun device number in utun error messages
Acked-by: Gert Doering I've left out the stray uncrustify fixes - not that I disagree with them, but they should go to their own patch. Sorry for not cleaning that up while merging the corresponding patch. Code change looks good. Not tested anything (only test compiled on MacOS). I wonder if we could just skip print the error message for this particular call (connect()) if the return is EBUSY? Or maybe "skip if utunnumber < 10 && EBUSY"? Your patch has been applied to the master branch. commit 1d86fae8740a6d025d550fdcb4aa13c755fc2e48 Author: Arne Schwabe Date: Sun Jul 26 01:50:23 2020 +0200 Include utun device number in utun error messages Signed-off-by: Arne Schwabe Acked-by: Gert Doering Message-Id: <20200725235023.22441-1-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20590.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] Remove --no-replay
Am 26.07.20 um 02:01 schrieb Arne Schwabe: > Am 17.07.20 um 19:10 schrieb David Sommerseth: >> The --no-replay feature is considered to be a security weakness, which >> was also highlighed during the OpenVPN 2.4 security audit [0]. This >> option was added to the DeprecatedOptions[1] list and has been reported >> as deprecated since OpenVPN 2.4. > > As a side note, removing this feature weakens the ability to use OpenVPN > is a pure tunnel without crypto (--auth none, --cipher none and > no-replay) since this removes the ability to disable replay proctection > when no authentication is enabled. (replay protection without auth is > silly as a attacker can just fake the replay id too.) > > Acked-By: Arne Schwabe I given that a bit of a thought. But we need to decide if we to support unencrypted transport only session or not in future. If we do not want to support them, then applying this patch is fine, otherwise we should restrict disabling no-replay to --auth none and also --auth none to --cipher none basically: --cipher != none => auth none and no-replay forbidden --cipher == none => allows auth none and also no-replay --cipher none and auth none, warn if no-replay is used that it does not prevent replay attacks. But do not fail since we would break a lot of setups. signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] Avoid sending push request after receving push reply
a little help.. On 26/07/2020 00:48, Arne Schwabe wrote: The introduction of IV_PROTO_REQUEST_PUSH (c290df55) sometimes causes the server to reply before we setup the push timer. The push reply will then clear a timer that has not been setup yet. We then start sending push request after we have gone through the whole initialisation already. This patch also clears the connestion_established timer that sets up the push request timer. This lead to the management_set_state(management, OPENVPN_STATE_GET_CONFIG, ...) function not being called. But a to display "waiting for configuration..." or I think this was meant to read as (moved 'a'): But to display a "waiting for configuration..." sending a "getting config state" after "initialisation" does not make sense anyway. Also add the IV_PROTO_REQUEST_PUSH feature as new feature in Changes.rst Signed-off-by: Arne Schwabe --- Changes.rst | 11 +++ src/openvpn/forward.c | 3 +++ src/openvpn/push.c| 1 + 3 files changed, 15 insertions(+) diff --git a/Changes.rst b/Changes.rst index 8fbaf419..e377a36c 100644 --- a/Changes.rst +++ b/Changes.rst @@ -37,6 +37,13 @@ Deferred client-connect asynchronous/deferred return of the configuration file in the same way as the auth-plugin. +Faster connection setup +A client will signal in the ``IV_PROTO`` variable that is in pull I think this is meant to read: variable that is in pull -> variable that it is in pull +mode. This allows the server to push the configuration options to +the client without waiting for a ``PULL_REQUEST`` message. The feature +is automatically enabled if both client and server support it and +reduces the of connection setup time by one round-trip time. + Deprecated features --- For an up-to-date list of all deprecated options, see this wiki page: @@ -72,6 +79,10 @@ User-visible Changes - Support for building with OpenSSL 1.0.1 has been removed. The minimum supported OpenSSL version is now 1.0.2. +- The GET_CONFIG management state is ommited if the server pushes ommited -> ommitted + the client configuration almost immediately as result of the + faster connection setup feature. + Overview of changes in 2.4 == diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 30a3fd46..759fdbe1 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -425,6 +425,9 @@ check_push_request_dowork(struct context *c) * * Options like --up-delay need to be triggered by this function which * checks for connection establishment. + * + * Note: The process_incoming_push_reply currently assumes that this function + * only sets ups the pull request timer when pull is enabled. ups -> up */ void check_connection_established(struct context *c) diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 84193afe..9c720b42 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -358,6 +358,7 @@ incoming_push_message(struct context *c, const struct buffer *buffer) } } event_timeout_clear(&c->c2.push_request_interval); +event_timeout_clear(&c->c2.wait_for_connect); } goto cleanup; ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Refuse PUSH_REQUEST as client/refactor process_incoming_push_request
Sanity check:
On 26/07/2020 00:51, Arne Schwabe wrote:
When a server sends a client a push request, the client will reply
with a push reply. The reply is bogus and almost empty since almost
all the options that are normally set (remote ip etc) are unset.
I checked 2.4 and master and this does not have any security implications
or other bugs but it is still better to refuse this.
This code also refactors the method to get rid of the ret variable to
make the code flow easier to understand.
Signed-off-by: Arne Schwabe
---
src/openvpn/push.c | 40 ++--
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 1c4f2033..84193afe 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -733,13 +733,19 @@ push_remove_option(struct options *o, const char *p)
int
process_incoming_push_request(struct context *c)
{
-int ret = PUSH_MSG_ERROR;
+/* if we receive a message a client we do not want to reply to it
+ * so limit this to multi server */
This sentence does not make sense, suggestion:
If a client receives a push request then ignore it. Only multi server
will reply to a push requests.
Something like that ..
+if (c->options.mode != MODE_SERVER)
+{
+return PUSH_MSG_ERROR;
+}
+
if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED)
{
const char *client_reason = tls_client_reason(c->c2.tls_multi);
send_auth_failed(c, client_reason);
-ret = PUSH_MSG_AUTH_FAILURE;
+return PUSH_MSG_AUTH_FAILURE;
}
else if (c->c2.context_auth == CAS_SUCCEEDED)
{
@@ -748,29 +754,27 @@ process_incoming_push_request(struct context *c)
openvpn_time(&now);
if (c->c2.sent_push_reply_expiry > now)
{
-ret = PUSH_MSG_ALREADY_REPLIED;
+return PUSH_MSG_ALREADY_REPLIED;
}
-else
-{
-/* per-client push options - peer-id, cipher, ifconfig,
ipv6-ifconfig */
-struct push_list push_list = { 0 };
-struct gc_arena gc = gc_new();
-if (prepare_push_reply(c, &gc, &push_list)
-&& send_push_reply(c, &push_list))
-{
-ret = PUSH_MSG_REQUEST;
-c->c2.sent_push_reply_expiry = now + 30;
-}
-gc_free(&gc);
+int ret = PUSH_MSG_ERROR;
+/* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig
*/
+struct push_list push_list = { 0 };
+struct gc_arena gc = gc_new();
+
+if (prepare_push_reply(c, &gc, &push_list)
+&& send_push_reply(c, &push_list))
+{
+ret = PUSH_MSG_REQUEST;
+c->c2.sent_push_reply_expiry = now + 30;
}
+gc_free(&gc);
+return ret;
}
else
{
-ret = PUSH_MSG_REQUEST_DEFERRED;
+return PUSH_MSG_REQUEST_DEFERRED;
}
-
-return ret;
}
static void
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
