On Wed, Aug 14, 2024 at 06:09:09PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Aug 14, 2024 at 06:00:55PM +0200, Phil Sutter wrote:
> > On Wed, Aug 14, 2024 at 01:51:21PM +0200, Pablo Neira Ayuso wrote:
> > > Bail out if rate are used:
> > >
> > > ruleset.nft:5:77-106: Error: Wrong rate format, expecting bytes or kbytes or mbytes
> > > add rule netdev firewall PROTECTED_IPS update @quota_temp_before { ip daddr quota over 45000 mbytes/second } add @quota_trigger { ip daddr }
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > improve error reporting while at this.
> > >
> > > Fixes: 6615676d825e ("src: add per-bytes limit")
> > > Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> > > ---
> > > v2: - change patch subject
> > > - use strndup() to fetch units in rate_parse() so limit rate does not break.
> > >
> > > src/datatype.c | 20 +++++++++++++-------
> > > 1 file changed, 13 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/datatype.c b/src/datatype.c
> > > index d398a9c8c618..297c5d0409d5 100644
> > > --- a/src/datatype.c
> > > +++ b/src/datatype.c
> > > @@ -1485,14 +1485,14 @@ static struct error_record *time_unit_parse(const struct location *loc,
> > > struct error_record *data_unit_parse(const struct location *loc,
> > > const char *str, uint64_t *rate)
> > > {
> > > - if (strncmp(str, "bytes", strlen("bytes")) == 0)
> > > + if (strcmp(str, "bytes") == 0)
> > > *rate = 1ULL;
> > > - else if (strncmp(str, "kbytes", strlen("kbytes")) == 0)
> > > + else if (strcmp(str, "kbytes") == 0)
> > > *rate = 1024;
> > > - else if (strncmp(str, "mbytes", strlen("mbytes")) == 0)
> > > + else if (strcmp(str, "mbytes") == 0)
> > > *rate = 1024 * 1024;
> > > else
> > > - return error(loc, "Wrong rate format");
> > > + return error(loc, "Wrong unit format, expecting bytes, kbytes or mbytes");
> > >
> > > return NULL;
> > > }
> >
> > I have local commits which introduce KBYTES and MBYTES keywords and
> > thereby kill the need for quota_unit and limit_bytes cases in
> > parser_bison.y. It still needs testing and is surely not solving all
> > issues there are, but I find it nicer than the partially redundant code
> > we have right now.
>
> Is this allowing for compact representation? ie. kbytes/second,
> because I remember this was the issue to follow this poor man
> approach.
You're right, I just checked and noticed my changes actually make things
worse, because the parser falls into the string case more often than
not. I wish there was a way to exclude string tokens via start
conditions.
For testing, I just removed the '{string}' case from scanner.l and
updated 'identifier' in parser_bison.y to accept QUOTED_STRING as well.
With that in place, all these parse correctly:
| nft add rule \"t\" \"c\" limit rate 1 kbytes/second
| nft add rule \"t\" \"c\" limit rate 1 kbytes/ second
| nft add rule \"t\" \"c\" limit rate 1 kbytes / second
| nft add rule \"t\" \"c\" limit rate 1 kbytes /second
| nft add rule \"t\" \"c\" limit rate 1kbytes/second
| nft add rule \"t\" \"c\" limit rate 1kbytes /second
| nft add rule \"t\" \"c\" limit rate 1kbytes/ second
> > My motivation for this was to maybe improve parser's ability to handle
> > lack of spaces in input. I still see the scanner fall into the generic
> > "string" token case which requires manual dissection in the parser.
> >
> > What is your motivation for the above changes?
>
> A user that accidentally used:
>
> quota over 10mbytes/second
>
> which is currently accepted, the /second part is misleading, as the
> commit described, this is now rejected after this patch.
I see. Similar, but distinct bug. :)
> > Maybe we could collect parser limitations around these units and see
> > what helps "the most"?
>
> I am fine with handling this from the parser instead, there is now
> flex start conditions that can help to enable these tokens on demand.
Maybe one could introduce a start condition which allows strings, but
it might turn into a mess given the wide use of them. I'll give it a try
and let you know.
Thanks, Phil
