[Swan-dev] end.host_addr's port

[Paul Wouters]

On Fri, 23 Aug 2019, Antony Antony wrote:

> yes too many organically grown workarounds. MOBIKE and NAT stretched these
> ideas quite a bit.

Indeed.

>> struct end {
>>        ip_address host_addr;
>>         ip_subnet client;
>>         bool has_client;
>>         bool has_client_wildcard;
>>         bool has_port_wildcard;
>>         uint16_t host_port;             /* where the IKE port is */
>>         uint16_t port;                  /* port number, if per-port keying */
>> }
>>
>> and am puzzled by .port vs .host_port and .client vs .host[addr].  My
>> working theory was that things were paired:

Some of the issues stem from freeswan's original concept of host with or
without subnets. It copies from host* to client* if no subnets are set.

I think Hugh used the per port keying to run multiple plutos on the same
host for testing decades ago. I'm not sure if that's where the "port" vs
"host_port" came from.

It is also possible that it came in with the NAT-T stuff, where we went
from IKE port to "IKE port or NAT IKE port".

It would make sense to clean this up. For instance whether or not to use
the NAT IKE port (4500) mostly depends on whether NAT was detected, so
that property should live in the state and not in c->spd.this/that.

>>    .client and .port (what IKEv2 calls a traffic selector)
>>    .host_addr and .host_port (the IKE endpoint)
>
> This theory sounds good, possibly over simplistic. It is missing the part
> dynamic change. The need copy from c to st and vice versa is complicated with
> MOBIKE. It touch what is in c and what is in st, especially for an
> instantiated connection.

Yes, I avoided the understanding and cleanup you are doing now when I
worked on MOBIKE. I tried to keep everything in struct state that would
"override" things, and tried to not change struct connection as much as
possible.

>> but, in the case of .host_addr, the code seems to be fighting itself
>> over what the port should be.  For instance:
>>
>> - in ikev2_ts.c the .host_addr's port is forced to the negotiated TS
>> client port:
>>
>>     c->spd.that.client = tmp_subnet_r;
>>     c->spd.that.port = st->st_ts_that.startport;
>>     c->spd.that.protocol = st->st_ts_that.ipprotoid;
>>     setportof(htons(c->spd.that.port),
>>           &c->spd.that.host_addr);
>>     setportof(htons(c->spd.that.port),
>>           &c->spd.that.client.addr);

The traffic selectors and the IKE address and port _should_ not be
related or mixed up or sync'ed in theory. But with "narrowing" we
have the case where the traffic selector set might be a subset of
the c->spd.that/this.client and c->spd.this/that.[port|protocol].
So what we then do is instantiate the connection, and only once
scrible over the this/that properties. From then on, these are
again "immutable" for that (instantiated) connection's lifetime.
Note that a connection supporting narrowing is for that reason
created as a CK_TEMPLATE, so even if the TS covers everything,
it is instantiated.

>> - but then in state.c:mobike, it's forced to the sender's port
>> (.sender has probably always had the port embedded in it).
>>
>>         /* MOBIKE responder processing request */
>>         c->spd.that.host_addr = md->sender;
>>         c->spd.that.host_port = hportof(&md->sender);
>
> My recollections are vague and I didn't double checked it in git master.
>
> you need IKE port in a connection for two reasons. To survive a restart,
> short interruptions, say dpd. Second for output "ipsec status"?

I think the copying from sender is more to survive NAT changes. If the
NAT router maps the port differently, we need to change our end to send
to this new (IP/)port. But we should only do that after we decrypted and
authenticated the packet as "new" so a forged replay would not force us
back. This is the area where there is some complicated interaction
between NAT routers changing things and MOBIKE changing things.

> Note when initiating a connection a call to set_state_ike_endpoints.
>
> set_state_ike_endpoints(st, c)
> 	st->st_remoteaddr = c->spd.that.host_addr;
> 	st->st_remoteport = c->spd.that.host_port;
>
> While adding MOBIKE, Paul and I realized copying port and host_addr to c
> would be a limitation for a longer connection interruptions and restart.
> However, we used a reasoning most use cases are only for right=%any. Those
> connections wouldn't initiate after say a dpd restart or when the other end
> reboots.
>
> The MOBIKE overwriting host_port and host_addr in c is a limitation!
> In most use cases Libreswan is MOBIKE responder with right=%any so it sort
> of works. That is why you are seeing overwriting what is in c, which is
> confusing.

Right. Although in this case, if the information is struct state was
more reliable, we could potentially not override the connection. Note
MOBIKE is incompatible with transport mode so there is always a
this/that client when we are changing the host addr/port.

> on the responder one odd bit for me is:
> calls in  instantiate()
> 	port = htons(dst->port);
> 	setportof(port, &dst->host_addr);

I have no memory of this specifically. It could be that this originally
comes from the md->st where the ip/port info lives before we have
matched a connection. Then when we match it, we might have (due to hairy
code) overwritten the connection's port to match the sender port in the
md.

>> A look at *_raw_eroute() shows .host_port is ignored (I thought it was
>> used, but it turns out that was only for prettying an error).

Yes, eroutes should only work on *client, not *host. And for transport
mode and for host-to-host tunnel mode connections, the host variables
are copied as client variables.

>> A look at .has_client shows more promise, the code seems to copy
>> .host_addr into .client vis:
>>
>>     /* default client to subnet containing only self
>>      * XXX This may mean that the client's address family doesn't match
>>      * tunnel_addr_family.
>>      */
>>     if (!c->spd.that.has_client)
>>         addrtosubnet(&c->spd.that.host_addr, &c->spd.that.client);
>>
>> and, I I'm guessing, is assuming that /host_addr's port is still set
>> to .port (the client port).
>
> Also note the comparisons to pluto_port.
>
> I think we can delete some of them.
> c->spd.this.port can go, use c->spd.this.client.addr.port instead

I think that is the wrong abstraction. The this/that.client should only
relate to the ipsec policy for src/dst and not contain anything related
to the host IP or port used for IKE. So in my opinion,
c->spd.this.client.addr.port should be removed or always be 0.

> host_port can be ditched in favor of host_addr's port. This might be a bit
> invasive change in v1 and v2.

Additionally, I'm about to add code to support leftikeport= and
rightikeport= to support openshift/kubernetes containers. So if we are
doing a cleanup, which is much needed, let's ensure we then also take
this new feature into account.

> I wonder what to do with ikev2 ts, same information has multiple copies in
> st, and in c.

Perhaps rewrite this only after we have ditched IKEv1 ?

> while we are at it, recent changes bc4d25c065b  #ifdef ENDPOINT_ADDRESS_PORT
> is a bit confusing to me. These started a while ago? 25bcb7f6cb0. Too many
> changes in flight.

I think this is Andrew's way of porting code and testing before flipping
everything to "new method" when he removes the define ?

Paul