Date: Sat, 11 Aug 2018 11:50:03 +0200
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
	"andres@anarazel.de" <andres@anarazel.de>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
	"robertmhaas@gmail.com" <robertmhaas@gmail.com>,
	"Schneider, Jeremy" <schnjere@amazon.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	"lalbin@scharp.org" <lalbin@scharp.org>
Subject: Re: BUG #15182: Canceling authentication due to timeout aka Denial
 of Service Attack
Message-ID: <20180811095003.GA2900@paquier.xyz>
References: <20180726204127.GA1754@paquier.xyz>
 <3D4F2733-92AE-4B74-8912-3C611B027721@amazon.com>
 <20180727023123.GE1754@paquier.xyz>
 <20180727.123702.77822416.horiguchi.kyotaro@lab.ntt.co.jp>
 <20180727041542.GH1754@paquier.xyz>
 <62728D88-85A0-4A9A-A4F8-67D96C1179E0@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="7AUc2qLy4jB3hD7Z"
Content-Disposition: inline
In-Reply-To: <62728D88-85A0-4A9A-A4F8-67D96C1179E0@amazon.com>
Precedence: bulk


--7AUc2qLy4jB3hD7Z
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi Nathan,

On Fri, Jul 27, 2018 at 02:40:42PM +0000, Bossart, Nathan wrote:
> 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.

I have been looking at this patch for VACUUM (perhaps we ought to spawn
a new thread as the cases of REINDEX and TRUNCATE have been addressed),
and I can see a problem with this approach.  If multiple transactions
are used with a list of relations vacuum'ed then simply moving around
the ACL checks would cause a new problem: if the relation vacuum'ed
disappears when processing it with vacuum_rel() after the list of
relations is built then we would get an ERROR instead of a skip because
of pg_class_ownercheck() which is not fail-safe.

Wouldn't it be enough to check just the ACLs in expand_vacuum_rel() and
get_all_vacuum_rels()?  The first is rather similar to what happens for
TRUNCATE, and the second is similar to what has been fixed for VACUUM.
We should also remove the relkind checks out of vacuum_skip_rel() as
those checks are not completely consistent for all those code paths...
--
Michael

--7AUc2qLy4jB3hD7Z
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAltusUsACgkQnvQgOdby
QH2c+Q//WxnhzskpZCTB2W2NkXhT4aJqIhuIb5Rgod6wMtKajrcZrb0AkXqUpnuK
NJboKq9QLHHS+2Y1uhJWfAZAgwNf0oqJ9fG1abd28wrbkhQYNd7eemATpQSTvdS4
jOi1CsuMCgkx8YwaYy1Pr+DZHZQN2ZasJ5IVL3+362/J7qXsQTsf8tAPgfc4TKck
AR8cDERwBagF3IrFvS+rbNezdF5wJJE1pA6IH1EBC2ZkWzqcm/rHboNB70DP2eH2
fsez5+kD/C89AHZc/e8MmGA1OJEXv22wBcCqtHs4dkS6ZfRE1TTWx2YH6KRmV9sk
klYUtD6zkfVhf5bgK9j2mKesDwgNhSxYD72RIq7HHixl43d2uo4VrSkYeGiZnzJ/
STWzxQKJdgQ+pe4YWbVzGBPPmV3blU5Tn7Km5vBvygNyGU5wtqbuvx8RrMsIxr6+
mPruujPKWJPL1Qkwz/3FuQkdGFlw59ZrtSRlRa1gCS1HWDvFkLPO3vmpGpUgYxaT
1BeCTugeQF010/6EiZ9p7/2GVIhLaMrI3aXykN/oeSSLSm7bV6Ulp161SiIbQqCp
WllA3cp4nyiJ7l0Iui3J7dndppvBjh3FZFrf/3pUIPDaJwxDSQLpysM/7zeEkhxT
lRrW+tU3OP8Xo7Y80rjuk6tTqWwIAd2C2LkDar60X5PGm/MW5rw=
=NAKF
-----END PGP SIGNATURE-----

--7AUc2qLy4jB3hD7Z--