[Swan-dev] Finding connections with %any fails due to only searching hostpair

[Andrew Cagney]

On Wed, 17 Mar 2021 at 08:58, Andrew Cagney <andrew.cagney at gmail.com> wrote:
>
> On Tue, 16 Mar 2021 at 23:19, Paul Wouters <paul at nohats.ca> wrote:
> >
> >
> > I've looked again at this bug: https://bugs.libreswan.org/show_bug.cgi?id=298
> >
> > I've recreated the issue in ikev2-connswitch-03
> >
> > The issue is:
> >
> > diff --git a/programs/pluto/ikev2_ts.c b/programs/pluto/ikev2_ts.c
> > index 9db2d91fab..3e47a7fcf8 100644
> > --- a/programs/pluto/ikev2_ts.c
> > +++ b/programs/pluto/ikev2_ts.c
> > @@ -1142,9 +1142,7 @@ bool v2_process_ts_request(struct child_sa *child,
> >                  if (hp == NULL)
> >                          continue;
> >
> > -               for (struct connection *d = hp->connections;
> > -                    d != NULL; d = d->hp_next) {
> > +               for (struct connection *d = connections; d != NULL; d = d->ac_next) {
> >                          /* groups are templates instantiated as GROUPINSTANCE */
> >                          if (d->policy & POLICY_GROUP) {
> >                                  continue;
> >                          }
> >
> >
> > There are three connections, which only differ in left/rightsubnet. So
> > they have a shared IKE SA. But east is configured with right=%any and
> > all three conns have the same rightid=
>
> Can this be expressed in terms of local and remote?
>
> hostpair maintains two tables:
>     local=address, remote=address
>     local=address, remote=any

looking around the above there's DHR's comment:

        /*
         * ??? the use of hp looks nonsensical.
         * Either the first non-empty host_pair should be used
         * (like the current code) and the following should
         * be broken into two loops: first find the non-empty
         * host_pair list, second look through the host_pair list.
         * OR
         * what's really meant is look at the host_pair for
         * each sra, something that matches the current
         * nested loop structure but not what it actually does.
         */

and this lookup:

                hp = find_host_pair(&sra->this.host_addr,
                                    &sra->that.host_addr);

so I suspect having the code also:

           find_host_pair(&sra->this.host_addr, &unset_address)

would do what you want and much faster.

However, along the lines of DHR's comment, is this just adding to the problem?

>
> > When west connects to east, east picks one of its static conns. If this
> > happens to _not_ match the subnets of the connection west picked to
> > initiate first, the original code does not find the other conns with
> > subnets because those are right=%any and not considered as part of
> > the hostpair.
> >
> > The above fix to just look through every connection fixes this. The
> > question is, should east have tried to add these right=%any connections
> > to its host pair? These connections have the same leftid and rightid.
> >
> > Or should we, if we tried the hostpair listings, _then_ fall back to
> > try all connections ?
> >
> > Is hostpair still a useful construct at all, or should we slowly phase
> > it out ?
> >
> > Paul
> > _______________________________________________
> > Swan-dev mailing list
> > Swan-dev at lists.libreswan.org
> > https://lists.libreswan.org/mailman/listinfo/swan-dev