On Sat, Feb 03, 2024 at 09:10:42AM +0100, Willy Tarreau wrote:
> On Fri, Feb 02, 2024 at 06:43:12PM +0000, Lukas Tribus wrote:
> > On Fri, 2 Feb 2024 at 18:42, John Lauro <johnala...@gmail.com> wrote:
> > >
> > > Seems like a lint style checker that doesn't require AI.
> > > For example, it could recognize that the / in /api isn't valid for
> > > req.hdr(host)
> > > [...]
> > > The _ in path_beg is also questionable. You can have _ in dns names,
> > > but are not valid in host names.
> >
> > [ CCing the mailing list again ]
> >
> > A primary use-case for ACLs is to match invalid values and headers (
> > for example in case of zero days).
> >
> > We can't restrict ACLs to valid things only, that would defeat the
> > purpose of ACLs.
>
> I totally agree. We have a diagnostic mode (-dD) which triggers warnings
> on a few more cases. The purpose is to say "this is totally valid but
> unusual, are you sure?". One example would be having two servers with the
> same cookie value, which commonly results from a copy-paste. The case
> above could possibly be another example, but in fact we could check for
> the presence of "and" or "or" on a line, or some other suspicious
> constructs such as those where a pattern has parenthesis and looks like
> a sample fetch function.
>
> But honestly, I think there's already a lot of effort involved in trying
> to help spot mistakes with various warnings, with correct rule ordering
> checks and with suggestions of alternate keywords in case of misspelling,
> all such things rely on fuzzy logic and take a lot of time while being
> very rarely used, so I'm never totally convinced that the benefits are
> worth the effort :-/
Well, this one was took just about the time needed for coffee to start to
make its effect :-)
How about this:
$ cat diag.cfg
global
stats socket /tmp/sock1 level admin mode 666
stats timeout 1h
listen l
bind :8080
mode http
timeout client 10s
timeout server 10s
timeout connect 10s
cookie SERVERID insert indirect
server s1 1.1.1.1:80 cookie blah1
server s2 1.1.1.2:80 cookie blah1
acl blah path a b // c
redirect location / if { req.hdr(host) -i example.com and path_beg
/api or req.hdr(host) -i example2.com }
global
It gives me no warning but triggers many diag warnings:
$ ./haproxy -c -f diag.cfg
$ ./haproxy -dD -c -f diag.cfg
[NOTICE] (23315) : haproxy version is 3.0-dev2-d480b7-58
[NOTICE] (23315) : path to executable is ./haproxy
[DIAG] (23315) : config : parsing [diag.cfg:15] : pattern '//' looks like
a failed attempt at commenting an end of line
[DIAG] (23315) : config : parsing [diag.cfg:16] : pattern 'and' looks
like a failed attempt at using an operator inside a pattern list
[DIAG] (23315) : config : parsing [diag.cfg:16] : pattern 'path_beg'
suspiciously looks like a known acl keyword
[DIAG] (23315) : config : parsing [diag.cfg:16] : pattern 'or' looks like
a failed attempt at using an operator inside a pattern list
[DIAG] (23315) : config : parsing [diag.cfg:16] : pattern 'req.hdr(host)'
suspiciously looks like a known sample fetch keyword
[DIAG] (23315) : config : parsing [diag.cfg:18] : global section detected
after a non-global one, the prevalence of their statements is unspecified
[DIAG] (23315) : config : parsing [diag.cfg:13] : 'server s2' : same
cookie value is set for a previous non-backup server in the same backend, it
may break connection persistence
Warnings were found.
The patch is only this (copy-pasted, for illustration purposes):
--- a/src/acl.c
+++ b/src/acl.c
@@ -546,6 +546,25 @@ struct acl_expr *parse_acl_expr(const char **args, char
**err, struct arg_list *
*/
if (!pat_ref_add(ref, arg, NULL, err))
goto out_free_expr;
+
+ if (global.mode & MODE_DIAG) {
+ if (strcmp(arg, "&&") == 0 || strcmp(arg, "and") == 0
||
+ strcmp(arg, "||") == 0 || strcmp(arg, "or") == 0)
+ ha_diag_warning("parsing [%s:%d] : pattern
'%s' looks like a failed attempt at using an operator inside a pattern list\n",
file, line, arg);
+ else if (strcmp(arg, "#") == 0 || strcmp(arg, "//")
== 0)
+ ha_diag_warning("parsing [%s:%d] : pattern
'%s' looks like a failed attempt at commenting an end of line\n", file, line,
arg);
+ else if (find_acl_kw(arg))
+ ha_diag_warning("parsing [%s:%d] : pattern
'%s' suspiciously looks like a known acl keyword\n", file, line, arg);
+ else {
+ const char *begw = arg, *endw;
+
+ for (endw = begw; is_idchar(*endw); endw++)
+ ;
+
+ if (endw != begw && find_sample_fetch(begw,
endw - begw))
+ ha_diag_warning("parsing [%s:%d] :
pattern '%s' suspiciously looks like a known sample fetch keyword\n", file,
line, arg);
+ }
+ }
args++;
}
I can merge it, if there's no other suggestion. Actually I think it's
not that difficult and such improvements could be low hanging fruits
for those who would like to contribute new features without knowing
all the details of the internals.
Willy
