Hi, first of all, feature-ACK, and "structure-ACK" - this is a lightweight implementation that can go into 2.6 with not much testing, which should "get the thing done" for most cases. A more sophisticated implementation with per-IP hashing (to avoid (ab-)using the rate-limiter to drown-out legitimate clients) can follwo for 2.7.
This said, most of the code looks good, except for this part:
On Thu, Jan 05, 2023 at 06:56:14PM +0100, Arne Schwabe wrote:
> index ee3783046..cf8622f46 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -864,6 +865,8 @@ init_options(struct options *o, const bool init_gc)
> o->n_bcast_buf = 256;
> o->tcp_queue_limit = 64;
> o->max_clients = 1024;
> + o->cf_initial_per = 10;
> + o->cf_initial_max = 100;
... so this is on-by-default...
> @@ -2613,6 +2618,10 @@ options_postprocess_verify_ce(const struct options
> *options,
> {
> msg(M_USAGE, "--connect-freq only works with --mode server
> --proto udp. Try --max-clients instead.");
> }
> + if (!proto_is_udp(ce->proto) && (options->cf_initial_max ||
> options->cf_initial_per))
> + {
> + msg(M_USAGE, "--connect-freq only works with --mode server
> --proto udp. Try --max-clients instead.");
> + }
... and if it is enabled for !UDP modes, we'll refuse cooperation.
But wouldn't that trigger for *all* TCP servers now, since it defaults
to "active"?
Besides that, the "--connect-freq" there is wrong and needs to be
"--connect-freq-initial" ;-)
[..]
> + else if (streq(p[0], "connect-freq-initial") && p[1] && p[2] && !p[3])
> + {
> + int cf_max, cf_per;
> +
> + VERIFY_PERMISSION(OPT_P_GENERAL);
> + cf_max = atoi(p[1]);
> + cf_per = atoi(p[2]);
> + if (cf_max < 0 || cf_per < 0)
" < 0 " or " <= 0"?
> + {
> + msg(msglevel, "--connect-freq-initial parms must be > 0");
> + goto err;
> + }
... because the inverse of ">0" is not "<0"...
> + options->cf_initial_max = cf_max;
> + options->cf_initial_per = cf_per;
> + }
... now we could of course document "0 turns it off", but then the
message needs to be ">= 0".
> +bool
> +reflect_filter_rate_limit_check(struct initial_packet_rate_limit *irl)
> +{
> + if (now > irl->last_period_reset + irl->period_length)
> + {
> + int dropped = max_int(irl->curr_period_counter -
> irl->max_per_period, 0);
> + if (dropped > 0)
> + {
> + msg(D_TLS_DEBUG_LOW, "Dropped %d initial handshake packets due
> to "
> + "--connect-freq-initial %d %d",
> + dropped, irl->max_per_period, irl->period_length);
> + }
> + irl->last_period_reset = now;
> + irl->curr_period_counter = 0;
> + }
> + /* avoid overflow by capping the counter to INT_MAX -1. Still counting
> + * the number of packets during a period is still useful */
> + irl->curr_period_counter = min_int(irl->curr_period_counter + 1, INT_MAX
> - 1);
This looks uglysh.
With a 10-seconds bucket, an (32bit) "int" would overflow at about
430 million packets per seconds - which is, I think, way beyond what
any reasonable system can handle (10G ethernet saturates at 15 Mpps).
But if you are really worried about overflows - why not use a (64bit)
long here?
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
