I tried this diff, and it broke the ability to use dynamic addresses. ie, the following rules should work:
pass in on gre52 inet proto icmp route-to (gre49:peer) pass in on vmx0 inet proto icmp route-to (gre:peer) however, other forms of dynamic interface addresses should fail. or do we want to support route-to if0:broadcast too? i have an updated diff to send out that includes both kernel and pfctl changes. i think the only thing i'm worried about in it is that it reuses the redirspec pool_opts stuff, and some of those might not make sense for route-to address selection dlg On Tue, Jan 12, 2021 at 08:45:22PM +0100, Alexandr Nedvedicky wrote: > Hello, > > proposed diff follows stuff discussed here [1] (pf route-to issues). I think > we've reached a consensus to change route-to/reply-to such the only supported > option will be next-hop (and list and table of next-hop addresses). > > I think bluhm@ and dlg@ have committed part of that change already. > the proposed diff updates pfctl(8) so parser will do 'a right thing', > namely: > > specifying host using form as 1.2.3.4@em0 is not supporte > anymore > > diff introduces a next_hop() function, which is a clone of > existing host(). unlike host(), the next_hop() does not accept > a name of local network interface. > > the diff also breaks existing regression tests. We can update > them once, we will agree on proposed diff. > > thanks and > regards > sashan > > [1] https://marc.info/?l=openbsd-tech&m=160308583701259&w=2 > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y > index 2b3e62b1a7e..536aec3286b 100644 > --- a/sbin/pfctl/parse.y > +++ b/sbin/pfctl/parse.y > @@ -3745,23 +3745,13 @@ pool_opt : BITMASK { > ; > > route_host : STRING { > - /* try to find @if0 address specs */ > - if (strrchr($1, '@') != NULL) { > - if (($$ = host($1, pf->opts)) == NULL) { > - yyerror("invalid host for route spec"); > - YYERROR; > - } > + if (($$ = next_hop($1, pf->opts)) == NULL) { > + /* error. "any" is handled elsewhere */ > free($1); > - } else { > - $$ = calloc(1, sizeof(struct node_host)); > - if ($$ == NULL) > - err(1, "route_host: calloc"); > - $$->ifname = $1; > - $$->addr.type = PF_ADDR_NONE; > - set_ipmask($$, 128); > - $$->next = NULL; > - $$->tail = $$; > + yyerror("could not parse host specification"); > + YYERROR; > } > + free($1); > } > | STRING '/' STRING { > char *buf; > @@ -3769,7 +3759,7 @@ route_host : STRING { > if (asprintf(&buf, "%s/%s", $1, $3) == -1) > err(1, "host: asprintf"); > free($1); > - if (($$ = host(buf, pf->opts)) == NULL) { > + if (($$ = next_hop(buf, pf->opts)) == NULL) { > /* error. "any" is handled elsewhere */ > free(buf); > yyerror("could not parse host specification"); > @@ -3795,33 +3785,6 @@ route_host : STRING { > $$->next = NULL; > $$->tail = $$; > } > - | dynaddr '/' NUMBER { > - struct node_host *n; > - > - if ($3 < 0 || $3 > 128) { > - yyerror("bit number too big"); > - YYERROR; > - } > - $$ = $1; > - for (n = $1; n != NULL; n = n->next) > - set_ipmask(n, $3); > - } > - | '(' STRING host ')' { > - struct node_host *n; > - > - $$ = $3; > - /* XXX check masks, only full mask should be allowed */ > - for (n = $3; n != NULL; n = n->next) { > - if ($$->ifname) { > - yyerror("cannot specify interface twice > " > - "in route spec"); > - YYERROR; > - } > - if (($$->ifname = strdup($2)) == NULL) > - errx(1, "host: strdup"); > - } > - free($2); > - } > ; > > route_host_list : route_host optweight optnl { > diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c > index 03317844e91..534b53b1198 100644 > --- a/sbin/pfctl/pfctl_parser.c > +++ b/sbin/pfctl/pfctl_parser.c > @@ -1615,17 +1615,16 @@ host(const char *s, int opts) > { > struct node_host *h = NULL, *n; > int mask = -1; > - char *p, *ps, *if_name; > + char *p, *ps; > const char *errstr; > > + if (strchr(s, '@') != NULL) > + err(1, "%s: '@' not expected in host spec. (%s)\n", > + __func__, s); > + > if ((ps = strdup(s)) == NULL) > err(1, "%s: strdup", __func__); > > - if ((if_name = strrchr(ps, '@')) != NULL) { > - if_name[0] = '\0'; > - if_name++; > - } > - > if ((p = strchr(ps, '/')) != NULL) { > mask = strtonum(p+1, 0, 128, &errstr); > if (errstr) { > @@ -1642,10 +1641,46 @@ host(const char *s, int opts) > goto error; > } > > - if (if_name && if_name[0]) > - for (n = h; n != NULL; n = n->next) > - if ((n->ifname = strdup(if_name)) == NULL) > - err(1, "%s: strdup", __func__); > + for (n = h; n != NULL; n = n->next) { > + n->addr.type = PF_ADDR_ADDRMASK; > + n->weight = 0; > + } > + > +error: > + free(ps); > + return (h); > +} > + > +struct node_host * > +next_hop(const char *s, int opts) > +{ > + struct node_host *h = NULL, *n; > + int mask = -1; > + char *p, *ps; > + const char *errstr; > + > + if (strchr(s, '@') != NULL) > + err(1, "%s: '@' not expected in next-hop spec. (%s)\n", > + __func__, s); > + > + if ((ps = strdup(s)) == NULL) > + err(1, "%s: strdup", __func__); > + > + if ((p = strchr(ps, '/')) != NULL) { > + mask = strtonum(p+1, 0, 128, &errstr); > + if (errstr) { > + fprintf(stderr, "netmask is %s: %s\n", errstr, p); > + goto error; > + } > + p[0] = '\0'; > + } > + > + if ((h = host_ip(ps, mask)) == NULL && > + (h = host_dns(ps, mask, (opts & PF_OPT_NODNS))) == NULL) { > + fprintf(stderr, "no IP address found for %s\n", s); > + goto error; > + } > + > for (n = h; n != NULL; n = n->next) { > n->addr.type = PF_ADDR_ADDRMASK; > n->weight = 0; > diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h > index a82854a0fea..c7ab3f967b6 100644 > --- a/sbin/pfctl/pfctl_parser.h > +++ b/sbin/pfctl/pfctl_parser.h > @@ -292,6 +292,7 @@ char *ifa_indextoname(unsigned int, > char *); > struct node_host *ifa_exists(const char *); > struct node_host *ifa_lookup(const char *, int); > struct node_host *host(const char *, int); > +struct node_host *next_hop(const char *, int); > > int append_addr(struct pfr_buffer *, char *, int, int); > int append_addr_host(struct pfr_buffer *, >
