Hi double-p!
On Wed, Mar 01, 2017 at 08:00:30PM +0100, Philipp Buehler wrote:
> Hi folks,
>
> after trying forth and back to overcome some limitations in relayd along
> multiple
> "instances" and rdomain/rtable I decided to scrub some rust of my C/yacc and
> produced the following diffs against -current to relayd and relayctl.
>
Nice.
> Feats:
> - relayd/relayctl: -s sockname; obviously and blatantly taken from
> ospfd/ospfctl
Regarding the -s:
There is inconsistency across our daemons.
I think it all started with bgpd, where we once had -s and -r options
in the daemon, but replaced them with a config option "socket path
NAME". This was supposed to be the model for other daemons as well,
but they either didn't adopt it or implemented the "old" -s option (it
remains in the *ctl, of course). AFAIK, I only have it in snmpd and
this would be a reference for relayd (except the agentx part in it).
Recently I saw somebody adding a -s to a newer daemon (I forgot which
one), but I forgot to cry out.
---snip---
Author: claudio <clau...@openbsd.org>
Date: Sun Jun 27 19:53:34 2010 +0000
Instead of specifying the control sockets on the command line have them
in bgpd.conf. This allows to add/modify restricted control sockets on
runtime.
Feature request by a few people how often forgot to add -r path when
restarting
bgpd (including myself).
NOTE: this removes the -s and -r arguments from bgpd so pay attention when
updateing.
jajaja sthen@, OK henning@
---snap---
> - relayd: -a anchorname; overcome the problem that one stopping relayd
> cleans
> out everything below 'relayd/*' or not having any seperation (includes) in
> first
> place
OK, setting the anchorname is fine. I have to look at the "cleaning" part.
> - relayd.conf: add 'rtable' as a tableopt; reasoning for "only" there:
> - route -T N exec will already put one into 'on rdomain N'
> - 'listen' already has an 'interface' option to bound to a specific rdomain
> (unless
> I am mistaken here on the kernel/addressing side
> - generally unsure if this has a real use case anyway
>
I do think that this has a use case and inter-rdomain is a feature
that was requested before (some people run it in bigger rdomain
setups).
Make sure that the rtable table option doesn't only alter the rules'
rtable, it should also be used for the health checks (check_*.c)
accordingly or you end up testing hosts in the wrong rdomain.
> Tests done:
> - running two relayd on different anchors, rdomains (via route exec) and so
> on,
> notable to me was the point of purging the main/final anchor in
> flush_rulesets.
>
> Caveats:
> - didnt write C/yacc in some years, so esp. string-handling could need some
> eyeballs
>
The parse.y part is small and looks fine.
But you should brush up your style(9) / KNF (at least in the anchor
part below) ;)
> Patch-white-space-drama; if the patches have mangled WS, there are also
> here:
> https://github.com/double-p/smtf/tree/master/patches
>
> Everybody coming, see you in Tokyo!
>
Bummer, I'm not there, it would be nice to meet you again.
Everyone: double-p was the first person who invited me into OpenBSD! \o/
> PS: iked lacks -s, ikectl doesnt... :)
Yes, I probably intended to put it in iked.conf as well ...
Reyk
> --
> pb
> Index: relayctl.8
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayctl/relayctl.8,v
> retrieving revision 1.32
> diff -u -p -r1.32 relayctl.8
> --- relayctl.8 28 Nov 2015 01:22:44 -0000 1.32
> +++ relayctl.8 28 Feb 2017 20:09:34 -0000
> @@ -23,6 +23,7 @@
> .Nd control the relay daemon
> .Sh SYNOPSIS
> .Nm
> +.Op Fl s Ar socket
> .Ar command
> .Op Ar argument ...
> .Sh DESCRIPTION
> @@ -32,6 +33,17 @@ program controls the
> .Xr relayd 8
> daemon.
> .Pp
> +The following options are available:
> +.Bl -tag -width Ds
> +.It Fl s Ar socket
> +Use
> +.Ar socket
> +instead of the default
> +.Pa /var/run/relayd.sock
> +to communicate with
> +.Xr relayd 8 .
> +.El
> +.Pp
> The following commands are available:
> .Bl -tag -width Ds
> .It Cm host disable Op Ar name | id
> @@ -105,6 +117,7 @@ again.
> .Sh FILES
> .Bl -tag -width "/var/run/relayd.sockXX" -compact
> .It Pa /var/run/relayd.sock
> +Default
> .Ux Ns -domain
> socket used for communication with
> .Xr relayd 8 .
> Index: relayctl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayctl/relayctl.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 relayctl.c
> --- relayctl.c 3 Sep 2016 14:44:21 -0000 1.57
> +++ relayctl.c 28 Feb 2017 20:09:34 -0000
> @@ -88,7 +88,8 @@ usage(void)
> {
> extern char *__progname;
>
> - fprintf(stderr, "usage: %s command [argument ...]\n", __progname);
> + fprintf(stderr, "usage: %s [-s socket] command [argument ...]\n",
> + __progname);
> exit(1);
> }
>
> @@ -101,9 +102,25 @@ main(int argc, char *argv[])
> int ctl_sock;
> int done = 0;
> int n, verbose = 0;
> + int ch;
> + char *sockname;
> +
> + sockname = RELAYD_SOCKET;
> + while ((ch = getopt(argc, argv, "s:")) != -1) {
> + switch (ch) {
> + case 's':
> + sockname = optarg;
> + break;
> + default:
> + usage();
> + /* NOTREACHED */
> + }
> + }
> + argc -= optind;
> + argv += optind;
>
> /* parse options */
> - if ((res = parse(argc - 1, argv + 1)) == NULL)
> + if ((res = parse(argc, argv)) == NULL)
> exit(1);
>
> /* connect to relayd control socket */
> @@ -112,7 +129,7 @@ main(int argc, char *argv[])
>
> bzero(&sun, sizeof(sun));
> sun.sun_family = AF_UNIX;
> - (void)strlcpy(sun.sun_path, RELAYD_SOCKET, sizeof(sun.sun_path));
> + (void)strlcpy(sun.sun_path, sockname, sizeof(sun.sun_path));
> reconnect:
> if (connect(ctl_sock, (struct sockaddr *)&sun, sizeof(sun)) == -1) {
> /* Keep retrying if running in monitor mode */
> @@ -121,7 +138,7 @@ main(int argc, char *argv[])
> usleep(100);
> goto reconnect;
> }
> - err(1, "connect: %s", RELAYD_SOCKET);
> + err(1, "connect: %s", sockname);
> }
>
> if (pledge("stdio", NULL) == -1)
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/parse.y,v
> retrieving revision 1.214
> diff -u -p -r1.214 parse.y
> --- parse.y 5 Jan 2017 13:53:09 -0000 1.214
> +++ parse.y 1 Mar 2017 15:50:24 -0000
> @@ -805,6 +805,13 @@ tableopts : CHECK tablecheck
> break;
> }
> }
> + | RTABLE NUMBER {
> + if ($2 < 0 || $2 > RT_TABLEID_MAX) {
> + yyerror("invalid rtable id %d", $2);
> + YYERROR;
> + }
> + table->conf.rtable = $2;
> + }
> ;
>
> /* should be in sync with sbin/pfctl/parse.y's hashkey */
> Index: pfe_filter.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/pfe_filter.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 pfe_filter.c
> --- pfe_filter.c 24 Jan 2017 10:49:14 -0000 1.61
> +++ pfe_filter.c 1 Mar 2017 15:50:24 -0000
> @@ -60,7 +60,7 @@ init_tables(struct relayd *env)
> i = 0;
>
> TAILQ_FOREACH(rdr, env->sc_rdrs, entry) {
> - if (strlcpy(tables[i].pfrt_anchor, RELAYD_ANCHOR "/",
> + if (strlcpy(tables[i].pfrt_anchor, env->sc_baseanchor,
> sizeof(tables[i].pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
> goto toolong;
> if (strlcat(tables[i].pfrt_anchor, rdr->conf.name,
> @@ -114,7 +114,7 @@ kill_tables(struct relayd *env)
>
> TAILQ_FOREACH(rdr, env->sc_rdrs, entry) {
> memset(&io, 0, sizeof(io));
> - if (strlcpy(io.pfrio_table.pfrt_anchor, RELAYD_ANCHOR "/",
> + if (strlcpy(io.pfrio_table.pfrt_anchor, env->sc_baseanchor,
> sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
> goto toolong;
> if (strlcat(io.pfrio_table.pfrt_anchor, rdr->conf.name,
> @@ -160,7 +160,7 @@ sync_table(struct relayd *env, struct rd
> io.pfrio_size = table->up;
> io.pfrio_size2 = 0;
> io.pfrio_buffer = addlist;
> - if (strlcpy(io.pfrio_table.pfrt_anchor, RELAYD_ANCHOR "/",
> + if (strlcpy(io.pfrio_table.pfrt_anchor, env->sc_baseanchor,
> sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
> goto toolong;
> if (strlcat(io.pfrio_table.pfrt_anchor, rdr->conf.name,
> @@ -274,7 +274,7 @@ flush_table(struct relayd *env, struct r
> return;
>
> memset(&io, 0, sizeof(io));
> - if (strlcpy(io.pfrio_table.pfrt_anchor, RELAYD_ANCHOR "/",
> + if (strlcpy(io.pfrio_table.pfrt_anchor, env->sc_baseanchor,
> sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
> goto toolong;
> if (strlcat(io.pfrio_table.pfrt_anchor, rdr->conf.name,
> @@ -343,7 +343,7 @@ sync_ruleset(struct relayd *env, struct
> return;
>
> bzero(anchor, sizeof(anchor));
> - if (strlcpy(anchor, RELAYD_ANCHOR "/", sizeof(anchor)) >=
> + if (strlcpy(anchor, env->sc_baseanchor, sizeof(anchor)) >=
> PF_ANCHOR_NAME_SIZE)
> goto toolong;
> if (strlcat(anchor, rdr->conf.name, sizeof(anchor)) >=
> @@ -406,10 +406,12 @@ sync_ruleset(struct relayd *env, struct
> rio.rule.dst.port_op = address->port.op;
> rio.rule.dst.port[0] = address->port.val[0];
> rio.rule.dst.port[1] = address->port.val[1];
> - rio.rule.rtableid = -1; /* stay in the main routing table */
> + rio.rule.rtableid = -1; /* default: main routing table */
> rio.rule.onrdomain = env->sc_rtable;
> DPRINTF("%s rtable %d",__func__,env->sc_rtable);
>
> + if (t->conf.rtable > 0)
> + rio.rule.rtableid = t->conf.rtable;
> if (rio.rule.proto == IPPROTO_TCP)
> rio.rule.timeout[PFTM_TCP_ESTABLISHED] =
> (u_int32_t)MINIMUM(rdr->conf.timeout.tv_sec,
> @@ -500,13 +502,14 @@ flush_rulesets(struct relayd *env)
> {
> struct rdr *rdr;
> char anchor[PF_ANCHOR_NAME_SIZE];
> + char *slash = NULL;
>
> if (!(env->sc_conf.flags & F_NEEDPF))
> return;
>
> kill_tables(env);
> TAILQ_FOREACH(rdr, env->sc_rdrs, entry) {
> - if (strlcpy(anchor, RELAYD_ANCHOR "/", sizeof(anchor)) >=
> + if (strlcpy(anchor, env->sc_baseanchor, sizeof(anchor)) >=
> PF_ANCHOR_NAME_SIZE)
> goto toolong;
> if (strlcat(anchor, rdr->conf.name, sizeof(anchor)) >=
> @@ -514,16 +517,21 @@ flush_rulesets(struct relayd *env)
> goto toolong;
> if (transaction_init(env, anchor) == -1 ||
> transaction_commit(env) == -1)
> - log_warn("%s: transaction for %s/ failed", __func__,
> - RELAYD_ANCHOR);
> + log_warn("%s: transaction for %s failed", __func__,
> + anchor);
> }
> - if (strlcpy(anchor, RELAYD_ANCHOR, sizeof(anchor)) >=
> - PF_ANCHOR_NAME_SIZE)
> + if (strlen(anchor) >= PF_ANCHOR_NAME_SIZE)
> goto toolong;
This line doesn't seem to make much sense.
- anchor might be uninitialized here (of sc_rdrs is empty)
- the previous value is not used in the following transaction
> + /* XXX: no trailing / in anchor or we leak the main anchor at exit */
> + bzero(anchor,sizeof(anchor));
> + if (( slash = strchr(env->sc_baseanchor, '/') ) != NULL ) {
> + *slash++ = '\0';
> + (void)strlcpy(anchor, slash, sizeof(anchor));
> + }
Do you mean "trailing" or "leading" anchor?
What about the following:
if (strlcpy(anchor, env->sc_baseanchor, sizeof(anchor)) >=
PF_ANCHOR_NAME_SIZE)
goto toolong;
anchor[strcspn(anchor, "/*")] = '\0';
relayd/ -> relayd
> if (transaction_init(env, anchor) == -1 ||
> transaction_commit(env) == -1)
> - log_warn("%s: transaction for %s failed", __func__,
> - RELAYD_ANCHOR);
> + log_warn("%s: transaction for %s/ failed", __func__,
> + anchor);
> log_debug("%s: flushed rules", __func__);
> return;
>
> @@ -620,7 +628,7 @@ check_table(struct relayd *env, struct r
> io.pfrio_esize = sizeof(struct pfr_tstats);
> io.pfrio_size = 1;
> io.pfrio_buffer = &tstats;
> - if (strlcpy(io.pfrio_table.pfrt_anchor, RELAYD_ANCHOR "/",
> + if (strlcpy(io.pfrio_table.pfrt_anchor, env->sc_baseanchor,
> sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
> goto toolong;
> if (strlcat(io.pfrio_table.pfrt_anchor, rdr->conf.name,
> Index: relayd.8
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.8,v
> retrieving revision 1.25
> diff -u -p -r1.25 relayd.8
> --- relayd.8 27 Jul 2015 14:50:58 -0000 1.25
> +++ relayd.8 1 Mar 2017 15:50:24 -0000
> @@ -23,8 +23,10 @@
> .Sh SYNOPSIS
> .Nm
> .Op Fl dnv
> +.Op Fl a Ar anchor
> .Op Fl D Ar macro Ns = Ns Ar value
> .Op Fl f Ar file
> +.Op Fl s Ar socket
> .Sh DESCRIPTION
> .Nm
> is a daemon to relay and dynamically redirect incoming connections to
> @@ -96,6 +98,9 @@ can be used to alter or report on the st
> .Pp
> The options are as follows:
> .Bl -tag -width Ds
> +.It Fl a Ar anchor
> +Specify an alternative anchorname.
Specify an alternative
.Ar anchor .
(pfctl also just uses "Ar anchor")
> +The default is 'relayd'.
> .It Fl D Ar macro Ns = Ns Ar value
> Define
> .Ar macro
> @@ -120,12 +125,17 @@ Configtest mode.
> Only check the configuration file for validity.
> .It Fl v
> Produce more verbose output.
> +.It Fl s Ar socket
> +Specify an alternative socket.
> +The default is
> +.Pa /var/run/relayd.sock
See comment on top
> .El
> .Sh FILES
> .Bl -tag -width "/var/run/relayd.sockXX" -compact
> .It Pa /etc/relayd.conf
> Default configuration file.
> .It Pa /var/run/relayd.sock
> +Default
> .Ux Ns -domain
> socket used for communication with
> .Xr relayctl 8 .
> Index: relayd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
> retrieving revision 1.165
> diff -u -p -r1.165 relayd.c
> --- relayd.c 24 Jan 2017 10:49:14 -0000 1.165
> +++ relayd.c 1 Mar 2017 15:50:24 -0000
> @@ -107,7 +107,7 @@ usage(void)
> {
> extern char *__progname;
>
> - fprintf(stderr, "usage: %s [-dnv] [-D macro=value] [-f file]\n",
> + fprintf(stderr, "usage: %s [-dnv] [-a anchor ] [-D macro=value] [-f
> file] [-s socket ]\n",
> __progname);
> exit(1);
> }
> @@ -121,12 +121,14 @@ main(int argc, char *argv[])
> struct relayd *env;
> struct privsep *ps;
> const char *conffile = CONF_FILE;
> + const char *ctlsock = RELAYD_SOCKET;
> + char *baseanchor = RELAYD_ANCHOR;
> enum privsep_procid proc_id = PROC_PARENT;
> int proc_instance = 0;
> const char *errp, *title = NULL;
> int argc0 = argc;
>
> - while ((c = getopt(argc, argv, "dD:nI:P:f:v")) != -1) {
> + while ((c = getopt(argc, argv, "a:dD:f:nI:P:s:v")) != -1) {
> switch (c) {
> case 'd':
> debug = 2;
> @@ -159,6 +161,14 @@ main(int argc, char *argv[])
> if (errp)
> fatalx("invalid process instance");
> break;
> + case 's':
> + ctlsock = optarg;
> + break;
> + case 'a':
> + if (strlen(optarg) >= PF_ANCHOR_NAME_SIZE)
> + fatalx("anchor too long");
> + (void)asprintf(&baseanchor, "%s/", optarg);
> + break;
> default:
> usage();
> }
> @@ -180,6 +190,7 @@ main(int argc, char *argv[])
> ps->ps_env = env;
> TAILQ_INIT(&ps->ps_rcsocks);
> env->sc_conffile = conffile;
> + env->sc_baseanchor = baseanchor;
either allocate or assign the string, you're mixing it
(baseanchor = RELAYD_ANCHOR vs asprintf(&baseanchor ...)
Maybe a simple baseanchor = optarg would be enough (see my chunk
above, we don't have to modify baseanchor anymore)
> env->sc_conf.opts = opts;
> TAILQ_INIT(&env->sc_hosts);
> TAILQ_INIT(&env->sc_sessions);
> @@ -200,7 +211,7 @@ main(int argc, char *argv[])
> errx(1, "unknown user %s", RELAYD_USER);
>
> /* Configure the control socket */
> - ps->ps_csock.cs_name = RELAYD_SOCKET;
> + ps->ps_csock.cs_name = ctlsock;
>
> log_init(debug, LOG_DAEMON);
> log_setverbose(verbose);
> Index: relayd.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.conf.5,v
> retrieving revision 1.175
> diff -u -p -r1.175 relayd.conf.5
> --- relayd.conf.5 27 Feb 2017 22:25:58 -0000 1.175
> +++ relayd.conf.5 1 Mar 2017 15:50:24 -0000
> @@ -386,6 +386,9 @@ It must be a multiple of the global inte
> Set the timeout in milliseconds for each host that is checked using
> TCP as the transport.
> This will override the global timeout, which is 200 milliseconds by default.
> +.It Ic rtable Ar id
> +This will insert the rule with its destination within routing table with
> +.Ar id .
> .El
> .Pp
> The following options will set the scheduling algorithm to select a
> Index: relayd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v
> retrieving revision 1.239
> diff -u -p -r1.239 relayd.h
> --- relayd.h 2 Feb 2017 08:24:16 -0000 1.239
> +++ relayd.h 1 Mar 2017 15:50:24 -0000
> @@ -477,6 +477,7 @@ struct table_config {
> int check;
> char demote_group[IFNAMSIZ];
> char ifname[IFNAMSIZ];
> + int rtable;
> struct timeval timeout;
> in_port_t port;
> int retcode;
> @@ -1054,6 +1055,7 @@ struct pfdata {
> struct relayd {
> struct relayd_config sc_conf;
> const char *sc_conffile;
> + char *sc_baseanchor;
> struct pfdata *sc_pf;
> int sc_rtsock;
> int sc_rtseq;
--
