Hi, thank you, most of this diff looks good to me. I left some comments inline.
On Sun, Apr 05, 2020 at 01:58:04AM +0900, Wataru Ashihara wrote: > The data wich sc_sock4 has is a little bit complicated: > > -------------------------------------------------------------------------------- > sc_sock4[0], sc_sock6[0] sc_sock4[1], sc_sock6[1] > -------------------------------------------------------------------------------- > no flag (non NAT-T) :500 (IKE_SA_INIT, IKE_AUTH) :4500 (not used) > no flag (NAT-T) :500 (IKE_SA_INIT) :4500 (IKE_AUTH) > -t :500 (IKE_SA_INIT, IKE_AUTH) :500 (never used) (!) > -T :4500 (IKE_SA_INIT, IKE_AUTH) NULL (never used) > -p4500 :4500 (IKE_SA_INIT, IKE_AUTH) :4500 (never used) (!) > -p14500 :14500 (IKE_SA_INIT, IKE_AUTH) NULL (never used) > -------------------------------------------------------------------------------- > > Here "IKE_AUTH" includes CREATE_CHILD_SA and INFORMATIONAL as well and > the same applies to sc_sock6. This patch eliminates the never-used > data: > > -------------------------------------------------------------------------------- > first call second call > (sc_sock4[0], sc_sock6[0]) (sc_sock4[1], > sc_sock6[1]) > -------------------------------------------------------------------------------- > no flag (non NAT-T) :500 (IKE_SA_INIT, IKE_AUTH) :4500 (not used) > no flag (NAT-T) :500 (IKE_SA_INIT) :4500 (IKE_AUTH) > -t :500 (IKE_SA_INIT, IKE_AUTH) NULL (never used) > -T (== -p4500) :4500 (IKE_SA_INIT, IKE_AUTH) NULL (never used) > -p14500 :14500 (IKE_SA_INIT, IKE_AUTH) NULL (never used) > -------------------------------------------------------------------------------- Looks good. > > I introduced "enum natt_mode" because "if" conditions in > parent_configure() had got to be rather complicated: > > if (((env->sc_opts & IKED_OPT_NATT) == 0) || ((env->sc_opts & > IKED_OPT_NONATT) != 0)) > config_setsocket(env, &ss, htons(IKED_IKE_PORT), PROC_IKEV2); > if (((env->sc_opts & IKED_OPT_NATT) != 0) || ((env->sc_opts & > IKED_OPT_NONATT) == 0)) > config_setsocket(env, &ss, htons(env->sc_nattport), PROC_IKEV2); > Makes sense, good idea. > > Index: sbin/iked/config.c > =================================================================== > RCS file: /cvs/src/sbin/iked/config.c,v > retrieving revision 1.55 > diff -u -r1.55 config.c > --- sbin/iked/config.c 24 Mar 2020 13:32:36 -0000 1.55 > +++ sbin/iked/config.c 4 Apr 2020 16:48:55 -0000 > @@ -545,6 +545,24 @@ > return (0); > } > > +/** > + * The first call of this function sets the UDP socket for IKE_INIT. > + * The second one is optional, setting the UDP socket for IKE_AUTH, > + * CREATE_CHILD_SA, and INFORMATIONAL. > + * > + * The port of each is determined by command line options: > + * > + * > -------------------------------------------------------------------------------- > + * first call second call > + * (sc_sock4[0], sc_sock6[0]) (sc_sock4[1], > sc_sock6[1]) > + * > -------------------------------------------------------------------------------- > + * no flag (non NAT-T) :500 (IKE_SA_INIT, IKE_AUTH) :4500 (not used) > + * no flag (NAT-T) :500 (IKE_SA_INIT) :4500 (IKE_AUTH) > + * -t :500 (IKE_SA_INIT, IKE_AUTH) NULL (never used) > + * -T (== -p4500) :4500 (IKE_SA_INIT, IKE_AUTH) NULL (never used) > + * -p14500 :14500 (IKE_SA_INIT, IKE_AUTH) NULL (never used) > + * > -------------------------------------------------------------------------------- > + */ I feel like this comment is a bit too verbose. I understand how the table helped you understand what is going on here, but now that you cleaned this up, wouldn't it be sufficient to write something like: /* * The first call of this function sets the UDP socket for IKEv2. * The second call is optional, setting the UDP socket used for NAT-T. */ > int > config_setsocket(struct iked *env, struct sockaddr_storage *ss, > in_port_t port, enum privsep_procid id) > @@ -562,7 +580,7 @@ > config_getsocket(struct iked *env, struct imsg *imsg, > void (*cb)(int, short, void *)) > { > - struct iked_socket *sock, **sptr, **nptr; > + struct iked_socket *sock, **sock0, **sock1; > > log_debug("%s: received socket fd %d", __func__, imsg->fd); > > @@ -577,23 +595,24 @@ > > switch (sock->sock_addr.ss_family) { > case AF_INET: > - sptr = &env->sc_sock4[0]; > - nptr = &env->sc_sock4[1]; > + sock0 = &env->sc_sock4[0]; > + sock1 = &env->sc_sock4[1]; > break; > case AF_INET6: > - sptr = &env->sc_sock6[0]; > - nptr = &env->sc_sock6[1]; > + sock0 = &env->sc_sock6[0]; > + sock1 = &env->sc_sock6[1]; > break; > default: > - fatal("config_getsocket: socket af"); > + fatal("config_getsocket: socket af: %u", > + sock->sock_addr.ss_family); > /* NOTREACHED */ > } > - if (*sptr == NULL) > - *sptr = sock; > - if (*nptr == NULL && > - socket_getport((struct sockaddr *)&sock->sock_addr) == > - IKED_NATT_PORT) > - *nptr = sock; > + if (*sock0 == NULL) > + *sock0 = sock; > + else if (*sock1 == NULL) > + *sock1 = sock; > + else > + fatalx("%s: too many call", __func__); > > event_set(&sock->sock_ev, sock->sock_fd, > EV_READ|EV_PERSIST, cb, sock); > Index: sbin/iked/iked.8 > =================================================================== > RCS file: /cvs/src/sbin/iked/iked.8,v > retrieving revision 1.25 > diff -u -r1.25 iked.8 > --- sbin/iked/iked.8 21 Jan 2020 07:02:45 -0000 1.25 > +++ sbin/iked/iked.8 4 Apr 2020 16:48:55 -0000 > @@ -85,6 +85,7 @@ > .Em net.inet.esp.udpencap_port > .Xr sysctl 2 > variable has to be set accordingly. > +Implies -t. Good find! > .It Fl S > Start > .Nm > Index: sbin/iked/iked.c > =================================================================== > RCS file: /cvs/src/sbin/iked/iked.c,v > retrieving revision 1.42 > diff -u -r1.42 iked.c > --- sbin/iked/iked.c 3 Apr 2020 09:11:23 -0000 1.42 > +++ sbin/iked/iked.c 4 Apr 2020 16:48:55 -0000 > @@ -67,6 +67,7 @@ > int c; > int debug = 0, verbose = 0; > int opts = 0; > + enum natt_mode natt_mode = NATT_DEFAULT; > in_port_t port = IKED_NATT_PORT; > const char *conffile = IKED_CONFIG; > struct iked *env = NULL; > @@ -103,14 +104,20 @@ > opts |= IKED_OPT_PASSIVE; > break; > case 'T': > - opts |= IKED_OPT_NONATT; > + if (natt_mode == NATT_FORCE) > + errx(1, "-T and -t/-p are mutually exclusive"); > + natt_mode = NATT_DISABLE; > break; > case 't': > - opts |= IKED_OPT_NATT; > + if (natt_mode == NATT_DISABLE) > + errx(1, "-T and -t are mutually exclusive"); > + natt_mode = NATT_FORCE; > break; > case 'p': > + if (natt_mode == NATT_DISABLE) > + errx(1, "-T and -p are mutually exclusive"); > port = atoi(optarg); > - opts |= IKED_OPT_NATT; > + natt_mode = NATT_FORCE; > break; > default: > usage(); > @@ -126,16 +133,13 @@ > fatal("calloc: env"); > > env->sc_opts = opts; > + env->natt_mode = natt_mode; > env->sc_nattport = port; > > ps = &env->sc_ps; > ps->ps_env = env; > TAILQ_INIT(&ps->ps_rcsocks); > > - if ((opts & (IKED_OPT_NONATT|IKED_OPT_NATT)) == > - (IKED_OPT_NONATT|IKED_OPT_NATT)) > - errx(1, "conflicting NAT-T options"); > - > if (strlcpy(env->sc_conffile, conffile, PATH_MAX) >= PATH_MAX) > errx(1, "config file exceeds PATH_MAX"); > > @@ -227,17 +231,18 @@ > bzero(&ss, sizeof(ss)); > ss.ss_family = AF_INET; > > - if ((env->sc_opts & IKED_OPT_NATT) == 0 && env->sc_nattport == > IKED_NATT_PORT) > + /* see comment on config_setsocket() */ > + if (env->natt_mode != NATT_FORCE) > config_setsocket(env, &ss, htons(IKED_IKE_PORT), PROC_IKEV2); > - if ((env->sc_opts & IKED_OPT_NONATT) == 0) > + if (env->natt_mode != NATT_DISABLE) > config_setsocket(env, &ss, htons(env->sc_nattport), PROC_IKEV2); > > bzero(&ss, sizeof(ss)); > ss.ss_family = AF_INET6; > > - if ((env->sc_opts & IKED_OPT_NATT) == 0 && env->sc_nattport == > IKED_NATT_PORT) > + if (env->natt_mode != NATT_FORCE) > config_setsocket(env, &ss, htons(IKED_IKE_PORT), PROC_IKEV2); > - if ((env->sc_opts & IKED_OPT_NONATT) == 0) > + if (env->natt_mode != NATT_DISABLE) > config_setsocket(env, &ss, htons(env->sc_nattport), PROC_IKEV2); > > /* > Index: sbin/iked/iked.h > =================================================================== > RCS file: /cvs/src/sbin/iked/iked.h,v > retrieving revision 1.140 > diff -u -r1.140 iked.h > --- sbin/iked/iked.h 2 Apr 2020 19:44:41 -0000 1.140 > +++ sbin/iked/iked.h 4 Apr 2020 16:48:55 -0000 > @@ -661,10 +661,17 @@ > * Daemon configuration > */ > > +enum natt_mode { > + NATT_DEFAULT, /* send/recv with both :500 and NAT-T port */ > + NATT_DISABLE, /* send/recv with only :500 */ > + NATT_FORCE, /* send/recv with only NAT-T port */ > +}; > + > struct iked { > char sc_conffile[PATH_MAX]; > > uint32_t sc_opts; > + enum natt_mode natt_mode; > uint8_t sc_passive; > uint8_t sc_decoupled; > in_port_t sc_nattport; > @@ -687,8 +694,8 @@ > uint8_t sc_certreqtype; > struct ibuf *sc_certreq; > > - struct iked_socket *sc_sock4[2]; > - struct iked_socket *sc_sock6[2]; > + struct iked_socket *sc_sock4[2]; /* IPv4; see > config_setsocket() */ > + struct iked_socket *sc_sock6[2]; /* IPv6; see > config_setsocket() */ > > struct iked_timer sc_inittmr; > #define IKED_INITIATOR_INITIAL 2 > Index: sbin/iked/ikev2.c > =================================================================== > RCS file: /cvs/src/sbin/iked/ikev2.c,v > retrieving revision 1.209 > diff -u -r1.209 ikev2.c > --- sbin/iked/ikev2.c 2 Apr 2020 19:44:41 -0000 1.209 > +++ sbin/iked/ikev2.c 4 Apr 2020 16:48:56 -0000 > @@ -1142,7 +1142,7 @@ > goto done; > } > > - if ((env->sc_opts & IKED_OPT_NONATT) == 0) { > + if (env->natt_mode != NATT_DISABLE) { > if (ntohs(port) == env->sc_nattport) { > /* Enforce NAT-T on the initiator side */ > log_debug("%s: enforcing NAT-T", __func__); > @@ -1959,7 +1959,7 @@ > goto done; > } > > - if (env->sc_opts & IKED_OPT_NATT) { > + if (env->natt_mode == NATT_FORCE) { > /* Enforce NAT-T/UDP-encapsulation by distorting the digest */ > rnd = arc4random(); > EVP_DigestUpdate(&ctx, &rnd, sizeof(rnd)); > @@ -2702,7 +2702,7 @@ > goto done; > } > > - if ((env->sc_opts & IKED_OPT_NONATT) == 0 && > + if ((env->natt_mode != NATT_DISABLE) && > msg->msg_local.ss_family != AF_UNSPEC) { > if ((len = ikev2_add_nat_detection(env, buf, &pld, &resp, len)) > == -1) > Index: sbin/iked/types.h > =================================================================== > RCS file: /cvs/src/sbin/iked/types.h,v > retrieving revision 1.34 > diff -u -r1.34 types.h > --- sbin/iked/types.h 22 Mar 2020 15:59:05 -0000 1.34 > +++ sbin/iked/types.h 4 Apr 2020 16:48:56 -0000 > @@ -46,9 +46,7 @@ > > #define IKED_OPT_VERBOSE 0x00000001 > #define IKED_OPT_NOACTION 0x00000002 > -#define IKED_OPT_NONATT 0x00000004 > -#define IKED_OPT_NATT 0x00000008 > -#define IKED_OPT_PASSIVE 0x00000010 > +#define IKED_OPT_PASSIVE 0x00000004 > > #define IKED_IKE_PORT 500 > #define IKED_NATT_PORT 4500 >