So to recap: "host" is the end's endpoint (ADDRESS:PORT), where PORT is the IKE port -> there's a "bug" were the "client"'s port is being stuffed into the host; the suspicion is that this comes from clientless OE connections where the code is passed an address - the "host" (with munged port, but addresses don't have ports) instead of the client
"client" is the end's subnet (but I think of it as traffic portal or traffic selector) - anything sent to this instead goes through IPSEC, in libreswan it can be: - a routing prefix (aka traditional subnet - an endpoint aka address:port (for OE this is a "clientless" connection, but what about non-oe, does the kernel need to handle it differently?) - an address (is this really a routing prefix but for one address, or an endpoint but with a wild port?) "shunt" is the blockage stuffed into the network stack blocking traffic; it could be a routing prefix (including single address) and/or endpoint; when triggered it gets "narrowed" creating a "client" Finally there's mobike which needs to update the "host". For "clientless" connections (OE?) there's a suspicion that it also updates the "client"; ulgh. So what next: - eliminate the redundant .end.port and just use .end.client'port - look at some of the other redunant .end.has_.. fields - predicate functions? - (locally) stop overwriting .host_addr'port and see what's really going on; and then ... - ... eliminate the redundant .end.host_port - ... consider something like passing the oe code a subnet rather than an address On Sat, 24 Aug 2019 at 14:35, Andrew Cagney <[email protected]> wrote: > 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
