<div dir="ltr"><div dir="ltr"><div>So to recap:<br></div><div><br></div><div>"host" is the end's endpoint (ADDRESS:PORT), where PORT is the IKE port</div><div>-> 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</div><div><br></div><div>"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:<br></div><div>- a routing prefix (aka traditional subnet</div><div><div>- 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?)</div></div><div>- an address (is this really a routing prefix but for one address, or an endpoint but with a wild port?)<br></div><div><br></div><div>"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"</div><div><br></div>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.<br></div><div><br></div><div>So what next:</div><div><br></div><div>- eliminate the redundant .end.port and just use .end.client'port</div><div>- look at some of the other redunant .end.has_.. fields - predicate functions?<br></div><div>- (locally) stop overwriting .host_addr'port and see what's really going on; and then ...</div><div>- ... eliminate the redundant .end.host_port<br></div><div>- ... consider something like passing the oe code a subnet rather than an address<br></div><div><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, 24 Aug 2019 at 14:35, Andrew Cagney <<a href="mailto:andrew.cagney@gmail.com">andrew.cagney@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I thought I had one problem; now I've got two!<br>
<br>
<br>
On Fri, 23 Aug 2019 at 10:41, Paul Wouters <<a href="mailto:paul@nohats.ca" target="_blank">paul@nohats.ca</a>> wrote:<br>
><br>
> On Fri, 23 Aug 2019, Antony Antony wrote:<br>
><br>
> > yes too many organically grown workarounds. MOBIKE and NAT stretched these<br>
> > ideas quite a bit.<br>
><br>
> Indeed.<br>
><br>
> >> struct end {<br>
> >> ip_address host_addr;<br>
> >> ip_subnet client;<br>
> >> bool has_client;<br>
> >> bool has_client_wildcard;<br>
> >> bool has_port_wildcard;<br>
> >> uint16_t host_port; /* where the IKE port is */<br>
> >> uint16_t port; /* port number, if per-port keying */<br>
> >> }<br>
> >><br>
> >> and am puzzled by .port vs .host_port and .client vs .host[addr]. My<br>
> >> working theory was that things were paired:<br>
><br>
> Some of the issues stem from freeswan's original concept of host with or<br>
> without subnets. It copies from host* to client* if no subnets are set.<br>
><br>
> I think Hugh used the per port keying to run multiple plutos on the same<br>
> host for testing decades ago. I'm not sure if that's where the "port" vs<br>
> "host_port" came from.<br>
<br>
That might explain:<br>
bool host_port_specific; /* if TRUE, then IKE ports are<br>
tested for */<br>
which seems to only change a log line<br>
<br>
> It is also possible that it came in with the NAT-T stuff, where we went<br>
> from IKE port to "IKE port or NAT IKE port".<br>
><br>
> It would make sense to clean this up. For instance whether or not to use<br>
> the NAT IKE port (4500) mostly depends on whether NAT was detected, so<br>
> that property should live in the state and not in c->spd.this/that.<br>
><br>
> >> .client and .port (what IKEv2 calls a traffic selector)<br>
> >> .host_addr and .host_port (the IKE endpoint)<br>
> ><br>
> > This theory sounds good, possibly over simplistic. It is missing the part<br>
> > dynamic change. The need copy from c to st and vice versa is complicated with<br>
> > MOBIKE. It touch what is in c and what is in st, especially for an<br>
> > instantiated connection.<br>
<br>
The dynamic change should only conceptually affect .host_addr / .host_port.<br>
(but might also cause .client to change if there isn't a client).<br>
<br>
> Yes, I avoided the understanding and cleanup you are doing now when I<br>
> worked on MOBIKE. I tried to keep everything in struct state that would<br>
> "override" things, and tried to not change struct connection as much as<br>
> possible.<br>
><br>
> >> but, in the case of .host_addr, the code seems to be fighting itself<br>
> >> over what the port should be. For instance:<br>
> >><br>
> >> - in ikev2_ts.c the .host_addr's port is forced to the negotiated TS<br>
> >> client port:<br>
> >><br>
> >> c->spd.that.client = tmp_subnet_r;<br>
> >> c->spd.that.port = st->st_ts_that.startport;<br>
> >> c->spd.that.protocol = st->st_ts_that.ipprotoid;<br>
> >> setportof(htons(c->spd.that.port),<br>
> >> &c->spd.that.host_addr);<br>
> >> setportof(htons(c->spd.that.port),<br>
> >> &c->spd.that.client.addr);<br>
><br>
> The traffic selectors and the IKE address and port _should_ not be<br>
> related or mixed up or sync'ed in theory. But with "narrowing" we<br>
> have the case where the traffic selector set might be a subset of<br>
> the c->spd.that/this.client and c->spd.this/that.[port|protocol].<br>
> So what we then do is instantiate the connection, and only once<br>
> scrible over the this/that properties. From then on, these are<br>
> again "immutable" for that (instantiated) connection's lifetime.<br>
> Note that a connection supporting narrowing is for that reason<br>
> created as a CK_TEMPLATE, so even if the TS covers everything,<br>
> it is instantiated.<br>
><br>
> >> - but then in state.c:mobike, it's forced to the sender's port<br>
> >> (.sender has probably always had the port embedded in it).<br>
> >><br>
> >> /* MOBIKE responder processing request */<br>
> >> c->spd.that.host_addr = md->sender;<br>
> >> c->spd.that.host_port = hportof(&md->sender);<br>
> ><br>
> > My recollections are vague and I didn't double checked it in git master.<br>
> ><br>
> > you need IKE port in a connection for two reasons. To survive a restart,<br>
> > short interruptions, say dpd. Second for output "ipsec status"?<br>
><br>
> I think the copying from sender is more to survive NAT changes. If the<br>
> NAT router maps the port differently, we need to change our end to send<br>
> to this new (IP/)port. But we should only do that after we decrypted and<br>
> authenticated the packet as "new" so a forged replay would not force us<br>
> back. This is the area where there is some complicated interaction<br>
> between NAT routers changing things and MOBIKE changing things.<br>
><br>
> > Note when initiating a connection a call to set_state_ike_endpoints.<br>
> ><br>
> > set_state_ike_endpoints(st, c)<br>
> > st->st_remoteaddr = c->spd.that.host_addr;<br>
> > st->st_remoteport = c->spd.that.host_port;<br>
> ><br>
> > While adding MOBIKE, Paul and I realized copying port and host_addr to c<br>
> > would be a limitation for a longer connection interruptions and restart.<br>
> > However, we used a reasoning most use cases are only for right=%any. Those<br>
> > connections wouldn't initiate after say a dpd restart or when the other end<br>
> > reboots.<br>
> ><br>
> > The MOBIKE overwriting host_port and host_addr in c is a limitation!<br>
> > In most use cases Libreswan is MOBIKE responder with right=%any so it sort<br>
> > of works. That is why you are seeing overwriting what is in c, which is<br>
> > confusing.<br>
><br>
> Right. Although in this case, if the information is struct state was<br>
> more reliable, we could potentially not override the connection. Note<br>
> MOBIKE is incompatible with transport mode so there is always a<br>
> this/that client when we are changing the host addr/port.<br>
><br>
> > on the responder one odd bit for me is:<br>
> > calls in instantiate()<br>
> > port = htons(dst->port);<br>
> > setportof(port, &dst->host_addr);<br>
><br>
> I have no memory of this specifically. It could be that this originally<br>
> comes from the md->st where the ip/port info lives before we have<br>
> matched a connection. Then when we match it, we might have (due to hairy<br>
> code) overwritten the connection's port to match the sender port in the<br>
> md.<br>
><br>
> >> A look at *_raw_eroute() shows .host_port is ignored (I thought it was<br>
> >> used, but it turns out that was only for prettying an error).<br>
><br>
> Yes, eroutes should only work on *client, not *host. And for transport<br>
> mode and for host-to-host tunnel mode connections, the host variables<br>
> are copied as client variables.<br>
><br>
> >> A look at .has_client shows more promise, the code seems to copy<br>
> >> .host_addr into .client vis:<br>
> >><br>
> >> /* default client to subnet containing only self<br>
> >> * XXX This may mean that the client's address family doesn't match<br>
> >> * tunnel_addr_family.<br>
> >> */<br>
> >> if (!c->spd.that.has_client)<br>
> >> addrtosubnet(&c->spd.that.host_addr, &c->spd.that.client);<br>
> >><br>
> >> and, I I'm guessing, is assuming that /host_addr's port is still set<br>
> >> to .port (the client port).<br>
> ><br>
> > Also note the comparisons to pluto_port.<br>
> ><br>
> > I think we can delete some of them.<br>
> > c->spd.this.port can go, use c->spd.this.client.addr.port instead<br>
><br>
> I think that is the wrong abstraction. The this/that.client should only<br>
> relate to the ipsec policy for src/dst and not contain anything related<br>
> to the host IP or port used for IKE. So in my opinion,<br>
> c->spd.this.client.addr.port should be removed or always be 0.<br>
><br>
> > host_port can be ditched in favor of host_addr's port. This might be a bit<br>
> > invasive change in v1 and v2.<br>
><br>
> Additionally, I'm about to add code to support leftikeport= and<br>
> rightikeport= to support openshift/kubernetes containers. So if we are<br>
> doing a cleanup, which is much needed, let's ensure we then also take<br>
> this new feature into account.<br>
><br>
> > I wonder what to do with ikev2 ts, same information has multiple copies in<br>
> > st, and in c.<br>
><br>
> Perhaps rewrite this only after we have ditched IKEv1 ?<br>
><br>
> > while we are at it, recent changes bc4d25c065b #ifdef ENDPOINT_ADDRESS_PORT<br>
> > is a bit confusing to me. These started a while ago? 25bcb7f6cb0. Too many<br>
> > changes in flight.<br>
><br>
> I think this is Andrew's way of porting code and testing before flipping<br>
> everything to "new method" when he removes the define ?<br>
<br>
Two things:<br>
<br>
- it wraps the current working theory on what the structure should<br>
look like in a #ifdef<br>
Of course the theory evolves, for instance I suspect adding<br>
uint8_t[16] to ip_address so that static initialisation is possible<br>
using hardwired bytes<br>
<br>
- when defined it breaks the compile in a way that helps flush out<br>
weird and wonderful problems, like the above, which is exposed because<br>
code is trying to pass a simple address to functions expecting<br>
address+port<br>
</blockquote></div></div>