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

On 7/26/18, 11:16 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Fri, Jul 27, 2018 at 12:37:02PM +0900, Kyotaro HORIGUCHI wrote:
>> I intended the same as Bossart said. It is not reasonable that
>> VACUUM and VACUUM FULL behaves differently on rejection for the
>> same reason. The objective of the first (or early) check is
>> rejecting (non-superuser's) unprivileged VACUUM FULL. Any
>> possible modifications on a syscache tuple before taking a lock
>> doesn't seem to harm in the point of view. Anyway the lock
>> acquired in expand_vacuum_rel is not held until vacuum_rel.
>
> Thinking about it harder, it is possible to pass down a status pointer
> that the callback of RangeVarGetRelidExtended could fill if we pass it
> down using the argument list.  This would need special handling for the
> case where expand_vacuum_rel() is NIL but that would be workable.  If
> vacuum_check_rel() does not need to work with elevel >= ERROR, then the
> set of log messages remains the same.

Perhaps I am misunderstanding the goal of the patch.  The original
issue reported above is for a lock pile-up that blocks connection
authentication.  Specifically, VACUUM FULL blocks waiting for an
AccessExclusiveLock on pg_authid in vacuum_rel(), even if the user
does not have the right permissions.  IIUC 0002 is adding a callback
to the OID lookup logic in expand_vacuum_rel() that aims to prevent
users from taking an AccessShareLock when they don't have permissions
to VACUUM the relation.  However, ISTM that this AccessShareLock is
not the issue.  If we allowed all users to get the AccessShareLock but
we ensured we called vacuum_skip_rel() prior to waiting for the
AccessExclusiveLock, I think we are good.  Of course, there's a chance
that a concurrent change would cause the role to lose permissions in
between expand_vacuum_rel() and vacuum_rel(), but that seems like a
risk we are taking regardless.

I think I'm essentially suggesting what you have in 0002 but without
the new RangeVarGetRelidExtended() callback.  I've attached a modified
version of 0002 that seems to fix the originally reported issue.  (I
haven't looked into any extra handling needed for ANALYZE or
partitioned tables.)  Running the same checks for all VACUUMs would
keep things simple and provide a more uniform user experience.
 
> The docs mentioned that shared catalogs are processed, so I did not
> bother, but visibly your comment is that we could be more precise about
> the ownership in this case?  An attempt is attached.

Sorry, I should have been clearer.  But yes, your update is what I was
thinking.

Nathan