[Openvpn-devel,1/2] Remember if we have seen a push request without async push

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
Related show

Commit Message

Arne Schwabe July 6, 2020, 4:35 p.m.
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(-)

Comments

Gert Doering July 6, 2020, 6:43 p.m. | #1
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
Arne Schwabe July 6, 2020, 8:28 p.m. | #2
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

Patch

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);