I thought I had one problem; now I've got two!
On Fri, 23 Aug 2019 at 10:41, Paul Wouters <[email protected]> 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 _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
