Date: Wed, 23 Sep 2020 09:46:50 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Peter Geoghegan <pg@bowt.ie>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Mark Dilger <mark.dilger@enterprisedb.com>,
	Amul Sul <sulamul@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: new heapcheck contrib module
Message-ID: <20200923134650.GS3063@tamriel.snowman.net>
References: 
 <CA+TgmoZT3kCOaKq6=m5wHGKh_9XQDbCC1LZXL6w_+NMB-n1+mw@mail.gmail.com>
 <CAAJ_b94hsrF4x7UKQj03416fAEXJorUpZ9WhsZZjqgv-4+Wg9A@mail.gmail.com>
 <50F6468F-5A5E-412A-86EC-DF24B549D733@enterprisedb.com>
 <CAAJ_b94+A1Gk5r5sBYbYM2K5hH_gyp_o=+gwJuJhn5ZEm5_rqw@mail.gmail.com>
 <CAAJ_b94Wh_7kM9JuJk7wnNsYSNPdSzz35cOEaE_ZRnPcqvupCg@mail.gmail.com>
 <7D603B53-8078-4AF0-851B-803FF4416501@enterprisedb.com>
 <CA+TgmoZbPtTgkQBGoco39uqKUWuv=0+TOkiNcCuqWWvUkJVuxQ@mail.gmail.com>
 <107F3C9D-2598-4E77-A4D0-0084BD3CBA30@enterprisedb.com>
 <CA+Tgmoa=REbwb49WVA1mKytFFhZq8eK9RfX6zyG=5LrOo_gSAQ@mail.gmail.com>
 <CAH2-Wz=PYjiL=6badH+NNyvCM1_LstEhCLQd3arLQWPtMkAX5w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="R0KbGFzrRdaLlFh/"
Content-Disposition: inline
In-Reply-To: 
 <CAH2-Wz=PYjiL=6badH+NNyvCM1_LstEhCLQd3arLQWPtMkAX5w@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--R0KbGFzrRdaLlFh/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Peter Geoghegan (pg@bowt.ie) wrote:
> On Tue, Sep 22, 2020 at 12:41 PM Robert Haas <robertmhaas@gmail.com> wrot=
e:
> > But now I see that there's no secondary permission check in the
> > verify_nbtree.c code. Is that intentional? Peter, what's the
> > justification for that?
>=20
> As noted by comments in contrib/amcheck/sql/check_btree.sql (the
> verify_nbtree.c tests), this is intentional. Note that we explicitly
> test that a non-superuser role can perform verification following
> GRANT EXECUTE ON FUNCTION ... .

> As I mentioned earlier, this is supported (or at least it is supported
> in my interpretation of things). It just isn't documented anywhere
> outside the test itself.

Would certainly be good to document this but I tend to agree with the
comments that ideally-

a) it'd be nice for a relatively low-privileged user/process could run
   the tests in an ongoing manner
b) we don't want to add more is-superuser checks
c) users shouldn't really be given the ability to see rows they're not
   supposed to have access to

In other places in the code, when an error is generated and the user
doesn't have access to the underlying table or doesn't have BYPASSRLS,
we don't include the details or the actual data in the error.  Perhaps
that approach would make sense here (or perhaps not, but it doesn't seem
entirely crazy to me, anyway).  In other words:

a) keep the ability for someone who has EXECUTE on the function to be
   able to run the function against any relation
b) when we detect an issue, perform a permissions check to see if the
   user calling the function has rights to read the rows of the table
   and, if RLS is enabled on the table, if they have BYPASSRLS
c) if the user has appropriate privileges, log the detailed error, if
   not, return a generic error with a HINT that details weren't
   available due to lack of privileges on the relation

I can appreciate the concerns regarding dead rows ending up being
visible to someone who wouldn't normally be able to see them but I'd
argue we could simply document that fact rather than try to build
something to address it, for this particular case.  If there's push back
on that then I'd suggest we have a "can read dead rows" or some such
capability that can be GRANT'd (in the form of a default role, I would
think) which a user would also have to have in order to get detailed
error reports from this function.

Thanks,

Stephen

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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJfa1HKAAoJEO1sijiDR2RVZ9QQAJYUkqQojHy8yTLK2g/btR9+
2an62Lbck65A+fnUrcaS3Xg8wlNBwSUMH8zmmtjP9F5f8txgkYINdYykCN+YjD4/
e/DMxu7lvmR7vsDMdveAB1XFKQObXgxLRwp6vTie13PQzkjDs+WI1zWCPN6pLCUn
Ec6dF2fxZU0+bNuxc2gDEi6H48gWNFGcnQ5ZSP9dfgj0zhbKASUpknh0EqYhj19g
dWdHTLhP0bjxBBkwYiN7QxSVSpxwvdpcv79eoh9IzMNvsou8tfXIVFRMX6jf95p0
GILHAjop0lJkFWBrm78LXgLYP3JaDe1w0lVu1zs06eyDQoyF0ks2gdTFKNo8JX8V
ZiZDEY12TaUKo8CMj2jTrcX3AZqw4qsCMqFshp85WGjeAH843Sv3ZaV4YKwwD8hr
NIL9SMPZzJAFxhQdDcpc1D79AekJN9PUEqH6xL06vvUeU/MmudJp7EojOP3MyFLq
Nu2Cgpztgxx16dfIWKiY2dcFYx9eLOW/RCq7rUsi4IqVOmaff8p0f/TV1W7J97y7
6N6vUimjlz8v/oiv8WKVPpQeT4DoEe7dDYOO/oaJqxkdeB93+qWNnx6YTCXhioe9
3fWP1/RSDu7WM3moww3L4rbxTNuq33m/dOcc2m0YrnvejteR4SnHt2bepVlj503N
8HBE4T5CRM5oxAH68i/F
=g4SJ
-----END PGP SIGNATURE-----

--R0KbGFzrRdaLlFh/--