| Message ID | 20200903114455.29341-1-themiron@yandex-team.ru |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [Openvpn-devel] Fix --remote protocol can't be set without port argument | expand |
On 03/09/2020 13:44, Vladislav Grishenko wrote: > According client-options.rst additional argumets ``port`` and ``proto`` > are both optional and it's allowed to have port absent and protocol set: > --remote args > Examples: > remote server.example.net tcp > > But when protocol is set without preceeding port argument, it is being > misparsed as a port with subsequent error: > RESOLVE: Cannot resolve host address: server.example.net:tcp > (Servname not supported for ai_socktype) > > Since protocol names are predefined and don't match service names, fix > this behavior by checking second argument for valid protocol first. > > Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru> Uhm ... I'm leaning towards a NAK to this patch and would rather suggest updating the man page to be correct. This is a mistake from my side when converting the man page to .rst files. The example should be: remote server.example.net 1194 tcp The OpenVPN 2.4 and prior releases has this line: --remote host [port] [proto] But this syntax was not supported by rst2man, so it was replaced with "args" and the examples coming below; which carried an error.
Hi David, > -----Original Message----- > From: David Sommerseth <openvpn@sf.lists.topphemmelig.net> > Sent: Tuesday, September 8, 2020 6:23 PM > To: Vladislav Grishenko <themiron@yandex-team.ru>; openvpn- > devel@lists.sourceforge.net > Subject: Re: [Openvpn-devel] [PATCH] Fix --remote protocol can't be set without > port argument > > On 03/09/2020 13:44, Vladislav Grishenko wrote: > > According client-options.rst additional argumets ``port`` and > > ``proto`` are both optional and it's allowed to have port absent and protocol > set: > > --remote args > > Examples: > > remote server.example.net tcp > > > > But when protocol is set without preceeding port argument, it is being > > misparsed as a port with subsequent error: > > RESOLVE: Cannot resolve host address: server.example.net:tcp > > (Servname not supported for ai_socktype) > > > > Since protocol names are predefined and don't match service names, fix > > this behavior by checking second argument for valid protocol first. > > > Uhm ... I'm leaning towards a NAK to this patch and would rather suggest > updating the man page to be correct. This is a mistake from my side when > converting the man page to .rst files. The example should be: > > remote server.example.net 1194 tcp > Hum. Initially all variants were supported, by checking numeric port and taking it as proto, if not numeric. Later port became string servname and optional logic was lost. Man pages still has all of them since that time: remote server.exmaple.net remote server.exmaple.net 1194 remote server.exmaple.net tcp At first look there is no need for proto w/o port set, why it can be important? With support of server host & port discovery (DNS SRV RFC 2782), port info is not required, only host and protocol make sense. So I though about this one little step forward toward ~consistent syntax. Does it makes any sense? > The OpenVPN 2.4 and prior releases has this line: > > --remote host [port] [proto] > > But this syntax was not supported by rst2man, so it was replaced with "args" > and the examples coming below; which carried an error. > > > -- > kind regards, > > David Sommerseth > OpenVPN Inc >
On 08/09/2020 21:01, Vladislav Grishenko wrote: > Hi David, > >> -----Original Message----- >> From: David Sommerseth <openvpn@sf.lists.topphemmelig.net> >> Sent: Tuesday, September 8, 2020 6:23 PM >> To: Vladislav Grishenko <themiron@yandex-team.ru>; openvpn- >> devel@lists.sourceforge.net >> Subject: Re: [Openvpn-devel] [PATCH] Fix --remote protocol can't be set without >> port argument >> >> On 03/09/2020 13:44, Vladislav Grishenko wrote: >>> According client-options.rst additional argumets ``port`` and >>> ``proto`` are both optional and it's allowed to have port absent and protocol >> set: >>> --remote args >>> Examples: >>> remote server.example.net tcp >>> >>> But when protocol is set without preceeding port argument, it is being >>> misparsed as a port with subsequent error: >>> RESOLVE: Cannot resolve host address: server.example.net:tcp >>> (Servname not supported for ai_socktype) >>> >>> Since protocol names are predefined and don't match service names, fix >>> this behavior by checking second argument for valid protocol first. >>> >> Uhm ... I'm leaning towards a NAK to this patch and would rather suggest >> updating the man page to be correct. This is a mistake from my side when >> converting the man page to .rst files. The example should be: >> >> remote server.example.net 1194 tcp >> > > Hum. Initially all variants were supported, by checking numeric port and > taking it as proto, if not numeric. Later port became string servname and > optional logic was lost. The man page history disagrees ... even though, it doesn't say this order is explicit. 2.4 - <https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage#lbAG> 2.3 - <https://community.openvpn.net/openvpn/wiki/Openvpn23ManPage#lbAG> 2.2 - <https://community.openvpn.net/openvpn/wiki/Openvpn22ManPage> 2.1 - <https://community.openvpn.net/openvpn/wiki/Openvpn21ManPage#lbAG> > Man pages still has all of them since that time: >> remote server.exmaple.net >> remote server.exmaple.net 1194 >> remote server.exmaple.net tcp These lines comes from this commit: <https://github.com/OpenVPN/openvpn/commit/f500c49c8e0a77ce665b11f6adbea4029cf3b85f> Where the last line (missing port number) is incorrect. I also don't see any commit changing the "remote" parsing in options.c in the way you describe. This section has just few changes over the years, but even back to changes 7-12 years ago, the second argument has always been processed as a port number and the third one protocol. Both causing an error if it is not a valid value. So putting protocol in the second argument would trigger an error in 2.1, 2.2, 2.3 and 2.4 as far as I can see. > > At first look there is no need for proto w/o port set, why it can be > important? With support of server host & port discovery (DNS SRV RFC 2782), > port info is not required, only host and protocol make sense. So I though > about this one little step forward toward ~consistent syntax. Does it makes > any sense? I don't see why OpenVPN's --remote parsing needs to comply with DNS SRV RFC standards. How is that related at all? I know we have patches for adding DNS SRV query support, but that doesn't imply passing this information via add_option(), does it? If you use --remote vpn.example.com --proto tcp .... that will already work. What will not work is --remote vpn.example.com tcp Also, this will work in config files: ------------------------------- port 5000 proto udp <connection> remote vpn1.example.com </connection> <connetion> remote vpn2.example.com proto tcp </connection> ------------------------------- All of these use cases makes sense, as it depends on a pre-set value. What does not make sense to me is to muddy a pretty clearly defined argument order which has been the standard since the beginning of OpenVPN.
Ok, thank you for clarification -- Best Regards, Vladislav Grishenko > -----Original Message----- > From: David Sommerseth <openvpn@sf.lists.topphemmelig.net> > Sent: Wednesday, September 9, 2020 10:49 PM > To: Vladislav Grishenko <themiron.ru@gmail.com>; openvpn- > devel@lists.sourceforge.net > Subject: Re: [Openvpn-devel] [PATCH] Fix --remote protocol can't be set without > port argument > > On 08/09/2020 21:01, Vladislav Grishenko wrote: > > Hi David, > > > >> -----Original Message----- > >> From: David Sommerseth <openvpn@sf.lists.topphemmelig.net> > >> Sent: Tuesday, September 8, 2020 6:23 PM > >> To: Vladislav Grishenko <themiron@yandex-team.ru>; openvpn- > >> devel@lists.sourceforge.net > >> Subject: Re: [Openvpn-devel] [PATCH] Fix --remote protocol can't be > >> set without port argument > >> > >> On 03/09/2020 13:44, Vladislav Grishenko wrote: > >>> According client-options.rst additional argumets ``port`` and > >>> ``proto`` are both optional and it's allowed to have port absent and > >>> protocol > >> set: > >>> --remote args > >>> Examples: > >>> remote server.example.net tcp > >>> > >>> But when protocol is set without preceeding port argument, it is > >>> being misparsed as a port with subsequent error: > >>> RESOLVE: Cannot resolve host address: server.example.net:tcp > >>> (Servname not supported for ai_socktype) > >>> > >>> Since protocol names are predefined and don't match service names, > >>> fix this behavior by checking second argument for valid protocol first. > >>> > >> Uhm ... I'm leaning towards a NAK to this patch and would rather > >> suggest updating the man page to be correct. This is a mistake from > >> my side when converting the man page to .rst files. The example should be: > >> > >> remote server.example.net 1194 tcp > >> > > > > Hum. Initially all variants were supported, by checking numeric port > > and taking it as proto, if not numeric. Later port became string > > servname and optional logic was lost. > > The man page history disagrees ... even though, it doesn't say this order is > explicit. > > 2.4 - > <https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage#lbAG> > 2.3 - > <https://community.openvpn.net/openvpn/wiki/Openvpn23ManPage#lbAG> > 2.2 - <https://community.openvpn.net/openvpn/wiki/Openvpn22ManPage> > 2.1 - > <https://community.openvpn.net/openvpn/wiki/Openvpn21ManPage#lbAG> > > > Man pages still has all of them since that time: > >> remote server.exmaple.net > >> remote server.exmaple.net 1194 > >> remote server.exmaple.net tcp > > These lines comes from this commit: > <https://github.com/OpenVPN/openvpn/commit/f500c49c8e0a77ce665b11f6a > dbea4029cf3b85f> > > Where the last line (missing port number) is incorrect. > > I also don't see any commit changing the "remote" parsing in options.c in the > way you describe. This section has just few changes over the years, but even > back to changes 7-12 years ago, the second argument has always been > processed as a port number and the third one protocol. Both causing an error if > it is not a valid value. So putting protocol in the second argument would trigger > an error in 2.1, 2.2, 2.3 and 2.4 as far as I can see. > > > > > At first look there is no need for proto w/o port set, why it can be > > important? With support of server host & port discovery (DNS SRV RFC > > 2782), port info is not required, only host and protocol make sense. > > So I though about this one little step forward toward ~consistent > > syntax. Does it makes any sense? > I don't see why OpenVPN's --remote parsing needs to comply with DNS SRV RFC > standards. How is that related at all? I know we have patches for adding DNS > SRV query support, but that doesn't imply passing this information via > add_option(), does it? > > If you use --remote vpn.example.com --proto tcp .... that will already work. > What will not work is --remote vpn.example.com tcp > > Also, this will work in config files: > ------------------------------- > port 5000 > proto udp > > <connection> > remote vpn1.example.com > </connection> > <connetion> > remote vpn2.example.com > proto tcp > </connection> > ------------------------------- > > All of these use cases makes sense, as it depends on a pre-set value. What does > not make sense to me is to muddy a pretty clearly defined argument order which > has been the standard since the beginning of OpenVPN. > > > -- > kind regards, > > David Sommerseth > OpenVPN Inc >
diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 8bf82c57..02ac08d8 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -5682,16 +5682,26 @@ add_option(struct options *options, re.remote = p[1]; if (p[2]) { - re.remote_port = p[2]; - if (p[3]) + /* Since port is optional, second parameter can be a protocol */ + int proto = ascii2proto(p[2]); + sa_family_t af = ascii2af(p[2]); + if (proto < 0) { - const int proto = ascii2proto(p[3]); - const sa_family_t af = ascii2af(p[3]); - if (proto < 0) + /* Second is not proto, port then. Protocol should be third */ + re.remote_port = p[2]; + if (p[3]) { - msg(msglevel, "remote: bad protocol associated with host %s: '%s'", p[1], p[3]); - goto err; + proto = ascii2proto(p[3]); + af = ascii2af(p[3]); + if (proto < 0) + { + msg(msglevel, "remote: bad protocol associated with host %s: '%s'", p[1], p[3]); + goto err; + } } + } + if (proto >= 0) + { re.proto = proto; re.af = af; }
According client-options.rst additional argumets ``port`` and ``proto`` are both optional and it's allowed to have port absent and protocol set: --remote args Examples: remote server.example.net tcp But when protocol is set without preceeding port argument, it is being misparsed as a port with subsequent error: RESOLVE: Cannot resolve host address: server.example.net:tcp (Servname not supported for ai_socktype) Since protocol names are predefined and don't match service names, fix this behavior by checking second argument for valid protocol first. Signed-off-by: Vladislav Grishenko <themiron@yandex-team.ru> --- src/openvpn/options.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)