Message-ID: <80af9ba7-ec24-9134-8fd9-00bf13f5c494@joeconway.com>
Date: Mon, 7 Feb 2022 11:13:04 -0500
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: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Joshua Brindle <joshua.brindle@crunchydata.com>,
 "Bossart, Nathan" <bossartn@amazon.com>, Stephen Frost <sfrost@snowman.net>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
References: <CAGB+Vh4Zv_TvKt2tv3QNS6tUM_F_9icmuj0zjywwcgVi4PAhFA@mail.gmail.com>
 <CA+TgmobiMfUoZ42ZDz_6Ne4zMznOjxfEb22W+jdNX29yL88agw@mail.gmail.com>
 <CAGB+Vh4cCTey_TvE_O0CZu-D+6fVXC8a+C-UTzqkg8oN5YcOrQ@mail.gmail.com>
 <CAGB+Vh7fgOfLtCG+01D8ch3icG2aEd38dvy9uavtKWrpf1_XZw@mail.gmail.com>
 <20211108204449.GO20998@tamriel.snowman.net>
 <CAGB+Vh5RpzZroX-RKLXYWyTidpWX6=qcbMoVWERotAWh-_CeUQ@mail.gmail.com>
 <5F9388A2-35EA-4DAA-BE05-68D55EEA6DC3@amazon.com>
 <CAGB+Vh4Om+DFD2KChk5bjz+F2c+AJWDdPqiKPNxf5Ld3CvwwFw@mail.gmail.com>
 <0ED10807-9E7F-4BB5-9473-4668EE68AE2D@amazon.com>
 <4117557.1641329799@sss.pgh.pa.us>
 <CAGB+Vh4jWh-KuV6qt0FKC3M=HCZT1X=Kn5E1g68s1+0OSfoCdQ@mail.gmail.com>
 <c81ba78d-ae4f-02bc-9518-61cbe00c1f2e@joeconway.com>
 <730520.1644168290@sss.pgh.pa.us>
 <CA+TgmoYeNB1PxMO-TYv4_Ky7XR3-3Le+fpL87T7QMR=OWc7U1Q@mail.gmail.com>
From: Joe Conway <mail@joeconway.com>
Subject: Re: [PATCH v2] use has_privs_for_role for predefined roles
In-Reply-To: <CA+TgmoYeNB1PxMO-TYv4_Ky7XR3-3Le+fpL87T7QMR=OWc7U1Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/80af9ba7-ec24-9134-8fd9-00bf13f5c494%40joeconway.com>
Precedence: bulk

On 2/7/22 10:35, Robert Haas wrote:
> On Sun, Feb 6, 2022 at 12:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Joe Conway <mail@joeconway.com> writes:
>> > I'd like to pick this patch up and see it through to commit/push.
>> > Presumably that will include back-patching to all supported pg versions.
>> > Before I go through the effort to back-patch, does anyone want to argue
>> > that this should *not* be back-patched?
>>
>> Hm, I'm -0.5 or so.  I think changing security-related behaviors
>> in a stable branch is a hard sell unless you are closing a security
>> hole.  This is a fine improvement for HEAD but I'm inclined to
>> leave the back branches alone.
> 
> I think the threshold to back-patch a clear behavior change is pretty
> high, so I do not think it should be back-patched.

ok

> I am also not convinced that a sufficient argument has been made for
> changing this in master. This isn't the only thread where NOINHERIT
> has come up lately, and I have to admit that I'm not a fan. Let's
> suppose that I have two users, let's say sunita and sri. In the real
> world, Sunita is Sri's manager and needs to be able to perform actions
> as Sri when Sri is out of the office, but it isn't desirable for
> Sunita to have Sri's privileges in all situations all the time. So we
> mark role sunita as NOINHERIT and grant sri to sunita. Then it turns
> out that Sunita also needs to be COPY TO/FROM PROGRAM, so we give her
> pg_execute_server_program. Now, if she can't exercise this privilege
> without setting role to the prefined role, that's bad, isn't it? I
> mean, we want her to be able to copy between *her* tables and various
> shell commands, not the tables owned by pg_execute_server_program, of
> which there are presumably none.

Easily worked around with one additional level of role:

8<---------------------------------------
nmx=# create user sunita;
CREATE ROLE
nmx=# create user sri superuser;
CREATE ROLE
nmx=# create user sri_alt noinherit;
CREATE ROLE
nmx=# grant sri to sri_alt;
GRANT ROLE
nmx=# grant sri_alt to sunita;
GRANT ROLE
nmx=# grant pg_execute_server_program to sunita;
GRANT ROLE
nmx=# set session authorization sri;
SET
nmx=# create table foo(id int);
CREATE TABLE
nmx=# insert into foo values(42);
INSERT 0 1
nmx=# set session authorization sunita;
SET
nmx=> select * from foo;
ERROR:  permission denied for table foo
nmx=> set role sri;
SET
nmx=# select * from foo;
  id
----
  42
(1 row)

nmx=# reset role;
RESET
nmx=> select current_user;
  current_user
--------------
  sunita
(1 row)

nmx=> create temp table sfoo(f1 text);
CREATE TABLE
nmx=> copy sfoo(f1) from program 'id';
COPY 1
nmx=> select f1 from sfoo;
                                            f1 

----------------------------------------------------------------------------------------
  uid=1001(postgres) gid=1001(postgres) 
groups=1001(postgres),108(ssl-cert),1002(pgconf)
(1 row)
8<---------------------------------------

> It seems to me that the INHERIT role flag isn't very well-considered.
> Inheritance, or the lack of it, ought to be decided separately for
> each inherited role. However, that would be a major architectural
> change.

Agreed -- that would be useful.

> But in the absence of that, it seems clearly better for predefined
> roles to disregard INHERIT and just always grant the rights they are
> intended to give. Because if we don't do that, then we end up with
> people having to SET ROLE to the predefined role and perform actions
> directly as that role, which seems like it can't be what we want. I
> almost feel like we ought to be looking for ways of preventing people
> from doing SET ROLE to a predefined role altogether, not encouraging
> them to do it.
I disagree with this though.

It is confusing and IMHO dangerous that the predefined roles currently 
work differently than regular roles eith respect to privilege inheritance.

In fact, I would extend that argument to the pseudo-role PUBLIC.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development