[Openvpn-devel] Add 'printing of port number' to mroute_addr_print_ex() for v4-mapped v6.

  For whatever reason, this function never printed port numbers for
IPv6 addresses (but it did for IPv4) - which creates a bit of
confusion for IPv6-mapped v4 addresses on a dual stack socket,
that will have ports numbers printed or not, depending on whether
it's a dual-stack v6 socket or single-stack v4.

This will not(!) add printing of port numbers for "proper" v6
addresses yet, because that might have adverse side effects to address
parsing elsewhere.

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/mroute.c | 7 +++++++
 1 file changed, 7 insertions(+)
  

Hi,

On 07/12/2018 22:33, Gert Doering wrote:
> For whatever reason, this function never printed port numbers for
> IPv6 addresses (but it did for IPv4) - which creates a bit of
> confusion for IPv6-mapped v4 addresses on a dual stack socket,
> that will have ports numbers printed or not, depending on whether
> it's a dual-stack v6 socket or single-stack v4.
> 
> This will not(!) add printing of port numbers for "proper" v6
> addresses yet, because that might have adverse side effects to address
> parsing elsewhere.
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  src/openvpn/mroute.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
> index 28940a8d..134f4c00 100644
> --- a/src/openvpn/mroute.c
> +++ b/src/openvpn/mroute.c
> @@ -477,6 +477,13 @@ mroute_addr_print_ex(const struct mroute_addr *ma,
>                  {
>                      buf_printf(&out, "%s", print_in_addr_t(maddr.v4mappedv6.addr,
>                                                             IA_NET_ORDER, gc));
> +		    /* we only print port numbers for v4mapped v6 as of
> +		     * today, because "v6addr:port" is too ambiguous
> +		     */

The comments above are indented with tab+spaces, while the code below is
just all spaces. Please use the all spaces everywhere.

> +                    if (maddr.type & MR_WITH_PORT)
> +                    {
> +                        buf_printf(&out, ":%d", ntohs(maddr.v6.port));
> +                    }

I don't understand how is this solving the ambiguity? Or you are just
saying: "we can't do much, let's just print the port anyway"? My
suggestion would be to surround the address with [], so basically change
the printf format above from %s to [%s]. Does it make sense?


Cheers,

>                  }
>                  else
>                  {
>
  

Hi,

On Sat, Dec 08, 2018 at 09:47:37AM +1000, Antonio Quartulli wrote:
> > +		    /* we only print port numbers for v4mapped v6 as of
> > +		     * today, because "v6addr:port" is too ambiguous
> > +		     */
> 
> The comments above are indented with tab+spaces, while the code below is
> just all spaces. Please use the all spaces everywhere.

Gah.  That was the intention, but the comment was added later on and
using the wrong editor :-)

> > +                    if (maddr.type & MR_WITH_PORT)
> > +                    {
> > +                        buf_printf(&out, ":%d", ntohs(maddr.v6.port));
> > +                    }
> 
> I don't understand how is this solving the ambiguity? 

These are *v4* addresses, just masquerading as v6 in the socket structure.

So 1.2.3.4:567 is never ambiguous.

For "true v6 addresses" (the other branch) we keep on not printing the
port number.

> Or you are just
> saying: "we can't do much, let's just print the port anyway"? My
> suggestion would be to surround the address with [], so basically change
> the printf format above from %s to [%s]. Does it make sense?

This is something I want to discuss.  Changing mroute_print_addr_ex()
for "true v6 addresses" and printing the port number in a new format
will affect status file printing, management interface, etc. - so it
needs to be well considered.

*This* patch just fixes the discrepancy that v4 addresses are printed
"with port" if doing "proto udp4", and "without port" if a v4 connect
comes in on a "proto udp / v6only=no" socket.  (We do not need to discuss
how many problems the v4mapped v6 address format brings with it...)

gert
  

Hi,

On 08/12/2018 18:03, Gert Doering wrote:
> Hi,
> 
> On Sat, Dec 08, 2018 at 09:47:37AM +1000, Antonio Quartulli wrote:
>>> +		    /* we only print port numbers for v4mapped v6 as of
>>> +		     * today, because "v6addr:port" is too ambiguous
>>> +		     */
>>
>> The comments above are indented with tab+spaces, while the code below is
>> just all spaces. Please use the all spaces everywhere.
> 
> Gah.  That was the intention, but the comment was added later on and
> using the wrong editor :-)
> 
>>> +                    if (maddr.type & MR_WITH_PORT)
>>> +                    {
>>> +                        buf_printf(&out, ":%d", ntohs(maddr.v6.port));
>>> +                    }
>>
>> I don't understand how is this solving the ambiguity? 
> 
> These are *v4* addresses, just masquerading as v6 in the socket structure.
> 
> So 1.2.3.4:567 is never ambiguous.
> 
> For "true v6 addresses" (the other branch) we keep on not printing the
> port number.
> 
>> Or you are just
>> saying: "we can't do much, let's just print the port anyway"? My
>> suggestion would be to surround the address with [], so basically change
>> the printf format above from %s to [%s]. Does it make sense?
> 
> This is something I want to discuss.  Changing mroute_print_addr_ex()
> for "true v6 addresses" and printing the port number in a new format
> will affect status file printing, management interface, etc. - so it
> needs to be well considered.
> 
> *This* patch just fixes the discrepancy that v4 addresses are printed
> "with port" if doing "proto udp4", and "without port" if a v4 connect
> comes in on a "proto udp / v6only=no" socket.  (We do not need to discuss
> how many problems the v4mapped v6 address format brings with it...)
> 

Thanks for the clarification.

The patch looks good and does what it says on the lid.
Tested by checking the status output (with and without the patch) and
everything looked good.

Now it only needs to have the tabs fixed on the comment lines, but other
than that:

Acked-by: Antonio Quartulli <antonio@openvpn.net>


Regards,
  

Patch has been applied to the master and release/2.4 branch (as I see
this as a bugfix).  Uncrustify applied, tabs are spaced.

"Print port numbers on v6 addresses" landed on my agenda again, need
to make up my mind on address format and then come up with a proposal.

commit 4543b13b8540836f6faf67a03b5358bb8bb94a4a (master)
commit c2f7058700bc12858a74c53266534c567d1b05f2 (release/2.4)
Author: Gert Doering
Date:   Fri Dec 7 13:33:03 2018 +0100

     Add 'printing of port number' to mroute_addr_print_ex() for v4-mapped v6.

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20181207123303.70827-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17996.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering