[Swan-dev] nat: ikeport commit broke DDNS tests

[Andrew Cagney]

This comment is salient:
       /*
         * .has_client means that .client contains a hardwired value,
         * if it doesn't then it is filled in later (for instance by
         * instantiate() calling default_end() after host_addr is
         * known).
         */
the ddns code isn't calling default_end() after updating .host_addr.

from:
https://testing.libreswan.org/v3.30-1728-ga28312f6bd-main/ikev2-ddns-03/OUTPUT/west.pluto.log.gz

extract_end(right) logging a warning:

connection "named": failed to convert 'right.libreswan.org' at load
time: not a numeric IPv4 address and name lookup failed (no validation
performed)

followed by default_end(), left succeeds but default_end(right)
returns immediately and silently (default_end() returns an error code
that is ignored :-):

| left host_port 500

which, correctly leads to:

| connect_to_host_pair: 192.1.2.45:500 <unspecified>:0 -> hp@(nil): none
| new hp at 0x7f52d4e8af88
added IKEv2 connection "named"

somewhere here:

updating pending dns lookups
| FOR_EACH_CONNECTION_... in connection_check_ddns
| pending ddns: changing connection "named" to CK_PERMANENT
| pending ddns: updating IP address for right.libreswan.org from
<unspecified> to 192.1.2.23
| connect_to_host_pair: 192.1.2.45:500 192.1.2.23:0 -> hp@(nil): none

default_end() should have been re-called.

On Mon, 21 Sep 2020 at 14:34, Andrew Cagney <andrew.cagney at gmail.com> wrote:

>
>
> On Mon, 21 Sep 2020 at 13:53, Paul Wouters <paul at nohats.ca> wrote:
>
>> On Sun, 20 Sep 2020, Paul Wouters wrote:
>>
>> > Subject: [Swan-dev] nat: ikeport commit broke DDNS tests
>>
>> Fixed with commit 82ffa122d2500bb7a4
>>
>> That's probably a bandaid and not the best solution, but it can be
>> cleaned up later when we cleanup the port variables more.
>>
>>
> There's more to this.   The original commit moved:
>
> @@ -818,14 +833,6 @@ static int extract_end(struct fd *whackfd,
>     leftright, src->host_ikeport);
>   dst->raw.host.ikeport = 0;
>   }
> - /*
> - * XXX: When DST is the peer setting .host_port to PLUTO_PORT
> - * (our port) is wrong.  IKE_UDP_PORT is the next best thing.
> - *
> - * But what if DST is THIS?  .host_port gets ignored?
> - */
> - dst->host_port = (dst->raw.host.ikeport ? dst->raw.host.ikeport :
> IKE_UDP_PORT);
>
> to:
>
> + /*
> + * XXX: When DST is the peer setting .host_port to PLUTO_PORT
> + * (our port) is wrong.  IKE_UDP_PORT is the next best thing.
> + *
> + * But what if DST is THIS?  .host_port gets ignored?
> + *
> + * If one end has an ikeport, the other must use ikport or nat
> + * port.
> + */
> + e->host_port = (e->raw.host.ikeport ? e->raw.host.ikeport :
> + remote_port ? NAT_IKE_UDP_PORT :
> + IKE_UDP_PORT);
> + dbg("%s host_port %d", leftright, e->host_port);
>
> where REMOTE_PORT is the other END's ikeport as specified in the config
> file.
>
> Putting back the original assignment means there's code using a wrong
> value.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20200921/6e49ec04/attachment.html>