diff mbox series

[Openvpn-devel] Allow setting an empty auth-token in push replies

Message ID 20241023134903.66485-1-rzvncj@gmail.com
State New
Headers
Series [Openvpn-devel] Allow setting an empty auth-token in push replies |

Commit Message

Razvan Cojocaru Oct. 23, 2024, 1:49 p.m. UTC 
  This in turn allows the server to signal to the client that it
should no longer attempt to reconnect, if it wants to keep the
client out after an AUTH_FAILED.

Signed-off-by: Razvan Cojocaru <rzvncj@gmail.com>
---
 src/openvpn/misc.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Gert Doering Oct. 23, 2024, 2:23 p.m. UTC | #1 
Hi,

On Wed, Oct 23, 2024 at 04:49:03PM +0300, Razvan Cojocaru wrote:
> This in turn allows the server to signal to the client that it
> should no longer attempt to reconnect, if it wants to keep the
> client out after an AUTH_FAILED.

This should not be necessary.  After an AUTH_FAILED the token is
already invalidated on the client side.

Can you describe your use case a bit better?  This code is complex
enough as it is, so I'm not really willing to add more cases to it
(which I do have to test then and forever) if not needed.

gert
Razvan Cojocaru Oct. 23, 2024, 2:40 p.m. UTC | #2 
On 10/23/24 17:23, Gert Doering wrote:
> Hi,
> 
> On Wed, Oct 23, 2024 at 04:49:03PM +0300, Razvan Cojocaru wrote:
>> This in turn allows the server to signal to the client that it
>> should no longer attempt to reconnect, if it wants to keep the
>> client out after an AUTH_FAILED.
> 
> This should not be necessary.  After an AUTH_FAILED the token is
> already invalidated on the client side.
> 
> Can you describe your use case a bit better?  This code is complex
> enough as it is, so I'm not really willing to add more cases to it
> (which I do have to test then and forever) if not needed.

Thanks for the quick reply!

Of course! The use case pertains to the device posture checks that 
OpenVPN has rolled out.

Long story short, these are meant to allow the server to make decisions 
on whether the client is allowed to connect (and stay connected, which 
is the part I'm addressing here) based on information about it.

For example, if it has an up-to-date antivirus, or if it has full disk 
encryption enabled.

So a client may connect and be allowed in, then for some reason its 
antivirus may not be updated for a considerable while, at which point a 
check will want to disconnect said client. Reconnects will be possible 
after whoever manages the client machine updates the antivirus.

In this case, we want to disconnect the client and it should stay 
disconnected. A simple AUTH_FAILED for this scenario will have the 
client attempt another connection. But if we invalidate the token, then 
the client will not attempt to reconnect.

Another reason for this patch is that the OpenVPN 3 client does allow 
setting the auth-token to an empty string, and when it is empty the 
logic is to not reconnect - so this would allow both version 2 and 
version 3 clients to behave the same way.


Thanks,
Razvan
Gert Doering Oct. 23, 2024, 2:43 p.m. UTC | #3 
Hi,

On Wed, Oct 23, 2024 at 05:40:43PM +0300, Razvan Cojocaru wrote:
> In this case, we want to disconnect the client and it should stay
> disconnected. A simple AUTH_FAILED for this scenario will have the client
> attempt another connection. But if we invalidate the token, then the client
> will not attempt to reconnect.

AUTH_FAILED should do this automatically - invalidate the token, that is.
Can you show a log where this is (not) happening?

gert
Razvan Cojocaru Oct. 23, 2024, 2:47 p.m. UTC | #4 
On 10/23/24 17:43, Gert Doering wrote:
> Hi,
> 
> On Wed, Oct 23, 2024 at 05:40:43PM +0300, Razvan Cojocaru wrote:
>> In this case, we want to disconnect the client and it should stay
>> disconnected. A simple AUTH_FAILED for this scenario will have the client
>> attempt another connection. But if we invalidate the token, then the client
>> will not attempt to reconnect.
> 
> AUTH_FAILED should do this automatically - invalidate the token, that is.
> Can you show a log where this is (not) happening?

Of course:

2024-10-23 14:52:06 us=368754 PUSH: Received control message: 
'PUSH_REPLY,auth-token'
2024-10-23 14:52:06 us=368851 UDPv4 WRITE [90] to 
[AF_INET]69.162.107.71:1194: P_ACK_V1 kid=0 pid=[ #13 ] [ 8 7 6 5 4 3 2 
1 ] DATA len=0
2024-10-23 14:52:06 us=368936 UDPv4 READ [163] from 
[AF_INET]69.162.107.71:1194: P_CONTROL_V1 kid=0 pid=[ #12 ] [ 2 3 4 5 ] 
pid=9 DATA len=85
2024-10-23 14:52:06 us=368972 AUTH: Received control message: 
AUTH_FAILED,No Stairway to Heaven allowed in this guitar store
2024-10-23 14:52:06 us=369228 TCP/UDP: Closing socket
2024-10-23 14:52:06 us=369287 SIGUSR1[soft,auth-failure (auth-token)] 
received, process restarting
2024-10-23 14:52:06 us=369346 Restart pause, 1 second(s)

And with this patch:

2024-10-23 17:46:58 us=427109 PUSH: Received control message: 
'PUSH_REPLY,auth-token'
2024-10-23 17:46:58 us=427149 UDPv4 WRITE [90] to 
[AF_INET]69.162.107.71:1194: P_ACK_V1 kid=0 pid=[ #12 ] [ 8 7 6 5 4 3 2 
1 ] DATA len=0
2024-10-23 17:46:58 us=427371 UDPv4 READ [163] from 
[AF_INET]69.162.107.71:1194: P_CONTROL_V1 kid=0 pid=[ #12 ] [ 2 3 4 5 ] 
pid=9 DATA len=85
2024-10-23 17:46:58 us=427403 AUTH: Received control message: 
AUTH_FAILED,No Stairway to Heaven allowed in this guitar store
2024-10-23 17:46:58 us=427414 register signal: SIGTERM (auth-failure)
2024-10-23 17:46:58 us=427427 SIGTERM received, sending exit 
notification to peer
2024-10-23 17:46:58 us=427442 signal_reset: signal UNKNOWN is cleared
2024-10-23 17:46:58 us=427464 UDPv4 WRITE [90] to 
[AF_INET]69.162.107.71:1194: P_ACK_V1 kid=0 pid=[ #13 ] [ 9 8 7 6 5 4 3 
2 ] DATA len=0
2024-10-23 17:46:58 us=427501 UDPv4 WRITE [41] to 
[AF_INET]69.162.107.71:1194: P_DATA_V2 kid=0 DATA len=40
2024-10-23 17:46:59 us=679084 register signal: SIGTERM 
(exit-with-notification)
2024-10-23 17:46:59 us=679264 TCP/UDP: Closing socket
2024-10-23 17:46:59 us=679300 net_route_v4_del: 100.96.0.0/11 via 
100.96.1.1 dev [NULL] table 0 metric -1
2024-10-23 17:46:59 us=679388 sitnl_send: checking for received messages
2024-10-23 17:46:59 us=679406 sitnl_send: rtnl: received 36 bytes
2024-10-23 17:46:59 us=679437 net_route_v4_del: 100.80.0.0/12 via 
100.96.1.1 dev [NULL] table 0 metric -1
2024-10-23 17:46:59 us=679477 sitnl_send: checking for received messages
2024-10-23 17:46:59 us=679500 sitnl_send: rtnl: received 36 bytes
2024-10-23 17:46:59 us=679519 delete_route_ipv6(fd:0:0:8000::/49)
2024-10-23 17:46:59 us=679531 net_route_v6_del: fd:0:0:8000::/49 via :: 
dev tun1 table 0 metric -1
2024-10-23 17:46:59 us=679618 sitnl_send: checking for received messages
2024-10-23 17:46:59 us=679639 sitnl_send: rtnl: received 36 bytes
2024-10-23 17:46:59 us=679663 delete_route_ipv6(fd:0:0:4000::/50)
2024-10-23 17:46:59 us=679678 net_route_v6_del: fd:0:0:4000::/50 via :: 
dev tun1 table 0 metric -1
2024-10-23 17:46:59 us=679743 sitnl_send: checking for received messages
2024-10-23 17:46:59 us=679763 sitnl_send: rtnl: received 36 bytes
2024-10-23 17:46:59 us=679801 Closing tun/tap interface
2024-10-23 17:46:59 us=679817 net_addr_v4_del: 100.96.1.2 dev tun1
2024-10-23 17:46:59 us=679930 sitnl_send: checking for received messages
2024-10-23 17:46:59 us=679976 sitnl_send: rtnl: received 36 bytes
2024-10-23 17:46:59 us=679997 net_addr_v6_del: fd:0:0:8100::2/64 dev tun1
2024-10-23 17:46:59 us=680107 sitnl_send: checking for received messages
2024-10-23 17:46:59 us=680129 sitnl_send: rtnl: received 36 bytes
2024-10-23 17:46:59 us=725831 SIGTERM[soft,exit-with-notification] 
received, process exiting


Thanks,
Razvan
Gert Doering Oct. 23, 2024, 2:50 p.m. UTC | #5 
Hi,

On Wed, Oct 23, 2024 at 05:47:51PM +0300, Razvan Cojocaru wrote:
> > AUTH_FAILED should do this automatically - invalidate the token, that is.
> > Can you show a log where this is (not) happening?
> 
> Of course:
> 
> 2024-10-23 14:52:06 us=368754 PUSH: Received control message:
> 'PUSH_REPLY,auth-token'
> 2024-10-23 14:52:06 us=368851 UDPv4 WRITE [90] to
> [AF_INET]69.162.107.71:1194: P_ACK_V1 kid=0 pid=[ #13 ] [ 8 7 6 5 4 3 2 1 ]
> DATA len=0
> 2024-10-23 14:52:06 us=368936 UDPv4 READ [163] from
> [AF_INET]69.162.107.71:1194: P_CONTROL_V1 kid=0 pid=[ #12 ] [ 2 3 4 5 ]
> pid=9 DATA len=85
> 2024-10-23 14:52:06 us=368972 AUTH: Received control message: AUTH_FAILED,No
> Stairway to Heaven allowed in this guitar store
> 2024-10-23 14:52:06 us=369228 TCP/UDP: Closing socket
> 2024-10-23 14:52:06 us=369287 SIGUSR1[soft,auth-failure (auth-token)]
> received, process restarting
> 2024-10-23 14:52:06 us=369346 Restart pause, 1 second(s)

OK, so I see what is happening - you're sending an AUTH_FAILED "out of
the blue", not in response to a client handshake, right?

OpenVPN 2 *should* invalidate the token upon the reconnect (and then
getting an AUTH_FAILED)... so what happens in this case if you let it
reconnect?

gert
Razvan Cojocaru Oct. 23, 2024, 3:01 p.m. UTC | #6 
On 10/23/24 17:50, Gert Doering wrote:
> OK, so I see what is happening - you're sending an AUTH_FAILED "out of
> the blue", not in response to a client handshake, right?

Exactly. In response to a client handshake there's no problem.

> OpenVPN 2 *should* invalidate the token upon the reconnect (and then
> getting an AUTH_FAILED)... so what happens in this case if you let it
> reconnect?

If we let it reconnect, the first re-connection attempt will fail at the 
handshake stage as expected.

But OpenVPN GUI tools will tend to show _two_ failed connections (the 
first "out of the blue" AUTH_FAILED one, and the second actual one), 
which is confusing for clients.


Thanks,
Razvan
Selva Nair Oct. 23, 2024, 3:25 p.m. UTC | #7 
On Wed, Oct 23, 2024 at 11:03 AM Razvan Cojocaru <rzvncj@gmail.com> wrote:

> On 10/23/24 17:50, Gert Doering wrote:
> > OK, so I see what is happening - you're sending an AUTH_FAILED "out of
> > the blue", not in response to a client handshake, right?
>
> Exactly. In response to a client handshake there's no problem.
>
> > OpenVPN 2 *should* invalidate the token upon the reconnect (and then
> > getting an AUTH_FAILED)... so what happens in this case if you let it
> > reconnect?
>
> If we let it reconnect, the first re-connection attempt will fail at the
> handshake stage as expected.
>

> But OpenVPN GUI tools will tend to show _two_ failed connections (the
> first "out of the blue" AUTH_FAILED one, and the second actual one),
> which is confusing for clients.
>

Wouldn't pushing "HALT" instead of "AUTH_FAILED" work in this case?
As in the management command "client-kill {cid} HALT" which calls
send_restart() with kill_msg = "HALT".

Selva
Razvan Cojocaru Oct. 23, 2024, 3:48 p.m. UTC | #8 
On 10/23/24 18:25, Selva Nair wrote:
> Wouldn't pushing "HALT" instead of "AUTH_FAILED" work in this case?
> As in the management command "client-kill {cid} HALT" which calls
> send_restart() with kill_msg = "HALT".

Possibly, however the intent has always been to use this feature to 
reject (authorize) clients (so this is a corner case of that, just that 
we can retract authorization at a later time), and in addition 
considerable work has already been done that relies on the AUTH_FAILED 
code paths.


Thanks,
Razvan
Selva Nair Oct. 23, 2024, 4:27 p.m. UTC | #9 
On Wed, Oct 23, 2024 at 11:47 AM Razvan Cojocaru <rzvncj@gmail.com> wrote:

> On 10/23/24 18:25, Selva Nair wrote:
> > Wouldn't pushing "HALT" instead of "AUTH_FAILED" work in this case?
> > As in the management command "client-kill {cid} HALT" which calls
> > send_restart() with kill_msg = "HALT".
>
> Possibly, however the intent has always been to use this feature to
> reject (authorize) clients (so this is a corner case of that, just that
> we can retract authorization at a later time), and in addition
> considerable work has already been done that relies on the AUTH_FAILED
> code paths.
>
>
Looks like a misuse of AUTH_FAILED to me. To kill a client while not in the
authentication phase, use code paths meant for that purpose.

Selva
diff mbox series

Patch

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 70ba5e4d..82ac8056 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -524,6 +524,11 @@  set_auth_token(struct user_pass *tk, const char *token)
         }
         protect_user_pass(tk);
     }
+    else
+    {
+        tk->defined = false;
+        tk->token_defined = false;
+    }
 }
 
 void