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