Re: [PATCH] Exponential backoff for auth_delay

Hi,

Thanks for the patch. I took a closer look at v3, so let me share some
review comments. Please push back if you happen to disagree with some of
it, some of this is going to be more a matter of personal preference.


1) I think it's a bit weird to have two options specifying amount of
time, but one is in seconds and one in milliseconds. Won't that be
unnecessarily confusing? Could we do both in milliseconds?


2) The C code defines the GUC as auth_delay.exponential_backoff, but the
SGML docs say <varname>auth_delay.exp_backoff</varname>.


3) Do we actually need the exponential_backoff GUC? Wouldn't it be
sufficient to have the maximum value, and if it's -1 treat that as no
backoff?


4) I think the SGML docs should more clearly explain that the delay is
initialized to auth_delay.milliseconds, and reset to this value after
successful authentication. The wording kinda implies that, but it's not
quite clear I think.


4) I've been really confused by the change from

     if (status != STATUS_OK)

   to

     if (status == STATUS_ERROR)

in auth_delay_checks, until I realized those two codes are not the only
ones, and we intentionally ignore STATUS_EOF. I think it'd be good to
mention that in a comment, it's not quite obvious (I only realized it
because the e-mail mentioned it).


5) I kinda like the custom that functions in a contrib module start with
a clear common prefix, say auth_delay_ in this case. Matter of personal
preference, ofc.


6) I'm a bit skeptical about some acr_array details. Firstly, why would
50 entries be enough? Seems like a pretty low and arbitrary number ...
Also, what happens if someone attempts to authenticate, fails to do so,
and then disconnects and never tries again? Or just changes IP? Seems
like the entry will remain in the array forever, no?

Seems like a way to cause a "limited" DoS - do auth failure from 50
different hosts, to fill the table, and now everyone has to wait the
maximum amount of time (if they fail to authenticate).

I think it'd be good to consider:

- Making the acr_array a hash table, and larger than 50 entries (I
wonder if that should be dynamic / customizable by GUC?).

- Make sure the entries are eventually expired, based on time (for
example after 2*max_delay?).

- It would be a good idea to log something when we get into the "full
table" and start delaying everyone by max_delay_seconds. (How would
anyone know that's what's happening, seems rather confusing.)


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company