Date: Mon, 14 Mar 2022 10:55:54 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Ashutosh Sharma <ashu.coek88@gmail.com>,
	Andrew Dunstan <andrew@dunslane.net>, Greg Stark <stark@mit.edu>,
	Jeremy Schneider <schneider@ardentperf.com>,
	Bruce Momjian <bruce@momjian.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com>,
	marvin_liang@qq.com, actyzhang@outlook.com
Subject: Re: pg_walinspect - a new extension to get raw WAL data and WAL stats
Message-ID: <20220314145554.GP10577@tamriel.snowman.net>
References: <CAE9k0P=9SReU_613TXytZmpwL3ZRpnC5zrf96UoNCATKpK-UxQ@mail.gmail.com>
 <CALj2ACXsu42jnjyV7zpNtGMmDCsvXr59A9x-C94uyHov2nmp6Q@mail.gmail.com>
 <CAE9k0P=+-BKGJhx_J+eCTx6dQhkno0Z+cWRThKMf43X6W=MzRw@mail.gmail.com>
 <CALj2ACUtqWX95uAj2VNJED0PnixEeQ=0MEzpouLi+zd_iTugRA@mail.gmail.com>
 <CAE9k0Pn4H48D7iK=T-B2CtnCCab7DgQ4MUUCLfzFf345HmkrGQ@mail.gmail.com>
 <CALj2ACVY37ed+BT_WeM2KQS1TUM90HO9NjNktaKCX2G6X5iXsQ@mail.gmail.com>
 <39c13407c8b7311e9a3b24f2df814696bfa2ebff.camel@j-davis.com>
 <CA+TgmoY4HYy89sgWTWgB9FPZ=S28=CDpDO73X-J7wRgd542hOQ@mail.gmail.com>
 <20220310205424.GJ10577@tamriel.snowman.net>
 <ff684608037760af06a332c975a3414f0c67c458.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="M52+6DBBqdEJXByq"
Content-Disposition: inline
In-Reply-To: <ff684608037760af06a332c975a3414f0c67c458.camel@j-davis.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: <https://www.postgresql.org/message-id/20220314145554.GP10577%40tamriel.snowman.net>
Precedence: bulk


--M52+6DBBqdEJXByq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Thu, 2022-03-10 at 15:54 -0500, Stephen Frost wrote:
> > The standard is basically that all of the functions it brings are
> > written to enforce the PG privilege system and you aren't able to use
> > the extension to bypass those privileges.  In some cases that means
> > that
>=20
> Every extension should follow that standard, right? If it doesn't (e.g.
> creating dangerous functions and granting them to public), then even
> superuser should not install it.

Every extension that's intended to be installed on a multi-user PG
system where the admin cares about security in the database, sure.  I
disagree that this applies universally to every system or every
extension.  Those are standards that modules we distribute in contrib
should meet but I don't know that we necessarily have to have them for,
say, modules in test.

> > the C-language functions installed have if(!superuser) ereport()
> > calls
>=20
> I'm curious why not rely on the grant system where possible? I thought
> we were trying to get away from explicit superuser checks.

We don't yet have capabilities for everything.  I agree that we should
be getting away from explicit superuser checks and explained below how
we might be able to in this case.

> > I've not looked back on this thread, but I'd expect pg_walinspect to
> > need those superuser checks and with those it *could* be marked as
> > trusted, but that again brings into question how useful it is to mark
> > it
> > thusly.
>=20
> As long as any functions are safely accessible to public or a
> predefined role, there is some utility for the 'trusted' marker.

I'm not sure that I agree, though I'm also not sure that it's a useful
thing to debate.  Still, if all of the functions in a particular
extension have explicit if (!superuser) ereport() checks in them, then
while installing it is 'safe' and it could be marked as 'trusted'
there's very little point in doing so as the only person who can get
anything useful from those functions is a superuser, and a superuser can
install non-trusted extensions anyway.  How is it useful to mark such an
extension as 'trusted'?

> As this patch is currently written, pg_monitor has access these
> functions, though I don't think that's the right privilege level at
> least for pg_get_raw_wal_record().

Yeah, I agree that pg_monitor isn't the right thing for such a function
to be checking.

> > I certainly don't think we should allow either database owners or
> > regular users on a system the ability to access the WAL traffic of
> > the
> > entire system.
>=20
> Agreed. That was not what I intended by asking if it should be marked
> 'trusted'. The marker only allows the non-superuser to run the CREATE
> EXTENSION command; it's up to the extension script to decide whether
> any non-superusers can do anything at all with the extension.

Sure.

> >   More forcefully- we should *not* be throwing more access
> > rights towards $owners in general and should be thinking about how we
> > can allow admins, providers, whomever, the ability to control what
> > rights users are given.  If they're all lumped under 'owner' then
> > there's no way for people to provide granular access to just those
> > things they wish and intend to.
>=20
> Not sure I understand, but that sounds like a larger discussion.

The point I was trying to make is that it's better to move in the
direction of things like pg_read_all_data rather than just declaring
that the owner of a database can read all of the tables in that
database, as an example.  Maybe we want to implicitly have such
privilege for the owner of the database too, but we should first make it
something that's able to be GRANT'd out to non-owners so that it's not
required that all of those privileges be given out together at once.

Note that to be considered an 'owner' of an object you have to be a
member of the role that owns the object, which means that said role is
necessarily able to also become the owning role too.  Lumping lots of
privileges together- all the rights that being an 'owner' of the object
conveys, plus the ability to also become that role directly and do
things as that role, works actively against the general idea of 'least
privilege'.

Thanks,

Stephen

--M52+6DBBqdEJXByq
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIcBAEBCgAGBQJiL1d6AAoJEO1sijiDR2RVTGoP/jtkgncpwT+zhjJmQIOw86vA
BvvlcYyitPa/MbFt0dA96McFsi1RPzq2DJttSzxrOXJ+S9eFYx0G55zFCssrVZC5
lJw4tFgrFiF9mn8gGmceYLL5FFidy35SRFNzajwq1NNKtEyjH9G7k3adr1NK51+M
8EPhEfmq0glKJxASgFXwbpWkeaMhgPdAH2yF8pRnIpGHeT8S6ZmbIdD4O954/KXc
1DRRUSG6mwY29JuqxMgBVLQVgAYi+uhFTDk8WASfY6xum7mSYUV7MPIH2Q+UGzxw
NBG1KCIB2WdmLQXGu9y1XvzjeQysg1hg8sumGLC6+urXljgmjyeFEOH3ThSPU9Gw
sT6bTJclJaWDBvN7z7Mmk1xl7LbcpxGieh47Eztnj2aFdqmC8AjjzWEib/d0y73q
gXzJiWwY36syR5yO/EcWHghVg4e4n2pjOd0vvIfZtfbQ9zYmsZD9Ty5NfOp99keY
kozpPop4sVtTcoqLKbziyTD0wr4YEZU3nfy8GqKUQ+Dl5qHvE3JSpOINMYcXqqej
iRoU5ye0Lg2ts2ReD4NvkGOkxIRFr/E3FDsAY0UQsRSFyxtxrrvYCWvs/XqmMt8P
KYgqzYzamI/jnYDkoE54rgqr5x5r4eaLOiAGbrW6kwuCO15msx/DmHgitJXiFLKa
wJG4cYvF2CT7jcyutLtN
=QCY0
-----END PGP SIGNATURE-----

--M52+6DBBqdEJXByq--