From: Joe Conway <mail@joeconway.com>
To: Stephen Frost <sfrost@snowman.net>
To: Joshua Brindle <joshua.brindle@crunchydata.com>
Cc: Bossart, Nathan <bossartn@amazon.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: [PATCH v2] use has_privs_for_role for predefined roles
Date: Mon, 21 Mar 2022 16:15:33 -0400
Message-ID: <6732fdfd-0ea4-941d-fd85-331ff48af5ce@joeconway.com> (raw)
In-Reply-To: <CAOuzzgqzTSwtnUwZXFL2Hx6M2TTTmK_SZyjp+AdTcQMhL_Eskg@mail.gmail.com>
References: <80af9ba7-ec24-9134-8fd9-00bf13f5c494@joeconway.com>
<CA+TgmoY7Hy2iiJ6SKPKjG9w4D+KiPYtZoXB=PpX2oQkwcK5rUw@mail.gmail.com>
<18eab9b6-1f77-72b2-83d3-1856c17f3fcb@joeconway.com>
<CA+Tgmoa-czkhuLM6ZGKsTTxM56k4Ba2=_2Ai5Uo-u+PSkRVNww@mail.gmail.com>
<CAGB+Vh5h29oiq=paMPoz_f8rAOhF0AFkAup6Wo2+td=JJ+8Brw@mail.gmail.com>
<CA+TgmoaGoWYcPX-T0uAheAfrwcvnLUw8N2FsC7TvqQku-qvJUQ@mail.gmail.com>
<0b1b6cfb-d752-28f6-7ccb-993998be0189@joeconway.com>
<CA+TgmoYhx2KtBKX6Na-050RS5x5O_=arTzA96VmJd7=6hGBtag@mail.gmail.com>
<20220209181333.GD1627503@nathanxps13>
<e2955bbc-f7f4-cb80-d35a-ff754898bab0@joeconway.com>
<20220210192829.GB1684442@nathanxps13>
<6135d0b7-3af9-d6fa-cdaa-990a2fc12147@joeconway.com>
<CAGB+Vh71kLZYq+vt=KB4n81PxG8Ty+ODtp_AM-Zzqf41GHK_Bg@mail.gmail.com>
<744cf762-47d3-050f-5fa1-d4f9e8dbae2e@joeconway.com>
<CAGB+Vh5MeDuUke6Y6PFLkyj-F_z06O=ROWyU-2QMi+ia6+RAPA@mail.gmail.com>
<CAOuzzgqzTSwtnUwZXFL2Hx6M2TTTmK_SZyjp+AdTcQMhL_Eskg@mail.gmail.com>
On 3/20/22 12:38, Stephen Frost wrote:
> Greetings,
>
> On Sun, Mar 20, 2022 at 18:31 Joshua Brindle
> <joshua.brindle@crunchydata.com <mailto:joshua.brindle@crunchydata.com>>
> wrote:
>
> On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail@joeconway.com
> <mailto:mail@joeconway.com>> wrote:
> >
> > On 3/3/22 11:26, Joshua Brindle wrote:
> > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail@joeconway.com
> <mailto:mail@joeconway.com>> wrote:
> > >>
> > >> On 2/10/22 14:28, Nathan Bossart wrote:
> > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
> > >> >> On 2/9/22 13:13, Nathan Bossart wrote:
> > >> >>> I do wonder if users find the differences between
> predefined roles and role
> > >> >>> attributes confusing. INHERIT doesn't govern role
> attributes, but it will
> > >> >>> govern predefined roles when this patch is applied. Maybe
> the role
> > >> >>> attribute system should eventually be deprecated in favor
> of using
> > >> >>> predefined roles for everything. Or perhaps the
> predefined roles should be
> > >> >>> converted to role attributes.
> > >> >>
> > >> >> Yep, I was suggesting that the latter would have been
> preferable to me while
> > >> >> Robert seemed to prefer the former. Honestly I could be
> happy with either of
> > >> >> those solutions, but as I alluded to that is probably a
> discussion for the
> > >> >> next development cycle since I don't see us doing that big
> a change in this
> > >> >> one.
> > >> >
> > >> > I agree. I still think Joshua's proposed patch is a
> worthwhile improvement
> > >> > for v15.
> > >>
> > >> +1
> > >>
> > >> I am planning to get into it in detail this weekend. So far I have
> > >> really just ensured it merges cleanly and passes make world.
> > >
> > > Rebased patch to apply to master attached.
> >
> > Well longer than I planned, but finally took a closer look.
> >
> > I made one minor editorial fix to Joshua's patch, rebased to current
> > master, and added two missing call sites that presumably are
> related to
> > recent commits for pg_basebackup.
>
> FWIW I pinged Stephen when I saw the basebackup changes included
> is_member_of and he didn't think they necessarily needed to be changed
> since they aren't accessible to human and you can't SET ROLE on a
> replication connection in order to access the role where inheritance
> was blocked.
>
> Yeah, though that should really be very clearly explained in comments
> around that code as anything that uses is_member should really be viewed
> as an exception. I also wouldn’t be against using has_privs there
> anyway and saying that you shouldn’t be trying to connect as one role on
> a replication connection and using the privs of another when you don’t
> automatically inherit those rights, but on the assumption that the
> committer intended is_member there because SET ROLE isn’t something one
> does on replication connections, I’m alright with it being as is.
Robert -- any opinion on this? If I am not mistaken it is code that you
are actively working on.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: mail@joeconway.com, sfrost@snowman.net, joshua.brindle@crunchydata.com, bossartn@amazon.com, nathandbossart@gmail.com, robertmhaas@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: [PATCH v2] use has_privs_for_role for predefined roles
In-Reply-To: <6732fdfd-0ea4-941d-fd85-331ff48af5ce@joeconway.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox