search

OpenVPN / openvpn

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

Windows: --persist-tun on client does not work when auth-token is in use #200

Closed selvanair closed 1 year ago

selvanair commented 1 year ago

(Edited: on further look, this is not specific to DCO) Describe the bug Client on Windows using dco-win and persist-tun fails to restart on SIGUSR1 at first attempt when auth-token is in use, and goes through a second round which succeeds. But it causes existing TCP connections through the tunnel to close, as if persist-tun is not in use. If auth-token is not in use, works as expected. Edit: even without DCO, tun gets re-opened killing existing connections thrugh the tunnel, though it doesnt go through an extra cycle of SIGUSR1. So the real issue is not specific to DCO.

To Reproduce Run windows client with --persist-tun and without --windows-driver foo option so that dco-win will get used. Connect to a server that will push an auth-token. Send SIGUSR1 to the client to restart.

Expected behaviour --persist-tun should work even when auth-token is in use.

Version information (please complete the following information):

Additional Comments This may not be specific to Windows and appears to be related to tun re-opening when pulled options change -- in this case auth-token changes. A known issue, possibly?

cron2 commented 1 year ago

Is this specific to windows? Or do other platforms do the same? (half the --persist* code path on windows are different...)

selvanair commented 1 year ago

Is this specific to windows? Or do other platforms do the same? (half the --persist* code path on windows are different...)

As per logs, essentially the same on Linux -- option change is triggered with auth-token and that causes tun re-open, route delete/add etc. See the log snippet below right after pulling options on SIGUSR1 restart.

Can't we exclude auth-token from push options digest calculation as done for peer-id:

static void
push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt)
{
    char line[OPTION_PARM_SIZE];
    while (buf_parse(buf, ',', line, sizeof(line)))
    {
        /* peer-id might change on restart and this should not trigger reopening tun */
        if (strprefix(line, "peer-id "))
        {
            continue;
        }
   ....

with persist-tun and auth-token

2022-12-18 11:14:54 us=498154 Preserving previous TUN/TAP instance: tun0
2022-12-18 11:14:54 us=498294 NOTE: Pulled options changed on restart, will need to close and reopen TUN/TAP device.
2022-12-18 11:14:54 us=498433 net_route_v4_del: 192.x.y.0/24 via 10.9.0.1 dev [NULL] table 0 metric 200
2022-12-18 11:14:54 us=498689 Closing DCO interface
2022-12-18 11:14:54 us=498790 net_addr_v4_del: 10.9.0.10 dev tun0
2022-12-18 11:14:54 us=499853 net_addr_v6_del: 2600::x/64 dev tun0
2022-12-18 11:14:54 us=501051 net_iface_del: delete tun0
2022-12-18 11:14:55 us=604285 net_route_v4_best_gw query: dst 0.0.0.0
2022-12-18 11:14:55 us=604646 net_route_v4_best_gw result: via 192.x.y.1 dev enp2s0
2022-12-18 11:14:55 us=604840 ROUTE_GATEWAY 192.x.y.1/255.255.255.0 IFACE=enp2s0 HWADDR=xxx
2022-12-18 11:14:55 us=605151 net_iface_new: add tun0 type ovpn-dco
2022-12-18 11:14:55 us=606625 DCO device tun0 opened
2022-12-18 11:14:55 us=606733 do_ifconfig, ipv4=1, ipv6=1
2022-12-18 11:14:55 us=606875 MANAGEMENT: >STATE:1671380095,ASSIGN_IP,,10.9.0.10,,,,,2600::x
2022-12-18 11:14:55 us=606961 net_iface_mtu_set: mtu 1500 for tun0
2022-12-18 11:14:55 us=607078 net_iface_up: set tun0 up
2022-12-18 11:14:55 us=607231 net_addr_v4_add: 10.9.0.10/24 dev tun0
2022-12-18 11:14:55 us=607366 net_iface_mtu_set: mtu 1500 for tun0
2022-12-18 11:14:55 us=607444 net_iface_up: set tun0 up
2022-12-18 11:14:55 us=607496 net_addr_v6_add: 2600::x/64 dev tun0
2022-12-18 11:14:55 us=607584 MANAGEMENT: >STATE:1671380095,ADD_ROUTES,,,,,,
2022-12-18 11:14:55 us=607626 net_route_v4_add: 192.x.y.0/24 via 10.9.0.1 dev [NULL] table 0 metric 200
2022-12-18 11:14:55 us=607727 Data Channel: using negotiated cipher 'AES-256-GCM'
2022-12-18 11:14:55 us=607784 Data Channel MTU parms [ mss_fix:1400 max_frag:0 tun_mtu:1500 tun_max_mtu:1600 headroom:136 payload:1768 tailroom:136 ET:0 ]
2022-12-18 11:14:55 us=607907 Initialization Sequence Completed

with persist-tun and no auth-token 

2022-12-18 12:30:04 us=941732 Preserving previous TUN/TAP instance: tun0
2022-12-18 12:30:04 us=942060 Data Channel: using negotiated cipher 'AES-256-GCM'
2022-12-18 12:30:04 us=942183 Data Channel MTU parms [ mss_fix:1400 max_frag:0 tun_mtu:1500 tun_max_mtu:1600 headroom:136 payload:1768 tailroom:136 ET:0 ]
2022-12-18 12:30:04 us=942496 Initialization Sequence Completed

Linux seems to handle such network resets better though --- for example, ssh connections through the tunnel recovers on tun-reopen on Linux, not on Windows.

cron2 commented 1 year ago

Hi,

On Sun, Dec 18, 2022 at 10:25:06AM -0800, Selva Nair wrote:

Can't we exclude auth-token from push options digest calculation as done for peer-id: [..] 2022-12-18 11:14:54 us=498154 Preserving previous TUN/TAP instance: tun0 2022-12-18 11:14:54 us=498294 NOTE: Pulled options changed on restart, will need to close and reopen TUN/TAP device.

Thanks for the log. I was a bit unclear on what the actual problem was, but this ist most clear now.

Yes, I think this would be a reasonable approach - auth-token is not a tun-relevant option. No idea where that place is in the code, but I can find it, or ACK+merge a patch from you, whoever gets there first :-)

gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany @.***

cron2 commented 1 year ago

Hi,

On Sun, Dec 18, 2022 at 08:27:23PM +0100, Gert Doering wrote:

Yes, I think this would be a reasonable approach - auth-token is not a tun-relevant option. No idea where that place is in the code, but I can find it, or ACK+merge a patch from you, whoever gets there first :-)

This is settled, the patch is already on the list.

Will take care of it, thanks.

gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany @.***

lstipakov commented 1 year ago

There is a bit more to that - at the moment there is no support for "Pulled options changed on restart, will need to close and reopen TUN/TAP device" case for dco-win. When this happens (for example, after reconnect with persist-tun client got new IP address or routes), client triggers SIGUSR1 and performs another reconnect. See this commit for details.

This is because DCO handle is a part of struct tuntap. Reopening tun device means wiping it. I wasn't sure this specific use case would justify dco/tuntap refactoring.

selvanair commented 1 year ago

There is a bit more to that - at the moment there is no support for "Pulled options changed on restart

That explains why two rounds of SIGUSR1 restarts in case of dco-win. Its doesn't look that critical as anyway routes get deleted and re-created when pull options change, so this extra restart may be only a minor annoyance. More important to restrict this tun-reopen case to only when tun-related options change (patch on list).