Fix client connection instant timeout

PATCH In review David Sommerseth 3 participants 5 messages Aug 25, 2016 – Aug 26, 2016 1 message not in the archive

Share & tools

David Sommerseth

author

2016-08-25 21:54 UTC #0 lore raw

PATCH 1/1 Fix client connection instant timeout

Commit b3e975824ea9ebae8dbea5b451c8d02525c83ffe moved the finalizing of TCP/UDP sockets before the UID/GID where dropped. But this did not factor that the timeout code had been revamped [1] in the mean time.

This ensures the timout initialization is done before the the socket finalizing has been completed.

[1] commit f2134b7bea37df15756c599b94f16d4bffafbbd6

Signed-off-by David Sommerseth <[email protected]...>

---
 src/openvpn/init.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

src/openvpn/init.c +3 -3

index ad4ebc3..5b6d246 100644
@@ -3696,6 +3696,9 @@ init_instance (struct context *c, const struct env_set *env, const unsigned int
     open_plugins (c, false, OPENVPN_PLUGIN_INIT_POST_DAEMON);
 #endif
 
+  /* initialise connect timeout timer */
+  do_init_server_poll_timeout(c);
+
   /* finalize the TCP/UDP socket */
   if (c->mode == CM_P2P || c->mode == CM_TOP || c->mode == CM_CHILD_TCP)
     do_init_socket_2 (c);

@@ -3706,9 +3709,6 @@ init_instance (struct context *c, const struct env_set *env, const unsigned int
    */
   do_uid_gid_chroot (c, c->c2.did_open_tun);
 
-  /* initialise connect timeout timer */
-  do_init_server_poll_timeout(c);
-
   /* initialize timers */
   if (c->mode == CM_P2P || child)
     do_init_timers (c, false);

Signature · 1 line

1.8.3.1

Gert Doering

2016-08-26 13:14 UTC #1 lore raw

Hi,

Quoted 8 lines · David Sommerseth

Commit b3e975824ea9ebae8dbea5b451c8d02525c83ffe moved the finalizing of TCP/UDP sockets before the UID/GID where dropped. But this did not factor that the timeout code had been revamped [1] in the mean time.
This ensures the timout initialization is done before the the socket finalizing has been completed.
[1] commit f2134b7bea37df15756c599b94f16d4bffafbbd6

This looks reasonable, and fixes the issue for me - in other words, my tests are happy

Test sets succeded: 1 1a 1b 1c 1d 2 2a 2b 2d 3 4 4a 5 8 9. Test sets failed: none.

If we keep the other one (Arne had some reservations because the amount of code in the "now-privileged" part is bigger than "just bind"), then this is a working fix - so, conditional ACK.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

David Sommerseth

2016-08-26 16:13 UTC #2 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 3 lines · Gert Doering

If we keep the other one (Arne had some reservations because the amount of code in the "now-privileged" part is bigger than "just bind"), then this is a working fix - so, conditional ACK.

If you look at the diff with these two patches and without, you basically move the do_uid_gid_chroot() from above do_init_server_poll_timeout() to down below do_init_2(). So this last patch just ensures the order of do_init_server_poll_timeout() and do_init_2() is preserved.

I can somehow see the argument that do_init_server_poll_timeout() don't need to run privileged, but since do_init_2() requires it and do_init_2() again depends on do_init_server_poll_timeout() having run I found this a more sane approach. Especially when considering what the event_timeout_init() call inside the do_init_server_poll_timeout() really does. This isn't really critical code paths, not even depending on data from the a user at all.

So if Arne feels we should reduce the amount of code running privileged during the init phase, that needs to be a very different solutions to most of the init functions. But I agree we probably can look into reducing the amount of privileged code, but I don't see that as a too urgent issue as of now.

- --
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJXwGqdAAoJEIbPlEyWcf3yk4UP/3pluKaVMOdgVGJW9F0DRdZb nS7xltrY/hU80T+hnphmlLJ533UXFsMbwikvqdmbmfdUymEioGjIEmvwfrZ7pQgI rwGf0FW6YHG/MpzZsfUUdgbCg36ea3PHcT2QJ/wvGeUKRLuEkzDAxuIQkdtbI/X4 iOrpEgIk30mjQ5UH14FMTgEG8UbQZuNiAzGe6fPftCJQI6asiv6pGheP2EpA82ll +EcSgJHHtwrDBZkYCqE31pxKNRO7NXDkm1aDxS06HNDkgc/9sQR+enZakFQ1JAOA Sb2/L24DyplfHcsn6r56ZMfsEhCS57X7cSmjeb4w1b9FG6jicGrKD3bqh+sn3SFi MOPYy78ITVqZ+gKwgrFXgp/Upr7qAm3dHF/l9lE8F8IVXlz6oRa1GDlBDd4UPh6i f1MwHxuw6MBhTfEJV86I4R0m3AWMbV4GPZDmO7UXnyoULs9OSIzfr46Jrr3BlTKD N0xqSFUxpMP6X8yp5fO8GXvqEZWhyCpjIQ+hIzo8SslL7ets1sgfvIu7Q0mz2ViL +w5y10SRLTRxoDjAtZCk04HI5DEDS4awvPq7HWFaswkPW6rbnOlfzPbzwxO4UyOP BWIxjTNa1RPazGGZx9cqsrcZWTW4WWuKuDQCJqp6AQ2g2tAjogY0/9DJPiO2Dk84 MwMhBbTdQFCe7bjxAwVF
=KKoV
-----END PGP SIGNATURE-----

A message that is not in the archive (sent off-list or not yet received): [email protected]...

Gert Doering

2016-08-26 18:58 UTC #4 lore raw

Hi,

Quoted 10 lines · David Sommerseth

Quoted 3 lines · Gert Doering
If we keep the other one (Arne had some reservations because the amount of code in the "now-privileged" part is bigger than "just bind"), then this is a working fix - so, conditional ACK.
If you look at the diff with these two patches and without, you basically move the do_uid_gid_chroot() from above do_init_server_poll_timeout() to down below do_init_2(). So this last patch just ensures the order of do_init_server_poll_timeout() and do_init_2() is preserved.

I understand that :-) - the point isn't the timeout initialization, but Arne had reservations about do_init_2() being "too big" - which is why the original patch from James wasn't merged months ago.

(Unfortunately we only discussed this on IRC, not formally on the list, so it never made a "true NAK")

But maybe the point is somewhat moot - now, only part of the code runs privileged, while before that, people needing privileged ports had to run *everything* privileged...

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

David Sommerseth

author

2016-08-26 15:15 UTC #5 lore raw

In reply to David Sommerseth

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Your patch has been applied to the master branch

commit 4db062901fba790ac35cbf0cdd360306d8f2b81f

Author: David Sommerseth
Date:   Thu Aug 25 22:42:03 2016 +0200

     Fix client connection instant timeout

     Signed-off-by: David Sommerseth <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: [email protected]...
     URL: https://www.mail-archive.com/[email protected]/msg00125.html

I also had James having a look at this patch, and he responded privately that this seems fine. Since that response did not go to the devel ML I skipped the Acked-by; but James' response should at least be mentioned.

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJXwF0mAAoJEIbPlEyWcf3yPDkP/AyI4PKiJIv0HK6NygWUU21n URY4dZG8V6xMoH+IIbKv1YhqUKNqfn8+1yCDlz10ovbvYwIx2X2AsoJVT1V/ZlNB 9ZK9841Wi4J0xK7gC44MK2gAJlLXiz1SuwXbqqGVKNrtCM/oqC+AHZZE+zYudYsp /hhiv1W+5BrUbOD7zq6QNmEQNB2AbCF3lnnJaRlptRtSC5wfOv6ut9UPANBmk5zu +gZs8PlLgtAVIWDTFlLpcD0lUb5o3yNqgRTi3yKzFdffDsPoiAE3YC3ifQYqLYZ1 Vv6rP0reeONaKd+nNjPe6ebNW0rcxvuvKdS54GnmrE5cJjBZtRdX6FmE1fwqGiZo ZzyFvWoLMBbNxaF31hKF3VaX2Vz/5oFCzVv7h0hnCVyQigO2k6BLkD4kpqWrxAac +yBb48/lySeUli/eHykl5fyR4JYarPQ19m9ZdVwtUtry3rHqfCBEbOmhbX7Kmbn9 AY6V0nFma0pcqC9vsMJskQs30bHKCLlZtGEy6f/6kz+1hU9kNIemMbEC7uHqK8Qa 8tbXQ46MvMJ9Oe5lyTHtqlyVVgwB8GLF50QdpnFW69VrrK0Xr6XMoca2FEQJvmxj 7SxU/zuPbBERdV3tZWf9S/wjbsmRdzIdgXJfzUoGLAjaLepXx6gsokgjBe+AI8bM tdWBfYfxUTJ1SxH6vUQ8
=kNnP
-----END PGP SIGNATURE-----