[Swan-dev] end.host_addr's port

[Andrew Cagney]

I thought I had one problem; now I've got two!


On Fri, 23 Aug 2019 at 10:41, Paul Wouters <paul at nohats.ca> wrote:
>
> 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.

That might explain:
        bool host_port_specific;        /* if TRUE, then IKE ports are
tested for */
which seems to only change a log line

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

The dynamic change should only conceptually affect .host_addr / .host_port.
(but might also cause .client to change if there isn't a client).

> 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 ?

Two things:

- it wraps the current working theory on what the structure should
look like in a #ifdef
Of course the theory evolves, for instance I suspect adding
uint8_t[16] to ip_address so that static initialisation is possible
using hardwired bytes

- when defined it breaks the compile in a way that helps flush out
weird and wonderful problems, like the above, which is exposed because
code is trying to pass a simple address to functions expecting
address+port