Message-ID: <6732fdfd-0ea4-941d-fd85-331ff48af5ce@joeconway.com>
Date: Mon, 21 Mar 2022 16:15:33 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Content-Language: en-US
To: Stephen Frost <sfrost@snowman.net>,
 Joshua Brindle <joshua.brindle@crunchydata.com>
Cc: "Bossart, Nathan" <bossartn@amazon.com>,
 Nathan Bossart <nathandbossart@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
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>
From: Joe Conway <mail@joeconway.com>
Subject: Re: [PATCH v2] use has_privs_for_role for predefined roles
In-Reply-To: 
 <CAOuzzgqzTSwtnUwZXFL2Hx6M2TTTmK_SZyjp+AdTcQMhL_Eskg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/6732fdfd-0ea4-941d-fd85-331ff48af5ce%40joeconway.com>
Precedence: bulk

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