On Fri, Sep 03, 2021 at 10:12:57AM +0200, Sebastian Benoit wrote:
> Tobias Heider(tobias.hei...@stusta.de) 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.
>
> A bit more below.
>
> > Index: iked.h
> > ===================================================================
> > RCS file: /cvs/src/sbin/iked/iked.h,v
> > retrieving revision 1.193
> > diff -u -p -r1.193 iked.h
> > --- iked.h 1 Sep 2021 15:30:06 -0000 1.193
> > +++ iked.h 2 Sep 2021 13:37:21 -0000
> > @@ -242,10 +242,9 @@ struct iked_policy {
> >
> > #define IKED_SKIP_FLAGS 0
> > #define IKED_SKIP_AF 1
> > -#define IKED_SKIP_PROTO 2
> > -#define IKED_SKIP_SRC_ADDR 3
> > -#define IKED_SKIP_DST_ADDR 4
> > -#define IKED_SKIP_COUNT 5
> > +#define IKED_SKIP_SRC_ADDR 2
> > +#define IKED_SKIP_DST_ADDR 3
> > +#define IKED_SKIP_COUNT 4
> > struct iked_policy *pol_skip[IKED_SKIP_COUNT];
> >
> > uint8_t pol_flags;
> > @@ -265,7 +264,8 @@ struct iked_policy {
> > int pol_af;
> > int pol_rdomain;
> > uint8_t pol_saproto;
> > - unsigned int pol_ipproto;
> > + unsigned int pol_ipproto[IKED_IPPROTO_MAX];
> > + unsigned int pol_nipproto;
> >
> > struct iked_addr pol_peer;
> > struct iked_static_id pol_peerid;
> > Index: parse.y
> > ===================================================================
> > RCS file: /cvs/src/sbin/iked/parse.y,v
> > retrieving revision 1.131
> > diff -u -p -r1.131 parse.y
> > --- parse.y 28 May 2021 18:01:39 -0000 1.131
> > +++ parse.y 2 Sep 2021 13:37:21 -0000
> > @@ -374,7 +374,7 @@ void copy_transforms(unsigned int,
> > const struct ipsec_xf **, unsigned int,
> > struct iked_transform **, unsigned int *,
> > struct iked_transform *, size_t);
> > -int create_ike(char *, int, uint8_t,
> > +int create_ike(char *, int, struct ipsec_addr_wrap
> > *,
> > int, struct ipsec_hosts *,
> > struct ipsec_hosts *, struct ipsec_mode *,
> > struct ipsec_mode *, uint8_t,
> > @@ -388,9 +388,9 @@ uint8_t x2i(unsigned char *);
> > int parsekey(unsigned char *, size_t, struct
> > iked_auth *);
> > int parsekeyfile(char *, struct iked_auth *);
> > void iaw_free(struct ipsec_addr_wrap *);
> > -static int create_flow(struct iked_policy *pol, struct
> > ipsec_addr_wrap *ipa,
> > +static int create_flow(struct iked_policy *pol, int, struct
> > ipsec_addr_wrap *ipa,
> > struct ipsec_addr_wrap *ipb);
> > -static int expand_flows(struct iked_policy *, struct
> > ipsec_addr_wrap *,
> > +static int expand_flows(struct iked_policy *, int, struct
> > ipsec_addr_wrap *,
> > struct ipsec_addr_wrap *);
> > static struct ipsec_addr_wrap *
> > expand_keyword(struct ipsec_addr_wrap *);
> > @@ -407,7 +407,6 @@ typedef struct {
> > uint8_t ikemode;
> > uint8_t dir;
> > uint8_t satype;
> > - uint8_t proto;
> > char *string;
> > uint16_t port;
> > struct ipsec_hosts *hosts;
> > @@ -415,6 +414,7 @@ typedef struct {
> > struct ipsec_addr_wrap *anyhost;
> > struct ipsec_addr_wrap *host;
> > struct ipsec_addr_wrap *cfg;
> > + struct ipsec_addr_wrap *proto;
> > struct {
> > char *srcid;
> > char *dstid;
> > @@ -449,8 +449,7 @@ typedef struct {
> > %token <v.number> NUMBER
> > %type <v.string> string
> > %type <v.satype> satype
> > -%type <v.proto> proto
> > -%type <v.number> protoval
> > +%type <v.proto> proto proto_list protoval
> > %type <v.hosts> hosts hosts_list
> > %type <v.port> port
> > %type <v.number> portval af rdomain
> > @@ -632,8 +631,21 @@ af : /* empty */ { $$ =
> > AF_UNSPEC; }
> >
> > 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?
Yes and no. If the ESP and AH cases are moved to protoval then it will
work like before. This allows using ah and esp in this rule even though
they are yacc keywords and wont match the protoval yacc rule.
> 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.
> > + ;
> > +
> > +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.
AFAIK yacc rules should be left expand. I don't fully remember the reason
but your example is not the common way.
>
> > ;
> >
> > 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 = $$;
> > }
It would be helpful to add AH and ESP cases here.
| AH {
if (($$ = calloc(1, sizeof(*$$))) == NULL)
err(1, "hosts: calloc");
$$->type = IPPROTO_AH;
$$->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?
>
> > + }
> > + pol.pol_ipproto[i] = ipp->type;
> > + pol.pol_nipproto++;
> > + }
> > +
> > pol.pol_flags = flags;
> > pol.pol_rdomain = rdomain;
> > memcpy(&pol.pol_auth, authtype, sizeof(struct iked_auth));
> > @@ -2826,10 +2856,13 @@ create_ike(char *name, int af, uint8_t i
> > if (hosts == NULL || hosts->src == NULL || hosts->dst == NULL)
> > fatalx("create_ike: no traffic selectors/flows");
> >
> > - for (ipa = hosts->src, ipb = hosts->dst; ipa && ipb;
> > - ipa = ipa->next, ipb = ipb->next)
> > - if (expand_flows(&pol, ipa, ipb))
> > - fatalx("create_ike: invalid flow");
> > + for (j = 0; j < pol.pol_nipproto; j++) {
> > + for (ipa = hosts->src, ipb = hosts->dst; ipa && ipb;
> > + ipa = ipa->next, ipb = ipb->next) {
> > + if (expand_flows(&pol, pol.pol_ipproto[j], ipa, ipb))
> > + fatalx("create_ike: invalid flow");
> > + }
> > + }
> >
> > for (j = 0, ipa = ikecfg; ipa; ipa = ipa->next, j++) {
> > if (j >= IKED_CFG_MAX)
> > @@ -2918,6 +2951,7 @@ done:
> > free(hosts);
> > }
> > iaw_free(ikecfg);
> > + iaw_free(ipproto);
> > RB_FOREACH_SAFE(flow, iked_flows, &pol.pol_flows, ftmp) {
> > RB_REMOVE(iked_flows, &pol.pol_flows, flow);
> > free(flow);
> > @@ -2929,7 +2963,7 @@ done:
> > }
> >
> > static int
> > -create_flow(struct iked_policy *pol, struct ipsec_addr_wrap *ipa,
> > +create_flow(struct iked_policy *pol, int proto, struct ipsec_addr_wrap
> > *ipa,
> > struct ipsec_addr_wrap *ipb)
> > {
> > struct iked_flow *flow;
> > @@ -2969,8 +3003,8 @@ create_flow(struct iked_policy *pol, str
> > }
> >
> > flow->flow_dir = IPSP_DIRECTION_OUT;
> > + flow->flow_ipproto = proto;
> > flow->flow_saproto = pol->pol_saproto;
> > - flow->flow_ipproto = pol->pol_ipproto;
> > flow->flow_rdomain = pol->pol_rdomain;
> >
> > if (RB_INSERT(iked_flows, &pol->pol_flows, flow) == NULL)
> > @@ -2984,11 +3018,15 @@ create_flow(struct iked_policy *pol, str
> > }
> >
> > static int
> > -expand_flows(struct iked_policy *pol, struct ipsec_addr_wrap *src,
> > +expand_flows(struct iked_policy *pol, int proto, struct ipsec_addr_wrap
> > *src,
> > struct ipsec_addr_wrap *dst)
> > {
> > struct ipsec_addr_wrap *ipa = NULL, *ipb = NULL;
> > int ret = -1;
> > + int srcaf, dstaf;
> > +
> > + srcaf = src->af;
> > + dstaf = dst->af;
> >
> > if (src->af == AF_UNSPEC &&
> > dst->af == AF_UNSPEC) {
> > @@ -2998,7 +3036,7 @@ expand_flows(struct iked_policy *pol, st
> > ipb = expand_keyword(dst);
> > if (!ipa || !ipb)
> > goto done;
> > - if (create_flow(pol, ipa, ipb))
> > + if (create_flow(pol, proto, ipa, ipb))
> > goto done;
> >
> > iaw_free(ipa);
> > @@ -3008,26 +3046,28 @@ expand_flows(struct iked_policy *pol, st
> > ipb = expand_keyword(dst);
> > if (!ipa || !ipb)
> > goto done;
> > - if (create_flow(pol, ipa, ipb))
> > + if (create_flow(pol, proto, ipa, ipb))
> > goto done;
> > } else if (src->af == AF_UNSPEC) {
> > src->af = dst->af;
> > ipa = expand_keyword(src);
> > if (!ipa)
> > goto done;
> > - if (create_flow(pol, ipa, dst))
> > + if (create_flow(pol, proto, ipa, dst))
> > goto done;
> > } else if (dst->af == AF_UNSPEC) {
> > dst->af = src->af;
> > ipa = expand_keyword(dst);
> > if (!ipa)
> > goto done;
> > - if (create_flow(pol, src, ipa))
> > + if (create_flow(pol, proto, src, ipa))
> > goto done;
> > - } else if (create_flow(pol, src, dst))
> > + } else if (create_flow(pol, proto, src, dst))
> > goto done;
> > ret = 0;
> > done:
> > + src->af = srcaf;
> > + dst->af = dstaf;
> > iaw_free(ipa);
> > iaw_free(ipb);
> > return (ret);
> > Index: policy.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/iked/policy.c,v
> > retrieving revision 1.83
> > diff -u -p -r1.83 policy.c
> > --- policy.c 1 Sep 2021 15:30:06 -0000 1.83
> > +++ policy.c 2 Sep 2021 13:37:21 -0000
> > @@ -223,9 +223,6 @@ policy_test(struct iked *env, struct ike
> > else if (key->pol_af && p->pol_af &&
> > key->pol_af != p->pol_af)
> > p = p->pol_skip[IKED_SKIP_AF];
> > - else if (key->pol_ipproto && p->pol_ipproto &&
> > - key->pol_ipproto != p->pol_ipproto)
> > - p = p->pol_skip[IKED_SKIP_PROTO];
> > else if (sockaddr_cmp((struct sockaddr *)&key->pol_peer.addr,
> > (struct sockaddr *)&p->pol_peer.addr,
> > p->pol_peer.addr_mask) != 0)
> > @@ -334,9 +331,6 @@ policy_calc_skip_steps(struct iked_polic
> > prev->pol_af != AF_UNSPEC &&
> > cur->pol_af != prev->pol_af)
> > IKED_SET_SKIP_STEPS(IKED_SKIP_AF);
> > - if (cur->pol_ipproto && prev->pol_ipproto &&
> > - cur->pol_ipproto != prev->pol_ipproto)
> > - IKED_SET_SKIP_STEPS(IKED_SKIP_PROTO);
> > if (IKED_ADDR_NEQ(&cur->pol_peer, &prev->pol_peer))
> > IKED_SET_SKIP_STEPS(IKED_SKIP_DST_ADDR);
> > if (IKED_ADDR_NEQ(&cur->pol_local, &prev->pol_local))
> > Index: print.c
> > ===================================================================
> > RCS file: /cvs/src/sbin/iked/print.c,v
> > retrieving revision 1.2
> > diff -u -p -r1.2 print.c
> > --- print.c 21 Mar 2021 22:18:00 -0000 1.2
> > +++ print.c 2 Sep 2021 13:37:21 -0000
> > @@ -90,8 +90,16 @@ print_policy(struct iked_policy *pol)
> >
> > print_verbose(" %s", print_xf(pol->pol_saproto, 0, saxfs));
> >
> > - if (pol->pol_ipproto)
> > - print_verbose(" proto %s", print_proto(pol->pol_ipproto));
> > + if (pol->pol_nipproto > 0) {
> > + print_verbose(" proto {");
> > + for (i = 0; i < pol->pol_nipproto; i++) {
> > + if (i == 0)
> > + print_verbose(" %s",
> > print_proto(pol->pol_ipproto[i]));
> > + else
> > + print_verbose(", %s",
> > print_proto(pol->pol_ipproto[i]));
> > + }
> > + print_verbose(" }");
> > + }
> >
> > if (pol->pol_af) {
> > if (pol->pol_af == AF_INET)
> > Index: types.h
> > ===================================================================
> > RCS file: /cvs/src/sbin/iked/types.h,v
> > retrieving revision 1.45
> > diff -u -p -r1.45 types.h
> > --- types.h 1 Sep 2021 15:30:06 -0000 1.45
> > +++ types.h 2 Sep 2021 13:37:21 -0000
> > @@ -63,6 +63,7 @@
> > #define IKED_PSK_SIZE 1024 /* XXX should be dynamic */
> > #define IKED_MSGBUF_MAX 8192
> > #define IKED_CFG_MAX 16 /* maximum CP attributes */
> > +#define IKED_IPPROTO_MAX 16
> > #define IKED_TAG_SIZE 64
> > #define IKED_CYCLE_BUFFERS 8 /* # of static buffers for mapping */
> > #define IKED_PASSWORD_SIZE 256 /* limited by most EAP types */
> >
--
:wq Claudio
