Date: Wed, 1 Aug 2018 02:44:52 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Andres Freund <andres@anarazel.de>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Jeremy Schneider <schnjere@amazon.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	"Albin, Lloyd P" <lalbin@scharp.org>
Subject: Re: BUG #15182: Canceling authentication due to timeout aka Denial
 of Service Attack
Message-ID: <20180731174452.GA32583@paquier.xyz>
References: <20180728001012.GC1882@paquier.xyz>
 <6D3CBDB6-C358-49F0-8D00-32E58AD2D3D3@amazon.com>
 <20180730003422.GA2878@paquier.xyz>
 <20180730.175354.32712514.horiguchi.kyotaro@lab.ntt.co.jp>
 <20180730102131.GC2878@paquier.xyz>
 <CA+TgmoaPDAN+Sm1VU9mjwd__HXg635vUgFYbep5T0PdcJstTMQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="G4iJoqBmSsgzjUCe"
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoaPDAN+Sm1VU9mjwd__HXg635vUgFYbep5T0PdcJstTMQ@mail.gmail.com>
Precedence: bulk


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

On Tue, Jul 31, 2018 at 10:34:57AM -0400, Robert Haas wrote:
> Just as a general statement, I don't think we should, as part of a
> patch for the issue discussed on this thread, make any changes AT ALL
> to who has permission to perform which operations.  We *certainly*
> should not back-patch such changes, but we really also should not make
> them on master unless they are discussed on a separate thread with a
> clear subject line and agreed by a clear consensus.

Well, perhaps spawning one thread for each command would make the most
sense then?  I am feeling a bit of confusion here.  There have been
three cases discussed up to now:
1) TRUNCATE, which results in a patch that has no behavioral changes.
2) VACUUM [FULL], where we are also heading toward a patch that has no
behavioral change per the last arguments exchanged, where we make sure
that permission checks are done before acquiring any locks.
3) REINDEX, where a database or a schema owner is able to take an
exclusive lock on a shared catalog with limited permissions.  That
sucks as this block calls to load_critical_index, but I would be ready
to buy to not back-patch such a change if the consensus reached is to
not skip shared catalogs if a database/schema owner has no ownership on
the shared catalog directly.

> This patch should only be about detecting cases where we lack
> permission earlier, before we try to acquire a lock.

Yeah, the REINDEX case is the only one among the three where the set of
relations worked on changes.  Please note that ownership checks happen
in this case for indexes, tables, database and schemas before taking a
lock on them.  Databases/systems and schemas just check for ownership of
respectively the database and the schema.

I'll be happy to create one thread per patch if that helps.  Thinking
about it this would attract more attention to each individual problem
reported.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAltgoBQACgkQnvQgOdby
QH3xhhAAmeQqeVc52DUPhu33Tek5XhkAq0rzJYiuGyr9nwlPAZZFX0djvNKcMOrd
9b4Dg/FH/vtRkiBnxvPXDhdBbaZM00J38pjmSd9i/Lfi19+bv53p1+DFzl60VgYF
t1VGCaNilbRqLzDpt0DvTKDMmlxNYjOHWRIbAvnMiQ8VVFFc4TTe35mkqEYXjHJl
/vcieNdt2vNbE20WJp0Ei3/ureBE6LvRKSbQ5MuieUBB7WmPeG96Gql3l2iYEqGf
OVh9YXByfQ4+KwGcj65dhD0sgciG5q+CdoCLZp3Du0gquvfcuWJKODhnHMXydS1r
lVLE7NixqvIdJvWAUMBQ0TKjqkEDqD18eiC+wptDlW5fqPcqQ/E862GPSCDmr+i3
BrICETIJmVSOfEHQ7W1hej29fBDyaL/+7DYbP3Ll9+DmSCH2PXAzTUBROac+/0fq
k6T2g8USTdiTSfLJEUfX0JGeTLhf9dNCJDpvuvG/FO+s3rPp+lamZ7ePEJbyK/L4
YvIbC3DDNEDHxfegoJTPxIN7vk+816hU72x4MS/rDgMQjKgtT9C3ctia4h8reVr4
VyHiLpx8iwqj0Db5DfM+JffPxn8yI9m4e+ZODkQp1uOUS8zVfZzMXtjspmY9f3bV
we7bjOFl4pj35JODEGwVKH3sPmy28myYfBobPjngF84olRxK9qM=
=VCYh
-----END PGP SIGNATURE-----

--G4iJoqBmSsgzjUCe--