Re: Patch proposal: New hooks in the connection path

Hi,

On 6/30/22 11:23 AM, Bharath Rupireddy wrote:
> On Thu, Jun 30, 2022 at 1:31 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> Hi hackers,
>>
>> While commit 960869da08 added some information about connections that have been successfully authenticated, there is
nometrics for connections that have not (or did not reached the authentication stage).
 
>>
>> Adding metrics about failed connections attempts could also help, for example with proper sampling, to:
>>
>> detect spikes in failed login attempts
>> check if there is a correlation between spikes in successful and failed connection attempts
>>
>> While the number of successful connections could also already been tracked with the ClientAuthentication_hook (and
alsothe ones that failed the authentication) we are missing metrics about:
 
>>
>> why the connection failed (could be bad password, bad database, bad user, missing CONNECT privilege...)
>> number of times the authentication stage has not been reached
>> why the authentication stage has not been reached (bad startup packets, timeout while processing startup
packet,...)
>>
>> Those missing metrics (in addition to the ones that can be already gathered) could provide value for:
>>
>> security investigations
>> anomalies detections
>> tracking application misconfigurations
>>
>> In an attempt to be able to provide those metrics, please find attached a patch proposal to add new hooks in the
connectionpath, that would be fired if:
 
>>
>> there is a bad startup packet
>> there is a timeout while processing the startup packet
>> user does not have CONNECT privilege
>> database does not exist
>>
>> For safety those hooks request the use of a const Port parameter, so that they could be used only for reporting
purpose(for example, we are working on an extension to record detailed login metrics counters).
 
>>
>> Another option could be to add those metrics in the engine itself (instead of providing new hooks to get them), but
thenew hooks option gives more flexibility on how to render and exploit them (there is a lot of information in the Port
Structthat one could be interested with).
 
>>
>> I’m adding this patch proposal to the commitfest.
>> Looking forward to your feedback,
> +1 for the idea. I've seen numerous cases where the login metrics
> (especially failed connections) are handy in analyzing stuff. And I'm
> okay with the hook approach than the postgres emitting the necessary
> metrics.

Thanks for looking at it!

> However, I'm personally not okay with having multiple hooks
> as proposed in the v1 patch.

I agree that it would be great to reduce the number of proposed hooks.

But,

>   Can we think of having a single hook

The proposed hooks are triggered during errors (means that the 
connection attempt break) and:

- In the connection paths that will not reach the 
ClientAuthentication_hook at all: those are the ones related to the bad 
startup packet and timeout while processing the startup packet.

or

- After the ClientAuthentication_hook is fired: those are the bad db 
oid, bad db name and bad perm ones.

So, It does look like having only one hook would require refactoring in 
the connection path and I'm not sure if this is worth it.

> or
> enhancing the existing ClientAuthentication_hook where we pass a
> PURPOSE parameter (CONN_SUCCESS, CONN_FAILURE, CONN_FOO, CONN_BAR
> ....) tp the hook?

I think one could already "predict" the bad db and bad perm errors 
within the current ClientAuthentication_hook.

But in case of multiple "possible" errors (within the same connection 
attempt) how could we know for sure the one that will be actually 
reported? That's why i think the best way is to put new hooks as close 
as possible to the place where the related errors are reported.

What do you think?

> With this approach, we don't need to spread out the
> postgres code with many hooks and the hook implementers will look at
> the PURPOSE parameter and deal with it accordingly.
>
> On the security aspect, we must ensure we don't leak any sensitive
> information such as password or SSH key to the new hook - if PGPORT
> has this information, maybe we need to mask that structure a bit
> before handing it off to the hook.

Yeah good point, I'll have a closer look if there is any information 
that could be present in the Port struct before the 
ClientAuthentication_hook is fired (means during the ones that are 
related to the startup packet) and that we would want to mask.

Regards,

Bertrand