On Fri, Apr 27, 2012 at 12:45:01PM +0100, Stuart Henderson wrote:
> On 2012/04/27 00:51, Lawrence Teo wrote:
> > The diff below fixes pfctl so that it will show the 'authpf/*' anchor
> > as intended:
>
> This is extremely useful for relayd/ftp-proxy too.
I agree. I bumped into this bug while writing a custom proxy that uses
'foo/*' anchors, so I think the fix will also help future proxies that
use these anchors.
> > Note that since this diff changes the behavior of
> > "pfctl -a 'foo/*' -sr", it will also change the pfload* regression
> > tests since those tests execute this command:
> > pfctl -o none -a 'regress/*' -gvvsr
> >
> > If this diff is correct, I would appreciate some guidance from the
> > developers on how to address the pfload* regression tests.
>
> First problem: -a 'regress/*' used to print anchor "regress",
> and now it _only_ prints sub-anchors. Following diff changes it
> to print the named anchor itself as well as sub-anchors, which
> is how I would expect recursive printing to work.
Thank you for catching this! I agree that the behavior shown by your
revised diff is much closer to what users expect.
> If '-s l' and maybe also '-s a' get the same treatment then I think
> I'll be OK with this going in.
Here's a revised diff that makes it work with '-s l'.
Index: pfctl.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.310
diff -u -p pfctl.c
--- pfctl.c 18 Apr 2012 14:42:17 -0000 1.310
+++ pfctl.c 28 Apr 2012 02:18:05 -0000
@@ -864,6 +864,7 @@ pfctl_show_rules(int dev, char *path, int opts, enum p
switch (format) {
case PFCTL_SHOW_LABELS:
if (pr.rule.label[0]) {
+ INDENT(depth, !(opts & PF_OPT_VERBOSE));
printf("%s %llu %llu %llu %llu"
" %llu %llu %llu %llu\n",
pr.rule.label,
@@ -1937,6 +1938,7 @@ main(int argc, char *argv[])
int optimize = PF_OPTIMIZE_BASIC;
int level;
char anchorname[MAXPATHLEN];
+ int anchor_wildcard = 0;
char *path;
char *lfile = NULL, *sfile = NULL;
const char *errstr;
@@ -2097,9 +2099,10 @@ main(int argc, char *argv[])
int len = strlen(anchoropt);
if (anchoropt[len - 1] == '*') {
- if (len >= 2 && anchoropt[len - 2] == '/')
+ if (len >= 2 && anchoropt[len - 2] == '/') {
anchoropt[len - 2] = '\0';
- else
+ anchor_wildcard = 1;
+ } else
anchoropt[len - 1] = '\0';
opts |= PF_OPT_RECURSE;
}
@@ -2137,11 +2140,19 @@ main(int argc, char *argv[])
pfctl_load_fingerprints(dev, opts);
pfctl_show_rules(dev, path, opts, PFCTL_SHOW_RULES,
anchorname, 0, 0, shownr);
+ if (anchor_wildcard)
+ pfctl_show_rules(dev, path, opts,
+ PFCTL_SHOW_RULES, anchorname, 0,
+ anchor_wildcard, shownr);
break;
case 'l':
pfctl_load_fingerprints(dev, opts);
pfctl_show_rules(dev, path, opts, PFCTL_SHOW_LABELS,
anchorname, 0, 0, shownr);
+ if (anchor_wildcard)
+ pfctl_show_rules(dev, path, opts,
+ PFCTL_SHOW_LABELS, anchorname, 0,
+ anchor_wildcard, shownr);
break;
case 'q':
pfctl_show_altq(dev, ifaceopt, opts,
I'm not sure what's the best way to handle '-s a' yet, since I think
users typically run 'pfctl -s a' to find out everything about their
full ruleset instead of specific anchors. Applying a recursive
behavior on '-s a' also seems complex. Do you have any thoughts on
what a command like "pfctl -a 'foo/*' -s a" should show?
> Next problem, and this is not directly relating to the above diff
> to fix printing, but it is highlighted by it: the anchors added by
> pfload regression tests are never emptied. Running 'make pfload-update'
> results in the same crap being added to lots of the files it produces.
> So it seems the -Fr / -Fa flush commands also need teaching how to work
> recursively and the regress Makefile changing to use this.
>
> If I bypass this problem by temporarily adjusting pfload${n}-update
> to use individual anchor names per rule (rather than '-a regress'
> I used '-a foo$n') I see that with the above diff an additional
> summary is printed each time, each rule-printing gets these additional
> lines:
>
> [ Skip steps: i=end d=end r=end f=end p=end sa=end da=end sp=end dp=end ]
> [ queue: qname= qid=0 pqname= pqid=0 ]
> [ Evaluations: 0 Packets: 0 Bytes: 0 States: 0
> ]
> + [ Skip steps: i=0 d=0 r=0 f=0 p=0 sa=0 da=0 sp=0 dp=0 ]
> + [ queue: qname= qid=0 pqname= pqid=0 ]
> + [ Evaluations: 0 Packets: 0 Bytes: 0 States: 0
> ]
>
> This is slightly ugly but I don't think it's a major problem.
>
> also spotted, though I don't know if it's a problem, I don't see
> a way to remove an anchor once created. But then again, I don't see
> why you don't end up with hundreds of old ftp-proxy anchors showing
> up...so not sure what's going on here.
I think ftp-proxy removes its sub-anchors by committing empty rulesets
into them. The code is in end_session() in
src/usr.sbin/ftp-proxy/ftp-proxy.c.
I tried to replicate that behavior on the shell by telling pfctl to
load /dev/null into the sub-anchor. It seems to work, though I have
not tested it thoroughly. Here's how my test session looks like when
I created a simple testanchor/xyz sub-anchor and then removed it
completely:
# pfctl -sr
anchor "testanchor/*" all
# pfctl -a 'testanchor/*' -sr
# echo pass | pfctl -a 'testanchor/xyz' -f -
# pfctl -a '*' -sr
anchor "testanchor/*" all {
anchor "xyz" all {
pass all flags S/SA
}
}
# pfctl -a 'testanchor/xyz' -f /dev/null
# pfctl -a '*' -sr
anchor "testanchor/*" all {
}
Lawrence
