[Swan-dev] end.host_addr's port

[Antony Antony]

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

I  am just sharing few  bits I re-collect from MOBIKE work.
btw have you noticed pluto_port yet? It is used as initial value host_port.

On Thu, Aug 22, 2019 at 05:06:34PM -0400, Andrew Cagney wrote:
> I'm looking at:
> 
> 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:
> 
>    .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.

for the record IKE is an IKEv2 termonlogy. during IKEv1 only days isakmp was 
popular.
 
> 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);
> 
> - 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"?

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.

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

> 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).
> 
> 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

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

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

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.