From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: smtpd: independant match and action order
To: "Omar Polo" <op@omarpolo.com>
Cc: tech@openbsd.org, Gilles Chehade <gilles.chehade@gmail.com>
Date: Sat, 31 May 2025 20:47:22 +0200

On Sat, 31 May 2025 18:36:41 +0200,
"Omar Polo" <op@omarpolo.com> wrote:
> 
> op@omarpolo.com wrote:
> > After an issue on github[1], I realized that we don't seem to document
> > that an action needs to be defined prior its use in a match statement.
> > 
> > [1]: https://github.com/OpenSMTPD/OpenSMTPD/issues/1285
> > 
> > However, do we need this restriction?  Diff belows moves the action
> > check after the configuration has been fully parsed.  The downside is
> > that now you get a generic "referenced unknown action foo" error without
> > a line number, but the plus side is that action and matches can be put
> > in any order.
> > 
> > The alternative would just be a man page diff to highlight the ordering
> > requirement.
> > 
> > Thoughts?
> 
> ping
>

I had encountered this error but forgot to report it or dig into it.

I not sure that define an action after match which uses it is good idea, it
smeels like a way which leads to error, and I think to make it clear in man
is the right way.

> diff /usr/src
> path + /usr/src
> commit - 93132b6cfa71f1351ecc1c8aaa0a62d5c278e1da
> blob - b4cf1f21ddb02dce7a4911285e33eebfcf517067
> file + usr.sbin/smtpd/parse.y
> --- usr.sbin/smtpd/parse.y
> +++ usr.sbin/smtpd/parse.y
> @@ -1508,21 +1508,13 @@ match_option match_options
>  | /* empty */
>  ;
>  
> -match_dispatcher:
> -STRING {
> -	if (dict_get(conf->sc_dispatchers, $1) == NULL) {
> -		yyerror("no such dispatcher: %s", $1);
> -		YYERROR;
> -	}
> -	rule->dispatcher = $1;
> -}
> -;
> -
>  action:
>  REJECT {
>  	rule->reject = 1;
>  }
> -| ACTION match_dispatcher
> +| ACTION STRING {
> +	rule->dispatcher = $2;
> +}
>  ;
>  
>  match:
> @@ -3125,6 +3117,15 @@ parse_config(struct smtpd *x_conf, const char *filenam
>  	popfile();
>  	endservent();
>  
> +	/* Make sure no unknown actions were referenced */
> +	TAILQ_FOREACH(rule, conf->sc_rules, r_entry) {
> +		if (dict_get(conf->sc_dispatchers, rule->dispatcher) == NULL) {
> +			log_warnx("error: referenced unknown action %s",
> +			    rule->dispatcher);
> +			errors++;
> +		}
> +	}
> +
>  	/* If the socket listener was not configured, create a default one. */
>  	if (!conf->sc_sock_listener) {
>  		memset(&listen_opts, 0, sizeof listen_opts);
> 

-- 
wbr, Kirill