[Swan-dev] end.host_addr's port

Andrew Cagney andrew.cagney at gmail.com
Wed Sep 11 14:26:41 UTC 2019


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 <andrew.cagney at gmail.com> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20190911/0b740a48/attachment.html>


More information about the Swan-dev mailing list