From: Antonio Quartulli <[email protected]> When a client exits due to one of the following reasons: * EEN received * AUTH_FAILED sent * RESTART sent 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, a new instance is created (for example because --duplicate-cn is set or because the same username is provided upon authentication) and the same IP is assigned, then 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. Note that this issue is DCO specific, because without DCO OpenVPN creates virtual routes (no system routing table involved) and makes the last connecting client own them. This means that the delayed exit routine won't have any iroute to delete. 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]> Acked-by: Arne Schwabe <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1683 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1683 This mail reflects revision 5 of this Change. Acked-by according to Gerrit (reflected above): Arne Schwabe <[email protected]> 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..6c5ab4d 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,23 @@ { 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. + * + * Standard/virtual iroutes (non-DCO case) are not affected because the + * last connecting client claiming the iroutes takes ownership. Therefore + * they are not removed during delayed cleanup. + */ + if (c->did_dco_iroutes) + { + c->did_dco_iroutes = false; + 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..ac9b986 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -479,7 +479,12 @@ const struct iroute *ir; const struct iroute_ipv6 *ir6; - dco_delete_iroutes(&m->top.net_ctx, &mi->context); + /* check if DCO iroutes were already removed when scheduling a delayed exit */ + if (mi->context.did_dco_iroutes) + { + mi->context.did_dco_iroutes = false; + dco_delete_iroutes(&m->top.net_ctx, &mi->context); + } if (TUNNEL_TYPE(mi->context.c1.tuntap) == DEV_TYPE_TUN) { @@ -1278,6 +1283,8 @@ if (TUNNEL_TYPE(mi->context.c1.tuntap) == DEV_TYPE_TUN) { mi->did_iroutes = true; + /* multi_learn_in{6}_addr_t takes care of installing the DCO iroute */ + mi->context.did_dco_iroutes = true; for (ir = mi->context.options.iroutes; ir != NULL; ir = ir->next) { if (ir->netbits >= 0) diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h index fa00822..5747175 100644 --- a/src/openvpn/openvpn.h +++ b/src/openvpn/openvpn.h @@ -507,6 +507,8 @@ bool did_we_daemonize; /**< Whether demonization has already * taken place. */ + bool did_dco_iroutes; /**< Whether DCO iroutes have been installed */ + struct context_persist persist; /**< Persistent %context. */ struct context_0 *c0; /**< Level 0 %context. */ 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); } _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
