Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org, Robert Haas <robertmhaas(at)gmail(dot)com>, Jeremy Schneider <schnjere(at)amazon(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, "Albin, Lloyd P" <lalbin(at)scharp(dot)org>
Subject: Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack
Date: 2018-07-26 05:24:16
Message-ID: 20180726052415.GC3927@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers

Hi,

So, I have spent the last couple of days trying to figure out a nice
solution for VACUUM, TRUNCATE and REINDEX, and attached is a set of
three patches to address the issues of this thread:
1) 0001 does the work for TRUNCATE, but using RangeVarGetRelidExtended
with a custom callback based on the existing truncate_check_rel(). I
had to split the session-level checks into a separate routine as no
Relation is available, but that finishes by being a neat result without
changing any user-facing behavior.
2) 0002 does the work for VACUUM. It happens that vacuum_rel() already
does all the skip-related checks we need to know about to decide if a
relation needs to be vacuum'ed or not, so I refactored things as
follows:
2-1) For a VACUUM manual listing relations, issue an ERROR if it cannot
be vacuum'ed. Previously vacuum_rel() would just log a WARNING and call
it a day *after* locking the relation. But as we need to rely on
RangeVarGetRelidExtended() an ERROR is necessary. The ERROR happens
only if VACUUM FULL is used.
2-2) When a relation list is not specified in a manual VACUUM command,
then the decision to skip the relation is done in get_all_vacuum_rels()
when building the relation list with the pg_class lookup. This logs a
DEBUG message when the relation is skipped, which is more information
that what we have now. The checks need to happen again in vacuum_rel as
the VACUUM work could be spawned across multiple transactions, where a
WARNING is logged.
3) REINDEX is already smart enough to check for ownership of relations
if one is manually listed and reports an ERROR. However it can cause
the instance to be stuck when doing a database-wide REINDEX on a
database using just the owner of this database. In this case it seems
to me that we need to make ReindexMultipleTables in terms of ACL
checks, as per 0003.

I quite like the shape of the patches proposed here, and the refactoring
is I think pretty clear. Each patch can be treated independently as
well. Comments are welcome. (Those patches are not indented yet, which
does not matter much at this stage anyway.)

Thanks,
--
Michael

Attachment Content-Type Size
0001-Refactor-TRUNCATE-execution-to-avoid-early-lock-look.patch text/x-diff 6.4 KB
0002-Refactor-VACUUM-execution-to-avoid-early-lock-lookup.patch text/x-diff 9.8 KB
0003-Restrict-access-to-system-wide-REINDEX-for-non-privi.patch text/x-diff 1.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2018-07-26 05:31:51 Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.
Previous Message Etsuro Fujita 2018-07-26 05:14:58 Re: print_path is missing GatherMerge and CustomScan support

Browse pgsql-bugs by date

  From Date Subject
Next Message Flo Rance 2018-07-26 08:04:57 Re: pgAdmin 4 not opening
Previous Message Michael Paquier 2018-07-26 04:49:07 Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack