| Message ID | 20201023113431.26691-2-arne@rfc2549.org |
|---|---|
| State | Rejected |
| Delegated to: | Antonio Quartulli |
| Headers | show |
| Series | [Openvpn-devel,1/5] Inline function tls_get_peer_info | expand |
Hi, On Fri, Oct 23, 2020 at 01:34:28PM +0200, Arne Schwabe wrote: > Make options.c only set xmit_hold when port_share is active to least > document this dependency. I have not actually tested if this dependency > is actually true (or if port_share could work without xmit_hold). I'm not sure I understand this patch, or the flow. Before this patch, xmit_hold is always set to "true" for TCP_SERVER, after this patch, only for server *and* port_share. What am I overlooking? gert
Am 24.10.20 um 21:43 schrieb Gert Doering: > Hi, > > On Fri, Oct 23, 2020 at 01:34:28PM +0200, Arne Schwabe wrote: >> Make options.c only set xmit_hold when port_share is active to least >> document this dependency. I have not actually tested if this dependency >> is actually true (or if port_share could work without xmit_hold). > > I'm not sure I understand this patch, or the flow. Before this patch, > xmit_hold is always set to "true" for TCP_SERVER, after this patch, > only for server *and* port_share. > > What am I overlooking? > No. This feature was introduced as part of port-share and I am not even sure it is really required for port-share. Normally we do not send anything unless there is a valid OpenVPN RESET packet. So I want to limit it again to part-share before I can really reliable say that even port-share doesn't need it. Arne
Hi, On 26/10/2020 10:49, Arne Schwabe wrote: > Am 24.10.20 um 21:43 schrieb Gert Doering: >> Hi, >> >> On Fri, Oct 23, 2020 at 01:34:28PM +0200, Arne Schwabe wrote: >>> Make options.c only set xmit_hold when port_share is active to least >>> document this dependency. I have not actually tested if this dependency >>> is actually true (or if port_share could work without xmit_hold). >> >> I'm not sure I understand this patch, or the flow. Before this patch, >> xmit_hold is always set to "true" for TCP_SERVER, after this patch, >> only for server *and* port_share. >> >> What am I overlooking? >> > > No. This feature was introduced as part of port-share and I am not even > sure it is really required for port-share. Normally we do not send > anything unless there is a valid OpenVPN RESET packet. So I want to > limit it again to part-share before I can really reliable say that even > port-share doesn't need it. > To add some more meat to the discussion... The xmit_hold option member was introduced by 6add6b2fe78c549d174729869e26cee917e31d5f ("Added --port-share option..."), so like Arne said it was added as aid of the port-share feature. My understanding is that setting this member to true will prevent a peer from sending any control packet until reliable_schedule_now() is called for the first time. Any idea why this could be helpful with port-share? Maybe in 2006 an OpenVPN server could decide to send something even before having received the opening packet from the connecting client? If that's the case, then I understand the hold. However, in the current code, reliable_schedule_now() is invoked when we do the first "burst" upon client connection. This seems to happen right after receiving the first valid HARD_RESET packet. For this reason I tend to agree with Arne that this "xmit_hold" knob is not really doing anything useful. Anybody can see why we need that for port-share? I'd rather try to understand the above and ditch this member entirely, if the answer is "we don't need it". Cheers,
Hi, On 25/03/2021 22:37, Antonio Quartulli wrote: > Anybody can see why we need that for port-share? > I'd rather try to understand the above and ditch this member entirely, > if the answer is "we don't need it". After more tests I indeed recovered the reason why this is needed with port-share. If we don't "hold back", upon connection the server will send its own SERVER_HARD_RESET, regardless of the client having sent a CLIENT_HARD_RESET or not. This behaviour indeed breaks port-share because the non-OpenVPN application on the other side will receive an unexpected packet and will most likely kill the connection. For this reason we need to hold any control channel packet until we have confirmed that the first packet is indeed a CLIENT_HARD_RESET. Regarding TCP, the only thing I can think about is that this flag helps mimicking UDP: instead of sending the HARD_RESET right after a TCP Connection is opened, the server will wait for the client packet first. In UDP I presume this is standard behaviour as we have no connection opening event. My guess is that this behaviour may be useful to avoid race conditions on the client where the SERVER_HARD_RESET is received when still unexpected? But maybe the code has evolved enough to not requiring this sequence of events anymore. Cheers,
Hi, On 25/03/2021 22:55, Antonio Quartulli wrote: > Hi, > > On 25/03/2021 22:37, Antonio Quartulli wrote: >> Anybody can see why we need that for port-share? >> I'd rather try to understand the above and ditch this member entirely, >> if the answer is "we don't need it". > > After more tests I indeed recovered the reason why this is needed with > port-share. > > If we don't "hold back", upon connection the server will send its own > SERVER_HARD_RESET, regardless of the client having sent a > CLIENT_HARD_RESET or not. > > This behaviour indeed breaks port-share because the non-OpenVPN > application on the other side will receive an unexpected packet and will > most likely kill the connection. > > For this reason we need to hold any control channel packet until we have > confirmed that the first packet is indeed a CLIENT_HARD_RESET. > > > Regarding TCP, the only thing I can think about is that this flag helps > mimicking UDP: instead of sending the HARD_RESET right after a TCP > Connection is opened, the server will wait for the client packet first. > > In UDP I presume this is standard behaviour as we have no connection > opening event. > > My guess is that this behaviour may be useful to avoid race conditions > on the client where the SERVER_HARD_RESET is received when still unexpected? > As discussed on IRC we don't really need this "hold" behaviour in our protocol. I may ask "why changing the current state though?", but I believe removing state variables from our code base (or some of its branches) is a good step towards simplification. My last nitpick would be to change the commit message, since now we have clarified how this works: ---- The xmit_hold flag is used to tell OpenVPN to not send any packet out before a RESET is received from the other peer upon new connection. The only real use case for setting this flag to true is when configuring OpenVPN with port-share (TCP). The reason being that OpenVPN server should not send any packet until it is confirmed that the connection was established by an actual OpenVPN client. If the connection was established by a non-OpenVPN client, sending a RESET will mess up with the protocol sharing the port. ---- * Basic TCP connection tests passed. * Compiled in my gitlab CI *WITH* issues: MINGW32 both 32bit and 64bit failed: i686-w64-mingw32-gcc -DHAVE_CONFIG_H -I. -I../.. -I../../include -I../../include -I../../src/compat -DWIN32_LEAN_AND_MEAN -DNTDDI_VERSION=NTDDI_VISTA -D_WIN32_WINNT=_WIN32_WINNT_VISTA -I/builds/ordex986/openvpn/tap-windows-9.21.2/include -I/root/opt/include -I/root/opt/include -I/root/opt/include -DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\" -municode -UUNICODE -Wall -Wno-unused-parameter -Wno-unused-function -Wno-stringop-truncation -g -O2 -std=c99 -MT lzo.o -MD -MP -MF .deps/lzo.Tpo -c -o lzo.o lzo.c In file included from buffer.h:28, from mtu.h:27, from win32.h:30, from init.c:36: init.c: In function 'do_open_tun': init.c:1809:62: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] msg(D_ROUTE, "interactive service msg_channel=%" PRIu64, (unsigned long long) c->options.msg_channel); ^ error.h:151:67: note: in definition of macro 'msg' #define msg(flags, ...) do { if (msg_test(flags)) {x_msg((flags), __VA_ARGS__);} EXIT_FATAL(flags); } while (false) ^~~~~~~~~~~ init.c: In function 'do_init_crypto_tls': init.c:2889:19: error: 'const struct options' has no member named 'port_share_host' && options->port_share_host) ^~ make[3]: *** [Makefile:743: init.o] Error 1 The first part is a warning that we can ignore for the sake of this patch. The bottom part is the actual error. Cheers,
Hi, As discussed with Arne in our latest meeting, this patch will be ignored for now. Hope was that the xmit_hold filed could be eventually removed, but it turned out it is actually needed. For this reason there is no real interest at this point to carry on this change. Regards,
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 034edba0..fdd9a6cc 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2908,12 +2908,15 @@ do_init_crypto_tls(struct context *c, const unsigned int flags) to.push_peer_info_detail = 0; } +#ifdef ENABLE_PORT_SHARE /* should we not xmit any packets until we get an initial * response from client? */ - if (to.server && options->ce.proto == PROTO_TCP_SERVER) + if (to.server && options->ce.proto == PROTO_TCP_SERVER + && options->port_share_host) { to.xmit_hold = true; } +#endif to.disable_occ = !options->occ;
Make options.c only set xmit_hold when port_share is active to least document this dependency. I have not actually tested if this dependency is actually true (or if port_share could work without xmit_hold). Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/init.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)