From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: let 'pfctl -a "*" -sT' show all tables
To: tech@openbsd.org
Date: Tue, 9 Jan 2024 18:24:03 +0100

Hello,

any interest in this?

thanks and
regards
sashan

On Tue, Nov 28, 2023 at 12:54:00AM +0100, Alexandr Nedvedicky wrote:
> Hello,
> 
> 3 weeks ago kn@ noticed a glitch in pfctl(8) [1] while he was playing with
> relayd(8). Diff below makes 'pfctl -a "*" -sT' to walk through all
> anchors/rulesets and show tables bound to rulesets.
> 
> the relayd(8) creates its tables under 'relayd/*' anchor. Those tables
> are never reported when we run 'pfctl -a "*" -sT'. In order to test my
> change one must create tables under non-root anchor/ruleset:
> 
>     pfctl -a test -t foo -T add 192.168.1.0/24
> 
> command above creates anchor/ruleset 'test' with table 'foo'. The diff 
> makes command 'pfctl -a "*" -sT' to produce output as follows:
> 
>     lumpy# ./pfctl -a "*" -sT
>     foo@test
> 
> If we create a more complex structure:
> 
>     pfctl -t bar -T add 192.168.2.0/24
>     pfctl -a this/is/test -t foobar -T add 172.16.0.0/16
> 
> then one can run 'pfctl -a "*" -sT' to see all tables:
> 
>     lumpy# ./pfctl -a "*" -sT
>     foobar@this/is/test
>     foo@test
>     bar
> 
> we can also ask pfctl(8) to dump tables in ruleset subtree:
> 
>     lumpy# ./pfctl -a "/this/*" -sT
>     foobar@this/is/test
> 
> The diff builds on existing function pfctl_recurse() which walks
> all anchors. The current pfctl_recurse() assumes clear action only.
> Therefore I'm introducing a new PF_OPT_CALLSHOW flag as a hint
> for pfctl_recurse() to adjust its output. I must admit I'm not
> happy about it. On the other hand the only alternative I could
> come up with is copy and modify pfctl_recurse().
> 
> OK to commit?
> 
> thanks and
> regards
> sashan 
> 
> [1] https://marc.info/?l=openbsd-bugs&m=169972184623318&w=2
> 
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
> index f4ff345a49a..ae93c8c841f 100644
> --- a/sbin/pfctl/pfctl.c
> +++ b/sbin/pfctl/pfctl.c
> @@ -118,6 +118,7 @@ int	pfctl_recurse(int, int, const char *,
>  int	pfctl_call_clearrules(int, int, struct pfr_anchoritem *);
>  int	pfctl_call_cleartables(int, int, struct pfr_anchoritem *);
>  int	pfctl_call_clearanchors(int, int, struct pfr_anchoritem *);
> +int	pfctl_call_showtables(int, int, struct pfr_anchoritem *);
>  
>  const char	*clearopt;
>  char		*rulesopt;
> @@ -2300,6 +2301,13 @@ pfctl_call_clearrules(int dev, int opts, struct pfr_anchoritem *pfra)
>  	return (pfctl_clear_rules(dev, opts, pfra->pfra_anchorname));
>  }
>  
> +int
> +pfctl_call_showtables(int dev, int opts, struct pfr_anchoritem *pfra)
> +{
> +	pfctl_show_tables(pfra->pfra_anchorname, opts);
> +	return (0);
> +}
> +
>  int
>  pfctl_call_clearanchors(int dev, int opts, struct pfr_anchoritem *pfra)
>  {
> @@ -2325,10 +2333,12 @@ pfctl_recurse(int dev, int opts, const char *anchorname,
>  	 * so that failures on one anchor do not prevent clearing others.
>  	 */
>  	opts |= PF_OPT_IGNFAIL;
> -	printf("Removing:\n");
> +	if ((opts & PF_OPT_CALLSHOW) == 0)
> +		printf("Removing:\n");
>  	SLIST_FOREACH_SAFE(pfra, anchors, pfra_sle, pfra_save) {
> -		printf("  %s\n", (*pfra->pfra_anchorname == '\0') ?
> -		    "/" : pfra->pfra_anchorname);
> +		if ((opts & PF_OPT_CALLSHOW) == 0)
> +			printf("  %s\n", (*pfra->pfra_anchorname == '\0') ?
> +			    "/" : pfra->pfra_anchorname);
>  		rv |= walkf(dev, opts, pfra);
>  		SLIST_REMOVE(anchors, pfra, pfr_anchoritem, pfra_sle);
>  		free(pfra->pfra_anchorname);
> @@ -2747,7 +2757,12 @@ main(int argc, char *argv[])
>  			pfctl_show_fingerprints(opts);
>  			break;
>  		case 'T':
> -			pfctl_show_tables(anchorname, opts);
> +			if (opts & PF_OPT_RECURSE) {
> +				opts |= PF_OPT_CALLSHOW;
> +				pfctl_recurse(dev, opts, anchorname,
> +				    pfctl_call_showtables);
> +			} else
> +				pfctl_show_tables(anchorname, opts);
>  			break;
>  		case 'o':
>  			pfctl_load_fingerprints(dev, opts);
> diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h
> index 01a61a49d01..625b9488073 100644
> --- a/sbin/pfctl/pfctl_parser.h
> +++ b/sbin/pfctl/pfctl_parser.h
> @@ -52,6 +52,7 @@
>  #define PF_OPT_RECURSE		0x04000
>  #define PF_OPT_PORTNAMES	0x08000
>  #define PF_OPT_IGNFAIL		0x10000
> +#define PF_OPT_CALLSHOW		0x20000
>  
>  #define PF_TH_ALL		0xFF
>  
> diff --git a/sbin/pfctl/pfctl_table.c b/sbin/pfctl/pfctl_table.c
> index d041b15d568..381a6f190ae 100644
> --- a/sbin/pfctl/pfctl_table.c
> +++ b/sbin/pfctl/pfctl_table.c
> @@ -369,21 +369,21 @@ print_table(struct pfr_table *ta, int verbose, int debug)
>  {
>  	if (!debug && !(ta->pfrt_flags & PFR_TFLAG_ACTIVE))
>  		return;
> -	if (verbose) {
> -		printf("%c%c%c%c%c%c%c\t%s",
> +	if (verbose)
> +		printf("%c%c%c%c%c%c%c\t",
>  		    (ta->pfrt_flags & PFR_TFLAG_CONST) ? 'c' : '-',
>  		    (ta->pfrt_flags & PFR_TFLAG_PERSIST) ? 'p' : '-',
>  		    (ta->pfrt_flags & PFR_TFLAG_ACTIVE) ? 'a' : '-',
>  		    (ta->pfrt_flags & PFR_TFLAG_INACTIVE) ? 'i' : '-',
>  		    (ta->pfrt_flags & PFR_TFLAG_REFERENCED) ? 'r' : '-',
>  		    (ta->pfrt_flags & PFR_TFLAG_REFDANCHOR) ? 'h' : '-',
> -		    (ta->pfrt_flags & PFR_TFLAG_COUNTERS) ? 'C' : '-',
> -		    ta->pfrt_name);
> -		if (ta->pfrt_anchor[0])
> -			printf("\t%s", ta->pfrt_anchor);
> -		puts("");
> -	} else
> -		puts(ta->pfrt_name);
> +		    (ta->pfrt_flags & PFR_TFLAG_COUNTERS) ? 'C' : '-');
> +
> +	printf("%s", ta->pfrt_name);
> +	if (ta->pfrt_anchor[0] != '\0')
> +		printf("@%s", ta->pfrt_anchor);
> +
> +	printf("\n");
>  }
>  
>  void
>