Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1683?usp=email
to look at the new patch set (#4).
Change subject: dco: remove iroute at client exit time instead of delayed exit
......................................................................
dco: remove iroute at client exit time instead of delayed exit
When a exits (because it sent an EEN or because it's connection
timed out) OpenVPN will perform some minimal cleanup and will
then postpone the actual instance purge by 5 seconds.
If iroutes are configured for the exiting client, the actual DCO
iroutes removal is also postponed.
If during this time window the same client reconnects and a new
instance is created (for example because --duplicate-cn is set or
because the same username is provided upon authentication) OpenVPN
will:
* create the new client instance;
* attempt adding the related DCO iroutes (which will fail because
EEXIST);
* 5 seconds timeout fires -> execute the delayed exit routine and
delete the DCO iroutes;
* no iroutes exists anymore on the server despite the client being
fully connected.
With this patch we move the DCO iroutes deletion to the actual
client exit time in order to avoid racing with a possible addition
being executed when the client reconnects. The new flow will be:
* client exits (due to EEN or timeout)
* DCO iroutes are immediately deleted
* client re-connects -> new instance created
* DCO iroutes are added -> SUCCESS
* 5 seconds timeout fires -> old instance client is fully purged
* client is connected and iroutes are in place as expected
This issue was reported by OpenVPN Access Server developers after
observing erratic iroutes disappearance with DCO in place.
Change-Id: I0ba723d12d433e6e020588b7b0c3ba10bcf8c44f
GitHub: closes openvpn/OpenVPN#1040
Signed-off-by: Antonio Quartulli <[email protected]>
---
M src/openvpn/dco.c
M src/openvpn/forward.c
M src/openvpn/forward.h
M src/openvpn/multi.c
M src/openvpn/push.c
5 files changed, 15 insertions(+), 7 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/83/1683/4
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index 1e6638b..403ba2f 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -733,6 +733,7 @@
}
ASSERT(TUNNEL_TYPE(c->c1.tuntap) == DEV_TYPE_TUN);
+ msg(D_DCO, "DCO: attempt removing iroutes from system table");
if (c->c2.push_ifconfig_defined)
{
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 27cfd36..74a330f 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -525,7 +525,7 @@
* Schedule a SIGTERM signal c->options.scheduled_exit_interval seconds from
now.
*/
bool
-schedule_exit(struct context *c)
+schedule_exit(openvpn_net_ctx_t *net_ctx, struct context *c)
{
const int n_seconds = c->options.scheduled_exit_interval;
/* don't reschedule if already scheduled. */
@@ -533,6 +533,15 @@
{
return false;
}
+
+ /* DCO iroutes must be removed now, because the delay introduced by this
+ * timer can create a race condition:
+ * the same client may reconnect before the old instance is purged, leading
+ * to DCO iroutes removal *after* reconnection, thus killing the routes
+ * for the new instance too.
+ */
+ dco_delete_iroutes(net_ctx, c);
+
tls_set_single_session(c->c2.tls_multi);
update_time();
reset_coarse_timers(c);
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index 0d3e492..374e1df 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -331,7 +331,7 @@
void process_ip_header(struct context *c, unsigned int flags, struct buffer
*buf,
struct link_socket *sock);
-bool schedule_exit(struct context *c);
+bool schedule_exit(openvpn_net_ctx_t *net_ctx, struct context *c);
static inline struct link_socket_info *
get_link_socket_info(struct context *c)
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a72dcd1..da8f822 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -479,8 +479,6 @@
const struct iroute *ir;
const struct iroute_ipv6 *ir6;
- dco_delete_iroutes(&m->top.net_ctx, &mi->context);
-
if (TUNNEL_TYPE(mi->context.c1.tuntap) == DEV_TYPE_TUN)
{
for (ir = mi->context.options.iroutes; ir != NULL; ir = ir->next)
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 564ce86..50053e9 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -202,7 +202,7 @@
* */
if (c->options.mode == MODE_SERVER)
{
- if (!schedule_exit(c))
+ if (!schedule_exit(&c->net_ctx, c))
{
/* Return early when we don't need to notify management */
return;
@@ -392,7 +392,7 @@
void
send_auth_failed(struct context *c, const char *client_reason)
{
- if (!schedule_exit(c))
+ if (!schedule_exit(&c->net_ctx, c))
{
msg(D_TLS_DEBUG, "exit already scheduled for context");
return;
@@ -493,7 +493,7 @@
void
send_restart(struct context *c, const char *kill_msg)
{
- schedule_exit(c);
+ schedule_exit(&c->net_ctx, c);
send_control_channel_string(c, kill_msg ? kill_msg : "RESTART", D_PUSH);
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1683?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0ba723d12d433e6e020588b7b0c3ba10bcf8c44f
Gerrit-Change-Number: 1683
Gerrit-PatchSet: 4
Gerrit-Owner: ordex <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
