| Message ID | 20200706163516.11390-1-arne@rfc2549.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Openvpn-devel,1/2] Remember if we have seen a push request without async push | expand |
Hi, On Mon, Jul 06, 2020 at 06:35:15PM +0200, Arne Schwabe wrote: > The logic if we already have seen a push request is still > correct/useful without async push. I am not entirely sure if also > deferred management authentication can trigger this code path but > it should be able to. The other benefit is removing a number of > ifdefs. NAK. In combination with async-auth (plugin) this triggers some sort of key inconsistency - the client does get the proper PUSH_REPLY, but key state is kaput ... Jul 6 20:41:30 gentoo openvpn[32657]: PLUGIN AUTH-PAM: BACKGROUND: name match found, query/match-string ['Password: ', 'Password:'] = 'PASSWORD' Jul 6 20:41:30 gentoo openvpn[32657]: PLUGIN AUTH-PAM: BACKGROUND: fbsd-tc-master: deferred auth: PAM succeeded Jul 6 20:41:32 gentoo tun-udp-p2mp-global-authpam[31884]: MULTI_sva: pool returned IPv4=194.97.145.74, IPv6=2001:608:3:814::1000 Jul 6 20:41:32 gentoo tun-udp-p2mp-global-authpam[31884]: OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_6f7cfcfda4cb5ecf1366685a7270c804.tmp Jul 6 20:41:32 gentoo tun-udp-p2mp-global-authpam[31884]: SENT CONTROL [cron2-freebsd-tc-amd64]: 'PUSH_REPLY,route 10.204.0.0 255.255.0.0,route-ipv6 fd00:abcd:204::/48,tun-ipv6,route-gateway 194.97.145.73,topology subnet,ping 10,ping-restart 30,compress lz4,ifconfig-ipv6 2001:608:3:814::1000/64 2001:608:3:814::1,ifconfig 194.97.145.74 255.255.255.248,peer-id 0,cipher AES-256-GCM' (status=1) Jul 6 20:41:32 gentoo tun-udp-p2mp-global-authpam[31884]: cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 PUSH: Received control message: 'PUSH_REQUEST' Jul 6 20:41:32 gentoo tun-udp-p2mp-global-authpam[31884]: cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 Key [AF_INET6]2001:608:0:814::f000:21:51780 [0] not initialized (yet), dropping packet. "I was about to ACK-and-merge this"... but it's reproduceable, async auth (without --async-push) is broken with this patch. gert
Am 06.07.20 um 20:43 schrieb Gert Doering: > Hi, > > On Mon, Jul 06, 2020 at 06:35:15PM +0200, Arne Schwabe wrote: >> The logic if we already have seen a push request is still >> correct/useful without async push. I am not entirely sure if also >> deferred management authentication can trigger this code path but >> it should be able to. The other benefit is removing a number of >> ifdefs. > > NAK. > > In combination with async-auth (plugin) this triggers some sort of > key inconsistency - the client does get the proper PUSH_REPLY, but > key state is kaput I will double check the patch but this might be that this code path is broken with async push too, when you do write status to file and do not close but wait for openvpn to pick up the state itself. Arne
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f1ced9b7..f6be6618 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -824,8 +824,8 @@ multi_create_instance(struct multi_context *m, const struct mroute_addr *real) mi->did_cid_hash = true; #endif -#ifdef ENABLE_ASYNC_PUSH mi->context.c2.push_request_received = false; +#ifdef ENABLE_ASYNC_PUSH mi->inotify_watch = -1; #endif @@ -2074,13 +2074,11 @@ script_failed: /* set context-level authentication flag */ mi->context.c2.context_auth = CAS_SUCCEEDED; -#ifdef ENABLE_ASYNC_PUSH /* authentication complete, send push reply */ if (mi->context.c2.push_request_received) { process_incoming_push_request(&mi->context); } -#endif } else { diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h index 4609af3e..a1308852 100644 --- a/src/openvpn/openvpn.h +++ b/src/openvpn/openvpn.h @@ -432,9 +432,7 @@ struct context_2 #if P2MP /* --ifconfig endpoints to be pushed to client */ -#ifdef ENABLE_ASYNC_PUSH bool push_request_received; -#endif bool push_ifconfig_defined; time_t sent_push_reply_expiry; in_addr_t push_ifconfig_local; diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 56d652a3..e7c3c08c 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -794,9 +794,7 @@ process_incoming_push_request(struct context *c) { int ret = PUSH_MSG_ERROR; -#ifdef ENABLE_ASYNC_PUSH c->c2.push_request_received = true; -#endif 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);
The logic if we already have seen a push request is still correct/useful without async push. I am not entirely sure if also deferred management authentication can trigger this code path but it should be able to. The other benefit is removing a number of ifdefs. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/multi.c | 4 +--- src/openvpn/openvpn.h | 2 -- src/openvpn/push.c | 2 -- 3 files changed, 1 insertion(+), 7 deletions(-)