diff mbox series

[Openvpn-devel] Fix --remote protocol can't be set without port argument

Message ID 20200903114455.29341-1-themiron@yandex-team.ru
State Rejected, archived
Headers
Series [Openvpn-devel] Fix --remote protocol can't be set without port argument |

Commit Message

Vladislav Grishenko Sept. 3, 2020, 1:44 a.m. UTC 
  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(-)

Comments

David Sommerseth Sept. 8, 2020, 3:22 a.m. UTC | #1 
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.
Vladislav Grishenko Sept. 8, 2020, 9:01 a.m. UTC | #2 
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
>
David Sommerseth Sept. 9, 2020, 7:49 a.m. UTC | #3 
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.
Vladislav Grishenko Sept. 9, 2020, 9:02 a.m. UTC | #4 
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 mbox series

Patch

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;
             }