OpenVPN / openvpn

OpenVPN is an open source VPN daemon
http://openvpn.net
Other
10.26k stars 2.92k forks source link

How management clients could identity OpenVPN restart via SIGHUP #499

Closed jkroepke closed 4 months ago

jkroepke commented 4 months ago

Hi,

I'm currently maintain a management client which handles the authentication for incoming users.

The management client also handles the re-authentication. Due some limitations with auth-tokens, the management client needs to store the session refresh token.

I carefully read the management client documentation

https://github.com/OpenVPN/openvpn/blob/54475711eb119f6fbb263880fca08d4b10df752a/doc/management-notes.txt#L1175-L1187

and thought it would be safe enough to bound tokens to CID. The CID should be unique and counts up unless the management interface connection is terminated.

However, today I figure out that kill -HUP $(pidof openvpn) does do a "hard restart", but it wont terminate the management client connections. Thats something what I did not expect while reading the docs

https://github.com/OpenVPN/openvpn/blob/54475711eb119f6fbb263880fca08d4b10df752a/doc/man-sections/signals.rst?plain=1#L4-L5

After kill -HUP the client ID starts and zero again which can create collision which the token store.

To mitigate the issue, OpenVPN could drop a >INFO message if os signal was received.

selvanair commented 4 months ago

However, today I figure out that kill -HUP $(pidof openvpn) does do a "hard restart", but it wont terminate the management client connections. Thats something what I did not expect while reading the docs

After kill -HUP the client ID starts and zero again which can create collision which the token store.

I'm not too familiar with how the server handles SIGHUP but if there is CID collision, it looks like a bug to me. I would think that either clients should be dropped or CID should not be reset.

Edit: Or do you mean to say that VPN clients are disconnected but your management interface client does not notice it? I thought the difference between initial auth and reauth should allow you to check whether this is a known client or not.

schwabe commented 4 months ago

HUP is an almost complete daemon restart that rereads config etc. That used to be fairly common and OpenVPN's behaviour is not an exception. If managment client is not dropped and needs a notification added that a sighup is restarted that is just an oversight since that scenario is probably quite uncommon. Patches are welcome but this doesn't feel like a super high priority.

jkroepke commented 4 months ago

managment client is not dropped

Yep, I check this via telnet. The connection will be alive even after SIGHUP. I would expect a drop here which is more expected in terms of a hard restart.

jkroepke commented 4 months ago

Patches are welcome but this doesn't feel like a super high priority.

https://github.com/OpenVPN/openvpn/blob/54475711eb119f6fbb263880fca08d4b10df752a/src/openvpn/openvpn.c#L337-L350

I guess put the close_management call into the while loop should resolve the issue?

selvanair commented 4 months ago

Not closing management client is the desired behaviour as we should be able to do SIGHUP from the management interface without getting disconnected. Changing that would break existing clients like OpenVPN-GUI.

What I meant was that all client tunnels should get closed. If that's the case, what's the harm in recycling CIDs? VPN clients will initiate new connections (not REAUTH) and the management client can handle them as such.

jkroepke commented 4 months ago

You are right. If OpenVPN sent DISCONNECT message for each closing connection to the management client, everything should be fine here.

selvanair commented 4 months ago

You will get the following messages on management interface if real-time state notification is on.

>STATE:timestamp,RECONNECTING,SIGHUP,...
>STATE:timestamp,ASSIGN_IP,...
>STATE:timestamp,CONNECTED,SUCCESS,...