On Fri, Sep 03, 2021 at 10:12:57AM +0200, Sebastian Benoit wrote:
> Tobias Heider([email protected]) on 2021.09.02 15:39:46 +0200:
> > The diff below makes iked accept a list of protocols for the "proto" config
> > option in iked.conf(5).
> > This would allow us to have a single policy with "proto { ipencap, ipv6 }"
> > to secure a gif(4) tunnel, instead of requiring one policy for each
> > protocol.
> >
> > ok?
>
> I only gave the parser bits a quick read.
>
> The manpage change would be nice to compare the parser with what you
> document.
Ack
> > proto : /* empty */ { $$ = 0; }
> > | PROTO protoval { $$ = $2; }
> > - | PROTO ESP { $$ = IPPROTO_ESP; }
> > - | PROTO AH { $$ = IPPROTO_AH; }
> > + | PROTO '{' proto_list '}' { $$ = $3; }
>
> this will cause an error with old configs?
>
> Maybe people depend on it for connectivity, which makes it critical on
> upgrade.
>
> Maybe its possible to keep old and new, and remove the old after 2 releases?
> (You can print a warning message in the parser for the old syntax).
>
> In any case this should get a mention in current.html.
I don't think anyone is using this which is why I dropped them.
The 'proto' option decides what traffic is matched by the flow.
This is the protocol INSIDE the tunnel. I can't think of a single
reason for using ESP or AH here.
Mentioning it in current.html still seems like a good idea.
> > +proto_list : protoval { $$ = $1; }
> > + | proto_list comma protoval {
> > + if ($3 == NULL)
> > + $$ = $1;
> > + else if ($1 == NULL)
> > + $$ = $3;
> > + else {
> > + $1->tail->next = $3;
> > + $1->tail = $3->tail;
> > + $$ = $1;
> > + }
> > + }
>
> why dont you make it
>
> | protoval comma proto_list {
>
> then you dont need the conditional.
>
It is basically a copy of host_list.
>
> > ;
> >
> > protoval : STRING {
> > @@ -644,7 +656,12 @@ protoval : STRING {
> > yyerror("unknown protocol: %s", $1);
> > YYERROR;
> > }
> > - $$ = p->p_proto;
> > +
> > + if (($$ = calloc(1, sizeof(*$$))) == NULL)
> > + err(1, "hosts: calloc");
> > +
> > + $$->type = p->p_proto;
> > + $$->tail = $$;
> > free($1);
> > }
> > | NUMBER {
> > @@ -652,6 +669,11 @@ protoval : STRING {
> > yyerror("protocol outside range");
> > YYERROR;
> > }
> > + if (($$ = calloc(1, sizeof(*$$))) == NULL)
> > + err(1, "hosts: calloc");
> > +
> > + $$->type = $1;
> > + $$->tail = $$;
> > }
> > ;
> >
> > @@ -2444,7 +2466,7 @@ copy_transforms(unsigned int type,
> > }
> >
> > int
> > -create_ike(char *name, int af, uint8_t ipproto,
> > +create_ike(char *name, int af, struct ipsec_addr_wrap *ipproto,
> > int rdomain, struct ipsec_hosts *hosts,
> > struct ipsec_hosts *peers, struct ipsec_mode *ike_sa,
> > struct ipsec_mode *ipsec_sa, uint8_t saproto,
> > @@ -2454,7 +2476,7 @@ create_ike(char *name, int af, uint8_t i
> > struct ipsec_addr_wrap *ikecfg, char *iface)
> > {
> > char idstr[IKED_ID_SIZE];
> > - struct ipsec_addr_wrap *ipa, *ipb;
> > + struct ipsec_addr_wrap *ipa, *ipb, *ipp;
> > struct iked_auth *ikeauth;
> > struct iked_policy pol;
> > struct iked_proposal *p, *ptmp;
> > @@ -2473,7 +2495,15 @@ create_ike(char *name, int af, uint8_t i
> > pol.pol_certreqtype = env->sc_certreqtype;
> > pol.pol_af = af;
> > pol.pol_saproto = saproto;
> > - pol.pol_ipproto = ipproto;
> > + for (i = 0, ipp = ipproto; ipp; ipp = ipp->next, i++) {
> > + if (i > IKED_IPPROTO_MAX) {
> > + yyerror("too many protocols");
> > + return (-1);
>
> elsewhere in create_ike() you use fatalx() for errors, is the return value
> checked everywhere?
It is only called from ikev2rule which always checks the return value.
create_ike() uses both versions and could use some cleanup, but "return (-1)"
seems to be the right way to handle this.
