Thread: Granting control of SUSET gucs to non-superusers

Hackers,

PostgreSQL defines a number of GUCs that can only be set by superusers.  I would like to support granting privileges on
subsetsof these to non-superuser roles, inspired by Stephen Frost's recent work on pg_read_all_data and
pg_write_all_dataroles. 

The specific use case motivating this work is that of a PostgreSQL service provider.  The provider guarantees certain
aspectsof the service, such as periodic backups, replication, uptime, availability, etc., while making no guarantees of
otheraspects, such as performance associated with the design of the schema or the queries executed.  The provider
shouldbe able to grant to the tenant privileges to set any GUC which cannot be used to "escape the sandbox" and
interferewith the handful of metrics being guaranteed.  Given that the guarantees made by one provider may differ from
thosemade by another, the exact set of GUCs which the provider allows the tenant to control may differ. 

By my count, there are currently 50 such GUCs, already broken down into 15 config groups.  Creating a single new role
pg_set_all_gucsseems much too coarse a control, but creating 50 new groups may be excessive.  We could certainly debate
whichGUCs could be used to escape the sandbox vs. which ones could not, but I would prefer a design that allows the
providerto make that determination.  The patch I would like to submit would only give the provider the mechanism for
controllingthese things, but would not make the security choices for them. 

Do folks think it would make sense to create a role per config group?  Adding an extra 15 default roles seems high to
me,but organizing the feature this way would make the roles easier to document, because there would be a one-to-one
correlationbetween the roles and the config groups. 

I have a WIP patch that I'm not attaching, but if I get any feedback, I might be able to adjust the patch before the
firstversion posted.  The basic idea is that it allows things like: 

    GRANT pg_set_stats_monitoring TO tenant_role;

And then tenant_role could, for example

    SET log_parser_stats TO off;

Thanks

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 04/30/21 19:19, Mark Dilger wrote:

> We could certainly debate which GUCs could be used to escape the sandbox
> vs. which ones could not, but I would prefer a design that allows the
> provider to make that determination.

I find myself wondering how many GUCs flagged SUSET are not flagged that way
because of a determination already made that they could be used to escape.
(Maybe some of the logging ones, only usable to conceal your escape.)

But there might be ways for a provider, scrutinizing each of those
individually, to conclude "this will not allow escape from the sandbox
/I/ have set up, provided the value being set satisfies constraints
x and y" ... a generalization of the LOAD from $libdir/plugins idea.

So that suggests to me some mechanism where a provider could grant
setting foo to role bar using validator baz().

Can SUSET GUCs be set from SECURITY DEFINER functions? Maybe there are
already the pieces to do that, minus some syntax sugar.

Regards,
-Chap

Stephen Frost <sfrost@snowman.net> writes:
> On Fri, Apr 30, 2021 at 19:19 Mark Dilger <mark.dilger@enterprisedb.com>
> wrote:
>> PostgreSQL defines a number of GUCs that can only be set by superusers.  I
>> would like to support granting privileges on subsets of these to
>> non-superuser roles, inspired by Stephen Frost's recent work on
>> pg_read_all_data and pg_write_all_data roles.

> New predefined roles are relatively inexpensive. That said, whatever sets
> we define need to have some meaning to them- one which is reasonably
> future-proofed so that we have some idea what category a new GUC would fit
> into.
> “Can’t be used to gain superuser” may be a sufficiently clear grouping, as
> was more or less contemplated by the “admin” approach.  If that doesn’t
> work though then we need an understanding of what the limits on these
> groups are, so we can competently fit new GUCs into these groups (or invent
> new ones if a new GUC truly falls outside all existing but I would expect
> that to be a rather rare case..).
> We may also wish to keep some GUCs superuser only when they really only
> make sense to be used in a developer context...

Hmm, is there really any point in that?  We already have roles
like "pg_write_server_files" and "pg_execute_server_program",
which allow trivial escalation to superuser if one wishes,
but are still useful as being roles you're a bit less likely
to break your database with accidentally than running as full
superuser.

So ISTM that "pg_set_superuser_parameters" could be treated as
being one of those same sorts of roles that you don't give out
to untrusted people, and then we don't have to worry about
exactly which GUCs might be exactly how dangerous to whom.

If we try to define it as being some lesser level of
privilege than that, I'm afraid we will spend lots of
not-very-productive time trying to classify the security
threats from different GUCs ... and they all have *some*
security issue involved, or they wouldn't be restricted in
the first place.  Plus, I'm not looking forward to having
to issue CVEs when we realize we misclassified something.

            regards, tom lane

> On Apr 30, 2021, at 4:28 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> “Can’t be used to gain superuser” may be a sufficiently clear grouping, as was more or less contemplated by the
“admin”approach.  If that doesn’t work though then we need an understanding of what the limits on these groups are, so
wecan competently fit new GUCs into these groups (or invent new ones if a new GUC truly falls outside all existing but
Iwould expect that to be a rather rare case..).  

When I first heard that providers want to build sandboxes around PostgreSQL, I thought the idea was a little silly,
becauseproviders can just spin up a virtual machine per tenant and give each tenant superuser privileges on their
respectiveVM.  Who cares if they mess it up after that? 

The problem with that idea turns out to be that the providers want to take responsibility for some of the database
maintenance,possibly including backups, replication, etc.  I think the set of controls the provider hands over to the
tenantwill depend very much on the division of responsibility.  If the provider is managing replication, then control
oversession_replication_role and wal_compression is unlikely to be handed to the tenant, but if the tenant is
responsiblefor their own replication scheme, it might be. 

Viewing all of this in terms of which controls allow the tenant to escape a hypothetical sandbox seems like the wrong
approach. Shouldn't we let service providers decide which controls would allow the tenant to escape the specific
sandboxthe provider has designed? 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 04/30/21 22:00, Mark Dilger wrote:
> Viewing all of this in terms of which controls allow the tenant to escape
> a hypothetical sandbox seems like the wrong approach.  Shouldn't we let
> service providers decide which controls would allow the tenant to escape
> the specific sandbox the provider has designed?

I agree that sounds more like the right approach. It seems to me that
in the general case, a provider might conclude that setting foo is
safe in the provider-designed sandbox /if the value being assigned
to it satisfies some provider-determined conditions/.

On 04/30/21 20:02, Chapman Flack wrote:
> So that suggests to me some mechanism where a provider could grant
> setting foo to role bar using validator baz().
>
> Can SUSET GUCs be set from SECURITY DEFINER functions? Maybe there are
> already the pieces to do that, minus some syntax sugar.

The answer seems to be yes: I just created a SECURITY DEFINER function
and used it to change a SUSET-only GUC setting.

So it seems the machinery is already in place with which a provider
could allow a chosen set of SUSET-only GUCs to be set, to values that
satisfy provider-determined conditions, by users in a provider-chosen
role.

Some pretty syntax like GRANT SETTING foo TO ROLE bar WHERE cond;
would simply be sugar on top.

Regards,
-Chap

> On May 1, 2021, at 7:07 AM, Chapman Flack <chap@anastigmatix.net> wrote:
>
> On 04/30/21 22:00, Mark Dilger wrote:
>> Viewing all of this in terms of which controls allow the tenant to escape
>> a hypothetical sandbox seems like the wrong approach.  Shouldn't we let
>> service providers decide which controls would allow the tenant to escape
>> the specific sandbox the provider has designed?
>
> I agree that sounds more like the right approach. It seems to me that
> in the general case, a provider might conclude that setting foo is
> safe in the provider-designed sandbox /if the value being assigned
> to it satisfies some provider-determined conditions/.

> So it seems the machinery is already in place with which a provider
> could allow a chosen set of SUSET-only GUCs to be set, to values that
> satisfy provider-determined conditions, by users in a provider-chosen
> role.

> Some pretty syntax like GRANT SETTING foo TO ROLE bar WHERE cond;
> would simply be sugar on top.

I agree with everything you say here.  I have some thoughts about usability....

I'd like the experience for the tenant to be as similar as possible to having superuser privileges on their own
cluster. The tenant may be migrating an application from a database that they currently manage themselves, and any need
touse different syntax from what they have been using is an extra hurdle that could derail the migration. 

Extra syntax for use by the service provider seems much easier to justify.

If the service provider can install extra role-aware check_hooks for gucs, and if the include directive for
postgresql.confcan specify a role under which a postgresql.conf.tenant file is processed, then the tenant can port
theirapplication and their config file and the only things that should break are those things the provider has
intentionallyprohibited. 

Does this sound like a reasonable approach?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 05/01/21 12:13, Mark Dilger wrote:
> Extra syntax for use by the service provider seems much easier to justify.
> 
> If the service provider can install extra role-aware check_hooks for gucs,
> and if the include directive for postgresql.conf can specify a role under
> which a postgresql.conf.tenant file is processed, then the tenant can port
> their application and their config file and the only things that should break
> are those things the provider has intentionally prohibited.

Maybe version 0 is where the provider just builds a shared object
to go in shared_preload_libraries. The provider has probably already
done a bunch of other stuff more challenging than that.

The GUC system would have to expose a way for the shared object to
chain extra_check_hooks off existing GUCs. An extra_check_hook can check
both the value and the role of the caller.

The configfile syntax for include-with-a-role would be the only other
missing piece.

Version 0.5 is maybe where someone contributes code for such a shared
object that is somewhat general and configured by a yaml file, or
something. (That would probably be easier if an extra_check_hook accepts
the usual void *extra context argument that existing GUC hooks don't.)

Regards,
-Chap

Hi,

On Fri, Apr 30, 2021 at 04:19:22PM -0700, Mark Dilger wrote:
> PostgreSQL defines a number of GUCs that can only be set by
> superusers.  I would like to support granting privileges on subsets of
> these to non-superuser roles, inspired by Stephen Frost's recent work
> on pg_read_all_data and pg_write_all_data roles.
> 
> The specific use case motivating this work is that of a PostgreSQL
> service provider.  The provider guarantees certain aspects of the
> service, such as periodic backups, replication, uptime, availability,
> etc., while making no guarantees of other aspects, such as performance
> associated with the design of the schema or the queries executed.  The
> provider should be able to grant to the tenant privileges to set any
> GUC which cannot be used to "escape the sandbox" and interfere with
> the handful of metrics being guaranteed.  Given that the guarantees
> made by one provider may differ from those made by another, the exact
> set of GUCs which the provider allows the tenant to control may
> differ.
> 
> By my count, there are currently 50 such GUCs, already broken down
> into 15 config groups.  Creating a single new role pg_set_all_gucs
> seems much too coarse a control, but creating 50 new groups may be
> excessive.  We could certainly debate which GUCs could be used to
> escape the sandbox vs. which ones could not, but I would prefer a
> design that allows the provider to make that determination.  The patch
> I would like to submit would only give the provider the mechanism for
> controlling these things, but would not make the security choices for
> them.
> 
> Do folks think it would make sense to create a role per config group?
> Adding an extra 15 default roles seems high to me, but organizing the
> feature this way would make the roles easier to document, because
> there would be a one-to-one correlation between the roles and the
> config groups.
> 
> I have a WIP patch that I'm not attaching, but if I get any feedback,
> I might be able to adjust the patch before the first version posted.
> The basic idea is that it allows things like:
> 
>     GRANT pg_set_stats_monitoring TO tenant_role;
> 
> And then tenant_role could, for example
> 
>     SET log_parser_stats TO off;

Just saying, I've proposed something very similar, albeit for a narrower
scope (mostly the Reporting and Logging category) here:
https://www.postgresql.org/message-id/flat/c2ee39152957af339ae6f3e851aef09930dd2faf.camel@credativ.de


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

On Sat, May 1, 2021 at 12:37 PM Chapman Flack <chap@anastigmatix.net> wrote:
> Maybe version 0 is where the provider just builds a shared object
> to go in shared_preload_libraries. The provider has probably already
> done a bunch of other stuff more challenging than that.
>
> The GUC system would have to expose a way for the shared object to
> chain extra_check_hooks off existing GUCs. An extra_check_hook can check
> both the value and the role of the caller.

I think there are two parts to this problem. First, the SP needs to be
able to delegate to some users but not others the ability to set
superuser GUCs. Second, the SP needs to be able to control which GUCs
the privileged users get to set and perhaps to what values. A hook of
the type you propose here seems like it might work reasonably well for
that second part, but it's not totally obvious to me how it helps with
the first part.

Instead of going to the extreme of one predefined role per GUC, maybe
we could see if the PGC_SUSET GUCs could be divided into buckets based
on the reason they are so marked? For example, log_parser_stats,
log_planner_stats, log_executor_stats, log_statement_stats,
log_btree_build_stats, trace_locks, trace_userlocks, trace_lwlocks,
log_min_duration_statement, and a bunch of others are probably all
SUSET just on the theory that only the superuser should have the right
to control what ends up in the log. But we could make a predefined
role that represents the right to control what ends up in the log, and
then all of those GUCs could be tied to that role. Is that too
coarse-grained? It might be.

One problem with having a separate predefined role for every PGC_SUSET
GUC is that it's no help for extensions. Both auto_explain and
pg_stat_statements have such GUCs, and there may be out-of-core
extensions that do as well. We should try to come up with a system
that doesn't leave them out in the cold.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 05/03/21 11:22, Robert Haas wrote:
>> The GUC system would have to expose a way for the shared object to
>> chain extra_check_hooks off existing GUCs. An extra_check_hook can check
>> both the value and the role of the caller.
> 
> I think there are two parts to this problem. First, the SP needs to be
> able to delegate to some users but not others the ability to set
> superuser GUCs. Second, the SP needs to be able to control which GUCs
> the privileged users get to set and perhaps to what values. A hook of
> the type you propose here seems like it might work reasonably well for
> that second part, but it's not totally obvious to me how it helps with
> the first part.

I guess I was thinking, but forgot to convey to the keyboard, that the
existence of a non-empty extra_check_hooks chain on a SUSET GUC (which
could only have been attached from a shared preload library) would
implicitly change SUSET to mean settable whenever accepted by the hook(s).

Regards,
-Chap

> On May 3, 2021, at 8:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> One problem with having a separate predefined role for every PGC_SUSET
> GUC is that it's no help for extensions. Both auto_explain and
> pg_stat_statements have such GUCs, and there may be out-of-core
> extensions that do as well. We should try to come up with a system
> that doesn't leave them out in the cold.

As things stand, all custom variables defined via the DefineCustom{Bool,Int,Real,String,Enum}Variable are placed in the
CUSTOM_OPTIONSconfig_group.  We could add a role for controlling any SUSET CUSTOM_OPTIONS GUCs, or we could extend
thosefunctions to take a config_group option, or perhaps some of both.  I haven't thought too much yet about whether
allowingextensions to place a custom GUC into one of the predefined groups would be problematic.  Any thoughts on that? 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, May 3, 2021 at 11:45 AM Chapman Flack <chap@anastigmatix.net> wrote:
> I guess I was thinking, but forgot to convey to the keyboard, that the
> existence of a non-empty extra_check_hooks chain on a SUSET GUC (which
> could only have been attached from a shared preload library) would
> implicitly change SUSET to mean settable whenever accepted by the hook(s).

Sure, but the hook still needs a way to know which users are entitled
to set the GUC.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, May 3, 2021 at 12:25 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> As things stand, all custom variables defined via the DefineCustom{Bool,Int,Real,String,Enum}Variable are placed in
theCUSTOM_OPTIONS config_group.  We could add a role for controlling any SUSET CUSTOM_OPTIONS GUCs, or we could extend
thosefunctions to take a config_group option, or perhaps some of both.  I haven't thought too much yet about whether
allowingextensions to place a custom GUC into one of the predefined groups would be problematic.  Any thoughts on that? 

Well...

One idea would be to get rid of PGC_SUSET altogether and instead have
a set of flags associated with each GUC, like PGF_SERVER_LOG,
PGF_CORRUPT_DATA, PGF_CRASH_SERVER. Then you could associate those
flags with particular predefined roles and grant them out to whoever
you want.

So if a GUC is flagged PGF_SERVER_LOG|PGF_CRASH_SERVER, then the
assumption is that it's security-sensitive because it both lets you
alter the contents of the server log and also lets you crash the
server. If you are granted both pg_server_log and pg_crash_server, you
can set it, otherwise not.

This is just wild brainstorming, but my point is that I don't think
doing it by options groups is particularly good, because it doesn't
really have any relationship to why those things are marked SUSET in
the first place. To take an example involving functions rather than
GUCs, the pageinspect functions are super-user only because you can
crash the server by inspecting malformed data that you supply as an
arbitrarily literal, but AFAIK the functions in pgstattuple have no
similar hazard, and are just super-only because we don't really know
who the superuser wants to authorize, and maybe it's not everybody. So
those cases are really different, even though both are extensions. I
think the same likely holds true for GUCs.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 05/03/21 13:23, Robert Haas wrote:
> On Mon, May 3, 2021 at 11:45 AM Chapman Flack <chap@anastigmatix.net> wrote:
>> I guess I was thinking, but forgot to convey to the keyboard, that the
>> existence of a non-empty extra_check_hooks chain on a SUSET GUC (which
>> could only have been attached from a shared preload library) would
>> implicitly change SUSET to mean settable whenever accepted by the hook(s).
> 
> Sure, but the hook still needs a way to know which users are entitled
> to set the GUC.

I was contemplating a version 0 with only that minimal support in core
for allowing a shared preload library to set such hooks (and allowing
include-with-a-role in config files), assuming service providers already
do sophisticated building of stuff to construct the environments they
provide, and a C shared object with hooks that enforce their designed
constraints wouldn't be an onerous burden on top of that.

Such providers could then be the laboratories of democracy building
various forms of such things, and if one of those ends up having a
reasonably general configuration mechanism and gets offered as a
contrib module later or for inclusion in core, well, that's version 0.5.

Regards,
-Chap

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, May 3, 2021 at 12:25 PM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
> > As things stand, all custom variables defined via the DefineCustom{Bool,Int,Real,String,Enum}Variable are placed in
theCUSTOM_OPTIONS config_group.  We could add a role for controlling any SUSET CUSTOM_OPTIONS GUCs, or we could extend
thosefunctions to take a config_group option, or perhaps some of both.  I haven't thought too much yet about whether
allowingextensions to place a custom GUC into one of the predefined groups would be problematic.  Any thoughts on that? 
>
> Well...
>
> One idea would be to get rid of PGC_SUSET altogether and instead have
> a set of flags associated with each GUC, like PGF_SERVER_LOG,
> PGF_CORRUPT_DATA, PGF_CRASH_SERVER. Then you could associate those
> flags with particular predefined roles and grant them out to whoever
> you want.
>
> So if a GUC is flagged PGF_SERVER_LOG|PGF_CRASH_SERVER, then the
> assumption is that it's security-sensitive because it both lets you
> alter the contents of the server log and also lets you crash the
> server. If you are granted both pg_server_log and pg_crash_server, you
> can set it, otherwise not.
>
> This is just wild brainstorming, but my point is that I don't think
> doing it by options groups is particularly good, because it doesn't
> really have any relationship to why those things are marked SUSET in
> the first place. To take an example involving functions rather than
> GUCs, the pageinspect functions are super-user only because you can
> crash the server by inspecting malformed data that you supply as an
> arbitrarily literal, but AFAIK the functions in pgstattuple have no
> similar hazard, and are just super-only because we don't really know
> who the superuser wants to authorize, and maybe it's not everybody. So
> those cases are really different, even though both are extensions. I
> think the same likely holds true for GUCs.

In general, I agree that we should be looking at predefined roles as
being similar to the Linux capabilities system- defining certain kinds
of operations which the user who has that role is allowed to do, and
then both in-core and extensions can make decisions based on what
capabilities the user has been GRANT'd.

Hopefully that would limit the amount of cases where a given capability
ends up being overly broad while at the same time allowing extensions to
sensibly be able to use the defined capabilities for their own needs.

As we do in other places, we should make it clear when a certain
capability allows a user with that capability to gain superuser access
as that may not always be clear to a user.

One thing that seems missing from this discussion and is part of what
paused my effort on the 'admin' role proposed towards the end of the
last cycle is that we really need to consider how this all plays with
ALTER SYSTEM and not just SUSET GUCs but also other (eg: POSTMASTER,
SIGHUP) GUCs.  That is- imv we should have a sensible solution for
more-or-less all GUCs and which would allow a non-superuser to be able
to set POSTMASTER and SIGHUP GUCs (and perhaps others..) through
ALTER SYSTEM.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> One thing that seems missing from this discussion and is part of what
> paused my effort on the 'admin' role proposed towards the end of the
> last cycle is that we really need to consider how this all plays with
> ALTER SYSTEM and not just SUSET GUCs but also other (eg: POSTMASTER,
> SIGHUP) GUCs.

Yeah, I'd meant to bring that up too.  The ability to use ALTER
SYSTEM freely is probably a much bigger use-case than messing with
SUSET variables within one's own session.

I'm still of the opinion that slicing and dicing this at the per-GUC
level is a huge waste of effort.  Just invent one role that lets
grantees set any GUC, document it as being superuser-equivalent,
and be done.

            regards, tom lane

On Mon, May 3, 2021 at 2:41 PM Stephen Frost <sfrost@snowman.net> wrote:
> In general, I agree that we should be looking at predefined roles as
> being similar to the Linux capabilities system- defining certain kinds
> of operations which the user who has that role is allowed to do, and
> then both in-core and extensions can make decisions based on what
> capabilities the user has been GRANT'd.

Cool.

> Hopefully that would limit the amount of cases where a given capability
> ends up being overly broad while at the same time allowing extensions to
> sensibly be able to use the defined capabilities for their own needs.

Yeah. I think it will be a little tricky to get right, as some of the
cases are a bit subjective, I think.

> As we do in other places, we should make it clear when a certain
> capability allows a user with that capability to gain superuser access
> as that may not always be clear to a user.

+1.

> One thing that seems missing from this discussion and is part of what
> paused my effort on the 'admin' role proposed towards the end of the
> last cycle is that we really need to consider how this all plays with
> ALTER SYSTEM and not just SUSET GUCs but also other (eg: POSTMASTER,
> SIGHUP) GUCs.  That is- imv we should have a sensible solution for
> more-or-less all GUCs and which would allow a non-superuser to be able
> to set POSTMASTER and SIGHUP GUCs (and perhaps others..) through
> ALTER SYSTEM.

I missed the earlier discussion on this topic, but I agree that this
is very important. I think that the discussion of capabilities might
help us get there. For instance, if I'm a service provider, and I give
user "bob" the pg_put_whatever_you_want_in_the_server_log role, and
GUCs are tagged so we know what GUCs that affects, then it seems
natural to me to allow Bob to set those GUCs via ALTER SYSTEM as well
as via ALTER USER or ALTER DATABASE. However, if I don't give him the
pg_frob_shell_commands role, he can't set archive_command.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, May 3, 2021 at 2:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm still of the opinion that slicing and dicing this at the per-GUC
> level is a huge waste of effort.  Just invent one role that lets
> grantees set any GUC, document it as being superuser-equivalent,
> and be done.

If you want to grant someone full superuser rights, you can do that
already. The trick is what to do when you want someone to be able to
administer the cluster in a meaningful way without giving them full
superuser rights.

I agree that in some cases it's fine to have predefined roles that are
known to permit easy escalation to superuser privileges, like
pg_execute_server_program. It doesn't provide any real security, but
like you said, it can help prevent mistakes. However, there is a real
use cases for a privileged user who cannot be permitted to escalate to
superuser or to the OS account, but still needs to be able to do some
administration of the cluster. The scenario Mark laid out in his
original post is very common. In fact, it may already be the dominant
model for PostgreSQL deployment, and if it isn't now, it will be in 5
years. Letting each individual company that's providing a hosted
PostgreSQL solution hack up its own solution to that problem, all of
which are subtly incompatible with each other and with upstream, is
not good for users or the project.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, May 3, 2021 at 2:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I'm still of the opinion that slicing and dicing this at the per-GUC
> > level is a huge waste of effort.  Just invent one role that lets
> > grantees set any GUC, document it as being superuser-equivalent,
> > and be done.
>
> If you want to grant someone full superuser rights, you can do that
> already. The trick is what to do when you want someone to be able to
> administer the cluster in a meaningful way without giving them full
> superuser rights.

I would suggest that both are useful, but the one-big-hammer does
nothing to answer the use-case which was brought up on this particular
thread (which is also certainly not the first time this has been
desired).  Instead, I would imagine that there would be a set of
predefined roles for the capabilities and then we might have another
role which is akin to 'pg_monitor' but is 'pg_admin' which is GRANT'd a
bunch of those capabilities and explicitly documented to be able to
become a superuser if they wished to.

Perhaps we would also have a "pg_notsuperuser_admin" which would be
GRANT'd all the capabilities, excluding the ones that could be used to
gain superuser access.

As has also been discussed recently, one of the big missing capabilities
for a "pg_notsuperuser_admin" is a 'create role' capability.  I realize
that's not exactly the same as GUCs but it's a big part of what's
missing to make all of this "run a service where the 'DBA' can do
everything except get out to the OS" stuff work out of the box.

Thanks,

Stephen

> On Apr 30, 2021, at 4:19 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> Hackers,
>
> PostgreSQL defines a number of GUCs that can only be set by superusers.  I would like to support granting privileges
onsubsets of these to non-superuser roles, inspired by Stephen Frost's recent work on pg_read_all_data and
pg_write_all_dataroles. 
>
> The specific use case motivating this work is that of a PostgreSQL service provider.  The provider guarantees certain
aspectsof the service, such as periodic backups, replication, uptime, availability, etc., while making no guarantees of
otheraspects, such as performance associated with the design of the schema or the queries executed.  The provider
shouldbe able to grant to the tenant privileges to set any GUC which cannot be used to "escape the sandbox" and
interferewith the handful of metrics being guaranteed.  Given that the guarantees made by one provider may differ from
thosemade by another, the exact set of GUCs which the provider allows the tenant to control may differ. 
>
> By my count, there are currently 50 such GUCs, already broken down into 15 config groups.  Creating a single new role
pg_set_all_gucsseems much too coarse a control, but creating 50 new groups may be excessive.  We could certainly debate
whichGUCs could be used to escape the sandbox vs. which ones could not, but I would prefer a design that allows the
providerto make that determination.  The patch I would like to submit would only give the provider the mechanism for
controllingthese things, but would not make the security choices for them. 
>
> Do folks think it would make sense to create a role per config group?  Adding an extra 15 default roles seems high to
me,but organizing the feature this way would make the roles easier to document, because there would be a one-to-one
correlationbetween the roles and the config groups. 
>
> I have a WIP patch that I'm not attaching, but if I get any feedback, I might be able to adjust the patch before the
firstversion posted.  The basic idea is that it allows things like: 
>
>    GRANT pg_set_stats_monitoring TO tenant_role;
>
> And then tenant_role could, for example
>
>    SET log_parser_stats TO off;

Ok, here is the first version of the patch for the list (though it is the second version I developed.)  The patch is
quitelong, but most of it is mechanical. 

Overview:

- guc.h defines a new set of privilege masks
- pg_authid.dat defines a new set of roles, with a one-to-one correlation to the privilege masks
- guc_tables.h extends struct config_generic to include a privilege mask field
- guc.c extends the structs for all variables to include a mask of privileges required to set the variable, and checks
theprivileges against the current user's role membership when trying to SET or ALTER SYSTEM SET 
- DefineCustom*Variable functions are extended to take a privileges mask, and all calls to these functions are extended
toinclude privileges for the custom variable being defined 
- new regression tests guc_priv_admin and guc_priv_tenant are defined.  The first creates a role "admin" and assigns it
membershipto all the new roles added in pg_authid.dat.  The second creates a role "tenant" and assigns it to just the
fewnew roles that appear reasonable for a tenant.  Both tests then go on to SET SESSION AUTHORIZATION to the new role
andthen attempt to SET, RESET, ALTER SYSTEM SET, and ALTER SYSTEM RESET most of the variables defined in guc.c.   These
testsmight be too verbose to be worth committing, but I thought they made an easy reference for those reviewing the
patchwho just want to quickly see the behavior. 

One of the consequences of the design is that if a user belongs to a role with permission to SET a variable, they can
alsoALTER SYSTEM SET that variable, at least to the extent that ALTER SYSTEM SET would allow the superuser to do so.
Notall variables can be changed via ALTER SYSTEM SET, though.  This means that some variables, "data_directory" for
example,cannot be changed by any of the new roles.  The first version of the patch, never posted, allowed 'include'
directivesin postgresql.conf to be annotated with roles, such that the included file would be processed with privileges
restrictedto just the listed roles.  This patch doesn't bother, since everything we are likely to care about can be
performedusing ALTER SYSTEM SET, but I can resurrect the 'include' directive logic if there is general demand for that. 

Any user can still SET a PGC_USERSET variable, just as before this patch, but the default permission to do so does not
translateinto permission to ALTER SYSTEM SET that same variable.  For that, the user needs to belong to a role with
permissionto set the variable, which in general for PGC_USERSET variables is the "pg_internal_settings" role.  I'm not
surethis is the right role for all of these, for example "password_encryption" seems like a better fit for role
"pg_interface_settings",but for the first patch posted to the list I didn't fuss too much about roles assigned to
PGC_USERSETvariables.  

I didn't bother updating the docs yet, as I doubt the set of privileges/roles in this patch will survive contact with
thislist.  They are: 

pg_internal_settings:
    - changes to purely internal behavior
pg_stats_settings:
    - changes to stats collection
pg_maintenance_settings
    - changes to autovacuum behavior
pg_storage_settings
    - changes to dealing with storage errors, such as fsync or checksum failure
pg_wal_settings
    - changes to wal, recovery, and replication settings
pg_logging_settings
    - changes to what gets logged
pg_interface_settings
    - changes to the external interface, such as port, authentication, etc.
pg_resource_usage
    - changes to memory, cpu, and disk usage
pg_filesystem_security
    - changes to where files and directories are located, permissions bits on
      files and directories, etc.
pg_exec_command
    - changes to external commands that get executed
pg_server_configuration
    - changes to the configuration of the server vis-a-vis the operating system
      facilities, such as shared memory model used
pg_security_settings
    - changes that relax security, such as turning off privilege checking,
      changing security critical logging settings, adjusting developer options
      which have security implications, or changing settings which could be
      used to create a denial of services attack

Note that some GUC variables have more than one privilege bit set, meaning a user must belong to all corresponding
rolesbefore they can change the setting.  For example, "log_file_mode" requires both pg_filesystem_security and
pg_logging_settings.



—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, May 12, 2021 at 11:59 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> I didn't bother updating the docs yet, as I doubt the set of privileges/roles in this patch will survive contact with
thislist.  They are:
 
>
> [ various things ]

Interesting classification. I think the trick here is going to be to
figure out how we should divide things up in a way that makes sense.
There are a couple of possible approaches that suggest themselves:

- One role for all settings, as suggested by Tom. Seems too
coarse-grained to be of any use.

- A separate grantable privilege for each setting. Very flexible, but unwieldy.

- Group things by which section of postgresql.conf they're in, and
then further restrict some of them as security-sensitive. This is
reasonably close to what you've got, but not exactly what you've got.
One issue is that it risks separating things that are in practice not
useful to separate, creating more predefined roles to manage than we
really need. With your division, what are the chances that someone
wants to grant pg_stats_settings but not pg_maintenance_settings or
the other way around?

- Group things by the security exposure that they present, along the
lines of what I proposed previously. This could be possibly combined
with some other categorization, e.g. section of postgresql.conf. But
if you don't do that, an idea like this in its pure form would say,
ok, well we have a role like pg_administrator which is entitled to
change all GUCs that we think aren't security-sensitive, and then
roles like pg_server_file_acccess, pg_execute_server_programs, etc.
that further restrict some GUCs. The risk here is that mashing too
many things together reduces the chances that somebody's going to be
able to get exactly what they want out of the system.

- Something else.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On May 12, 2021, at 12:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> - Group things by which section of postgresql.conf they're in, and
> then further restrict some of them as security-sensitive. This is
> reasonably close to what you've got, but not exactly what you've got.
> One issue is that it risks separating things that are in practice not
> useful to separate, creating more predefined roles to manage than we
> really need. With your division, what are the chances that someone
> wants to grant pg_stats_settings but not pg_maintenance_settings or
> the other way around?

I think our conversation off-list was worth enough to reiterate here....

When classifying GUC variables, the philosophy of classification needs to be consistent and easily understandable so
that,among other considerations, all future GUC variables have a reasonable chance of be classified correctly by their
patchauthors and committers.  The patch I posted falls short in this regard.  You and I discussed two organizational
options:

Theme+Security:
  - security is considered as falling into three groupings:  (a) host security, which includes files and permissions,
runningexternal commands, etc., (b) network security, which includes all connection options and authentications, and
(c)schema security, which includes database internal object security like rls, object ownership, etc. 
  - theme is based on the GUC config_group, either having one theme per config_group, or basing the theme on the prefix
ofthe config_group such that, for example, QUERY_TUNING_METHOD, QUERY_TUNING_COST, QUERY_TUNING_GEQO, and
QUERY_TUNING_OTHERcould all be in one theme named "pg_query_tuning". 

Admin+Security
  - security works the same as Theme+Security
  - a pg_admin role is required to set all non PGC_USERSET gucs, but some of those gucs *also* require one or more of
thesecurity roles 

The Theme+Security approach might be insufficient for extensibility, given that 3rd-party custom GUCs might not have a
correspondingtheme.  The Admin+Security approach appears better in this regard. 

Admin+Security seems sufficient, in conjunction with Chapman's idea of extensible check_hooks.

Thoughts?


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On May 12, 2021, at 12:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > - Group things by which section of postgresql.conf they're in, and
> > then further restrict some of them as security-sensitive. This is
> > reasonably close to what you've got, but not exactly what you've got.
> > One issue is that it risks separating things that are in practice not
> > useful to separate, creating more predefined roles to manage than we
> > really need. With your division, what are the chances that someone
> > wants to grant pg_stats_settings but not pg_maintenance_settings or
> > the other way around?
>
> I think our conversation off-list was worth enough to reiterate here....
>
> When classifying GUC variables, the philosophy of classification needs to be consistent and easily understandable so
that,among other considerations, all future GUC variables have a reasonable chance of be classified correctly by their
patchauthors and committers.  The patch I posted falls short in this regard.  You and I discussed two organizational
options:
>
> Theme+Security:
>   - security is considered as falling into three groupings:  (a) host security, which includes files and permissions,
runningexternal commands, etc., (b) network security, which includes all connection options and authentications, and
(c)schema security, which includes database internal object security like rls, object ownership, etc. 
>   - theme is based on the GUC config_group, either having one theme per config_group, or basing the theme on the
prefixof the config_group such that, for example, QUERY_TUNING_METHOD, QUERY_TUNING_COST, QUERY_TUNING_GEQO, and
QUERY_TUNING_OTHERcould all be in one theme named "pg_query_tuning". 
>
> Admin+Security
>   - security works the same as Theme+Security
>   - a pg_admin role is required to set all non PGC_USERSET gucs, but some of those gucs *also* require one or more of
thesecurity roles 
>
> The Theme+Security approach might be insufficient for extensibility, given that 3rd-party custom GUCs might not have
acorresponding theme.  The Admin+Security approach appears better in this regard. 
>
> Admin+Security seems sufficient, in conjunction with Chapman's idea of extensible check_hooks.

I'm not entirely following what the difference here is that's being
suggested.  At a high level, I like the idea of defining capabilities
along the lines of "host security", "network security", "schema
security".  I do think we should consider maybe breaking those down a
bit more but I don't know that we'd really need to have much more.

In general, I'm not really keen on such a generic role as 'pg_admin'.  I
would have thought we'd have a matrix where we have categories for GUCs
and roles which are allowed to modify those categories, with the
additional requirement of having host/network/schema capability for
those GUCs which imply that level of access.  Having the low-level
capabilities plus the GUC groups would seem likely to cover most cases
that 3rd party extensions might wish for, in a pretty granular way,
though we could always consider adding more in the future.

Thanks,

Stephen

> On May 13, 2021, at 10:41 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Mark Dilger (mark.dilger@enterprisedb.com) wrote:
>>> On May 12, 2021, at 12:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> - Group things by which section of postgresql.conf they're in, and
>>> then further restrict some of them as security-sensitive. This is
>>> reasonably close to what you've got, but not exactly what you've got.
>>> One issue is that it risks separating things that are in practice not
>>> useful to separate, creating more predefined roles to manage than we
>>> really need. With your division, what are the chances that someone
>>> wants to grant pg_stats_settings but not pg_maintenance_settings or
>>> the other way around?
>>
>> I think our conversation off-list was worth enough to reiterate here....
>>
>> When classifying GUC variables, the philosophy of classification needs to be consistent and easily understandable so
that,among other considerations, all future GUC variables have a reasonable chance of be classified correctly by their
patchauthors and committers.  The patch I posted falls short in this regard.  You and I discussed two organizational
options:
>>
>> Theme+Security:
>>  - security is considered as falling into three groupings:  (a) host security, which includes files and permissions,
runningexternal commands, etc., (b) network security, which includes all connection options and authentications, and
(c)schema security, which includes database internal object security like rls, object ownership, etc. 
>>  - theme is based on the GUC config_group, either having one theme per config_group, or basing the theme on the
prefixof the config_group such that, for example, QUERY_TUNING_METHOD, QUERY_TUNING_COST, QUERY_TUNING_GEQO, and
QUERY_TUNING_OTHERcould all be in one theme named "pg_query_tuning". 
>>
>> Admin+Security
>>  - security works the same as Theme+Security
>>  - a pg_admin role is required to set all non PGC_USERSET gucs, but some of those gucs *also* require one or more of
thesecurity roles 
>>
>> The Theme+Security approach might be insufficient for extensibility, given that 3rd-party custom GUCs might not have
acorresponding theme.  The Admin+Security approach appears better in this regard. 
>>
>> Admin+Security seems sufficient, in conjunction with Chapman's idea of extensible check_hooks.
>
> I'm not entirely following what the difference here is that's being
> suggested.  At a high level, I like the idea of defining capabilities
> along the lines of "host security", "network security", "schema
> security".  I do think we should consider maybe breaking those down a
> bit more but I don't know that we'd really need to have much more.

The distinction that Theme+Security would make is that capabilities can be categorized by the area of the system:
  -- planner
  -- replication
  -- logging
  ...
but also by the security implications of what is being done:
  -- host
  -- schema
  -- network

So if a GUC variable is related to replication, but also impacts the security of libpq connections to the server, then
you'dneed to be a member of both pg_replication_role and pg_network_admin.  If another GUC variable is related to
logging,but also impacts the file permissions or ownership of the log file, you'd need to be a member of both
pg_logging_roleand pg_host_admin. 


The Admin+Security idea would instead say that to SET any GUC variable other than PGC_USERSET gucs, or to ALTER SYSTEM
SETon any GUC variable, you'd need to be a member of pg_admin_role.  If the GUC variable also impacts host security
(filepermissions, etc.) you'd have to also be a member of pg_host_admin, or if it impacts security of connections to
theserver, you'd have to also be a member of pg_network_admin. 

I'm just making up names like "pg_replication_role" and such for illustration.

> In general, I'm not really keen on such a generic role as 'pg_admin'.  I
> would have thought we'd have a matrix where we have categories for GUCs
> and roles which are allowed to modify those categories, with the
> additional requirement of having host/network/schema capability for
> those GUCs which imply that level of access.

Yeah, that's the Theme+Security idea, or at least it seems so to me.

> Having the low-level
> capabilities plus the GUC groups would seem likely to cover most cases
> that 3rd party extensions might wish for, in a pretty granular way,
> though we could always consider adding more in the future.

I'm imagining the security concerns splitting three ways, and the themes splitting on the order of ten different ways.
Wecan quibble over how fine grained the themes should be.  There is a simplicity argument to having them be one-to-one
withthe config_group. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, 2021-05-13 at 11:42 -0700, Mark Dilger wrote:
> The distinction that Theme+Security would make is that capabilities
> can be categorized by the area of the system:
>   -- planner
>   -- replication
>   -- logging
>   ...
> but also by the security implications of what is being done:
>   -- host
>   -- schema
>   -- network
Since the "security" buckets are being used for both proposals -- how
you would deal with overlap between them? When a GUC gives you enough
host access to bleed into the schema and network domains, does it get
all three attributes assigned to it, and thus require membership in all
three roles?

(Thanks, by the way, for this thread -- I think a "capability system"
for superuser access is a great idea.)

--Jacob

Greetings,

* Jacob Champion (pchampion@vmware.com) wrote:
> On Thu, 2021-05-13 at 11:42 -0700, Mark Dilger wrote:
> > The distinction that Theme+Security would make is that capabilities
> > can be categorized by the area of the system:
> >   -- planner
> >   -- replication
> >   -- logging
> >   ...
> > but also by the security implications of what is being done:
> >   -- host
> >   -- schema
> >   -- network
> Since the "security" buckets are being used for both proposals -- how
> you would deal with overlap between them? When a GUC gives you enough
> host access to bleed into the schema and network domains, does it get
> all three attributes assigned to it, and thus require membership in all
> three roles?

The question is about exactly what the operation is, not about what that
operation might allow someone to be able to do by using that access.

'network' might, in theory, allow someone to connect out on a port that
happens to have a bash shell that's running as root on the local box too
which means that it "could" be used to gain 'host' access but that's not
really our concern.

To that point, if it's allowing access to run programs on the host then
'host' is required, but I don't think we should also require 'network'
for 'run programs on the host' because someone might run 'curl' with
that access- that's an issue for the admin and the curl utility to
figure out.

> (Thanks, by the way, for this thread -- I think a "capability system"
> for superuser access is a great idea.)

We've been working in that direction for a long time. :)

Thanks,

Stephen

> On May 13, 2021, at 12:18 PM, Jacob Champion <pchampion@vmware.com> wrote:
>
> On Thu, 2021-05-13 at 11:42 -0700, Mark Dilger wrote:
>> The distinction that Theme+Security would make is that capabilities
>> can be categorized by the area of the system:
>>  -- planner
>>  -- replication
>>  -- logging
>>  ...
>> but also by the security implications of what is being done:
>>  -- host
>>  -- schema
>>  -- network
> Since the "security" buckets are being used for both proposals -- how
> you would deal with overlap between them? When a GUC gives you enough
> host access to bleed into the schema and network domains, does it get
> all three attributes assigned to it, and thus require membership in all
> three roles?

Yeah, from a security standpoint, pg_host_admin basically gives everything away.  I doubt service providers would give
the"host" or "network" security to their tenants, but they would probably consider giving "schema" security to the
tenants.

> (Thanks, by the way, for this thread -- I think a "capability system"
> for superuser access is a great idea.)

I am happy to work on this, and appreciate feedback....

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On May 13, 2021, at 12:30 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>
>
>> On May 13, 2021, at 12:18 PM, Jacob Champion <pchampion@vmware.com> wrote:
>>
>> On Thu, 2021-05-13 at 11:42 -0700, Mark Dilger wrote:
>>> The distinction that Theme+Security would make is that capabilities
>>> can be categorized by the area of the system:
>>> -- planner
>>> -- replication
>>> -- logging
>>> ...
>>> but also by the security implications of what is being done:
>>> -- host
>>> -- schema
>>> -- network
>> Since the "security" buckets are being used for both proposals -- how
>> you would deal with overlap between them? When a GUC gives you enough
>> host access to bleed into the schema and network domains, does it get
>> all three attributes assigned to it, and thus require membership in all
>> three roles?
>
> Yeah, from a security standpoint, pg_host_admin basically gives everything away.  I doubt service providers would
givethe "host" or "network" security to their tenants, but they would probably consider giving "schema" security to the
tenants.
>
>> (Thanks, by the way, for this thread -- I think a "capability system"
>> for superuser access is a great idea.)
>
> I am happy to work on this, and appreciate feedback....

Please find attached five new patches each intended to reduce the number of administrative tasks that require superuser
privileges.

v3-0001 adds a new pg_logical_replication role with permission to manage publications and subscriptions.

v3-0002 adds a new pg_host_security role with permission to manage extensions, event triggers and tablespaces.

v3-0003 adds a new pg_network_security role with pemission to manage foreign servers and data wrappers.

v3-0004 adds a new pg_database_security role with permission to perform many actions that would otherwise require
superuser,so long as those actions do not compromise the security of the host or network.  This role, along with
pg_logical_replication,is intended to be safe to delegate to the tenant of a database provided as a service. 

v3-0005 associates all GUC variables with security roles and allows both SET and ALTER SYSTEM SET on those variables by
usersbelonging to the necessary security role(s).  This patch extends the significance of the pg_host_security,
pg_network_security,and pg_database_security roles added in the previous patches, as those roles are associated with
GUCvariables that implicate the same security concerns. 

These patches likely still need some adjustment, as there are a large number of security relevant permission decisions
inhere which some hackers may debate, but I think these are mature enough to solicit feedback. 

I admit right upfront that the regression tests guc_priv_admin and guc_priv_tenant in v3-0005 could be made to cover a
subsetof GUC variables rather than the full set of them, but I'm delaying pruning them down until I know if the rest of
thepatches are basically acceptable. 




—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, May 25, 2021 at 01:33:54PM -0700, Mark Dilger wrote:
> v3-0001 adds a new pg_logical_replication role with permission to manage publications and subscriptions.

> v3-0004 adds a new pg_database_security role with permission to perform many
> actions that would otherwise require superuser, so long as those actions do
> not compromise the security of the host or network.  This role, along with
> pg_logical_replication, is intended to be safe to delegate to the tenant of
> a database provided as a service.

pg_logical_replication would not be safe to delegate that way:
https://postgr.es/m/flat/CACqFVBbx6PDq%2B%3DvHM0n78kHzn8tvOM-kGO_2q_q0zNAMT%2BTzdA%40mail.gmail.com

> On May 27, 2021, at 11:06 PM, Noah Misch <noah@leadboat.com> wrote:
>
> On Tue, May 25, 2021 at 01:33:54PM -0700, Mark Dilger wrote:
>> v3-0001 adds a new pg_logical_replication role with permission to manage publications and subscriptions.
>
>> v3-0004 adds a new pg_database_security role with permission to perform many
>> actions that would otherwise require superuser, so long as those actions do
>> not compromise the security of the host or network.  This role, along with
>> pg_logical_replication, is intended to be safe to delegate to the tenant of
>> a database provided as a service.
>
> pg_logical_replication would not be safe to delegate that way:
> https://postgr.es/m/flat/CACqFVBbx6PDq%2B%3DvHM0n78kHzn8tvOM-kGO_2q_q0zNAMT%2BTzdA%40mail.gmail.com

Oh, I agree that this patch set does not go the extra step to make it safe.  You are quite right to push back, as my
emailwas poorly worded.  I should have said "intended to be eventually made safe to delegate". The idea is that the
patchset addresses most places in the sources where we test for superuser and tests instead for (superuser ||
<SOME_ROLE>),and then uses that same set of roles to control who has sufficient privileges to set GUCs.  The
pg_host_securityand pg_network_security roles are not intended to eventually be safe to delegate.  Or at least, I can't
seeany clear path to getting there.  The pg_database_security and pg_logical_replication roles should be ones we can
makesafe.  If we can agree as a community which set of roles are appropriate, then we can have separate patches as
neededfor tightening the security around them. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-05-26 05:33, Mark Dilger wrote:
>> On May 13, 2021, at 12:30 PM, Mark Dilger 
>> <mark.dilger@enterprisedb.com> wrote:
>> 
>> 
>> 
>>> On May 13, 2021, at 12:18 PM, Jacob Champion <pchampion@vmware.com> 
>>> wrote:
>>> 
>>> On Thu, 2021-05-13 at 11:42 -0700, Mark Dilger wrote:
>>>> The distinction that Theme+Security would make is that capabilities
>>>> can be categorized by the area of the system:
>>>> -- planner
>>>> -- replication
>>>> -- logging
>>>> ...
>>>> but also by the security implications of what is being done:
>>>> -- host
>>>> -- schema
>>>> -- network
>>> Since the "security" buckets are being used for both proposals -- how
>>> you would deal with overlap between them? When a GUC gives you enough
>>> host access to bleed into the schema and network domains, does it get
>>> all three attributes assigned to it, and thus require membership in 
>>> all
>>> three roles?
>> 
>> Yeah, from a security standpoint, pg_host_admin basically gives 
>> everything away.  I doubt service providers would give the "host" or 
>> "network" security to their tenants, but they would probably consider 
>> giving "schema" security to the tenants.
>> 
>>> (Thanks, by the way, for this thread -- I think a "capability system"
>>> for superuser access is a great idea.)
>> 
>> I am happy to work on this, and appreciate feedback....
> 
> Please find attached five new patches each intended to reduce the
> number of administrative tasks that require superuser privileges.
> 
> v3-0001 adds a new pg_logical_replication role with permission to
> manage publications and subscriptions.
> 
> v3-0002 adds a new pg_host_security role with permission to manage
> extensions, event triggers and tablespaces.
> 
> v3-0003 adds a new pg_network_security role with pemission to manage
> foreign servers and data wrappers.
> 
> v3-0004 adds a new pg_database_security role with permission to
> perform many actions that would otherwise require superuser, so long
> as those actions do not compromise the security of the host or
> network.  This role, along with pg_logical_replication, is intended to
> be safe to delegate to the tenant of a database provided as a service.
> 
> v3-0005 associates all GUC variables with security roles and allows
> both SET and ALTER SYSTEM SET on those variables by users belonging to
> the necessary security role(s).  This patch extends the significance
> of the pg_host_security, pg_network_security, and pg_database_security
> roles added in the previous patches, as those roles are associated
> with GUC variables that implicate the same security concerns.
> 
> These patches likely still need some adjustment, as there are a large
> number of security relevant permission decisions in here which some
> hackers may debate, but I think these are mature enough to solicit
> feedback.
> 
> I admit right upfront that the regression tests guc_priv_admin and
> guc_priv_tenant in v3-0005 could be made to cover a subset of GUC
> variables rather than the full set of them, but I'm delaying pruning
> them down until I know if the rest of the patches are basically
> acceptable.

Thanks for working on this topic, I appreciate it!

BTW, do these patches enable non-superusers to create user with
bypassrls?
Since I failed to apply the patches and didn't test them,
I may have overlooked something but I didn't find the
corresponding codes.

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

> On Jun 14, 2021, at 5:51 AM, torikoshia <torikoshia@oss.nttdata.com> wrote:
>
> Thanks for working on this topic, I appreciate it!

Thank you for taking a look!

> BTW, do these patches enable non-superusers to create user with
> bypassrls?

No, I did not break out the ability to create such users.

> Since I failed to apply the patches and didn't test them,
> I may have overlooked something but I didn't find the
> corresponding codes.

Do you believe that functionality should be added?  I have not thought much about that issue.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-06-14 23:53, Mark Dilger wrote:
>> On Jun 14, 2021, at 5:51 AM, torikoshia <torikoshia@oss.nttdata.com> 
>> wrote:
>> 
>> Thanks for working on this topic, I appreciate it!
> 
> Thank you for taking a look!
> 
>> BTW, do these patches enable non-superusers to create user with
>> bypassrls?
> 
> No, I did not break out the ability to create such users.
> 
>> Since I failed to apply the patches and didn't test them,
>> I may have overlooked something but I didn't find the
>> corresponding codes.
> 
> Do you believe that functionality should be added?  I have not thought
> much about that issue.

I just noticed that because I was looking into operations that can only 
be done by superusers.

It might be somewhat inconvenient in PostgreSQL service providers that 
don't give users superuser privileges, but at least I don't have a 
specific demand for it.


Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

Please find attached a new set of patches.

> On May 27, 2021, at 11:06 PM, Noah Misch <noah@leadboat.com> wrote:
>
> pg_logical_replication would not be safe to delegate that way:
> https://postgr.es/m/flat/CACqFVBbx6PDq%2B%3DvHM0n78kHzn8tvOM-kGO_2q_q0zNAMT%2BTzdA%40mail.gmail.com

v3-0001 creates a pg_logical_replication role and respects privileges on tables in the table sync and apply workers.
Withthis change, by creating a user in role pg_logical_replication, only giving that user INSERT, UPDATE, DELETE, or
TRUNCATEprivileges as appropriate on the intended tables, and having that user rather than a superuser create a
subscription,one may prevent the replication of unwanted DML on these tables as well as the replication of any DML to
anyother tables. 

> On Jun 14, 2021, at 5:51 AM, torikoshia <torikoshia@oss.nttdata.com> wrote:
>
> BTW, do these patches enable non-superusers to create user with
> bypassrls?

v3-0004 creates a pg_database_security role and allows users in this role to create roles with BYPASSRLS.




—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Jun 29, 2021, at 6:25 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> Please find attached a new set of patches.

And again, this time attaching a fifth patch which includes the work to allow users who belong to the right security
roleto SET and ALTER SYSTEM SET variables without being a superuser. 


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> 1 июля 2021 г., в 20:59, Mark Dilger <mark.dilger@enterprisedb.com> написал(а):
>
>
>
>> On Jun 29, 2021, at 6:25 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>>
>> Please find attached a new set of patches.
>
> And again, this time attaching a fifth patch which includes the work to allow users who belong to the right security
roleto SET and ALTER SYSTEM SET variables without being a superuser. 

I'm not sure, but maybe we should allow replication role to change session_replication_role?

Best regards, Andrey Borodin.

> On Jul 5, 2021, at 1:50 AM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> I'm not sure, but maybe we should allow replication role to change session_replication_role?

Thanks, Andrey, for taking a look.

Yes, there is certainly some logic to that suggestion.  The patch v4-0005 only delegates authority to perform ALTER
SYSTEMSET to three roles:  pg_database_security, pg_network_security, and pg_host_security.  I don't mind expanding
thislist to include the replication attribute, but I am curious about opinions on the general design.  There may be an
advantagein keeping the list short.  In particular, as the list gets longer, will it get harder to decide which role to
associatewith each new GUC that gets added?  For third-party extensions, will it be harder for them to decide in any
principledway which role to assign to each GUC that they add?  There are multiple ways to cut up the set of all GUCs.
database/host/networkis not an entirely clean distinction, and perhaps database/host/network/replication is better, but
I'muncertain.  

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 1, 2021 at 9:42 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>
>
> > On Jun 29, 2021, at 6:25 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> >
> > Please find attached a new set of patches.
>
> And again, this time attaching a fifth patch which includes the work to allow users who belong to the right security
roleto SET and ALTER SYSTEM SET variables without being a superuser.
 

One of the patches
v4-0004-Add-default-role-for-database-operations.patch does not apply
on head, please post an updated patch:
patching file src/backend/commands/dbcommands.c
Hunk #1 succeeded at 748 (offset -15 lines).
Hunk #2 FAILED at 780.
Hunk #3 succeeded at 1883 (offset -42 lines).
1 out of 3 hunks FAILED -- saving rejects to file
src/backend/commands/dbcommands.c.rej

Regards,
Vignesh

On Fri, May 28, 2021 at 1:42 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > pg_logical_replication would not be safe to delegate that way:
> > https://postgr.es/m/flat/CACqFVBbx6PDq%2B%3DvHM0n78kHzn8tvOM-kGO_2q_q0zNAMT%2BTzdA%40mail.gmail.com
>
> Oh, I agree that this patch set does not go the extra step to make it safe.  You are quite right to push back, as my
emailwas poorly worded.  I should have said "intended to be eventually made safe to delegate". The idea is that the
patchset addresses most places in the sources where we test for superuser and tests instead for (superuser ||
<SOME_ROLE>),and then uses that same set of roles to control who has sufficient privileges to set GUCs.  The
pg_host_securityand pg_network_security roles are not intended to eventually be safe to delegate.  Or at least, I can't
seeany clear path to getting there.  The pg_database_security and pg_logical_replication roles should be ones we can
makesafe.  If we can agree as a community which set of roles are appropriate, then we can have separate patches as
neededfor tightening the security around them. 

I don't think that we want to commit a patch to add a
pg_logical_replication role that can "eventually" be made staff to
delegate to non-superusers. Whatever issues need to be fixed should be
fixed first, and then this change can be considered afterwards. It
seems like you try to fix at least some of the issues in the patch,
because I see permission checks being added in
src/backend/replication/logical/worker.c, and I don't think that
should happen in the same patch that adds the new predefined role. I
also think it should be accompanied not only by new test cases (which
you seem to have added, though I have not reviewed them in detail) but
also documentation changes (which seem to be missing, since the doc
changes are all about the new predefined role). This is a really
significant behavior change to logical replication IMV and shouldn't
just be slipped into some other patch.

It also seems based on Noah's comments and your response that there
might be some other issue here, and I haven't understood what that is,
but I think that should also be fixed separately, and first.
Considering all this, I would suggest not having this be patch #1 in
your series; make something come first that doesn't have
prerequisites.

--
Robert Haas
EDB: http://www.enterprisedb.com

> On Jul 22, 2021, at 8:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> I don't think that we want to commit a patch to add a
> pg_logical_replication role that can "eventually" be made staff to
> delegate to non-superusers.

Certainly not.  What I meant on May 28 by "eventually" was that the patch set (posted May 25 and named "v3") had not
yetimplemented such security, as I was fishing for comments from the community about whether the basic division of
superuserinto these new roles was the right division.  Having gotten little feedback on that, on June 29 I posted
anotherpatch set (confusingly also named "v3", my apologies) in which patch 0001 had expanded to include new security
restrictions.

> Whatever issues need to be fixed should be
> fixed first, and then this change can be considered afterwards. It
> seems like you try to fix at least some of the issues in the patch,
> because I see permission checks being added in
> src/backend/replication/logical/worker.c, and I don't think that
> should happen in the same patch that adds the new predefined role.

Prior to this patch, the logical replication workers run under the userid of the owner of the subscription.  This is
unchangedafter the patch.  The real difference is that prior to the patch, only superusers can own subscriptions, so
checkingpermissions on tables during replication would be silly (though not harmful).  The worker is assured of passing
allsuch permission checks by virtue of being a superuser.  After the patch, since subscription owners need not be
superusers,the permission checks are no longer silly.  There is no assurance that they have permission to apply changes
toa table, so naturally that has to be checked, and it is.  

I don't really see this as two separate patches, since the addition of the permissions checks without the addition of
non-superusersas logical replication workers is silly.  But I don't mind that much, either.  I'll break them in two for
thenext patch set. 

> I
> also think it should be accompanied not only by new test cases (which
> you seem to have added, though I have not reviewed them in detail) but
> also documentation changes (which seem to be missing, since the doc
> changes are all about the new predefined role). This is a really
> significant behavior change to logical replication IMV and shouldn't
> just be slipped into some other patch.

I'm not sure what is meant by "slipped into some other patch", but I *think* you mean that the documentation changes
shouldnot be in a separate patch from the behavioral changes.  I agree with that.  I'll add documentation of the
changesto logical replication in the same patch as the changes themselves. 

> It also seems based on Noah's comments and your response that there
> might be some other issue here, and I haven't understood what that is,
> but I think that should also be fixed separately, and first.
> Considering all this, I would suggest not having this be patch #1 in
> your series; make something come first that doesn't have
> prerequisites.

The issue that gets thrown around in the email archive is that "arbitrary code" can be made to run on the subscriber
side. As I understand the problem, this is because trigger functions can be created on tables with arbitrary code in
them,and that code will be executed under the userid of the user who causes the trigger to fire during an
insert/update/deleterather than as the user who created the trigger.  This of course is not peculiar to logical
replication;it is how triggers work generally.  What is peculiar is that a non-superuser who can create tables,
triggers,publications and subscriptions can get the logical replication worker to perform inserts/updates/deletes on
thosetables, thereby firing those triggers, and executing the trigger code as superuser.  That is ordinarily not
somethingthat a user can do simply by creating a table with a trigger, since there would be no mechanism to force the
superuserto perform operations on the table. 

After patch 0001 (which will be split in the next patch set, but hasn't been split yet) the user who creates the
subscriptionis also the user whose permissions are checked when operating on the table and executing the trigger.  This
closesthe security hole, so far as I am aware.  I would very much like more eyeballs on this patch, and if anybody sees
whythis is an insufficient solution, please speak up.  But it's not as if I punted the security issue down the road to
someill-defined future patch.  On the contrary, this patch both creates the ability to delegate subscription creation
authorityto a non-superuser and closes the security hole which that would otherwise entail, or at least, that is the
intent.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 1, 2021 at 11:59 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote
> > On Jun 29, 2021, at 6:25 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> > Please find attached a new set of patches.
>
> And again, this time attaching a fifth patch which includes the work to allow users who belong to the right security
roleto SET and ALTER SYSTEM SET variables without being a superuser.
 

In general, I don't like this kind of coding:

-       /* Superusers bypass all permission checking. */
-       if (superuser_arg(roleid))
+       /*
+        * Superusers and members of the pg_host_security role bypass all
+        * permission checking.
+        */
+       if (superuser_arg(roleid) ||
+               has_privs_of_role(roleid, ROLE_PG_HOST_SECURITY))
                return true;

From a theoretical point of view, the superuser has the privileges of
every role, so this is redundant. From a coding point of view,
has_privs_of_role() does a superuser_arg() test already, and it seems
likely to be a loser to do it twice. Arguably, we should take the same
approach in code comments and documentation, rather than saying "You
must be superuser or a member of role XYZ" everywhere, but there seems
to be some existing precedent for mentioning superuser explicitly in
those cases, so maybe it's fine. I think it's kind of weird though,
because in other places we don't do it, e.g.:

      unique or primary key constraint in the referenced table.  The user
      must have <literal>REFERENCES</literal> permission on the referenced table
      (either the whole table, or the specific referenced columns).  The

We could have said "or be superuser" there, but we didn't. It doesn't
seem efficient to say "or be superuser" every time we mention a
required permission, rather than just taking it as a given that the
superuser has all permissions. Yet, again, there's some precedent for
your approach:

   To create a database, you must be a superuser or have the special
   <literal>CREATEDB</literal> privilege.
   See <xref linkend="sql-createrole"/>.

So I don't know. At the very least I think we should not do it as an
"or" in the code; what we want to do in comments and documentation I'm
less sure.

I think 0002 needs more explanation of the theory behind the specific
permissions granted. It enumerates what they are in both the commit
message and the documentation, but no theory is offered explaining why
these permissions are included and not others. I think my idea was
that "host" security would encompass everything that touches the
filesystem on the server where the database is running. I agree that
this naturally includes the ability to create a tablespace and
probably, at least for symmetry, the ability to drop it. But, you
can't ALTER a tablespace's location, so I see no reason why that
should be tied to this permission. I think it's arguable whether it
includes creating and dropping extensions, but I would argue that it
shouldn't. True, the extensions include SQL files installed in the
filesystem, and shared libraries also installed on the filesystem, but
ultimately everything you ever do involves files in some way, so I
don't see that as a very compelling argument. These operations on
extensions don't let you modify the filesystem in any way, and they
only let you read from carefully sandboxed things that are designed
for precisely that purpose, so the system administrator really already
has good control. The sorts of things I'd include in this category are
things like server-side COPY FROM or COPY TO. When we come to the
third thing the patch includes in this category, creating and dropping
event triggers, I *really* don't understand why that one is considered
host security. That one isn't touching the filesystem even to the
extent that the extension stuff is; it seems to me to be purely
internal to the database. Yeah, OK, that could involve writing files
because we make catalog entries, but so could any DDL. Now, maybe
there's a theory of operation that you have in mind that makes this
all make more sense the way you have it, but if so, it seems not to be
spelled out anywhere in the patch itself or the commit message you
wrote for it, so I'm in the dark.

I also tend to think that functions like pg_read_file() ought to come
with execute permission pre-granted, with grant option, to
pg_host_security, and perhaps similarly for adminpack.

From the department of nitpicking, all four of your commit messages
begin with the word "Reducing" which I think should be just "Reduce".
Otherwise, at least to me, it doesn't look like a proper sentence.

0004 has this kind of thing all over the place:

-       /* Superusers bypass all permission checking. */
-       if (superuser_arg(roleid))
+       /*
+        * Superusers and members of the pg_database_security role bypass all
+        * permission checking.
+        */

If that were true, the pg_database_security role would be equivalent
to superuser, which isn't the intent, so I think the comment needs
more thought. Also, I think that somewhere in the patch, either in
code comments or at the very least in the commit message, there needs
to be some justification of why the approach taken here is correct.
Like, the idea here is that if you have pg_database_security, you can
do whatever you want to objects within the database as long as you
don't try to touch the network or the host filesystem. So that would
imply that you can do anything you like to databases. So it sorta
makes sense to me that the patch goes about that by changing
pg_database_aclmask(). But I would feel better if there were some
explanation somewhere of why such a change is expecting to allow
precisely DDL-related database commands and nothing else. I think
that's true if pg_database_aclmask() is used for that purpose and not
for any other purpose, which may well be true, but I think it would be
best to be more explicit about the assumptions. I'm sure we don't want
a lengthy comment about this in every pg_*_aclmask() function, but I
think we should have a general explanation of it somewhere.

I also think that in this case, as in 0002 and 0003, we really need
some documentation of what this new role is all about. The
documentation changes in 0004 are really extremely minimal. Users need
to understand what they can expect to happen if they grant this new
role to someone, and hackers need to understand how to update the code
the next time they're patching something that interacts with this, and
I do not think that what you've got here now is going to be sufficient
to meet the needs of either group. (I realize that you may have been
planning to wait until there was more consensus to flesh this out, but
because the definitional issues here are so tricky, I don't think it
can wait in this case.)

In 0005, I do not think the function name role_has_privileges() is
sufficiently specific. Maybe role_can_change_guc()? Also, I think here
again you should draft some documentation changes. We're going to need
to indicate a category for every GUC somehow, and I'm not quite sure
how we're going to do that. If you want to just do a few examples for
now and also provide some general documentation on how the system is
intended to work, we can wait to do every GUC until we settle on how
to categorize everything, but I think we need to see what the general
plan looks like there. Consider the way that we currently indicate (a)
the GUC's data type and (b) when the GUC can be changed. The former is
shown in fixed point type in parentheses after the GUC name. The
latter is indicated by adding a sentence such as "This parameter can
only be set at server start." to everything that is PGC_POSTMASTER (I
think). What are we going to do with this new categorization?

I see that you've categorized things like restart_after_crash and
zero_damaged_pages as GUC_HOST_SECURITY. I think I like that, but it
again begs the definitional question. If host security basically means
touching the server filesystem, well, restart_after_crash doesn't. It
can be justified from the perspective that restart_after_crash is a
property of the host system, not something strictly internal to the
database. So when you go to write definitions of what these categories
are actually supposed to mean, they've got to be written in such a way
that these categorizations end up looking correct. Or else these have
got to be recategorized somehow. Anyway the point is that it "feels
good" but as you have it without the definitions it's hard to really
know.

The categorization of the logging GUCs looks haphazard to me. Why is
log_duration GUC_HOST_SECURITY but debug_print_parse is
GUC_DATABASE_SECURITY, for example? Again, we need clear definitions,
but I'm inclined to think this doesn't look great. I even less
understand why autovacuum is classified as GUC_HOST_SECURITY. That
seems like it's probably database security, while db_user_namespace
feels to me like network security. Another oddity is the replication
settings, which seem to be mostly classified as GUC_HOST_SECURITY. I
can see why you don't want to make them GUC_DATABASE_SECURITY, but eh,
what do they have to do with host security? It's similarly odd to me
that hash_mem_multiplier is GUC_DATABASE_SECURITY while work_mem, for
which it is a multiplier, is GUC_HOST_SECURITY.

Perhaps we need to break this up into a few more buckets to make sense
of it; I'm not really sure. For example, we could add buckets for
controlling what goes to the server log, resource utilization, system
integrity, and split inbound and outbound network security. Well, now
I just turned your three predefined roles into seven, which maybe is a
bad idea, but perhaps it's worth it if it gets us to a place where we
can clearly categorize everything. On the other hand, maybe if we did
that there'd just be a new set of things that look a little ambiguous.
I don't know. I guess trying to write a good set of definitions might
be job one.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Jul 22, 2021 at 1:29 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> Certainly not.  What I meant on May 28 by "eventually" was that the patch set (posted May 25 and named "v3") had not
yetimplemented such security, as I was fishing for comments from the community about whether the basic division of
superuserinto these new roles was the right division.  Having gotten little feedback on that, on June 29 I posted
anotherpatch set (confusingly also named "v3", my apologies) in which patch 0001 had expanded to include new security
restrictions.

Oh.

> Prior to this patch, the logical replication workers run under the userid of the owner of the subscription.  This is
unchangedafter the patch.  The real difference is that prior to the patch, only superusers can own subscriptions, so
checkingpermissions on tables during replication would be silly (though not harmful).  The worker is assured of passing
allsuch permission checks by virtue of being a superuser.  After the patch, since subscription owners need not be
superusers,the permission checks are no longer silly.  There is no assurance that they have permission to apply changes
toa table, so naturally that has to be checked, and it is. 

Aren't you supposing that the set of superusers never changes? Unless
we have some code for this that we don't have elsewhere, a superuser
could create a subscription and then be de-superuser'd, or the
subscription's owner could be altered.

> > I
> > also think it should be accompanied not only by new test cases (which
> > you seem to have added, though I have not reviewed them in detail) but
> > also documentation changes (which seem to be missing, since the doc
> > changes are all about the new predefined role). This is a really
> > significant behavior change to logical replication IMV and shouldn't
> > just be slipped into some other patch.
>
> I'm not sure what is meant by "slipped into some other patch", but I *think* you mean that the documentation changes
shouldnot be in a separate patch from the behavioral changes.  I agree with that.  I'll add documentation of the
changesto logical replication in the same patch as the changes themselves. 

I just meant that I think the behavioral change to logical replication
is significant in its own right and should be a separate patch.
Perhaps it's not as significant as I thought, but I still think it
should be made separately and likely documented as an incompatibility
with previous releases, unless I'm still confused.

> > It also seems based on Noah's comments and your response that there
> > might be some other issue here, and I haven't understood what that is,
> > but I think that should also be fixed separately, and first.
> > Considering all this, I would suggest not having this be patch #1 in
> > your series; make something come first that doesn't have
> > prerequisites.
>
> The issue that gets thrown around in the email archive is that "arbitrary code" can be made to run on the subscriber
side. As I understand the problem, this is because trigger functions can be created on tables with arbitrary code in
them,and that code will be executed under the userid of the user who causes the trigger to fire during an
insert/update/deleterather than as the user who created the trigger.  This of course is not peculiar to logical
replication;it is how triggers work generally.  What is peculiar is that a non-superuser who can create tables,
triggers,publications and subscriptions can get the logical replication worker to perform inserts/updates/deletes on
thosetables, thereby firing those triggers, and executing the trigger code as superuser.  That is ordinarily not
somethingthat a user can do simply by creating a table with a trigger, since there would be no mechanism to force the
superuserto perform operations on the table. 
>
> After patch 0001 (which will be split in the next patch set, but hasn't been split yet) the user who creates the
subscriptionis also the user whose permissions are checked when operating on the table and executing the trigger.  This
closesthe security hole, so far as I am aware.  I would very much like more eyeballs on this patch, and if anybody sees
whythis is an insufficient solution, please speak up.  But it's not as if I punted the security issue down the road to
someill-defined future patch.  On the contrary, this patch both creates the ability to delegate subscription creation
authorityto a non-superuser and closes the security hole which that would otherwise entail, or at least, that is the
intent.

OK. I thought Noah must be talking about some other problem, because
on May 28th you wrote "Oh, I agree that this patch set does not go the
extra step to make it safe" and I failed to understand that you
thought you'd addressed this in v4.

--
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Thu, Jul 1, 2021 at 11:59 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote
> > > On Jun 29, 2021, at 6:25 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> > > Please find attached a new set of patches.
> >
> > And again, this time attaching a fifth patch which includes the work to allow users who belong to the right
securityrole to SET and ALTER SYSTEM SET variables without being a superuser. 
>
> In general, I don't like this kind of coding:
>
> -       /* Superusers bypass all permission checking. */
> -       if (superuser_arg(roleid))
> +       /*
> +        * Superusers and members of the pg_host_security role bypass all
> +        * permission checking.
> +        */
> +       if (superuser_arg(roleid) ||
> +               has_privs_of_role(roleid, ROLE_PG_HOST_SECURITY))
>                 return true;
>
> >From a theoretical point of view, the superuser has the privileges of
> every role, so this is redundant. From a coding point of view,
> has_privs_of_role() does a superuser_arg() test already, and it seems
> likely to be a loser to do it twice. Arguably, we should take the same
> approach in code comments and documentation, rather than saying "You
> must be superuser or a member of role XYZ" everywhere, but there seems
> to be some existing precedent for mentioning superuser explicitly in
> those cases, so maybe it's fine. I think it's kind of weird though,
> because in other places we don't do it, e.g.:
>
>       unique or primary key constraint in the referenced table.  The user
>       must have <literal>REFERENCES</literal> permission on the referenced table
>       (either the whole table, or the specific referenced columns).  The

I tend to agree that it'd be better to clean this up and just use
has_privs_of_role() and not include explicit superuser checks.  I don't
think we need to constantly re-remind ourselves in the code that
superusers are members of all roles.

> We could have said "or be superuser" there, but we didn't. It doesn't
> seem efficient to say "or be superuser" every time we mention a
> required permission, rather than just taking it as a given that the
> superuser has all permissions. Yet, again, there's some precedent for
> your approach:
>
>    To create a database, you must be a superuser or have the special
>    <literal>CREATEDB</literal> privilege.
>    See <xref linkend="sql-createrole"/>.

I'm a bit on the fence about the documentation side...  I could be
convinced either way, really, but I generally agree that it'd be good to
pick one and be consistent.  I don't think the places where we do/don't
mention it were done for any particular reason.

> So I don't know. At the very least I think we should not do it as an
> "or" in the code; what we want to do in comments and documentation I'm
> less sure.

Agreed.

> I think 0002 needs more explanation of the theory behind the specific
> permissions granted. It enumerates what they are in both the commit
> message and the documentation, but no theory is offered explaining why
> these permissions are included and not others. I think my idea was
> that "host" security would encompass everything that touches the
> filesystem on the server where the database is running. I agree that
> this naturally includes the ability to create a tablespace and
> probably, at least for symmetry, the ability to drop it. But, you
> can't ALTER a tablespace's location, so I see no reason why that
> should be tied to this permission. I think it's arguable whether it
> includes creating and dropping extensions, but I would argue that it
> shouldn't. True, the extensions include SQL files installed in the
> filesystem, and shared libraries also installed on the filesystem, but
> ultimately everything you ever do involves files in some way, so I
> don't see that as a very compelling argument. These operations on
> extensions don't let you modify the filesystem in any way, and they
> only let you read from carefully sandboxed things that are designed
> for precisely that purpose, so the system administrator really already
> has good control. The sorts of things I'd include in this category are
> things like server-side COPY FROM or COPY TO. When we come to the
> third thing the patch includes in this category, creating and dropping
> event triggers, I *really* don't understand why that one is considered
> host security. That one isn't touching the filesystem even to the
> extent that the extension stuff is; it seems to me to be purely
> internal to the database. Yeah, OK, that could involve writing files
> because we make catalog entries, but so could any DDL. Now, maybe
> there's a theory of operation that you have in mind that makes this
> all make more sense the way you have it, but if so, it seems not to be
> spelled out anywhere in the patch itself or the commit message you
> wrote for it, so I'm in the dark.

I agree that installing extensions and event triggers are different
things, and if what we're left with is "create tablespaces" then maybe
we should have that as an explicit permission on its own.

> I also tend to think that functions like pg_read_file() ought to come
> with execute permission pre-granted, with grant option, to
> pg_host_security, and perhaps similarly for adminpack.

When it comes to these- we already have pg_read_server_files and
pg_write_server_files, so I'm not sure I see why it'd make sense to have
another thing that grants filesystem access like this..?

> 0004 has this kind of thing all over the place:
>
> -       /* Superusers bypass all permission checking. */
> -       if (superuser_arg(roleid))
> +       /*
> +        * Superusers and members of the pg_database_security role bypass all
> +        * permission checking.
> +        */
>
> If that were true, the pg_database_security role would be equivalent
> to superuser, which isn't the intent, so I think the comment needs
> more thought. Also, I think that somewhere in the patch, either in
> code comments or at the very least in the commit message, there needs
> to be some justification of why the approach taken here is correct.
> Like, the idea here is that if you have pg_database_security, you can
> do whatever you want to objects within the database as long as you
> don't try to touch the network or the host filesystem. So that would
> imply that you can do anything you like to databases. So it sorta
> makes sense to me that the patch goes about that by changing
> pg_database_aclmask(). But I would feel better if there were some
> explanation somewhere of why such a change is expecting to allow
> precisely DDL-related database commands and nothing else. I think
> that's true if pg_database_aclmask() is used for that purpose and not
> for any other purpose, which may well be true, but I think it would be
> best to be more explicit about the assumptions. I'm sure we don't want
> a lengthy comment about this in every pg_*_aclmask() function, but I
> think we should have a general explanation of it somewhere.

Considering it's a role, and roles aren't specific to databases, I don't
think "pg_database_security" is a good name.  To me, at least, it
implies a way to allow a given user the ability to do most everything in
a specific database, but that's not the case at all, and further saying
'security' comes across as suggesting that this would be a role granted
to someone setting up permissions or otherwise modifying the database's
security aspects, not someone who is being given full access to every
object in the system.

I'm also left wondering if this doesn't end up introducing opportunities
for someone with this role to become superuser pretty easily.  Maybe it
does and maybe we're ok with that, but I would think that it'd be really
useful to have a role that can't become superuser easily which can
access/modify most objects in the system.

> I also think that in this case, as in 0002 and 0003, we really need
> some documentation of what this new role is all about. The
> documentation changes in 0004 are really extremely minimal. Users need
> to understand what they can expect to happen if they grant this new
> role to someone, and hackers need to understand how to update the code
> the next time they're patching something that interacts with this, and
> I do not think that what you've got here now is going to be sufficient
> to meet the needs of either group. (I realize that you may have been
> planning to wait until there was more consensus to flesh this out, but
> because the definitional issues here are so tricky, I don't think it
> can wait in this case.)

Agreed.

> In 0005, I do not think the function name role_has_privileges() is
> sufficiently specific. Maybe role_can_change_guc()? Also, I think here
> again you should draft some documentation changes. We're going to need
> to indicate a category for every GUC somehow, and I'm not quite sure
> how we're going to do that. If you want to just do a few examples for
> now and also provide some general documentation on how the system is
> intended to work, we can wait to do every GUC until we settle on how
> to categorize everything, but I think we need to see what the general
> plan looks like there. Consider the way that we currently indicate (a)
> the GUC's data type and (b) when the GUC can be changed. The former is
> shown in fixed point type in parentheses after the GUC name. The
> latter is indicated by adding a sentence such as "This parameter can
> only be set at server start." to everything that is PGC_POSTMASTER (I
> think). What are we going to do with this new categorization?

Agreed.

> I see that you've categorized things like restart_after_crash and
> zero_damaged_pages as GUC_HOST_SECURITY. I think I like that, but it
> again begs the definitional question. If host security basically means
> touching the server filesystem, well, restart_after_crash doesn't. It
> can be justified from the perspective that restart_after_crash is a
> property of the host system, not something strictly internal to the
> database. So when you go to write definitions of what these categories
> are actually supposed to mean, they've got to be written in such a way
> that these categorizations end up looking correct. Or else these have
> got to be recategorized somehow. Anyway the point is that it "feels
> good" but as you have it without the definitions it's hard to really
> know.

I don't really see either of those as being filesystem changing things.

> The categorization of the logging GUCs looks haphazard to me. Why is
> log_duration GUC_HOST_SECURITY but debug_print_parse is
> GUC_DATABASE_SECURITY, for example? Again, we need clear definitions,
> but I'm inclined to think this doesn't look great. I even less
> understand why autovacuum is classified as GUC_HOST_SECURITY. That
> seems like it's probably database security, while db_user_namespace
> feels to me like network security. Another oddity is the replication
> settings, which seem to be mostly classified as GUC_HOST_SECURITY. I
> can see why you don't want to make them GUC_DATABASE_SECURITY, but eh,
> what do they have to do with host security? It's similarly odd to me
> that hash_mem_multiplier is GUC_DATABASE_SECURITY while work_mem, for
> which it is a multiplier, is GUC_HOST_SECURITY.

> Perhaps we need to break this up into a few more buckets to make sense
> of it; I'm not really sure. For example, we could add buckets for
> controlling what goes to the server log, resource utilization, system
> integrity, and split inbound and outbound network security. Well, now
> I just turned your three predefined roles into seven, which maybe is a
> bad idea, but perhaps it's worth it if it gets us to a place where we
> can clearly categorize everything. On the other hand, maybe if we did
> that there'd just be a new set of things that look a little ambiguous.
> I don't know. I guess trying to write a good set of definitions might
> be job one.

It's often the case that logging/auditing are handled by a different
group than those who might be creating/modifying objects.  Yet another
group is often the group that actually handles granting access.  Broad
classes being:

- Users
- Auditors (controls what's logged, what is audited, etc)
- Security (controls who has access to what)

Note that 'security' and 'auditors' shouldn't have access to the actual
data either, or have the ability to do things like modify data.  Not
sure all of this quite fits what we're going for here but figured it
might help with sorting out what other buckets we need.

Thanks,

Stephen

On Thu, Jul 22, 2021 at 3:00 PM Stephen Frost <sfrost@snowman.net> wrote:
> I'm a bit on the fence about the documentation side...  I could be
> convinced either way, really, but I generally agree that it'd be good to
> pick one and be consistent.  I don't think the places where we do/don't
> mention it were done for any particular reason.
>
> > So I don't know. At the very least I think we should not do it as an
> > "or" in the code; what we want to do in comments and documentation I'm
> > less sure.
>
> Agreed.

Thanks for weighing in.

> > I also tend to think that functions like pg_read_file() ought to come
> > with execute permission pre-granted, with grant option, to
> > pg_host_security, and perhaps similarly for adminpack.
>
> When it comes to these- we already have pg_read_server_files and
> pg_write_server_files, so I'm not sure I see why it'd make sense to have
> another thing that grants filesystem access like this..?

It's awkward. I think that we can't afford to create a separate
predefined role for every single thing that someone could
theoretically want to sever, because then we'll have a zillion of them
and it will be unmaintainable. So the idea was to try to break up
everything someone might want to do either via DDL or by setting GUCs
into one of three categories: internal to the database
(pg_database_security), facing outward toward the network
(pg_network_security), and facing inward toward the host
(pg_host_security). If we didn't have any predefined security-related
roles already, this would still have complications, but as things
stand it has more, because as you point out, pg_read_server_files
overlaps with pg_host_security. But what do we do about that? Neither
pg_read_server_files nor pg_write_server_files covers the ability to
create tablespaces or set log_directory, but I think it's more
desirable to lump those things together in one bucket than to have a
ton of separate buckets for each individual thing. I guess one option
would to grant the existing roles pg_read_server_files and
pg_write_server_files to the new pg_host_security role, or whatever we
decide to call it (pg_access_server_filesystem?
pg_administer_server_files? pg_hack_postgres_account?). But I'm open
to suggestions. See also below here on the overall intent.

> I'm also left wondering if this doesn't end up introducing opportunities
> for someone with this role to become superuser pretty easily.  Maybe it
> does and maybe we're ok with that, but I would think that it'd be really
> useful to have a role that can't become superuser easily which can
> access/modify most objects in the system.

Creating something like that is precisely the intention here because,
like you, I think that would be extremely handy. If it's possible for
that role to become superuser, we've lost the plot.

> I don't really see either of those as being filesystem changing things.

I think the thought process here was that if you are a managed
services provider you would not want the user to change
zero_damaged_pages or wal_sync_method or things like that because that
stuff is the provider's responsibility; similar for the recovery
settings. But yes ... we need something better here, I think.

> It's often the case that logging/auditing are handled by a different
> group than those who might be creating/modifying objects.  Yet another
> group is often the group that actually handles granting access.  Broad
> classes being:
>
> - Users
> - Auditors (controls what's logged, what is audited, etc)
> - Security (controls who has access to what)
>
> Note that 'security' and 'auditors' shouldn't have access to the actual
> data either, or have the ability to do things like modify data.  Not
> sure all of this quite fits what we're going for here but figured it
> might help with sorting out what other buckets we need.

Hmm, interesting point. The division between the "security" group, who
I suppose would be the DBA, and the "auditors" group is one I had
thought about only slightly.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Jul 22, 2021, at 11:21 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> When we come to the
> third thing the patch includes in this category, creating and dropping
> event triggers, I *really* don't understand why that one is considered
> host security. That one isn't touching the filesystem even to the
> extent that the extension stuff is; it seems to me to be purely
> internal to the database. Yeah, OK, that could involve writing files
> because we make catalog entries, but so could any DDL. Now, maybe
> there's a theory of operation that you have in mind that makes this
> all make more sense the way you have it, but if so, it seems not to be
> spelled out anywhere in the patch itself or the commit message you
> wrote for it, so I'm in the dark.

I agree with the need to document the theory behind all this.  Event triggers are dangerous because they can trap a
superuserinto executing code they do not intend: 

  create table super_special (big_darn_secret integer);
  revoke all privileges on super_special from public;
  insert into super_special values (42);
  -- imagine that "untrustworth_bob" is a member of as-yet unimplemented role
  -- pg_database_security, and has the ability to create event triggers; to simulate
  -- that, we'll put bob into superuser temporarily while the event trigger is
  -- created, then remove superuser.
  create role untrustworthy_bob superuser;
  set session authorization untrustworthy_bob;
  create function update_super_special_big_darn_secret() returns event_trigger as $$
  begin
    -- note that non-superusers should draw an error if they try this
    update super_special set big_darn_secret = big_darn_secret + 1;

    -- note that non-pg_host_security roles should draw an error if they try this
    perform pg_rotate_logfile();
  end;
  $$ language plpgsql;
  create event trigger secret_sauce on sql_drop
    execute procedure update_super_special_big_darn_secret();
  reset session authorization;
  alter role untrustworthy_bob nosuperuser;
  set session authorization untrustworthy_bob;
  update super_special set big_darn_secret = big_darn_secret + 1;
  ERROR:  permission denied for table super_special
  select pg_rotate_logfile();
  ERROR:  permission denied for function pg_rotate_logfile
  reset session authorization;
  select * from super_special;
   big_darn_secret
  -----------------
                42
  (1 row)

  create table foo_tmp (t integer);
  drop table foo_tmp;
  WARNING:  rotation not possible because log collection not active
  select * from super_special;
   big_darn_secret
  -----------------
                43
  (1 row)

When the superuser dropped table foo_tmp, pg_rotate_logfile() got called, as did an update of table super_special.  Any
otherfunction could have been called from there instead.  That's a big deal.  If creating event triggers is delegated
tononsuperuser members of pg_database_security, I think it means that pg_database_security has a privilege escalation
pathto become superuser.  Since pg_host_security already has such an escalation path, it makes more sense to me to use
thisrole for event trigger creation.  The argument for dropping event triggers is less clear, but it seems that event
triggersmay be used to implement an auditing system, and we wouldn't want pg_database_security to be enough privilege
tocircumvent the auditing system. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 22, 2021 at 5:35 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> I agree with the need to document the theory behind all this.  Event triggers are dangerous because they can trap a
superuserinto executing code they do not intend:
 

That's true. Regular triggers and views can also do that, and so can
operators and functions that capture queries intended to reference
system-defined objects. It's already the case that a superuser who
executes any optimizable query potentially compromises the system
unless (a) they first sanitize their search_path and (b) the query is
a SELECT that involves no views or an INSERT, UPDATE, or DELETE on a
table that has no triggers. However, event triggers owned by
non-superusers would extend the hazard to nearly all DDL commands.

Classifying event triggers as "host" security doesn't seem like the
right solution, though. I think the only way that fits into the "host"
security category is if you use some kind of definition that works by
exclusion: things that are "database" security shouldn't have
consequence X, therefore anything that does must go in some other
category. I think that kind of definition is very hard for people to
understand, though. It's much nicer to have definitions that tell you
what does go into a category than what doesn't.

I suppose one alternative we could consider is just leaving some
things uncategorized. I had hoped we could put everything in a bucket
so that the superuser role was really divided into pieces and not just
having bits chipped off of it, but maybe that's too ambitious. The
more we drift into saying that some things like "well this has to be
host security because database security breaks the model" the more
useless host security is likely to be as a concept. It's not a related
collection of things any more; it's just whatever didn't fit in the
other bucket. And nobody wants to GRANT a_bunch_of_other_stuff.

However, I also wonder whether we should think about engineering a
solution to this problem. For example, we have a row_security GUC. If
you set it to off, no RLS policies will be applied; if applying one
would have been mandatory, you will get an error instead. I don't
think that exact design will work here because there's no such thing
as permission to bypass event triggers, but we could have a
fire_event_triggers GUC (or whatever we call it) which is normally
"on" but privileged users can turn it off. Now if you're worried about
this problem, you have an easy way around it.

And I think that's a good illustration of why it's a bad idea to
categorize things according to whether or not they have a certain
consequence. Suppose we said, ah well, let's make event triggers
"host" security because it's too dangerous to make them "database"
security. Well then say someone comes along and implements the feature
I just described, reducing the danger. Do we then reclassify that
feature as "database" security? The original rationale for making it
something else is no longer valid, but on the other hand, what about
backward compatibility? Classifying things based on what they do,
rather than on the ultimate consequences that they may have, avoids
this kind of conundrum.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Jul 23, 2021, at 6:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> And I think that's a good illustration of why it's a bad idea to
> categorize things according to whether or not they have a certain
> consequence.

Well, one very big reason for wanting to break superuser into separate roles is to make postgres-as-a-service providers
comfortablegranting more privileges to customers.  If we design any privilege escalations into one of those roles, then
nosensible service provider is ever going to grant it to anybody, which fairly much defeats the purpose of this work.
Theprivilege escalations we need to prevent are not just escalations to superuser, but also escalations to other
privilegedroles.  Contrary to this design goal, the "pg_host_security" role is a bit of a synonym for "superuser",
sincebeing able to write files or execute shell commands is a path to superuser, and we can't do too much about that.
"pg_database_security","pg_network_security", and "pg_logical_replication" are not synonyms for "superuser". 

I like your idea of designing some extra security around event triggers to resolve their privilege escalation problems.
A GUC seems the wrong approach to me. 

I think a superuser-only GUC to suppress firing event triggers won't quite cut it, because the other privileged roles
wouldstill be in danger of being trapped by a clever pg_database_security event trigger author; but extending
permissionson the GUC to include the other roles would mean that they, and not just superuser, could evade event
triggerbased auditing solutions.  That is odd, because you wouldn't expect granting pg_network_security or
pg_logical_replicationto have anything to do with granting privilege to defeat audit logging. 

A superuser-only GUC for this is also a bit too heavy handed.  The superuser may not want to circumvent all event
triggers,just those put in place by the pg_database_security role.  If that sounds arbitrary, just consider the
postgres-as-a-servicecase.  The superuser wants to be able to grant pg_database_security to the customer, but doesn't
wantthe customer to be able to use that to trap the service provider. 

Instead of a GUC, how about checking permissions inside event triggers for both the user firing the trigger *and* the
triggerowner.  That's a backward compatibility break, but maybe not a bad one, since until now only superusers have
beenallowed to create event triggers.  Systems which create an event trigger using a role that later has superuser
revoked,or which change ownership to a non-superuser, will see a behavior change.  I'm not super happy with that, but I
thinkit is better than the GUC based solution.  Event triggers owned by a superuser continue to work as they do now.
Eventtriggers owned by a non-superuser cannot be used to force a privileged user to run a command that the event
triggerowner could not have run for themself. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Jul 23, 2021, at 6:22 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > And I think that's a good illustration of why it's a bad idea to
> > categorize things according to whether or not they have a certain
> > consequence.
>
> Well, one very big reason for wanting to break superuser into separate roles is to make postgres-as-a-service
providerscomfortable granting more privileges to customers.  If we design any privilege escalations into one of those
roles,then no sensible service provider is ever going to grant it to anybody, which fairly much defeats the purpose of
thiswork.  The privilege escalations we need to prevent are not just escalations to superuser, but also escalations to
otherprivileged roles.  Contrary to this design goal, the "pg_host_security" role is a bit of a synonym for
"superuser",since being able to write files or execute shell commands is a path to superuser, and we can't do too much
aboutthat.   "pg_database_security", "pg_network_security", and "pg_logical_replication" are not synonyms for
"superuser".
>
> I like your idea of designing some extra security around event triggers to resolve their privilege escalation
problems. A GUC seems the wrong approach to me. 
>
> I think a superuser-only GUC to suppress firing event triggers won't quite cut it, because the other privileged roles
wouldstill be in danger of being trapped by a clever pg_database_security event trigger author; but extending
permissionson the GUC to include the other roles would mean that they, and not just superuser, could evade event
triggerbased auditing solutions.  That is odd, because you wouldn't expect granting pg_network_security or
pg_logical_replicationto have anything to do with granting privilege to defeat audit logging. 

These considerations were addressed with row_security by allowing the
GUC to be set by anyone, but throwing an ERROR if RLS would have been
required by the query instead of just allowing it.  I don't see any
obvious reason why that couldn't be the case for event triggers..?

> A superuser-only GUC for this is also a bit too heavy handed.  The superuser may not want to circumvent all event
triggers,just those put in place by the pg_database_security role.  If that sounds arbitrary, just consider the
postgres-as-a-servicecase.  The superuser wants to be able to grant pg_database_security to the customer, but doesn't
wantthe customer to be able to use that to trap the service provider. 

Having a trust system for triggers, functions, etc, where you can say
whose triggers you're ok running might be interesting but it also seems
like an awful lot of work and I'm not sure that it's actually really
that much better than a GUC similar to row_security.

> Instead of a GUC, how about checking permissions inside event triggers for both the user firing the trigger *and* the
triggerowner.  That's a backward compatibility break, but maybe not a bad one, since until now only superusers have
beenallowed to create event triggers.  Systems which create an event trigger using a role that later has superuser
revoked,or which change ownership to a non-superuser, will see a behavior change.  I'm not super happy with that, but I
thinkit is better than the GUC based solution.  Event triggers owned by a superuser continue to work as they do now.
Eventtriggers owned by a non-superuser cannot be used to force a privileged user to run a command that the event
triggerowner could not have run for themself. 

I'm not following what this suggestion is, exactly.  What permissions
are being checked inside the event trigger being run, exactly..?  Who
would get to set those permissions?  Any object owner, today, can GRANT
access to any other user in the system, we don't prevent that.

Thanks,

Stephen

> On Jul 23, 2021, at 9:20 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> These considerations were addressed with row_security by allowing the
> GUC to be set by anyone, but throwing an ERROR if RLS would have been
> required by the query instead of just allowing it.  I don't see any
> obvious reason why that couldn't be the case for event triggers..?

Because a postgres-as-a-service provider may want to install their own event triggers as well as allowing the customer
todo so, and it seems too coarse grained to either skip all of them or none of them.  It's perfectly reasonable to want
toskip your customer's event triggers while not skipping your own. 

>> A superuser-only GUC for this is also a bit too heavy handed.  The superuser may not want to circumvent all event
triggers,just those put in place by the pg_database_security role.  If that sounds arbitrary, just consider the
postgres-as-a-servicecase.  The superuser wants to be able to grant pg_database_security to the customer, but doesn't
wantthe customer to be able to use that to trap the service provider. 
>
> Having a trust system for triggers, functions, etc, where you can say
> whose triggers you're ok running might be interesting but it also seems
> like an awful lot of work and I'm not sure that it's actually really
> that much better than a GUC similar to row_security.

My first impression was that it is too much work, which is why I put event trigger creation into the pg_host_security
bucket. It might be more sane to just leave it as superuser-only.  But if we're going to fix this and make it a
pg_database_securityusable feature, then I think we need to solve the problems a naive approach would create for
serviceproviders. 

>> Instead of a GUC, how about checking permissions inside event triggers for both the user firing the trigger *and*
thetrigger owner.  That's a backward compatibility break, but maybe not a bad one, since until now only superusers have
beenallowed to create event triggers.  Systems which create an event trigger using a role that later has superuser
revoked,or which change ownership to a non-superuser, will see a behavior change.  I'm not super happy with that, but I
thinkit is better than the GUC based solution.  Event triggers owned by a superuser continue to work as they do now.
Eventtriggers owned by a non-superuser cannot be used to force a privileged user to run a command that the event
triggerowner could not have run for themself. 
>
> I'm not following what this suggestion is, exactly.  What permissions
> are being checked inside the event trigger being run, exactly..?  Who
> would get to set those permissions?  Any object owner, today, can GRANT
> access to any other user in the system, we don't prevent that.

I don't think GRANT is really relevant here, as what I'm trying to avoid is a less privileged user trapping a more
privilegeduser into running a function that the less privileged user can't directly run.  Certainly such a user cannot
GRANTprivilege on a function that they cannot even run, else your system has a privilege escalation hazard already.  

It's a substantial change to the security model, but the idea is that inside an event trigger, we'd
SetUserIdAndSecContextto a new type of context similar to SECURITY_LOCAL_USERID_CHANGE but where instead of simply
changingto the owner of the event trigger, we'd be changing to a virtual user who is defined to only have the
privilegesof the intersection of the current user and the event trigger owner.  That entails at least two problems,
thoughI don't see that they are insoluble.  First, all places in the code that check permissions need to check in a way
thatworks in this mode.  We might not be able to call GetUserId() as part of aclchecks any longer, and instead have to
callsome new function GetVirtualUserId() as part of aclchecks, reserving GetUserId() just for cases where you're not
tryingto perform a permissions check.  Second, since event triggers can cause other event triggers to fire, we'd need
thesevirtual users to be able to nest, so we'd have to push a stack of users and check all of them in each case, and
we'dhave to think carefully about how to handle errors, since GetUserIdAndSecContext and SetUserIdAndSecContext are
calledinside transaction start and end, where errors must not be raised.  But since transaction start and end would
neverwant to set or reset the state to a virtual user, those should never throw, and the calls elsewhere are at liberty
tothrow if they like, so we'd just have to be careful to use the right version of these operations in the right places. 

This all sounds a bit much, but it has knock-on benefits.  We'd be able to preserve the historical behavior of table
triggerswhile having a mode that behaves in this new way, which means that privileged users could be a bit more
cavalierthan they can now about DML against user defined tables.  This mode might become the standard operating mode
forservice provider scripts, whether running as superuser, as pg_network_security, or whatever.  This might also be
usedto secure operations on indexes over user defined functions.  If the index operation is run in this mode, the index
operationcould throw an error rather than performing a function maliciously embedded inside the index function call. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul 23, 2021 at 12:11 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> A superuser-only GUC for this is also a bit too heavy handed.

Yeah, but you're inventing a system for allowing the restriction on a
GUC to be something other than is-superuser in the very patch we're
talking about. So it could be something like is-database-security.
Therefore I don't grok the objection.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Jul 23, 2021, at 1:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> Yeah, but you're inventing a system for allowing the restriction on a
> GUC to be something other than is-superuser in the very patch we're
> talking about. So it could be something like is-database-security.
> Therefore I don't grok the objection.

I'm not objecting to how hard it would be to implement.  I'm objecting to the semantics.  If the only non-superuser who
canset the GUC is pg_database_security, then it is absolutely worthless in preventing pg_database_security from
trappingactions performed by pg_network_security members.  On the other hand, if pg_network_security can also set the
GUC,then pg_network_security can circumvent audit logging that pg_database_security put in place.  What's the point in
havingthese as separate roles if they can circumvent each other's authority? 


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Jul 23, 2021, at 1:57 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> What's the point in having these as separate roles if they can circumvent each other's authority?

That was probably too brief a reply, so let me try again.  If the GUC circumvents the event trigger, then my answer
abovestands.  If the GUC merely converts the event trigger into an error, then you have the problem that the customer
cancreate event triggers which the service provider will need to disable (because they cause the service providers
legitimateactions to error rather than succeed).  Presumably the service provider can disable them logged in as
superuser. But that means the service customer has their event trigger turned off, at least for some length of time,
whichis not good if the event trigger is performing audit logging for compliance purposes, etc.  Also, we can't say
whetherpg_network_security role has been given to the customer, or if that is being kept for the provider's use only,
sowe're not really sure whether pg_network_security should be able to do these sorts of things, but in the case that
theservice provider is keeping pg_network_security for themself, it seems they wouldn't want the customer to cause
pg_network_securityoperations to fail.  We can't make too many assumptions about the exact relationship between those
tworoles. 



—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Jul 23, 2021, at 2:04 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> If the GUC merely converts the event trigger into an error, then you have the problem that the customer can create
eventtriggers which the service provider will need to disable (because they cause the service providers legitimate
actionsto error rather than succeed). 

I'd like to expound on this a little more.

Imagine the service provider has scripts that perform actions within the database, such as physical replication, or the
creationand removal of database users in response to actions taken at the service portal web interface, and they don't
wantthe actions performed by those scripts to be leveraged by the customer to break out of the jail. 

The customer has event triggers which perform no illicit activities.  They don't try to break out of the jail.  But for
compliancewith HIPAA regulations (or whatever), they need to audit log everything, and they can't just have the service
provider'sactions unlogged. 

What to do?  If the service provider disables the event triggers, then the customer will fail their regulation audit.
Ifthe service provider allows the event triggers to fire, the customer might create a new event trigger embedding
illicitactions.  The service provider is totally stuck. 

OTOH, if there were a mechanism by which an event trigger could run with only the intersection of the privileges
enjoyedby the service provider's scripts and the customer's event trigger owner, then the service provider can allow
theirown actions to be logged, without fear that any hijacking of their privilege will occur. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul 23, 2021 at 4:57 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > On Jul 23, 2021, at 1:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > Yeah, but you're inventing a system for allowing the restriction on a
> > GUC to be something other than is-superuser in the very patch we're
> > talking about. So it could be something like is-database-security.
> > Therefore I don't grok the objection.
>
> I'm not objecting to how hard it would be to implement.  I'm objecting to the semantics.  If the only non-superuser
whocan set the GUC is pg_database_security, then it is absolutely worthless in preventing pg_database_security from
trappingactions performed by pg_network_security members.  On the other hand, if pg_network_security can also set the
GUC,then pg_network_security can circumvent audit logging that pg_database_security put in place.  What's the point in
havingthese as separate roles if they can circumvent each other's authority? 

Right, that would be bad. I had forgotten how this worked, but it
seems that event triggers are called with the privileges of the user
whose action caused the event trigger to be fired, not the privileges
of the user who owns the trigger. So as you say, if you can get
somebody to do something that causes an event trigger to be fired, you
can do anything they can do. As far as I can see, the only reasonable
conclusion is that, unless we change the security model, doing
anything with event triggers will have to remain superuser-only. In
other words I don't think we can give it to any of
pg_database_security or pg_host_security or pg_network_security, or
any similar role. We could have a pg_event_triggers role that is
documented as able to usurp superuser, but I don't see the point.

Now, the other alternative is changing the security model for event
triggers, but I am not sure that really fixes anything. You proposed
having a new mode where someone could only do things that could be
done by either user, but that troubles me for a number of reasons. One
is that it often makes a difference who actually did a particular
operation. For example it might be that alice and bob both have the
ability to give charlie permission on some table, but the ACL for that
table will record who actually issued the grant. It might be that both
alice and bob have the ability to create a table, but the table will
be owned by whoever actually does. Suppose bob is about to be
terminated but can arrange for alice (who is a star performer) to
grant permissions to his accomplice charlie, thus arranging for those
permissions to survive his impending termination. That's bad.

Also, what about just throwing an ERROR? Anybody's allowed to do that,
but that doesn't mean that it's OK for one user to block everything
some other user wants to do. If seward and bates respectively have
pg_database_security and pg_network_security, it's OK for seward to
interfere with attempts by bates to access database objects, but it's
not OK for seward to prevent bates from reconfiguring network access
to PostgreSQL. Because event triggers don't fire for ALTER SYSTEM or
DDL commands on global objects, we might almost be OK here, but I'm
not sure if it's completely OK.

I'm pretty sure that the reason we set this up the way we did was
because we assumed that the person creating the event trigger would
always have maximum privileges i.e. superuser. Therefore, it seemed
"safer" to run the code under the less-privileged account. If we'd
thought about this from the perspective of having non-superuser-owned
event triggers, I think we would have made the opposite decision,
since running code as yourself in somebody else's session is less
dangerous than running code as somebody else straight up.

--
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,


* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jul 23, 2021 at 4:57 PM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
> > > On Jul 23, 2021, at 1:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > > Yeah, but you're inventing a system for allowing the restriction on a
> > > GUC to be something other than is-superuser in the very patch we're
> > > talking about. So it could be something like is-database-security.
> > > Therefore I don't grok the objection.
> >
> > I'm not objecting to how hard it would be to implement.  I'm objecting to the semantics.  If the only non-superuser
whocan set the GUC is pg_database_security, then it is absolutely worthless in preventing pg_database_security from
trappingactions performed by pg_network_security members.  On the other hand, if pg_network_security can also set the
GUC,then pg_network_security can circumvent audit logging that pg_database_security put in place.  What's the point in
havingthese as separate roles if they can circumvent each other's authority? 
>
> Right, that would be bad. I had forgotten how this worked, but it
> seems that event triggers are called with the privileges of the user
> whose action caused the event trigger to be fired, not the privileges
> of the user who owns the trigger. So as you say, if you can get
> somebody to do something that causes an event trigger to be fired, you
> can do anything they can do. As far as I can see, the only reasonable
> conclusion is that, unless we change the security model, doing
> anything with event triggers will have to remain superuser-only. In
> other words I don't think we can give it to any of
> pg_database_security or pg_host_security or pg_network_security, or
> any similar role. We could have a pg_event_triggers role that is
> documented as able to usurp superuser, but I don't see the point.

Right- event triggers work just the same as how regular triggers on
tables do and how RLS works.  All of these also have the possibility of
leveraging security definer functions, of course, but that doesn't
address the issue of the trigger author attempting to attack the
individual running the trigger.

I do think it'd be useful to have a pg_event_triggers or such role, so
that someone could create them without being a superuser.  A bit more
discussion about that below though..

> Now, the other alternative is changing the security model for event
> triggers, but I am not sure that really fixes anything. You proposed
> having a new mode where someone could only do things that could be
> done by either user, but that troubles me for a number of reasons. One
> is that it often makes a difference who actually did a particular
> operation. For example it might be that alice and bob both have the
> ability to give charlie permission on some table, but the ACL for that
> table will record who actually issued the grant. It might be that both
> alice and bob have the ability to create a table, but the table will
> be owned by whoever actually does. Suppose bob is about to be
> terminated but can arrange for alice (who is a star performer) to
> grant permissions to his accomplice charlie, thus arranging for those
> permissions to survive his impending termination. That's bad.

As I understood Mark's suggestion, the trigger would run but would have
the privileges of the intersection of both user's permissions, which is
an interesting idea but not one we've got any way to really do today as
each privilege check would now need to check two different roles for
privilege- and if one of the privilege checks fails, then what..?
Presumably there would be an ERROR returned, meaning that the operation
would be able to be prevented from happening by the trigger author,
which was objected to as not being acceptable either, per below.

> Also, what about just throwing an ERROR? Anybody's allowed to do that,
> but that doesn't mean that it's OK for one user to block everything
> some other user wants to do. If seward and bates respectively have
> pg_database_security and pg_network_security, it's OK for seward to
> interfere with attempts by bates to access database objects, but it's
> not OK for seward to prevent bates from reconfiguring network access
> to PostgreSQL. Because event triggers don't fire for ALTER SYSTEM or
> DDL commands on global objects, we might almost be OK here, but I'm
> not sure if it's completely OK.

Regular table triggers can be used to block someone from messing with
that table, so this isn't entirely unheard of.  Deciding that someone
with event trigger access is allowed to prevent certain things from
happening in a database that they're allowed to connect and create event
triggers in may not be completely unreasonable.

> I'm pretty sure that the reason we set this up the way we did was
> because we assumed that the person creating the event trigger would
> always have maximum privileges i.e. superuser. Therefore, it seemed
> "safer" to run the code under the less-privileged account. If we'd
> thought about this from the perspective of having non-superuser-owned
> event triggers, I think we would have made the opposite decision,
> since running code as yourself in somebody else's session is less
> dangerous than running code as somebody else straight up.

Not sure that this is really the case- as noted above, it's the same for
table-level triggers and RLS.

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Jul 23, 2021, at 2:04 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> >
> > If the GUC merely converts the event trigger into an error, then you have the problem that the customer can create
eventtriggers which the service provider will need to disable (because they cause the service providers legitimate
actionsto error rather than succeed). 
>
> I'd like to expound on this a little more.
>
> Imagine the service provider has scripts that perform actions within the database, such as physical replication, or
thecreation and removal of database users in response to actions taken at the service portal web interface, and they
don'twant the actions performed by those scripts to be leveraged by the customer to break out of the jail. 

These specific use-cases are interesting because they seem to all be
about global-level objects, right?  Surely the service provider would be
better off having a separate database that they connect to, which no one
else has access to.  Not to mention that event triggers can't be created
on global objects anyway, but even so.

On the flip side- if we create a way for roles to be created by a
non-superuser in a way that doesn't end up giving away the farm (unlike
CREATEROLE privs today), then suddenly the service provider doesn't have
any need to use a superuser role or otherwise privileged role to perform
that action and they could either punt that entirely to the client to
deal with, or have a way to log in *as* the client to perform the
action while not risking the client ending up getting the service
provider's system to run code that the client wrote.

And this last point is the most relevant to all of this, in my view.
Everything that service providers provide web forms and such for to
allow the client to perform things that they can't just GRANT directly
to the client's account is the problem- they don't write those web pages
and run things as superuser because they want to, they do it because the
database system doesn't allow them any way to do it that doesn't give up
the farm (or, perhaps in some cases, they have to do *other* things too,
that clearly they can't just give the client's account access to do,
like creating tablespaces where the volume has to also be created and
attached to the instance, but those are cases where we probably don't
need to come up with a better solution..?  though I'm not against a role
to allow creating tablespaces that isn't a superuser, since the service
provider would probably be happier running that action with that role as
a good way to reduce risk further).

> The customer has event triggers which perform no illicit activities.  They don't try to break out of the jail.  But
forcompliance with HIPAA regulations (or whatever), they need to audit log everything, and they can't just have the
serviceprovider's actions unlogged. 
>
> What to do?  If the service provider disables the event triggers, then the customer will fail their regulation audit.
If the service provider allows the event triggers to fire, the customer might create a new event trigger embedding
illicitactions.  The service provider is totally stuck. 
>
> OTOH, if there were a mechanism by which an event trigger could run with only the intersection of the privileges
enjoyedby the service provider's scripts and the customer's event trigger owner, then the service provider can allow
theirown actions to be logged, without fear that any hijacking of their privilege will occur. 

If all such actions could be performed by the client role, then the
service provider suddenly doesn't have such a concern or issue- they can
tell the client to do whatever it is and then it gets logged properly.
In my view, that's really the end goal here, we just need to build these
things in a way that granting such privileges to the client doesn't end
up giving them a way to get superuser privileges.

Thanks,

Stephen

On Mon, Jul 26, 2021 at 4:05 PM Stephen Frost <sfrost@snowman.net> wrote:
> As I understood Mark's suggestion, the trigger would run but would have
> the privileges of the intersection of both user's permissions, which is
> an interesting idea but not one we've got any way to really do today as
> each privilege check would now need to check two different roles for
> privilege- and if one of the privilege checks fails, then what..?
> Presumably there would be an ERROR returned, meaning that the operation
> would be able to be prevented from happening by the trigger author,
> which was objected to as not being acceptable either, per below.

I think I may not have expressed myself clearly enough here. What I'm
concerned about is: Alice should not be permitted to preventing Bob
from doing something which Bob is allowed to do and Alice is not
allowed to do. If Alice is the administrator of PostgreSQL's XYZ
subsystem, she can permit Bob from using it if she wishes. But if Bob
is an administrator of XYZ and Alice is not, there shouldn't be a way
for Alice to obstruct Bob's access to that system.

Do you agree?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Jul 26, 2021 at 4:12 PM Robert Haas <robertmhaas@gmail.com> wrote:
> I think I may not have expressed myself clearly enough here. What I'm
> concerned about is: Alice should not be permitted to preventing Bob
> from doing something which Bob is allowed to do and Alice is not
> allowed to do. If Alice is the administrator of PostgreSQL's XYZ
> subsystem, she can permit Bob from using it if she wishes. But if Bob

argh, typo. I meant prevent, not permit.

> is an administrator of XYZ and Alice is not, there shouldn't be a way
> for Alice to obstruct Bob's access to that system.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Stephen Frost <sfrost@snowman.net> writes:
> As I understood Mark's suggestion, the trigger would run but would have
> the privileges of the intersection of both user's permissions, which is
> an interesting idea but not one we've got any way to really do today as
> each privilege check would now need to check two different roles for
> privilege- and if one of the privilege checks fails, then what..?
> Presumably there would be an ERROR returned, meaning that the operation
> would be able to be prevented from happening by the trigger author,
> which was objected to as not being acceptable either, per below.

I've not been paying close attention, so maybe this was already
considered, but ...

What if we allow event triggers owned by non-superusers, but only fire
them on commands performed by the trigger's owner?  This sidesteps all
the issues of who has which privileges and whether Alice is malicious
towards Bob or vice versa, because there is no change of privilege
domain.  Admittedly, it fails to cover some use-cases, but I think it
would still handle a lot of interesting cases.  The impression I have
is that a lot of applications do everything under just one or a few
roles.

Possibly this could be generalized to "fire on commands performed by
any role the trigger owner is a member of", but then I'm a bit less
sure that it's safe from both roles' perspectives.

            regards, tom lane

> On Jul 26, 2021, at 1:12 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> Alice should not be permitted to preventing Bob
> from doing something which Bob is allowed to do and Alice is not
> allowed to do.

That sounds intuitively reasonable, though it depends on what "which Bob is allowed to do" means.  For instance, if
Aliceis only allowed to enable or disable connections to the database, and she disables them, then she has prevented
Bobfrom, for example, creating tables, something which Bob is otherwise allowed to do, because without the ability to
connect,he cannot create tables. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Jul 26, 2021 at 4:12 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > I think I may not have expressed myself clearly enough here. What I'm
> > concerned about is: Alice should not be permitted to preventing Bob
> > from doing something which Bob is allowed to do and Alice is not
> > allowed to do. If Alice is the administrator of PostgreSQL's XYZ
> > subsystem, she can permit Bob from using it if she wishes. But if Bob
>
> argh, typo. I meant prevent, not permit.
>
> > is an administrator of XYZ and Alice is not, there shouldn't be a way
> > for Alice to obstruct Bob's access to that system.

> Do you agree?

so ... yes and no.  There's an awful lot being ascribed to
'administrator' without any definition of it being actually given.  We
are working in this thread to explicitly split up superuser privileges
to allow them to be granted to non-superusers and talking about cases
where those privileges end up interacting with each other.  Is Alice, as
the 'network' manager considered an 'administrator' of XYZ?  Is Bob, as
the 'database' manager considered an 'administrator'?  Perhaps both are,
perhaps neither are.  It doesn't seem helpful to be vague.

If Alice is given the right to create event triggers in a given
database, then that's explicitly giving Alice the right to block anyone
from dropping tables in that database because that's an inherent part of
the event trigger system.  Should superusers be able to bypass that?
Yes, they probably should be able to and, ideally, they'd be able to do
that just in a particular session.  Should a user who has been allowed
to modify certain GUCs that perhaps Alice hasn't been allowed to modify
be able to be prevented from modifying those GUCs by Alice, when neither
is a superuser?  That's definitely a trickier question and I don't know
that I've got an answer offhand.

Thanks,

Stephen

On 2021-Jul-26, Tom Lane wrote:

> What if we allow event triggers owned by non-superusers, but only fire
> them on commands performed by the trigger's owner?  This sidesteps all
> the issues of who has which privileges and whether Alice is malicious
> towards Bob or vice versa, because there is no change of privilege
> domain.  Admittedly, it fails to cover some use-cases, but I think it
> would still handle a lot of interesting cases.  The impression I have
> is that a lot of applications do everything under just one or a few
> roles.

This is similar but not quite an idea I had: have event triggers owned
by non-superusers run for all non-superusers, but not for superusers.
It is still the case that all non-superusers have to trust everyone with
the event-trigger-create permission, but that's probably the database
owner so most of the time you have to trust them already.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)

Greetings,

* Alvaro Herrera (alvherre@alvh.no-ip.org) wrote:
> On 2021-Jul-26, Tom Lane wrote:
>
> > What if we allow event triggers owned by non-superusers, but only fire
> > them on commands performed by the trigger's owner?  This sidesteps all
> > the issues of who has which privileges and whether Alice is malicious
> > towards Bob or vice versa, because there is no change of privilege
> > domain.  Admittedly, it fails to cover some use-cases, but I think it
> > would still handle a lot of interesting cases.  The impression I have
> > is that a lot of applications do everything under just one or a few
> > roles.
>
> This is similar but not quite an idea I had: have event triggers owned
> by non-superusers run for all non-superusers, but not for superusers.
> It is still the case that all non-superusers have to trust everyone with
> the event-trigger-create permission, but that's probably the database
> owner so most of the time you have to trust them already.

This sort of logic is what has caused issues with CREATEROLE, imv.  It's
simply not so simple as "don't run this when the superuser flag is set"
because non-superuser roles can become superusers.  We need something
better to have something like this actually be safe.  Tom's suggestion
would work, of course, but it would mean having to create event triggers
for all the roles in the system, and would those roles who own those
event triggers be able to disable them..?  If so, it would almost
certainly be against the point of an auditing event trigger..

Thanks,

Stephen

I wrote:
> Possibly this could be generalized to "fire on commands performed by
> any role the trigger owner is a member of", but then I'm a bit less
> sure that it's safe from both roles' perspectives.

After further thought, I can't poke a hole in that concept.
We'd keep the rule that the trigger executes as the calling user.
Therefore, the trigger cannot perform any action that the calling
user couldn't do if she chose.  Conversely, since the trigger
owner could become a member of that role and then do whatever the
trigger intends to do, this scheme does not give the trigger owner
any new abilities either.  All we've done is provide what some
programming languages call an observer or annotation.

I also like the fact that with this rule, superusers' ability to
create event triggers that fire for everything is not a special case.

            regards, tom lane

Stephen Frost <sfrost@snowman.net> writes:
> ... Tom's suggestion
> would work, of course, but it would mean having to create event triggers
> for all the roles in the system, and would those roles who own those
> event triggers be able to disable them..?

Uh, why not?  If you own the trigger, you can drop it, so why shouldn't
you be able to temporarily disable it?

> If so, it would almost
> certainly be against the point of an auditing event trigger..

If you want auditing capability, you make an auditor role that is
a member of every other role, and then it owns the trigger.  (If
you need to audit superuser actions too, then the auditor has to
be a superuser itself, but that's no worse than before; and I'd
argue that non-superusers shouldn't be able to audit superusers
anyway.)

            regards, tom lane

On 2021-Jul-26, Tom Lane wrote:

> Stephen Frost <sfrost@snowman.net> writes:
> > ... Tom's suggestion
> > would work, of course, but it would mean having to create event triggers
> > for all the roles in the system, and would those roles who own those
> > event triggers be able to disable them..?
> 
> Uh, why not?  If you own the trigger, you can drop it, so why shouldn't
> you be able to temporarily disable it?

I think an auditing system that can be turned off by the audited user is
pretty much useless.  Or did I misunderstood what you are suggesting?

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Crear es tan difícil como ser libre" (Elsa Triolet)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Jul-26, Tom Lane wrote:
>> Uh, why not?  If you own the trigger, you can drop it, so why shouldn't
>> you be able to temporarily disable it?

> I think an auditing system that can be turned off by the audited user is
> pretty much useless.  Or did I misunderstood what you are suggesting?

For auditing purposes, you make a trusted role that owns the trigger,
and is a member of the roles whose actions are to be audited (but NOT
vice versa).  I think that any idea that the auditing role doesn't
need to be trusted that much is foolhardy.  What we can buy here is
not requiring the auditing role to be full superuser ... assuming that
you don't need auditing of superusers.

            regards, tom lane

On Mon, Jul 26, 2021 at 4:28 PM Stephen Frost <sfrost@snowman.net> wrote:
> so ... yes and no.  There's an awful lot being ascribed to
> 'administrator' without any definition of it being actually given.  We
> are working in this thread to explicitly split up superuser privileges
> to allow them to be granted to non-superusers and talking about cases
> where those privileges end up interacting with each other.  Is Alice, as
> the 'network' manager considered an 'administrator' of XYZ?  Is Bob, as
> the 'database' manager considered an 'administrator'?  Perhaps both are,
> perhaps neither are.  It doesn't seem helpful to be vague.

XYZ was intended to stand in for something like 'network' or
'database' or whatever other particular part of PostgreSQL Alice might
be charged with administering.

> If Alice is given the right to create event triggers in a given
> database, then that's explicitly giving Alice the right to block anyone
> from dropping tables in that database because that's an inherent part of
> the event trigger system.  Should superusers be able to bypass that?
> Yes, they probably should be able to and, ideally, they'd be able to do
> that just in a particular session.

I agree.

> Should a user who has been allowed
> to modify certain GUCs that perhaps Alice hasn't been allowed to modify
> be able to be prevented from modifying those GUCs by Alice, when neither
> is a superuser?  That's definitely a trickier question and I don't know
> that I've got an answer offhand.

My answer would be "no".

I concede Mark's point in another email that if Alice can entirely
prevent Bob from connecting to the database then by inference she can
also prevent him from exercising any other privileges he may have. I'm
prepared to say that's OK; if Alice is administering network
connections to the database and cuts everyone else off, then I guess
that's just how it is. But if Bob does somehow succeed in getting a
connection to the database, then he should be able to exercise his
right to change those GUCs which he has permission to change. Alice
shouldn't be able to thwart that.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Jul 26, 2021 at 4:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> After further thought, I can't poke a hole in that concept.
> We'd keep the rule that the trigger executes as the calling user.
> Therefore, the trigger cannot perform any action that the calling
> user couldn't do if she chose.  Conversely, since the trigger
> owner could become a member of that role and then do whatever the
> trigger intends to do, this scheme does not give the trigger owner
> any new abilities either.  All we've done is provide what some
> programming languages call an observer or annotation.
>
> I also like the fact that with this rule, superusers' ability to
> create event triggers that fire for everything is not a special case.

I think this has potential. In a managed services environment, you can
imagine the provider as the super-duper user, having the ability to do
anything - because they control the box, so there's really no stopping
it - but presumably very little interest in what happens within the
database. Then you have the tenant, who is a semi-super-user,
authorized by the provider to do anything internal to the database
that the provider doesn't think will cause them problems. With the
setup you're proposing here, I suppose what the provider needs to do
is have a role like 'tenant' and make all the other tenant role
members of that master role. Then the tenant can log in as 'tenant' as
set up event triggers that will apply to all of those users, but
there's no security compromise for the provider because the role (or
roles) that they use to log in are not members of 'tenant'.

I thought for a while there might be a problem with tenant users
creating event triggers and then altering the owner to 'tenant' but I
think now that was backwards thinking. 'tenant' is a member of all of
the tenant users but not the other way around, so they can't give
their event triggers away to 'tenant'.

Do I have that right?

I agree with you that it's really nice that this eliminates the
special case for superusers.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Jul 27, 2021 at 10:24 AM Isaac Morland <isaac.morland@gmail.com> wrote:
> Isn’t this backwards? If all those roles are members of "tenant" then they can do anything "tenant" can do. The
reversemight work - make "tenant" a member of all the related roles - although I haven’t thought through in detail. 

Dang it, yes. The tenant needs to be members of all the other users,
not the other way around. I spent a long time trying to not get that
backwards and still did.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> Dang it, yes. The tenant needs to be members of all the other users,
> not the other way around. I spent a long time trying to not get that
> backwards and still did.

The "membership" terminology is inherently confusing I fear.
Maybe better to say that all the roles-to-be-audited must
be GRANTed to the "tenant" role?

            regards, tom lane

> On Jul 22, 2021, at 1:01 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> It's awkward. I think that we can't afford to create a separate
> predefined role for every single thing that someone could
> theoretically want to sever, because then we'll have a zillion of them
> and it will be unmaintainable. So the idea was to try to break up
> everything someone might want to do either via DDL or by setting GUCs
> into one of three categories: internal to the database
> (pg_database_security), facing outward toward the network
> (pg_network_security), and facing inward toward the host
> (pg_host_security). If we didn't have any predefined security-related
> roles already, this would still have complications, but as things
> stand it has more, because as you point out, pg_read_server_files
> overlaps with pg_host_security. But what do we do about that?

I gave up on the idea of splitting all superuser functions into three roles.

Patch v5-0001 refactors the guc code to allow non-superuser roles to be associated with guc variables.  Any such role
canthen set the variable, including via "alter system set".  The patch stops short of creating any new roles or
assigningany roles to any guc variable. 

Patches v5-0002 through v5-0005 create four new roles for managing host resource settings, vacuum settings, autovacuum
settings,and logging settings.  That last one excludes "where to log" settings, because we don't want the role to be
ableto write to arbitrary locations on the server.  Remaining guc variables not in these four categories continue to
belongto the superuser. 

Patches v5-0006 and v5-0007 allow non-superusers to own event triggers, and limit the event triggers to only running
forevents triggered by roles that the event trigger owner belongs to.  This is backward compatible, because event
triggershave historically belonged only to superusers, and superusers have implicit membership in all groups. 

Patches v5-0008 through v5-0010 allow non-superusers to own subscriptions while restricting the tablesync and apply
workersto only work on tables that the subscription owner has permissions on.  This is almost backward compatible,
becausesubscriptions have historically belonged only to superusers, as above, except for unlikely scenarios where
superusershave given ownership to non-superusers.  In those cases, the new code will refuse to apply in situations
wherethe old code would blindly apply changes.  Does anybody see a problem with this? 

Patch v5-0011 is a bug fix posted elsewhere that hasn't been committed yet but which must be committed in preparation
forv5-0012. 

Patch v5-0012 creates a new role, pg_manage_database_objects, which can do anything with an object that the owner could
dowith it, as long as the owner is not a superuser.  This role is intended as a "tenant" role, and is in some sense a
lesspowerful replacement for the pg_database_security role previously proposed. 

I doubt that I will create any replacement for the pg_host_security role previously proposed, as I think that role is
justsynonymous with "superuser", so it serves no purpose. 

I am uncertain about creating a role similar to the pg_network_security role previously proposed, as the changes to how
publicationsand subscriptions work in patches v5-0008 through v5-0010 may be enough.  In any event, I'd like feedback
onthose patches before designing one or more additional roles for this. 


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Jul 22, 2021, at 1:01 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > It's awkward. I think that we can't afford to create a separate
> > predefined role for every single thing that someone could
> > theoretically want to sever, because then we'll have a zillion of them
> > and it will be unmaintainable. So the idea was to try to break up
> > everything someone might want to do either via DDL or by setting GUCs
> > into one of three categories: internal to the database
> > (pg_database_security), facing outward toward the network
> > (pg_network_security), and facing inward toward the host
> > (pg_host_security). If we didn't have any predefined security-related
> > roles already, this would still have complications, but as things
> > stand it has more, because as you point out, pg_read_server_files
> > overlaps with pg_host_security. But what do we do about that?
>
> I gave up on the idea of splitting all superuser functions into three roles.

I can't say that I blame you for that. :)  For my 2c, at least, the ones
proposed never really felt like they were very directly tied to specific
capabilities, which I think was one of the issues with that approach.

> Patch v5-0001 refactors the guc code to allow non-superuser roles to be associated with guc variables.  Any such role
canthen set the variable, including via "alter system set".  The patch stops short of creating any new roles or
assigningany roles to any guc variable. 

Haven't looked at the patch yet but this does generally seem like an
interesting approach.

> Patches v5-0002 through v5-0005 create four new roles for managing host resource settings, vacuum settings,
autovacuumsettings, and logging settings.  That last one excludes "where to log" settings, because we don't want the
roleto be able to write to arbitrary locations on the server.  Remaining guc variables not in these four categories
continueto belong to the superuser. 

We do have a role today who is allowed to write to arbitrary locations
on the server, so I wonder if for those log settings we'd include a
requirement for the user to have both of those roles instead..?

> Patches v5-0006 and v5-0007 allow non-superusers to own event triggers, and limit the event triggers to only running
forevents triggered by roles that the event trigger owner belongs to.  This is backward compatible, because event
triggershave historically belonged only to superusers, and superusers have implicit membership in all groups. 

While I generally agree that this doesn't end up opening up security
issues, it's going to certainly be a change in how event triggers work
as they'll no longer *always* fire, and that seems quite at odds with
how triggers are generally thought of.  So much so that I worry about
mis-use due to this.  Then again, if we're going to go down this route
at all, I can't think of any particular way to avoid the security issues
of running the trigger for everyone when it's owned by a non-superuser.

> Patches v5-0008 through v5-0010 allow non-superusers to own subscriptions while restricting the tablesync and apply
workersto only work on tables that the subscription owner has permissions on.  This is almost backward compatible,
becausesubscriptions have historically belonged only to superusers, as above, except for unlikely scenarios where
superusershave given ownership to non-superusers.  In those cases, the new code will refuse to apply in situations
wherethe old code would blindly apply changes.  Does anybody see a problem with this? 

This doesn't particularly bother me, at least.

> Patch v5-0011 is a bug fix posted elsewhere that hasn't been committed yet but which must be committed in preparation
forv5-0012. 

No idea what it is as I hadn't looked yet, but if it's a bug fix then
shouldn't it be separated and back-patched..?

> Patch v5-0012 creates a new role, pg_manage_database_objects, which can do anything with an object that the owner
coulddo with it, as long as the owner is not a superuser.  This role is intended as a "tenant" role, and is in some
sensea less powerful replacement for the pg_database_security role previously proposed. 

This I have to object to pretty strongly- we have got to get away from
the idea that just because X isn't a superuser or isn't owned by a
superuser that it's fine to allow some non-superuser to mess with it.
In particlar, just because a role isn't explicitly marked as a superuser
doesn't mean that the role can't *become* a superuser, or that it hasn't
got privileged access to the system in other ways, such as by being a
member of other predefined roles that perhaps the role who is a member
of pg_manage_database_objects doesn't have.  Such a check against
modifying of "superuser owned" objects implies that it's providing some
kind of protection against the role being able to become a superuser
when it doesn't actually provide that protection in any kind of reliable
fashion and instead ends up fooling the user.

This is the issue with CREATEROLE and we definitely shouldn't be
doubling-down on that mistake, and also brings up the point that I, at
least, had certainly hoped that part of this effort would include a way
for roles to be created by a user with an appropriate predefined role,
and w/o CREATEROLE (which would then be deprecated or, ideally, just
outright removed).  I get that this doesn't have to be in the first
patch or even patch set going down this road but the lack of discussion
or of any coordination between this effort and the other one that is
trying to address the CREATEROLE issue seems likely to land us in a bad
place with two distinct approaches being used.

> I doubt that I will create any replacement for the pg_host_security role previously proposed, as I think that role is
justsynonymous with "superuser", so it serves no purpose. 
>
> I am uncertain about creating a role similar to the pg_network_security role previously proposed, as the changes to
howpublications and subscriptions work in patches v5-0008 through v5-0010 may be enough.  In any event, I'd like
feedbackon those patches before designing one or more additional roles for this. 

"Able to create network connections" sure seems like a useful
capabilitiy to be able to delegate and which would cover postgres_fdw
and dblink use-cases also.

Thanks,

Stephen

On Mon, Aug 23, 2021 at 2:13 PM Stephen Frost <sfrost@snowman.net>
wrote:> This I have to object to pretty strongly- we have got to get
away from
> the idea that just because X isn't a superuser or isn't owned by a
> superuser that it's fine to allow some non-superuser to mess with it.
> In particlar, just because a role isn't explicitly marked as a superuser
> doesn't mean that the role can't *become* a superuser, or that it hasn't
> got privileged access to the system in other ways, such as by being a
> member of other predefined roles that perhaps the role who is a member
> of pg_manage_database_objects doesn't have.  Such a check against
> modifying of "superuser owned" objects implies that it's providing some
> kind of protection against the role being able to become a superuser
> when it doesn't actually provide that protection in any kind of reliable
> fashion and instead ends up fooling the user.

I think you make a good point here, but it seems to me that we need
*something*. We need a way to create a "lead tenant" role that can
create other tenant roles and then, err, boss them around. Not only
drop the roles again, but also drop or alter or change the owner of
their objects, or really bypass any security those roles would like to
assert as against the lead tenant. If we can't see a way to create
some sort of role of that sort, then I don't think we can really say
we've solved anything much.

> This is the issue with CREATEROLE and we definitely shouldn't be
> doubling-down on that mistake, and also brings up the point that I, at
> least, had certainly hoped that part of this effort would include a way
> for roles to be created by a user with an appropriate predefined role,
> and w/o CREATEROLE (which would then be deprecated or, ideally, just
> outright removed).  I get that this doesn't have to be in the first
> patch or even patch set going down this road but the lack of discussion
> or of any coordination between this effort and the other one that is
> trying to address the CREATEROLE issue seems likely to land us in a bad
> place with two distinct approaches being used.

Is there an active effort to do something about CREATEROLE? Do you
have a link to the thread? I feel like this is one of those things
that has occasioned discussion over the years but I am not aware of an
active project or a specific proposal to do something about this.

Maybe this can be solved from the other end? Like, as opposed to
working by exception and saying, "well, everything but superusers,"
maybe we need to explicitly declare who is included. Like, perhaps we
could somehow represent the fact that role A has super-powers with
respect to roles B, C, D, and any future roles that A may create, but
not other roles that exist on the system, or something of that sort?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Aug 23, 2021 at 2:13 PM Stephen Frost <sfrost@snowman.net>
> wrote:> This I have to object to pretty strongly- we have got to get
> away from
> > the idea that just because X isn't a superuser or isn't owned by a
> > superuser that it's fine to allow some non-superuser to mess with it.
> > In particlar, just because a role isn't explicitly marked as a superuser
> > doesn't mean that the role can't *become* a superuser, or that it hasn't
> > got privileged access to the system in other ways, such as by being a
> > member of other predefined roles that perhaps the role who is a member
> > of pg_manage_database_objects doesn't have.  Such a check against
> > modifying of "superuser owned" objects implies that it's providing some
> > kind of protection against the role being able to become a superuser
> > when it doesn't actually provide that protection in any kind of reliable
> > fashion and instead ends up fooling the user.
>
> I think you make a good point here, but it seems to me that we need
> *something*. We need a way to create a "lead tenant" role that can
> create other tenant roles and then, err, boss them around. Not only
> drop the roles again, but also drop or alter or change the owner of
> their objects, or really bypass any security those roles would like to
> assert as against the lead tenant. If we can't see a way to create
> some sort of role of that sort, then I don't think we can really say
> we've solved anything much.

Sure, but we can't just use the "superuser" flag for that, we need
something better.  The "better" in my mind here would be akin to what
we're thinking about doing for event triggers, but for roles which
actually already have a distinction between becoming a role vs. being
able to GRANT that role to another role, and that's the 'admin' option.

In other words, the user we imagine being GRANT'd this hypothetical
pg_manage_database_objects role wouldn't actually need that role to
explicitly give them access to be able to modify the objects of other
roles- it would be able to do that by virtue of just being a member of
those roles.  The roles who are allowed to modify existing role
membership should have the 'admin' right on those roles, and what we
just need is a new predefined role that's basically "allow roles to be
created or dropped" but where the only roles which can be GRANT'd by a
user with that ability are the ones that they have admin rights on, and
the only roles that they're allowed to drop they also have to have admin
rights on.

> > This is the issue with CREATEROLE and we definitely shouldn't be
> > doubling-down on that mistake, and also brings up the point that I, at
> > least, had certainly hoped that part of this effort would include a way
> > for roles to be created by a user with an appropriate predefined role,
> > and w/o CREATEROLE (which would then be deprecated or, ideally, just
> > outright removed).  I get that this doesn't have to be in the first
> > patch or even patch set going down this road but the lack of discussion
> > or of any coordination between this effort and the other one that is
> > trying to address the CREATEROLE issue seems likely to land us in a bad
> > place with two distinct approaches being used.
>
> Is there an active effort to do something about CREATEROLE? Do you
> have a link to the thread? I feel like this is one of those things
> that has occasioned discussion over the years but I am not aware of an
> active project or a specific proposal to do something about this.

Hrmpf, I had been thinking of this:

https://www.postgresql.org/message-id/flat/c2ee39152957af339ae6f3e851aef09930dd2faf.camel@credativ.de

registered in the CF here: https://commitfest.postgresql.org/34/2918/

though I see now that it isn't trying to explicitly deal with the
CREATEROLE bit (which I had understood from some other discussion was a
topic of interest to the author), but is definitely caught up in the
discussion about who is allowed to set what GUCs, and therefore still
seems rather related to me.

> Maybe this can be solved from the other end? Like, as opposed to
> working by exception and saying, "well, everything but superusers,"
> maybe we need to explicitly declare who is included. Like, perhaps we
> could somehow represent the fact that role A has super-powers with
> respect to roles B, C, D, and any future roles that A may create, but
> not other roles that exist on the system, or something of that sort?

Isn't this exactly what having the 'admin' option on a role is?  You're
GRANT'd that role and further are allowed to then GRANT that role to
other roles.  Being a member of that role means you're considered to
have 'ownership' level rights for all the objects that that role owns
too.

Maybe also need to have some condition around "you can only set
attributes on roles which you already have", or maybe we need to invent
'admin' options for each of the role attributes if we think it needs to
be more granular.  The only other thing I can think of is that we should
also contemplate what to do with things like CONNECTION LIMIT, VALID
UNTIL, perhaps others.  Those aren't currently allowed to be modified by
a role who has 'admin' rights on another role and so maybe we make those
require the new 'pg_manage_roles' predefined role and the 'admin' option
on a given role to be set.

I'm not against the idea of inventing something new here too... but the
'admin' option sure looks like what we're talking about here.

Thanks,

Stephen

> On Aug 23, 2021, at 11:13 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> This I have to object to pretty strongly- we have got to get away from
> the idea that just because X isn't a superuser or isn't owned by a
> superuser that it's fine to allow some non-superuser to mess with it.

I thought we were trying to create a set of roles which could cumulatively do everything *inside the sandbox* that
superusercan do, but which cannot escape the sandbox.  I would think this pg_manage_database_objects role would be
reasonablein the context of that effort. 

> In particlar, just because a role isn't explicitly marked as a superuser
> doesn't mean that the role can't *become* a superuser, or that it hasn't
> got privileged access to the system in other ways, such as by being a
> member of other predefined roles that perhaps the role who is a member
> of pg_manage_database_objects doesn't have.

The implementation does not allow pg_manage_database_objects to mess with objects that are owned by a role which
satisfiessuperuser_arg().  If you are renting out a database to a tenant and change the ownership of stuff to a
non-superuser,then you get what you get.  But why would you do that? 

> Such a check against
> modifying of "superuser owned" objects implies that it's providing some
> kind of protection against the role being able to become a superuser
> when it doesn't actually provide that protection in any kind of reliable
> fashion and instead ends up fooling the user.

Please provide steps to reproduce this issue.  Assume that a database is initialized and that everything is owned by
thesystem.  A "tenant" role is created and granted pg_manage_database_objects, and other non-superuser roles are
created. Now, what exactly can "tenant" do that you find objectionable? 

> This is the issue with CREATEROLE and we definitely shouldn't be
> doubling-down on that mistake, and also brings up the point that I, at
> least, had certainly hoped that part of this effort would include a way
> for roles to be created by a user with an appropriate predefined role,
> and w/o CREATEROLE (which would then be deprecated or, ideally, just
> outright removed).

Well, pg_manage_database_objects has no special ability to create or drop roles.  I thought separating those powers
mademore sense than grouping them together.  We can have a new role for doing what you say, but that seems redundant
withCREATEROLE.  I didn't want this patch set to be bogged down waiting for a consensus on how to change the CREATEROLE
privilege.

>  I get that this doesn't have to be in the first
> patch or even patch set going down this road but the lack of discussion
> or of any coordination between this effort and the other one that is
> trying to address the CREATEROLE issue seems likely to land us in a bad
> place with two distinct approaches being used.

I'm confused.  This patch set doesn't come within a country mile of CREATEROLE.  Why should this patch set have to
coordinatewith that one?  I'm not arguing with you -- merely asking what I'm misunderstanding? 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Aug 23, 2021, at 11:13 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > This I have to object to pretty strongly- we have got to get away from
> > the idea that just because X isn't a superuser or isn't owned by a
> > superuser that it's fine to allow some non-superuser to mess with it.
>
> I thought we were trying to create a set of roles which could cumulatively do everything *inside the sandbox* that
superusercan do, but which cannot escape the sandbox.  I would think this pg_manage_database_objects role would be
reasonablein the context of that effort. 

I wasn't objecting to the general concept of trying to have a role that
can do lots of things inside the sandbox but aren't allowed to escape
it.  I was specifically objecting to the idea that just checking if an
object is directly owned by a superuser is not sufficient to prevent a
role from being able to escape the sandbox.

> > In particlar, just because a role isn't explicitly marked as a superuser
> > doesn't mean that the role can't *become* a superuser, or that it hasn't
> > got privileged access to the system in other ways, such as by being a
> > member of other predefined roles that perhaps the role who is a member
> > of pg_manage_database_objects doesn't have.
>
> The implementation does not allow pg_manage_database_objects to mess with objects that are owned by a role which
satisfiessuperuser_arg().  If you are renting out a database to a tenant and change the ownership of stuff to a
non-superuser,then you get what you get.  But why would you do that? 

Simply using superuser_arg() isn't sufficient is exactly the point that
I'm trying to make.  As a 'landlord', I might very well want to have
some kind of 'landlord' role that isn't directly a superuser but which
could *become* a superuser by having been GRANT'd a superuser role- but
I certainly don't want that role's objects to be able to be messed with
by the tenant.

> > Such a check against
> > modifying of "superuser owned" objects implies that it's providing some
> > kind of protection against the role being able to become a superuser
> > when it doesn't actually provide that protection in any kind of reliable
> > fashion and instead ends up fooling the user.
>
> Please provide steps to reproduce this issue.  Assume that a database is initialized and that everything is owned by
thesystem.  A "tenant" role is created and granted pg_manage_database_objects, and other non-superuser roles are
created. Now, what exactly can "tenant" do that you find objectionable? 

If one of those other non-superuser roles is, itself, a role that can
become a superuser and it decides to create some functions for its own
purposes, then the tenant role would be able to modify those functions,
allowing the tenant to gain access to the non-superuser role, and from
there being able to gain access to superuser.

Something along these lines, basically:

CREATE USER tenant;
GRANT pg_manage_database_objects TO tenant;
CREATE USER landlord;
GRANT postgres TO landlord;
SET ROLE landlord;
CREATE FUNCTION do_stuff();
put call to do_stuff() into a cronjob
SET ROLE tenant;
CREATE OR REPLACE do_stuff(); -- with code to take over landlord

poof- tenant has ability to be landlord and then further to become
postgres.

All of the above applies beyond just superuser too- consider a
non-superuser role which has been grant'd pg_execute_server_program.
That won't trip up superuser_arg() but it sure would allow a role to
break out of the sandbox.

> > This is the issue with CREATEROLE and we definitely shouldn't be
> > doubling-down on that mistake, and also brings up the point that I, at
> > least, had certainly hoped that part of this effort would include a way
> > for roles to be created by a user with an appropriate predefined role,
> > and w/o CREATEROLE (which would then be deprecated or, ideally, just
> > outright removed).
>
> Well, pg_manage_database_objects has no special ability to create or drop roles.  I thought separating those powers
mademore sense than grouping them together.  We can have a new role for doing what you say, but that seems redundant
withCREATEROLE.  I didn't want this patch set to be bogged down waiting for a consensus on how to change the CREATEROLE
privilege.

CREATEROLE doesn't work to give to folks generally because of the issues
above- its check is, similarly, too simple and always has been.  This
isn't news either, it's been discussed in various places from time to
time and is part of why people who run cloud providers end up either not
giving out that role attribute and providing another way, or they hack
up the PG core code to handle the way that attribute works differently.

> >  I get that this doesn't have to be in the first
> > patch or even patch set going down this road but the lack of discussion
> > or of any coordination between this effort and the other one that is
> > trying to address the CREATEROLE issue seems likely to land us in a bad
> > place with two distinct approaches being used.
>
> I'm confused.  This patch set doesn't come within a country mile of CREATEROLE.  Why should this patch set have to
coordinatewith that one?  I'm not arguing with you -- merely asking what I'm misunderstanding? 

Perhaps it's just because I'm looking at the exact same issues cropping
up here that do with the CREATEROLE situation and I'd really like to
find a solution that gets us away from putting out a half-solution that
won't actually be directly usable by the folks who care about making
sure people don't break out of the sandbox because of the issues
outlined above.

Thanks,

Stephen

> On Aug 23, 2021, at 12:51 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> Simply using superuser_arg() isn't sufficient is exactly the point that
> I'm trying to make.  As a 'landlord', I might very well want to have
> some kind of 'landlord' role that isn't directly a superuser but which
> could *become* a superuser by having been GRANT'd a superuser role- but
> I certainly don't want that role's objects to be able to be messed with
> by the tenant.

> If one of those other non-superuser roles is, itself, a role that can
> become a superuser

If you have a sandbox-superuser who can do anything within the sandbox but nothing outside the sandbox, then you need a
prettygood wall at the periphery of the sandbox.  Breaking sandbox-superuser-ishness into multiple distinct privileges
ratherthan one monolithic privilege doesn't change the need for a good wall at the periphery.  The
pg_manage_database_objectsrole doesn't encompass all sandbox-superuser privileges, but it is on that side of the wall. 

We could agree to move the wall a little, and say that non-superuser roles who have the ability to become superusers
areon the other side of the wall.  That's fine.  I'd have to rework the patch a bit, but conceptually that seems
doable. We could also say that non-superusers who are members of privileged roles (pg_execute_server_programs,
pg_signal_backend,etc) are likewise on the other side of that wall. 

Does that work?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Aug 23, 2021, at 12:51 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > Simply using superuser_arg() isn't sufficient is exactly the point that
> > I'm trying to make.  As a 'landlord', I might very well want to have
> > some kind of 'landlord' role that isn't directly a superuser but which
> > could *become* a superuser by having been GRANT'd a superuser role- but
> > I certainly don't want that role's objects to be able to be messed with
> > by the tenant.
>
> > If one of those other non-superuser roles is, itself, a role that can
> > become a superuser
>
> If you have a sandbox-superuser who can do anything within the sandbox but nothing outside the sandbox, then you need
apretty good wall at the periphery of the sandbox.  Breaking sandbox-superuser-ishness into multiple distinct
privilegesrather than one monolithic privilege doesn't change the need for a good wall at the periphery.  The
pg_manage_database_objectsrole doesn't encompass all sandbox-superuser privileges, but it is on that side of the wall. 
>
> We could agree to move the wall a little, and say that non-superuser roles who have the ability to become superusers
areon the other side of the wall.  That's fine.  I'd have to rework the patch a bit, but conceptually that seems
doable. We could also say that non-superusers who are members of privileged roles (pg_execute_server_programs,
pg_signal_backend,etc) are likewise on the other side of that wall. 
>
> Does that work?

I'd much rather we go down the path that Robert had suggested where we
find a way to make a connection between the tenant role and everything
that they create, and leave everything that is outside of that box on
the other side of the 'wall'.  There's also the risk that the landlord
creates a role one day but then GRANT's superuser rights to that role on
another day, that happened to be after the tenant managed to gain
control of that role.  That kind of thing is something we should work
hard to make difficult to happen- the landlord should have to explicitly
give the tenant control over something that the landlord creates, it
shouldn't happen automagically.

Having hard-coded lists of which predefined roles are 'ok' and which
aren't sounds generally bad and I don't think we'd actually want to
include all predefined roles in that list either (even if it'd be fine
today, which I don't think it is given things like pg_monitor and
pg_signal_backend, though perhaps there could be some debate over
those...).

Thanks,

Stephen

> On Aug 23, 2021, at 11:13 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
>> Patch v5-0011 is a bug fix posted elsewhere that hasn't been committed yet but which must be committed in
preparationfor v5-0012. 
>
> No idea what it is as I hadn't looked yet, but if it's a bug fix then
> shouldn't it be separated and back-patched..?

It is already a patch waiting for commit.

Discussion: https://www.postgresql.org/message-id/1F238937-7CC2-4703-A1B1-6DC225B8978A%40enterprisedb.com

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Aug 23, 2021, at 1:46 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> I'd much rather we go down the path that Robert had suggested where we
> find a way to make a connection between the tenant role and everything
> that they create, and leave everything that is outside of that box on
> the other side of the 'wall'.

I am coming around to this way of thinking.  The main difficulty here stems (as you know) from how CREATEROLE was
implemented. You and Tom had conversations about that back in 2005 [1], and Tom even suggested perhaps roles have
owners:

> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:

> Possibly for 8.2 we could invent a notion of roles having owners.
> Offhand I don't see any harm in letting non-CREATEROLE users create
> non-login roles, and manipulate the membership of roles they have
> created (or that have been assigned to them by a superuser).  On the
> other hand, it could be that the WITH ADMIN OPTION feature is already
> sufficient for this.  This really needs some thought ...


Making roles owners of roles they create, and giving them the power to manipulate objects which belong to roles they
own(recursively), seems to solve most of our problems we have been discussing.  The remaining problem is that roles
withoutcreaterole or superuser cannot create other roles.  We don't want tenants to need either of those things, at
leastnot as they are currently defined.  We could either modify the createrole privilege to be far less powerful, or
createa new privilege. 

If role owners can alter and drop roles they own (and ones those roles own, etc.) then we could redefine CREATEROLE to
reallyjust mean the ability to create new roles.  The ability to alter or drop roles would not stem from having
CREATEROLE,but rather from owning the role.  For setups where one admin role has CREATEROLE and creates all other roles
(exceptthe superuser which created the admin) nothing changes.  In setups with multiple admins, where none own the
others,each admin would have its own fiefdom, managing everything downstream from itself, but having no special
privilegeover the other fiefdoms.  I think that setup wasn't implemented for 8.1 more for lack of time than because it
wasunwanted. 

Alternately, we could just create a new privilege parallel to CREATEROLE, but that seems confusing more than helpful.

Thoughts?


[1] https://www.postgresql.org/message-id/17554.1120258001%40sss.pgh.pa.us
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Aug 27, 2021, at 3:47 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> Making roles owners of roles they create, and giving them the power to manipulate objects which belong to roles they
own(recursively), seems to solve most of our problems we have been discussing.   

Not hearing any responses, this part is implemented in v6-0018 and v6-0019.

> The remaining problem is that roles without createrole or superuser cannot create other roles.  We don't want tenants
toneed either of those things, at least not as they are currently defined.  We could either modify the createrole
privilegeto be far less powerful, or create a new privilege. 
>
> If role owners can alter and drop roles they own (and ones those roles own, etc.) then we could redefine CREATEROLE
toreally just mean the ability to create new roles.  The ability to alter or drop roles would not stem from having
CREATEROLE,but rather from owning the role.  For setups where one admin role has CREATEROLE and creates all other roles
(exceptthe superuser which created the admin) nothing changes.  In setups with multiple admins, where none own the
others,each admin would have its own fiefdom, managing everything downstream from itself, but having no special
privilegeover the other fiefdoms.  I think that setup wasn't implemented for 8.1 more for lack of time than because it
wasunwanted. 

This really deserves more thought from the list.  CREATEROLE's behavior is unchanged in this patch set.

> On Aug 23, 2021, at 11:13 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
>> Patches v5-0002 through v5-0005 create four new roles for managing host resource settings, vacuum settings,
autovacuumsettings, and logging settings.  That last one excludes "where to log" settings, because we don't want the
roleto be able to write to arbitrary locations on the server.  Remaining guc variables not in these four categories
continueto belong to the superuser. 
>
> We do have a role today who is allowed to write to arbitrary locations
> on the server, so I wonder if for those log settings we'd include a
> requirement for the user to have both of those roles instead..?

Following your advice, pg_manage_logging_settings + pg_write_server_files is made sufficient to set "where to log"
settingsin v6-0005. 

Patches v6-0002 through v6-0005 add roles intended to allow tenants to set values for a limited number of appropriate
gucvariables.  That seems fine for the purpose of facilitating postgres-as-a-service. 

There is another reason to have roles with the power to SET or ALTER SYSTEM SET guc variables, though.  For large
deploymentsof postgres databases in the cloud, being able to log in as a non-superuser role in order to configure the
databasemeans having one fewer reasons to need to allow superuser connections to the server.  That's valuable for its
ownsake. 

Patches v6-0006 through v6-0012 add yet more roles with authority to set additional guc variables.  They don't go quite
sofar as including all gucs, but the majority of gucs are covered, and we can add additional groupings if anybody has
suggestions.




—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Aug 31, 2021, at 6:41 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> <v6-0019-Giving-role-owners-control-over-owned-roles.patch>

Synopsis:

The major change in version 7 is a reworking of role ownership and the CREATEROLE attribute to make it no longer a
near-superuserprivilege.  This new functionality is in v7-0021. 

Details:

The changes in version 7 of the patchset are:

v7-0001 is a new patch that introduces a single new regression test covering various aspects of the permissions system
surroundingcreating, altering, dropping and granting membership in roles.  The functional changes in v7-0021 do not
causepre-existing regression test failures, not even when running check-world, despite fundamentally changing how much
ofthis works.  This new test adds coverage for create role, and as each patch in the series introduces changes, is
modifiedto reflect them. 

v6-0001 through v6-0019 correspond to v7-0002 through v7-0020 and are mostly unchanged, but are updated to apply
cleanlyto the current git master, to fix a bug that was present in the v6 patch set, to update the regression tests for
securitylabels where CREATEROLE is used, and to update the create_role regression test from v7-0001 as needed per
patch.

v7-0021 redesigns the CREATEROLE attribute to no longer bestow nearly so much power.  The ability to alter or drop a
roleno longer flows from having the CREATEROLE attribute, but rather from being the role's owner.  The ADMIN option
worksas before, but role owners implicitly have ADMIN on roles which they own. 

Roles with the CREATEROLE attribute may create new roles, but those new roles may not be created with privileges which
thecreating role lacks.  Specifically, SUPERUSER, REPLICATION, BYPASSRLS, CREATEDB, CREATEROLE and LOGIN privilege may
notbe granted the new role unless the creating role has them.  (This rule is adhered to but trivial in the case of the
CREATEROLEprivilege, since the creator must necessarily have that one.)  When creating a new role using the IN ROLE,
ROLE,or ADMIN clauses, the creating role must have sufficient privileges on the roles named by these clauses to perform
theGRANTs these roles entail.  Merely having the CREATEROLE attribute is insufficient to perform arbitrary grants of
rolememberships. 

The INHERIT, VALID UNTIL, and CONNECTION LIMIT attributes are not thought about as privileges in the patch; perhaps
theyshould be?  It would be quite reasonable to say that a role with a finite connection limit should have that limit
thoughtabout as a "pool" and should have to assign connection rights from that pool to other roles it creates.
Likewise,a role with a VALID UNTIL limit could be constrained to only create roles with VALID UNTIL less than or equal
toits own limit.  Perhaps a NOINHERIT role should only be able to create NOINHERIT roles?  The patch does none of these
things,but feedback is much appreciated. 

The docs are adjusted, but drop_role.sgml may need to be further adjusted:

 <para>
  The SQL standard defines <command>DROP ROLE</command>, but it allows
  only one role to be dropped at a time, and it specifies different
  privilege requirements than <productname>PostgreSQL</productname> uses.
 </para>

I lack a copy of the SQL standard, so I'm uncertain if this patch has, by chance, changed the privilege requirements to
matchthat of the spec? 


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9/15/21 10:38 AM, Mark Dilger wrote:
>> On Aug 31, 2021, at 6:41 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>>
>> <v6-0019-Giving-role-owners-control-over-owned-roles.patch>
> Synopsis:
>
> The major change in version 7 is a reworking of role ownership and the CREATEROLE attribute to make it no longer a
near-superuserprivilege.  This new functionality is in v7-0021.
 
>
> Details:
>
> The changes in version 7 of the patchset are:
>
> v7-0001 is a new patch that introduces a single new regression test covering various aspects of the permissions
systemsurrounding creating, altering, dropping and granting membership in roles.  The functional changes in v7-0021 do
notcause pre-existing regression test failures, not even when running check-world, despite fundamentally changing how
muchof this works.  This new test adds coverage for create role, and as each patch in the series introduces changes, is
modifiedto reflect them.
 
>
> v6-0001 through v6-0019 correspond to v7-0002 through v7-0020 and are mostly unchanged, but are updated to apply
cleanlyto the current git master, to fix a bug that was present in the v6 patch set, to update the regression tests for
securitylabels where CREATEROLE is used, and to update the create_role regression test from v7-0001 as needed per
patch.
>
> v7-0021 redesigns the CREATEROLE attribute to no longer bestow nearly so much power.  The ability to alter or drop a
roleno longer flows from having the CREATEROLE attribute, but rather from being the role's owner.  The ADMIN option
worksas before, but role owners implicitly have ADMIN on roles which they own.
 
>
> Roles with the CREATEROLE attribute may create new roles, but those new roles may not be created with privileges
whichthe creating role lacks.  Specifically, SUPERUSER, REPLICATION, BYPASSRLS, CREATEDB, CREATEROLE and LOGIN
privilegemay not be granted the new role unless the creating role has them.  (This rule is adhered to but trivial in
thecase of the CREATEROLE privilege, since the creator must necessarily have that one.)  When creating a new role using
theIN ROLE, ROLE, or ADMIN clauses, the creating role must have sufficient privileges on the roles named by these
clausesto perform the GRANTs these roles entail.  Merely having the CREATEROLE attribute is insufficient to perform
arbitrarygrants of role memberships.
 
>
> The INHERIT, VALID UNTIL, and CONNECTION LIMIT attributes are not thought about as privileges in the patch; perhaps
theyshould be?  It would be quite reasonable to say that a role with a finite connection limit should have that limit
thoughtabout as a "pool" and should have to assign connection rights from that pool to other roles it creates.
Likewise,a role with a VALID UNTIL limit could be constrained to only create roles with VALID UNTIL less than or equal
toits own limit.  Perhaps a NOINHERIT role should only be able to create NOINHERIT roles?  The patch does none of these
things,but feedback is much appreciated.
 
>
> The docs are adjusted, but drop_role.sgml may need to be further adjusted:
>
>  <para>
>   The SQL standard defines <command>DROP ROLE</command>, but it allows
>   only one role to be dropped at a time, and it specifies different
>   privilege requirements than <productname>PostgreSQL</productname> uses.
>  </para>
>
> I lack a copy of the SQL standard, so I'm uncertain if this patch has, by chance, changed the privilege requirements
tomatch that of the spec?
 
>
>

This patch set is failing to apply for me - it fails on patch 2.


I haven't dug terribly deeply into it yet, but I notice that there is a
very large increase in test volume, which appears to account for much of
the 44635 lines of the patch set. I think we're probably going to want
to reduce that. We've had complaints in the past from prominent hackers
about adding too much volume to the regression tests.


I do like the basic thrust of reducing the power of CREATEROLE. There's
an old legal maxim I learned in my distant youth that says "nemo dat
quod non habet" - Nobody can give something they don't own. This seems
to be in that spirit, and I approve :-)


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

> On Sep 21, 2021, at 12:58 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> This patch set is failing to apply for me - it fails on patch 2.

Thanks for looking!  I have pulled together a new patch set which applies cleanly against current master.

> I haven't dug terribly deeply into it yet, but I notice that there is a
> very large increase in test volume, which appears to account for much of
> the 44635 lines of the patch set. I think we're probably going to want
> to reduce that. We've had complaints in the past from prominent hackers
> about adding too much volume to the regression tests.

The v8 patch set is much smaller, with the reduction being in the size of regression tests covering which roles can
performSET, RESET, ALTER SYSTEM SET, and ALTER SYSTEM RESET and on which GUCs.  The v7 patch set did exhaustive testing
onthis, which is why it was so big.  The v8 set does just a sampling of GUCs per role.  The total number of lines for
thepatch set drops from 44635 to 13026, with only 1960 lines total between the .sql and .out tests of GUC privileges. 

> I do like the basic thrust of reducing the power of CREATEROLE. There's
> an old legal maxim I learned in my distant youth that says "nemo dat
> quod non habet" - Nobody can give something they don't own. This seems
> to be in that spirit, and I approve :-)

Great!  I'm glad to hear the approach has some support.


Other changes in v8:

Add a new test for subscriptions owned by non-superusers to verify that the tablesync and apply workers replicating
theirsubscription won't replicate into schemas and tables that the subscription owner lacks privilege to touch.  The
logicto prevent that existed in the v7 patch, but I overlooked adding tests for it.  Fixed. 

Allow non-superusers to create event triggers.  The logic already existed and is unchanged to handle event triggers
ownedby non-superusers and conditioning those triggers firing on the (trigger-owner, role-performing-event) pair.  The
v7patch set didn't go quite so far as allowing non-superusers to create event triggers, but that undercuts much of the
benefitof the changes for no obvious purpose. 


Not changed in v8, but worth discussing:

Non-superusers are still prohibited from creating subscriptions, despite improvements to the security around that
circumstance. Improvements to the security model around event triggers does not have to also include permission for
non-superuserto create event triggers, but v8 does both.  These could be viewed as inconsistent choices, but I struck
thebalance this way because roles creating event triggers does not entail them doing anything that they can't already
do,whereas allowing arbitrary users to create subscriptions would entail an ordinary user causing external network
connectionsbeing initiated.  We likely need to create another privileged role and require a non-superuser to be part of
thatrole before they can create subscriptions.  That seems, however, like something to do as a follow-on patch, since
tighteningup the security on subscriptions as done in this patch doesn't depend on that. 




—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9/27/21, 11:16 AM, "Mark Dilger" <mark.dilger@enterprisedb.com> wrote:
> On Sep 21, 2021, at 12:58 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> I do like the basic thrust of reducing the power of CREATEROLE. There's
>> an old legal maxim I learned in my distant youth that says "nemo dat
>> quod non habet" - Nobody can give something they don't own. This seems
>> to be in that spirit, and I approve :-)
>
> Great!  I'm glad to hear the approach has some support.

I'd also like to voice my support for this effort.  I haven't been
following this thread too closely, but I did take a gander at the
latest patch set.  There is a lot to unpack here.  I think this could
easily be split into 3 or 4 threads.

The changes for adding GUC management roles seem pretty
straightforward and would likely be helpful for service providers.
However, I was kind of surprised that membership to such roles also
provided access to ALTER SYSTEM SET.  IMO there's quite a big
difference between allowing a user to set a GUC per-session versus
cluster-wide.  With these patches, if I just want to allow a user to
set a GUC like temp_file_limit or log_statement, I also have to give
them the ability to change it (and several other GUCs) for all roles
on the system.

I haven't spent too much time looking at the event trigger and logical
replication changes yet.

For the CREATEROLE changes, the main thing on my mind is how this
might impact upgrades.  IIUC roles with CREATEROLE will lose many
privileges after pg_upgrade.  I think one way to deal with this would
be to have such upgrades grant all the privileges they are losing, but
most CREATEROLE roles likely aren't using the full extent of their
powers, so that approach may be a little extreme.  Perhaps it is okay
to just add a blurb in the release notes about this backwards-
incompatible change.

Another interesting thing I found is that if a role has ownership of
a role that later obtains SUPERUSER, the owning role basically loses
all control of the role.  It makes sense to avoid letting non-
superusers mess with superusers, but this led me to wonder if there
should be a mechanism for transferring role ownership (e.g., ALTER
ROLE or REASSIGNED OWNED BY).  Presently, REASSIGNED OWNED BY fails
with an "unexpected classid" ERROR.  Such functionality might also
come in handy for the pg_dump changes for maintaining role ownership.

Nathan

Greetings,

* Bossart, Nathan (bossartn@amazon.com) wrote:
> On 9/27/21, 11:16 AM, "Mark Dilger" <mark.dilger@enterprisedb.com> wrote:
> > On Sep 21, 2021, at 12:58 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> >> I do like the basic thrust of reducing the power of CREATEROLE. There's
> >> an old legal maxim I learned in my distant youth that says "nemo dat
> >> quod non habet" - Nobody can give something they don't own. This seems
> >> to be in that spirit, and I approve :-)
> >
> > Great!  I'm glad to hear the approach has some support.
>
> I'd also like to voice my support for this effort.  I haven't been
> following this thread too closely, but I did take a gander at the
> latest patch set.  There is a lot to unpack here.  I think this could
> easily be split into 3 or 4 threads.

I tend to agree.  I'm also generally supportive but following everything
that's going on in this particular patch set isn't easy.

> For the CREATEROLE changes, the main thing on my mind is how this
> might impact upgrades.  IIUC roles with CREATEROLE will lose many
> privileges after pg_upgrade.  I think one way to deal with this would
> be to have such upgrades grant all the privileges they are losing, but
> most CREATEROLE roles likely aren't using the full extent of their
> powers, so that approach may be a little extreme.  Perhaps it is okay
> to just add a blurb in the release notes about this backwards-
> incompatible change.

This is definitely a pretty big change.  There needs to be a bigger and
independent discussion about the general concept of role 'self
administration' as we talk about it in the comments of the role system
and which this doesn't really address too.  I've been planning for a
while to start a specific thread about that and I'll try to do that so
that we can discuss that specifically, as it's quite relevant to all of
this, in my view.

> Another interesting thing I found is that if a role has ownership of
> a role that later obtains SUPERUSER, the owning role basically loses
> all control of the role.  It makes sense to avoid letting non-
> superusers mess with superusers, but this led me to wonder if there
> should be a mechanism for transferring role ownership (e.g., ALTER
> ROLE or REASSIGNED OWNED BY).  Presently, REASSIGNED OWNED BY fails
> with an "unexpected classid" ERROR.  Such functionality might also
> come in handy for the pg_dump changes for maintaining role ownership.

I really think we need to stop addressing roles explicitly as
'superuser' vs. 'non-superuser', because a non-superuser role can be
GRANT'd a superuser role, which makes that distinction really not
sensible.  This has continued to be a problem and we need to cleanly
address it.  Not sure exactly how to do that today but it's certainly an
issue.

Thanks,

Stephen

On 10/4/21, 7:08 PM, "Stephen Frost" <sfrost@snowman.net> wrote:
> I really think we need to stop addressing roles explicitly as
> 'superuser' vs. 'non-superuser', because a non-superuser role can be
> GRANT'd a superuser role, which makes that distinction really not
> sensible.  This has continued to be a problem and we need to cleanly
> address it.  Not sure exactly how to do that today but it's certainly an
> issue.

Agreed.  Maybe one option is to convert most of the role attributes to
be predefined roles.  Then we could just check for membership in
pg_superuser instead of trying to deal with membership in roles that
have the SUPERUSER attribute.

Nathan

On Mon, Sep 27, 2021 at 11:45 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
> Thanks for looking!  I have pulled together a new patch set which applies cleanly against current master.

Hi Mark, thanks for this work. I'm late to be here in this thread,
please note that I didn't go through the entire thread as it is quite
long for me to read.

I have a question: it looks like the view pg_backend_memory_contexts
and the function pg_log_backend_memory_contexts are superuser only.
Isn't it a good idea to allow users with a pg_monitor or some other
similar role to use these as well? This question may be unrelated here
but I'm curious to know whether your patch set has a solution.

Regards,
Bharath Rupireddy.

On Mon, Oct 4, 2021 at 8:22 PM Bossart, Nathan <bossartn@amazon.com> wrote:
> The changes for adding GUC management roles seem pretty
> straightforward and would likely be helpful for service providers.
> However, I was kind of surprised that membership to such roles also
> provided access to ALTER SYSTEM SET.  IMO there's quite a big
> difference between allowing a user to set a GUC per-session versus
> cluster-wide.  With these patches, if I just want to allow a user to
> set a GUC like temp_file_limit or log_statement, I also have to give
> them the ability to change it (and several other GUCs) for all roles
> on the system.

There's also ALTER ROLE and ALTER DATABASE, which provide more ways to
set GUCs. I agree that we could choose make distinctions here, but I
think if we make too many distinctions, it will become complicated to
administer. And if we don't have *any* way to delegate ALTER SYSTEM,
then I think we have missed the mark.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Oct 6, 2021, at 10:48 PM, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi Mark, thanks for this work. I'm late to be here in this thread,
> please note that I didn't go through the entire thread as it is quite
> long for me to read.

Thanks for joining.

> I have a question: it looks like the view pg_backend_memory_contexts
> and the function pg_log_backend_memory_contexts are superuser only.
> Isn't it a good idea to allow users with a pg_monitor or some other
> similar role to use these as well? This question may be unrelated here
> but I'm curious to know whether your patch set has a solution.

Possibly, but I have stopped adding new topics to this particular patch set, as I'm already getting requests to break
itinto separate sets and email threads.  

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Oct 7, 2021 at 10:29 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > I have a question: it looks like the view pg_backend_memory_contexts
> > and the function pg_log_backend_memory_contexts are superuser only.
> > Isn't it a good idea to allow users with a pg_monitor or some other
> > similar role to use these as well? This question may be unrelated here
> > but I'm curious to know whether your patch set has a solution.
>
> Possibly, but I have stopped adding new topics to this particular patch set, as I'm already getting requests to break
itinto separate sets and email threads.
 

Thanks Mark. I will discuss it in a separate thread.

Regards,
Bharath Rupireddy.

Le lundi 27 septembre 2021, 20:15:05 CEST Mark Dilger a écrit :
> > On Sep 21, 2021, at 12:58 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
> >
> > This patch set is failing to apply for me - it fails on patch 2.
>
> Thanks for looking!  I have pulled together a new patch set which applies
> cleanly against current master.
> > I haven't dug terribly deeply into it yet, but I notice that there is a
> > very large increase in test volume, which appears to account for much of
> > the 44635 lines of the patch set. I think we're probably going to want
> > to reduce that. We've had complaints in the past from prominent hackers
> > about adding too much volume to the regression tests.
>
> The v8 patch set is much smaller, with the reduction being in the size of
> regression tests covering which roles can perform SET, RESET, ALTER SYSTEM
> SET, and ALTER SYSTEM RESET and on which GUCs.  The v7 patch set did
> exhaustive testing on this, which is why it was so big.  The v8 set does
> just a sampling of GUCs per role.  The total number of lines for the patch
> set drops from 44635 to 13026, with only 1960 lines total between the .sql
> and .out tests of GUC privileges.
> > I do like the basic thrust of reducing the power of CREATEROLE. There's
> > an old legal maxim I learned in my distant youth that says "nemo dat
> > quod non habet" - Nobody can give something they don't own. This seems
> > to be in that spirit, and I approve :-)
>
> Great!  I'm glad to hear the approach has some support.
>
>
> Other changes in v8:
>
> Add a new test for subscriptions owned by non-superusers to verify that the
> tablesync and apply workers replicating their subscription won't replicate
> into schemas and tables that the subscription owner lacks privilege to
> touch.  The logic to prevent that existed in the v7 patch, but I overlooked
> adding tests for it.  Fixed.
>
> Allow non-superusers to create event triggers.  The logic already existed
> and is unchanged to handle event triggers owned by non-superusers and
> conditioning those triggers firing on the (trigger-owner,
> role-performing-event) pair.  The v7 patch set didn't go quite so far as
> allowing non-superusers to create event triggers, but that undercuts much
> of the benefit of the changes for no obvious purpose.
>
>
> Not changed in v8, but worth discussing:
>
> Non-superusers are still prohibited from creating subscriptions, despite
> improvements to the security around that circumstance.  Improvements to the
> security model around event triggers does not have to also include
> permission for non-superuser to create event triggers, but v8 does both.
> These could be viewed as inconsistent choices, but I struck the balance
> this way because roles creating event triggers does not entail them doing
> anything that they can't already do, whereas allowing arbitrary users to
> create subscriptions would entail an ordinary user causing external network
> connections being initiated.  We likely need to create another privileged
> role and require a non-superuser to be part of that role before they can
> create subscriptions.  That seems, however, like something to do as a
> follow-on patch, since tightening up the security on subscriptions as done
> in this patch doesn't depend on that.

The changes proposed around subscription management make a lot of sense,
especially considering that now that we don't allow to run ALTER SUBSCRIPTION
REFRESH in a function anymore, there is no way to delegate this to a non
superuser (using a security definer function). Since it doesn't involve the
rest of the patchset, patches 16, 17 and 18 could be separated in another
thread / patchset.

--
Ronan Dunklau

On Mon, 2021-09-27 at 11:15 -0700, Mark Dilger wrote:
> Allow non-superusers to create event triggers.  The logic already
> existed and is unchanged to handle event triggers owned by non-
> superusers and conditioning those triggers firing on the (trigger-
> owner, role-performing-event) pair.  The v7 patch set didn't go quite
> so far as allowing non-superusers to create event triggers, but that
> undercuts much of the benefit of the changes for no obvious purpose.

The thread on role self-administration seems like a dependency here.
And it doesn't look like there's consensus that we should be
conditioning event trigger firing on role membership:

https://postgr.es/m/20211005043438.GB314685@rfd.leadboat.com

Instead, how about:

* make a predefined role pg_event_trigger that allows creating event
triggers
* make it an error for a superuser to fire an event trigger created by
a non-superuser

It doesn't solve the problem hierarchically, but we don't solve other
predefined role privileges hierarchically, either (and for many of them
it makes no sense).

A downside is that the privileged event trigger creator could
accidentally make life annoying for a superuser that's trying to issue
DDL: the superuser would need to disable the event trigger, perform the
action, then re-enable it. But that shouldn't be a practical problem in
sane setups -- superusers shouldn't be performing a lot of DDL, and if
they are, it's good to be explicit that they are bypassing something
configured by their pseudo-admin.

Regards,
    Jeff Davis

> On Oct 19, 2021, at 12:28 PM, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2021-09-27 at 11:15 -0700, Mark Dilger wrote:
>> Allow non-superusers to create event triggers.  The logic already
>> existed and is unchanged to handle event triggers owned by non-
>> superusers and conditioning those triggers firing on the (trigger-
>> owner, role-performing-event) pair.  The v7 patch set didn't go quite
>> so far as allowing non-superusers to create event triggers, but that
>> undercuts much of the benefit of the changes for no obvious purpose.
>
> The thread on role self-administration seems like a dependency here.
> And it doesn't look like there's consensus that we should be
> conditioning event trigger firing on role membership:
>
> https://postgr.es/m/20211005043438.GB314685@rfd.leadboat.com

I have noticed the lack of consensus.  The resistance to having roles own other roles should get more attention, I
think.

Stephen and I went into the weeds on what DROP ROLE rolename CASCADE should mean, but I don't think that should hold up
theidea of role ownership.  He wanted a different command to do the work rather than this command, but I don't see
anythingin what he wrote to suggest that the idea is unacceptable, only a different preference on how that
functionalitygets spelled. 

There was also some difference in interpretation on what exact differences there are between "ownership" and
"dependency". To me, "ownership" is a subtype of dependency, just as "is indexed by" and "is contained in" are subtypes
ofdependency.  Indexes are dependent on the tables they index, tables are dependent on schemas that contain them,
objectsare dependent on roles that own them, and so forth.  Stephen seemed to have a different view.  I'm not at all
clearon whether his different view is a showstopper. 

Before redesigning the way we fix up event triggers for v15, I'd like to have a sense of how contentious all this is.
Ifit's just a matter of definitions and command spellings, we can work around it. 

Thanks for participating in this thread, BTW.

> Instead, how about:
>
> * make a predefined role pg_event_trigger that allows creating event
> triggers
> * make it an error for a superuser to fire an event trigger created by
> a non-superuser

I think blocking superuser actions is a non-starter, but you address that below....

> It doesn't solve the problem hierarchically, but we don't solve other
> predefined role privileges hierarchically, either (and for many of them
> it makes no sense).
>
> A downside is that the privileged event trigger creator could
> accidentally make life annoying for a superuser that's trying to issue
> DDL: the superuser would need to disable the event trigger, perform the
> action, then re-enable it. But that shouldn't be a practical problem in
> sane setups -- superusers shouldn't be performing a lot of DDL, and if
> they are, it's good to be explicit that they are bypassing something
> configured by their pseudo-admin.

I'd prefer not to assume much about the sanity of the setup, and I agree the superuser should be able to
unconditionallydisable the offending event trigger, but I think it is a pretty poor solution that a superuser would
needto disable and then re-enable a trigger.  Other commands in other sessions would be able to sneak through during
thewindow of time when the trigger is disabled.  Wouldn't it be much cleaner to have superuser bypass the trigger? 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2021-10-19 at 13:17 -0700, Mark Dilger wrote:
> Wouldn't it be much cleaner to have superuser bypass the trigger?

Maybe it could be a user property like "BYPASS_EVENT_TRIGGERS", and
only superusers could adjust it (like the SUPERUSER and REPLICATION
properties).

I suppose it would default to BYPASS_EVENT_TRIGGERS for superusers and
not for non-superusers. A little awkward to have different defaults,
but it seems sensible in this case.

Would this bypass all event triggers, or only the event triggers of
another user?

Regards,
    Jeff Davis

> On Oct 19, 2021, at 3:18 PM, Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Tue, 2021-10-19 at 13:17 -0700, Mark Dilger wrote:
>> Wouldn't it be much cleaner to have superuser bypass the trigger?
>
> Maybe it could be a user property like "BYPASS_EVENT_TRIGGERS", and
> only superusers could adjust it (like the SUPERUSER and REPLICATION
> properties).
>
> I suppose it would default to BYPASS_EVENT_TRIGGERS for superusers and
> not for non-superusers. A little awkward to have different defaults,
> but it seems sensible in this case.
>
> Would this bypass all event triggers, or only the event triggers of
> another user?

The difficulty is that non-superuser owned event triggers could be something of a minefield for scripts run as
superuser. The cleanest way around that would be to have them never fire in response to superuser actions.
Installationscould still have event triggers that cover all users, including superusers, as long as they have those
triggersowned by superuser. 

The implementation in the patch set does this, but with finer grained precision, because the universe of roles is
dividedinto more than just superuser vs. non-superuser. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2021-10-19 at 18:52 -0400, Stephen Frost wrote:
> > The implementation in the patch set does this, but with finer
> > grained precision, because the universe of roles is divided into
> > more than just superuser vs. non-superuser.
> 
> This last point is particularly important. Non-super users may be
> able to become superuser and those roles which are able to need to
> also be protected. Only protecting superuser roles themselves is
> *not* enough. 

I'm trying to suggest an approach that is flexible enough (not limited
to superusers), but also addresses Noah's complaint that ordinary role
membership should not implicitly control event trigger firing.

The most direct way to address Noah's complaint is to make a new
explicit user option BYPASS_EVENT_TRIGGERS, which controls whether
event triggers fire or not.

To create an event trigger, a user would still need to be a member of
predefined role pg_create_event_trigger, which would still be a highly
privileged user that can essentially take over any user without
BYPASS_EVENT_TRIGGER. In practice, the members of
pg_create_event_trigger would be pseudo-superusers, or highly-
privileged roles that come along with a C extension that needs event
triggers.

Details:
  * Event triggers created by a superuser would never be bypassed.
  * Superusers would always bypass event triggers unless the event
trigger is owned by another superuser.
  * If a role is highly privileged and/or can become superuser, it
should have BYPASS_EVENT_TRIGGERS specified so that members of
pg_create_event_trigger can't escalate to that role.
  * Normal users should not have BYPASS_EVENT_TRIGGERS.

Another benefit of this is that it makes this event trigger change
independent of the Role Self-Administration discussion, so it could
potentially be committed sooner.

A downside is that with my suggestion, event triggers would still be
for the highly-privileged roles only. Allowing unprivileged users to
create event triggers that have limited scope might allow some really
interesting use cases. There might be some options here, like allowing
any user to create an event trigger that only affects that user.

Regards,
    Jeff Davis

(Aside: I'm not really sure where the line is between adding a
predefined role and adding a user option. Are user options just the old
way, and predefined roles the new way?)

> On Oct 20, 2021, at 10:20 AM, Jeff Davis <pgsql@j-davis.com> wrote:
>
> I'm trying to suggest an approach that is flexible enough (not limited
> to superusers), but also addresses Noah's complaint that ordinary role
> membership should not implicitly control event trigger firing.
>
> The most direct way to address Noah's complaint is to make a new
> explicit user option BYPASS_EVENT_TRIGGERS, which controls whether
> event triggers fire or not.

I'd like to have a much clearer understanding of Noah's complaint first.  There are multiple things to consider: (1)
therole which owns the trigger, (2) the role which is performing an action which would cause the trigger to fire, (3)
theset of roles role #1 belongs to, (4) the set of roles role #1 has ADMIN privilege on, (5) the set of roles that role
#2belongs to, and (6) the set of roles that role #2 has ADMIN privilege on.  Maybe more? 

And that's before we even get into having roles own other roles, which the event trigger patches *do not depend on*.
Inthe patch set associated with this thread, the event trigger stuff is in patches 0014 and 0015.  The changes to
CREATEROLEand role ownership are not until patches 0019, 0020, and 0021.  (I'm presently writing another set of emails
tosplit this all into four threads/patch sets.)  

I'd like to know precisely which combinations of these six things are objectionable, and why.  There may be a way
aroundthe objections without needing to create new user options or new privileged roles. 

> Another benefit of this is that it makes this event trigger change
> independent of the Role Self-Administration discussion, so it could
> potentially be committed sooner.

I don't think the two are related, though I can understand the confusion.  That is, in fact, a small part of why I'm
splittingthe patches into different email threads. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, 2021-10-20 at 10:32 -0700, Mark Dilger wrote:
> I'd like to have a much clearer understanding of Noah's complaint
> first.  There are multiple things to consider: (1) the role which
> owns the trigger, (2) the role which is performing an action which
> would cause the trigger to fire, (3) the set of roles role #1 belongs
> to, (4) the set of roles role #1 has ADMIN privilege on, (5) the set
> of roles that role #2 belongs to, and (6) the set of roles that role
> #2 has ADMIN privilege on.  Maybe more?
> 
> And that's before we even get into having roles own other roles,
> which the event trigger patches *do not depend on*.  In the patch set
> associated with this thread, the event trigger stuff is in patches
> 0014 and 0015.  The changes to CREATEROLE and role ownership are not
> until patches 0019, 0020, and 0021.  (I'm presently writing another
> set of emails to split this all into four threads/patch sets.) 
> 
> I'd like to know precisely which combinations of these six things are
> objectionable, and why.  There may be a way around the objections
> without needing to create new user options or new privileged roles.

I can't speak for Noah, but my interpretation is that it would be
surprising if GRANT/REVOKE or membership in an ordinary role had
effects other than "permission denied" errors. It might make sense for
event trigger firing in all the cases we can think of, but it would
certainly be strange if we started accumulating a collection of
behaviors that implicitly change when you move in or out of a role.

That's pretty general, so to answer your question: it seems like a
problem to use #3-6 in the calculation about whether to fire an event
trigger.

However, if we have a concept of role *ownership*, that's something
new. It may be less surprising to use that to determine additional
behaviors, like whether event triggers fire.

We can also consider adding some additional language to the CREATE
EVENT TRIGGER syntax to make it more explicit what the scope is. For
instance:

   CREATE EVENT TRIGGER name
    ON event
    [ FOR {ALL|OWNED} ROLES ]
    [ WHEN filter_variable IN (filter_value [, ... ]) [ AND ... ] ]
    EXECUTE { FUNCTION | PROCEDURE } function_name()

For a superuser ALL and OWNED would be the same, but regular users
would need to specify "FOR OWNED ROLES" or they'd get an error.

Regards,
    Jeff Davis

I have now received multiple requests to split this patchset into multiple parts, including some off-list.  I'll be
postingfour new patch sets on four new topics: 


New privileged roles which can SET and ALTER SYSTEM SET (v8-0002 through v8-0013)

Non-superuser event trigger owners (v8-0014 and v8-0015)

Non-superuser subscription owners (v8-0016 through v8-0018)

CREATEROLE and role ownership hierarchies (v8-0001 and v8-0019 through v8-0021)


I cannot presume everyone will be interested, so I will only cc'd Andrew on the new threads, given his status as a
reviewer. I don't mean by this to cut anyone out of the discussion. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, 2021-05-27 at 23:06 -0700, Noah Misch wrote:
> pg_logical_replication would not be safe to delegate that way:
> 
https://postgr.es/m/flat/CACqFVBbx6PDq%2B%3DvHM0n78kHzn8tvOM-kGO_2q_q0zNAMT%2BTzdA%40mail.gmail.com

What do you mean "that way"? Do you mean it's not safe to delegate
subscription creation to non-superusers at all?

From the thread above, I don't see anything so dangerous that it can't
be delegated:

  * persistent background workers on subscriber
    - still seems reasonable to delegate to a privileged user
  * arbitrary code execution by the apply worker on subscriber
    - apply worker runs as subscription owner, so doesn't seem
      like a problem?
  * connection info may be visible to non-superusers
    - seems either solvable or not necessarily a problem

Regards,
    Jeff Davis

On Wed, Oct 20, 2021 at 1:20 PM Jeff Davis <pgsql@j-davis.com> wrote:
> A downside is that with my suggestion, event triggers would still be
> for the highly-privileged roles only. Allowing unprivileged users to
> create event triggers that have limited scope might allow some really
> interesting use cases. There might be some options here, like allowing
> any user to create an event trigger that only affects that user.

I think that's basically giving up the important part of this idea,
which is to allow meaningful administration without superuser
privileges. "highly-privileged roles only" sounds like in practice it
would amount to the superuser or someone who can become the superuser
-- and thus probably wouldn't include the "master tenant" role in a
service provider environment.

I don't really see what the problem is with Tom's proposal[1,2], or
why the role self-administration thread is necessarily a blocker. It's
true that if X creates an event trigger and it fires for Y because X
can become Y, then Y might be able to revoke membership in Y from X
and thus circumvent the event trigger firing. But that is a severable
problem. We can fail to solve that problem and still be better off
than today, because at least with the proposed change a cooperating
group of users (or one whose ability to execute GRANT and REVOKE is
restricted by some other means) can benefit from event triggers
without any of them being superuser. If we make this change *and also*
resolve the role self-administration problem, then it can also work in
cases where a more privileged user needs to enforce event trigger
firing against a less-privileged user.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

[1] http://postgr.es/m/214052.1627331086@sss.pgh.pa.us
[2] http://postgr.es/m/216038.1627333077@sss.pgh.pa.us

On Wed, 2021-10-20 at 16:36 -0400, Stephen Frost wrote:
> > I think that's basically giving up the important part of this idea,
> > which is to allow meaningful administration without superuser
> > privileges. "highly-privileged roles only" sounds like in practice
> > it
> > would amount to the superuser or someone who can become the
> > superuser
> > -- and thus probably wouldn't include the "master tenant" role in a
> > service provider environment.
> 
> I’m in agreement with Robert on this point.

I'm OK to move past this and continue with Mark's approach.

Noah made the original complaint, though, so he might have something to
add.

Regards,
    Jeff Davis

On Wed, Oct 20, 2021 at 11:27:11AM -0700, Jeff Davis wrote:
> On Wed, 2021-10-20 at 10:32 -0700, Mark Dilger wrote:
> > I'd like to have a much clearer understanding of Noah's complaint
> > first.  There are multiple things to consider: (1) the role which
> > owns the trigger, (2) the role which is performing an action which
> > would cause the trigger to fire, (3) the set of roles role #1 belongs
> > to, (4) the set of roles role #1 has ADMIN privilege on, (5) the set
> > of roles that role #2 belongs to, and (6) the set of roles that role
> > #2 has ADMIN privilege on.  Maybe more?

> > I'd like to know precisely which combinations of these six things are
> > objectionable, and why.  There may be a way around the objections
> > without needing to create new user options or new privileged roles.
> 
> I can't speak for Noah, but my interpretation is that it would be
> surprising if GRANT/REVOKE or membership in an ordinary role had
> effects other than "permission denied" errors. It might make sense for
> event trigger firing in all the cases we can think of, but it would
> certainly be strange if we started accumulating a collection of
> behaviors that implicitly change when you move in or out of a role.
> 
> That's pretty general, so to answer your question: it seems like a
> problem to use #3-6 in the calculation about whether to fire an event
> trigger.

Exactly.  That's the main point.  Also, it's currently a best practice for
only non-LOGIN roles to have members.  The proposed approach invites folks to
abandon that best practice.  I take the two smells as a sign that we'll regret
adopting the proposal, despite not knowing how it will go seriously wrong.

On Wed, Oct 20, 2021 at 12:09:08PM -0700, Jeff Davis wrote:
> On Thu, 2021-05-27 at 23:06 -0700, Noah Misch wrote:
> > pg_logical_replication would not be safe to delegate that way:
> > 
> https://postgr.es/m/flat/CACqFVBbx6PDq%2B%3DvHM0n78kHzn8tvOM-kGO_2q_q0zNAMT%2BTzdA%40mail.gmail.com
> 
> What do you mean "that way"? Do you mean it's not safe to delegate
> subscription creation to non-superusers at all?

I meant "pg_logical_replication would not be safe to delegate to the tenant of
a database provided as a service."  It's not safe today, but it can be made
safe:

> From the thread above, I don't see anything so dangerous that it can't
> be delegated:
> 
>   * persistent background workers on subscriber
>     - still seems reasonable to delegate to a privileged user

Agreed, I don't have a problem with pg_logical_replication implying that
ability.  If you can create this worker, you can bypass ADMIN OPTION by
running the GRANT/REVOKE inside a subscription.  That's probably fine if
documented, or else is_admin_of_role() could prevent it.


>   * arbitrary code execution by the apply worker on subscriber
>     - apply worker runs as subscription owner, so doesn't seem
>       like a problem?

Sounds right.  I think Mark Dilger drafted a patch to add ACL checks and a TAP
test confirming that the worker does get permission denied.  That change has
no disadvantage, so this problem is on the way to getting solved.

>   * connection info may be visible to non-superusers
>     - seems either solvable or not necessarily a problem

Yes.

The other matter from the thread you linked is "the connection to the
publisher must enforce the equivalent of dblink_security_check()".  I think
Mark Dilger drafted a patch for that, too.

Greetings,

* Noah Misch (noah@leadboat.com) wrote:
> On Wed, Oct 20, 2021 at 11:27:11AM -0700, Jeff Davis wrote:
> > On Wed, 2021-10-20 at 10:32 -0700, Mark Dilger wrote:
> > > I'd like to have a much clearer understanding of Noah's complaint
> > > first.  There are multiple things to consider: (1) the role which
> > > owns the trigger, (2) the role which is performing an action which
> > > would cause the trigger to fire, (3) the set of roles role #1 belongs
> > > to, (4) the set of roles role #1 has ADMIN privilege on, (5) the set
> > > of roles that role #2 belongs to, and (6) the set of roles that role
> > > #2 has ADMIN privilege on.  Maybe more?
>
> > > I'd like to know precisely which combinations of these six things are
> > > objectionable, and why.  There may be a way around the objections
> > > without needing to create new user options or new privileged roles.
> >
> > I can't speak for Noah, but my interpretation is that it would be
> > surprising if GRANT/REVOKE or membership in an ordinary role had
> > effects other than "permission denied" errors. It might make sense for
> > event trigger firing in all the cases we can think of, but it would
> > certainly be strange if we started accumulating a collection of
> > behaviors that implicitly change when you move in or out of a role.
> >
> > That's pretty general, so to answer your question: it seems like a
> > problem to use #3-6 in the calculation about whether to fire an event
> > trigger.
>
> Exactly.  That's the main point.  Also, it's currently a best practice for
> only non-LOGIN roles to have members.  The proposed approach invites folks to
> abandon that best practice.  I take the two smells as a sign that we'll regret
> adopting the proposal, despite not knowing how it will go seriously wrong.

This seems like a pretty good point, which leads me to again think that
we should explicitly add a way for an individual who can create event
triggers to be able to specify for whom the event trigger should fire,
and only allow them to specify roles other than their own provided they
have been given that authority (either explicitly somehow or implicitly
based on some defined access that they have to that other role).

Thanks,

Stephen

On Mon, Oct 25, 2021 at 12:20 PM Stephen Frost <sfrost@snowman.net> wrote:
> > Exactly.  That's the main point.  Also, it's currently a best practice for
> > only non-LOGIN roles to have members.  The proposed approach invites folks to
> > abandon that best practice.  I take the two smells as a sign that we'll regret
> > adopting the proposal, despite not knowing how it will go seriously wrong.
>
> This seems like a pretty good point, which leads me to again think that
> we should explicitly add a way for an individual who can create event
> triggers to be able to specify for whom the event trigger should fire,
> and only allow them to specify roles other than their own provided they
> have been given that authority (either explicitly somehow or implicitly
> based on some defined access that they have to that other role).

I agree that Noah has a reasonably good point here. I don't think it's
a total slam-dunk but it it's certainly not a stupid argument.
Conceding that point for the purposes of discussion, I don't
understand how this kind of proposal gets us out from under the
problem. Surely, it can't be the case that user X can cause event
trigger E to run as user Y unless X can become Y, because to do so
would allow X to usurp Y's privileges, except in the corner case where
Y never does anything that can trigger an event trigger. But if X has
to be able to become Y in order to force E to be run by Y, then I
think we've made no progress in addressing Noah's complaint.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Oct 25, 2021 at 12:20 PM Stephen Frost <sfrost@snowman.net> wrote:
> > > Exactly.  That's the main point.  Also, it's currently a best practice for
> > > only non-LOGIN roles to have members.  The proposed approach invites folks to
> > > abandon that best practice.  I take the two smells as a sign that we'll regret
> > > adopting the proposal, despite not knowing how it will go seriously wrong.
> >
> > This seems like a pretty good point, which leads me to again think that
> > we should explicitly add a way for an individual who can create event
> > triggers to be able to specify for whom the event trigger should fire,
> > and only allow them to specify roles other than their own provided they
> > have been given that authority (either explicitly somehow or implicitly
> > based on some defined access that they have to that other role).
>
> I agree that Noah has a reasonably good point here. I don't think it's
> a total slam-dunk but it it's certainly not a stupid argument.

Ok.

> Conceding that point for the purposes of discussion, I don't
> understand how this kind of proposal gets us out from under the
> problem. Surely, it can't be the case that user X can cause event
> trigger E to run as user Y unless X can become Y, because to do so
> would allow X to usurp Y's privileges, except in the corner case where
> Y never does anything that can trigger an event trigger. But if X has
> to be able to become Y in order to force E to be run by Y, then I
> think we've made no progress in addressing Noah's complaint.

X having rights over Y is what would allow X to create an event trigger
which fires when Y does $something, but the act of GRANT'ing Y to X
wouldn't make it automatically start happening.  The latter is what I
believed Noah's concern was around.

The downside there though is that GRANT'ing of roles to other roles is
how we build up sets of roles and you'd certainly wish to be able to
leverage that when deciding which roles a given event trigger should
fire for.  If we made that work for event triggers then you'd still have
the case that *some* GRANT A to B would cause event triggers to suddenly
start happening for B without other actions being taken.  Still, in that
case you could create specific such roles to manage that independently
of which roles happened to have admin rights over which other roles.

Examples might help here.

CREATE ROLE X;
CREATE ROLE Y;
CREATE ROLE Z;

GRANT Y to X;
GRANT Z to X;

SET ROLE X;
CREATE EVENT TRIGGER do_stuff();

Under one approach, that event trigger then fires for X, Y and Z.  What
if you don't actually want it to though?  What if some role Q is later
created and GRANT'd to X?  Then the event trigger would also fire for
them.

Consider instead:

CREATE ROLE X;
CREATE ROLE Y;
CREATE ROLE Z;

GRANT Y to X;
GRANT Z to X;

SET ROLE X;
CREATE EVENT TRIGGER FOR Y do_stuff();

Now, X has explicitly said that they want the event trigger to fire for
role Y and if the event trigger fires or not is no longer based on
membership in the role creating the trigger but instead depends on being
the role which the event trigger was explicitly defined to fire on.

Does membership in role Y cause the event trigger to fire for that role?
I'd argue that the answer is probably 'yes', but at least it's no longer
tied back to membership in X (the owner of the trigger).  That Noah
explicitly mentioned 'login' roles vs. 'non-login' roles makes me think
this is more in line with what the argument was about- the owner of the
trigger would almost certainly be a 'login' role.  All that said, this
is definitely a complex area and there's certainly a lot of different
ways we could go.

Thanks,

Stephen

On Mon, Oct 25, 2021 at 2:30 PM Stephen Frost <sfrost@snowman.net> wrote:
> Does membership in role Y cause the event trigger to fire for that role?
> I'd argue that the answer is probably 'yes', but at least it's no longer
> tied back to membership in X (the owner of the trigger).  That Noah
> explicitly mentioned 'login' roles vs. 'non-login' roles makes me think
> this is more in line with what the argument was about- the owner of the
> trigger would almost certainly be a 'login' role.  All that said, this
> is definitely a complex area and there's certainly a lot of different
> ways we could go.

I mean I get all this. I am not convinced that it's a big problem,
because it seems a bit hypothetical, but if it is a problem, then
introducing some explicit mechanism to control which triggers fire for
which users is a solution. I'm a bit concerned that it's just going to
make it complicated to configure your event triggers to no real
benefit. Suppose that, as a master tenant, have 10 event triggers and
100 users and all the users are supposed to run all the event
triggers. When I add user #101, if I have to say, yes, I want that
user to fire the same 10 event triggers, running a separate SQL
command for each of one, that's kind of annoying. If I can just create
the new user and I automatically gain membership in that user and it
therefore fires all my event triggers, I get the behavior I wanted
anyway without having to do any special steps.

But also, Noah writes: "Also, it's currently a best practice for only
non-LOGIN roles to have members.  The proposed approach invites folks
to abandon that best practice."

The kind of mechanism you're proposing here doesn't seem to make that
any less likely.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Oct 25, 2021, at 11:30 AM, Stephen Frost <sfrost@snowman.net> wrote:

> Consider instead:
>
> CREATE ROLE X;
> CREATE ROLE Y;
> CREATE ROLE Z;
>
> GRANT Y to X;
> GRANT Z to X;
>
> SET ROLE X;
> CREATE EVENT TRIGGER FOR Y do_stuff();
>
> Now, X has explicitly said that they want the event trigger to fire for
> role Y and if the event trigger fires or not is no longer based on
> membership in the role creating the trigger but instead depends on being
> the role which the event trigger was explicitly defined to fire on.

I don't think your proposal quite works, because the set of users you'd like to audit with an event trigger based
auditingsystem may be both large and dynamic: 

CREATE ROLE batman;
CREATE ROLE robin;

SET ROLE batman;
CREATE ROLE gotham_residents NOLOGIN;
CREATE ROLE riddler IN ROLE gotham_residents LOGIN;
-- create millions of other Gotham residents....
CREATE EVENT TRIGGER FOR gotham_residents audit_criminal_activity();

Batman is not superuser, but he's pretty powerful, and he wants to audit all the criminal activity in Gotham.  How
shouldhe expect this example to work? 

Having the "FOR gotham_residents" clause mean anybody with membership in role gotham_residents is problematic, because
itmeans that being granted into a role both increases and decreases your freedoms, rather than merely giving you more
freedoms. If Batman covets privileges that Robin has, but wants not to be subjected to any event triggers that fire for
Robin,he both wants into and out of role Robin. 

Having "FOR gotham_residents" mean that only actions performed by role "gotham_residents" should fire the trigger is
useless,since Gotham residents don't log in as that, but as themselves.  Batman won't catch anybody this way. 

Having to list each new resident to the trigger is tedious and error-prone.  Batman may not be able to pass a
complianceaudit. 

Having Batman *own* all residents in Gotham city would work, if we can agree on a role ownership system.  It has the
downsidethat only a role's (direct or indirect) owner can do the auditing, though.  That's more flexible than what we
havetoday, where only superuser can do it, but maybe some people would want to argue for a different solution with even
moreflexibility?  A grantable privilege perhaps?  But whatever it is, the reasoning about who gets audited and who does
notmust be clear enough that Batman can pass a compliance audit. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Oct 25, 2021 at 3:25 PM Robert Haas <robertmhaas@gmail.com> wrote:
> But also, Noah writes: "Also, it's currently a best practice for only
> non-LOGIN roles to have members.  The proposed approach invites folks
> to abandon that best practice."
>
> The kind of mechanism you're proposing here doesn't seem to make that
> any less likely.

OK, I have a new idea. Any system for multi-tenancy necessarily
involves keeping track of the association between users and tenants -
i.e. for any given user, to which tenant do they belong? In Mark's
system, you do that by creating a role, and you make that role the
owner of all of the other roles that are part of that tenant (so that
it can drop them) and you also make it a member of all of those other
roles (so that it can use their privileges).

But the thing that defines a tenant need not be a role. It can be some
other kind of object. Suppose we invent a CREATE TENANT command. Every
user is associated with exactly one tenant, and can optionally be an
administrator for that tenant. If you are the administrator for a
tenant, you can create event triggers that affect every user
associated with that tenant. You also have the privileges of every
user associated with that tenant. If you have the CREATEROLE
privilege, you can create new users who will be associated with the
same tenant as you are, and you can drop existing users that are
associated with that tenant (but not roles associated with other
tenants).

This is effectively the same thing as Mark's proposal, but just using
a new kind of object (TENANT) where Mark used an existing kind of
object (ROLE). And it addresses Noah's concern, perhaps, because with
the approach the tenant administrator isn't a member of every role,
but rather just gets the privileges of all the roles as if they were.
You might argue that's a distinction without a difference, but I don't
think so: the superuser is in effect a member of every role as things
stand, and the whole idea of this project is to all for
quasi-superusers who can administer a subset of the users in the
system, so something of this kind seems like it has to exist for the
proposal to achieve its object. But it need not be role membership per
se, and maybe shouldn't be.

I don't know if this is a better idea than what Mark did, but I think
it has some appealing properties. One is that you don't need to (and
indeed can't) have people log in as the lead tenant role directly -
because that concept does not exist. If Google is your customer, then
in Mark's proposal, you have a high-privilege 'google' account and you
log into that when you want to do high-privilege things. But what if
you have multiple people who are entitled to administer the Google
tenant? Either they share access to that single account, or, well, I
don't know. A second user could have membership in every login and
non-login role for the tenant, but only one user can be the owner of
all of those roles, so maybe there's no other way to do it. If the
tenant is a separate concept that sits to one side of the role system,
you can just make multiple users administrators of the google tenant,
and that should be fine. And that way, each one has their own password
and can be separately disabled if that person leaves the company or
demoted to a non-administrator if they get moved to a different job.

There might well be problems with this idea, either on a grand scale
that make the whole thing a non-starter, or on a smaller scale that
mean that the definition of what it means to be a tenant administrator
needs fine-tuning ... but I don't know what they are, so I thought I'd
throw this out there and see what people think.

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Oct 27, 2021, at 1:10 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> This is effectively the same thing as Mark's proposal, but just using
> a new kind of object (TENANT) where Mark used an existing kind of
> object (ROLE). And it addresses Noah's concern, perhaps, because with
> the approach the tenant administrator isn't a member of every role,
> but rather just gets the privileges of all the roles as if they were.
> You might argue that's a distinction without a difference, but I don't
> think so: the superuser is in effect a member of every role as things
> stand, and the whole idea of this project is to all for
> quasi-superusers who can administer a subset of the users in the
> system, so something of this kind seems like it has to exist for the
> proposal to achieve its object. But it need not be role membership per
> se, and maybe shouldn't be.

It feels to me that the traditional concept of users and groups could map, one-to-one, onto users and roles, but we've
mappedboth users and groups, many-to-one, onto roles, leaving no distinct concept of groups, and now we're proposing
addinga concept called "tenant" that means something like "group".  I find that simultaneously helpful and pretty
confusing.

Compare that to the help and confusion created by my proposal.  The idea that roles can own roles, just as roles can
owntables, indexes, etc., doesn't seem confusing to me, but perhaps it does to others.  If you accept that roles can
ownroles, then the idea that roles can drop roles that they own, or change characteristics of roles that they own, is
entirelyanalogous to roles being able to drop or alter any other sort of object that they own.  To me, that is
perfectlyconsistent and unsurprising, but again, perhaps not to others. 

Noah's concern, as I understood it, was not about roles owning roles, but about role membership being what controls if
anevent trigger fires.  If anything, that concern stems from the lack of role ownership, not the existence of it,
becauseI wrote the event trigger patch set to not depend on the role ownership patch set.  Once you have a concept of
roleownership, it is perfectly natural that the trigger could fire based on whether the trigger owner is the owner of
(orthe same as) the role performing the action.  That completely sidesteps the concern about the event trigger role
needingto be a member of any log-in role, because you no longer need the event trigger owner to be a member of the
log-inrole. 

There are semantic details to be worked out with role ownership, such as whether a role owner automatically has the
privilegesof roles it owns, whether such privilege, if any, should behave à la INHERIT or NOINHERIT, whether superusers
shouldown roles they create or whether there should be a special rule that superuser created roles should belong to the
bootstrapsuperuser, etc.  The patch set has taken a position on each of these, because it cannot be implemented without
somechoice being made, but many of these decisions could be changed if they are the source of confusion.  If, on the
otherhand, having parallel concepts "role A owns role B" and "role C is a member of role D" is too confusing for people
toever keep straight, then perhaps we need something like "tenant" to help lessen the confusion. 


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, 2021-10-27 at 16:10 -0400, Robert Haas wrote:
> But the thing that defines a tenant need not be a role. It can be
> some
> other kind of object. Suppose we invent a CREATE TENANT command. 

Would it be a recursive concept? Could a tenant create a sub-tenant?

Regards,
    Jeff Davis

On Thu, 2021-10-28 at 11:02 -0700, Mark Dilger wrote:
> It feels to me that the traditional concept of users and groups could
> map, one-to-one, onto users and roles, but we've mapped both users
> and groups, many-to-one, onto roles, leaving no distinct concept of
> groups, and now we're proposing adding a concept called "tenant" that
> means something like "group".  I find that simultaneously helpful and
> pretty confusing.

That's a good point. There are a lot of concepts involved; adding one
more could certainly cause confusion.

But I don't think the concept of role ownership has zero potential
confusion, either. For instance, I could certainly imagine a user A
creating a role B (and therefore owning it), and then doing "GRANT A TO
B". Is there a reason to do that, or is the user confused about what
membership versus ownership mean?

> Noah's concern, as I understood it, was not about roles owning roles,
> but about role membership being what controls if an event trigger
> fires.  If anything, that concern stems from the lack of role
> ownership, not the existence of it, because I wrote the event trigger
> patch set to not depend on the role ownership patch set.

Your patch[0] causes role membership to control whether and event
trigger fires. If it was solely based on role *ownership* and had
nothing to do with role *membership*, that does seem better to me.

[0] 
https://postgr.es/m/914FF898-5AC4-4E02-8A05-3876087007FB@enterprisedb.com

Regards,
    Jeff Davis

On Fri, Oct 29, 2021 at 6:56 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Wed, 2021-10-27 at 16:10 -0400, Robert Haas wrote:
> > But the thing that defines a tenant need not be a role. It can be
> > some
> > other kind of object. Suppose we invent a CREATE TENANT command.
>
> Would it be a recursive concept? Could a tenant create a sub-tenant?

I could imagine a system like this with or without that concept.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

> On Oct 29, 2021, at 4:46 PM, Jeff Davis <pgsql@j-davis.com> wrote:
>
> But I don't think the concept of role ownership has zero potential
> confusion, either. For instance, I could certainly imagine a user A
> creating a role B (and therefore owning it), and then doing "GRANT A TO
> B". Is there a reason to do that, or is the user confused about what
> membership versus ownership mean?

In general, I think that would be the result of the user being confused.  But it is hard to say that definitively,
becauseperhaps users A and C want to create a single user B with the union of both their roles, and have agreed to
perform:

user_a%  CREATE ROLE B;
user_a%  GRANT A TO B;
user_c%  GRANT C TO B;

The easiest way of thinking about role ownership is that a role's owner is superuser in so far as that role is
concerned. It can drop them, modify them, take their objects away from them, assign other objects to them, etc.
Anythinga superuser could do to impoverish them, their owner can do to impoverish them.  The difference is that an
actualsuperuser can enrich them with anything the superuser likes, whereas their owner can only enrich them with
objectsand privileges that the owner itself has rights to assign. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Oct 29, 2021, at 4:46 PM, Jeff Davis <pgsql@j-davis.com> wrote:
> > But I don't think the concept of role ownership has zero potential
> > confusion, either. For instance, I could certainly imagine a user A
> > creating a role B (and therefore owning it), and then doing "GRANT A TO
> > B". Is there a reason to do that, or is the user confused about what
> > membership versus ownership mean?
>
> In general, I think that would be the result of the user being confused.  But it is hard to say that definitively,
becauseperhaps users A and C want to create a single user B with the union of both their roles, and have agreed to
perform:
>
> user_a%  CREATE ROLE B;
> user_a%  GRANT A TO B;
> user_c%  GRANT C TO B;
>
> The easiest way of thinking about role ownership is that a role's owner is superuser in so far as that role is
concerned. It can drop them, modify them, take their objects away from them, assign other objects to them, etc.
Anythinga superuser could do to impoverish them, their owner can do to impoverish them.  The difference is that an
actualsuperuser can enrich them with anything the superuser likes, whereas their owner can only enrich them with
objectsand privileges that the owner itself has rights to assign. 

I can generally get behind the idea that a user who has been allowed to
create other roles should be able to do various other things with that
role, but should also be limited by what rights they themselves have
(unlike how CREATEROLE works today).

That said, I have a hard time seeing why we're drawing this distinction
of 'ownership' as being ultimately different from simple 'admin' rights
on a role.  In other words, beyond the ability to actually create/drop
roles, having 'admin' rights on a role already conveys just about
everything 'ownership' would.  The things that are getting in the way
there are:

 - Ability to actually create/alter/drop roles, this needs to be
   addressed somehow but doesn't necessarily imply a need for
   'ownership' as a concept.

 - Restriction of a role from being able to implicitly have 'admin'
   rights on itself, as I started a discussion about elsewhere.

 - Some system for deciding who event triggers should fire for.  I don't
   think this should really be tied into the question of who has admin
   rights on whom except to the extent that maybe you can only cause
   event triggers to fire for roles you've got admin rights on (or maybe
   membership in).

One thing that comes to mind is that event triggers aren't the only
thing out there and I have to wonder if we should be thinking about
other things.  As a thought exercise- how is an event trigger really
different from a table-level trigger?  Anyone who has the ability to
create objects is able to create tables, create functions, create
operators, and a user logging in and running SQL can certainly end up
running those things with their privileges.  We've generally recognized
that that's not great and there's been work to get it so that the
'public' schema that everyone has in their search_path by default won't
be world-writable but that isn't exactly a cure-all for the general
problem.

One of the interesting bits is that there's two sides to this.  On the
one hand, as a user, maybe I don't want to run functions of people who I
don't trust.  As an admin/superuser/landlord, maybe I want to require
everyone who I have authority over to run these functions/event
triggers.  I'm not sure that we can find a solution to everything with
this but figure I'd share these thoughts.

Last thought I'll share is that I do believe we're going to want to
provide flexibility when it comes to defining who event triggers run
for, as a given admin may wish for that set to be different from the set
of roles that they ultimately have control over.  I dislike tying these
two things together at such a core level and therefore continue to feel
that CREATE EVENT TRIGGER should be extended in some fashion to allow
individuals who can create them to specify who they are to run for.
Open to different ideas as to how a user could express that, but it
feels to me like that should be a core part of the definition of a
user-defined event trigger (ie: could be "FOR ALL ROLES I OWN" or
whatever, and maybe that's the default, but having that be the only
option isn't appealing).

Thanks,

Stephen

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Oct 25, 2021, at 11:30 AM, Stephen Frost <sfrost@snowman.net> wrote:
>
> > Consider instead:
> >
> > CREATE ROLE X;
> > CREATE ROLE Y;
> > CREATE ROLE Z;
> >
> > GRANT Y to X;
> > GRANT Z to X;
> >
> > SET ROLE X;
> > CREATE EVENT TRIGGER FOR Y do_stuff();
> >
> > Now, X has explicitly said that they want the event trigger to fire for
> > role Y and if the event trigger fires or not is no longer based on
> > membership in the role creating the trigger but instead depends on being
> > the role which the event trigger was explicitly defined to fire on.
>
> I don't think your proposal quite works, because the set of users you'd like to audit with an event trigger based
auditingsystem may be both large and dynamic: 
>
> CREATE ROLE batman;
> CREATE ROLE robin;
>
> SET ROLE batman;
> CREATE ROLE gotham_residents NOLOGIN;
> CREATE ROLE riddler IN ROLE gotham_residents LOGIN;
> -- create millions of other Gotham residents....
> CREATE EVENT TRIGGER FOR gotham_residents audit_criminal_activity();
>
> Batman is not superuser, but he's pretty powerful, and he wants to audit all the criminal activity in Gotham.  How
shouldhe expect this example to work? 
>
> Having the "FOR gotham_residents" clause mean anybody with membership in role gotham_residents is problematic,
becauseit means that being granted into a role both increases and decreases your freedoms, rather than merely giving
youmore freedoms.  If Batman covets privileges that Robin has, but wants not to be subjected to any event triggers that
firefor Robin, he both wants into and out of role Robin. 

The privileges afforded to 'robin' could be GRANT'd to another role
created for that purpose which is then GRANT'd to 'batman' though.
Indeed, that role could be used as the role which GRANT's 'robin' those
rights in the first place too.  This kind of permission management is
largely the point of the role-based system we have.

> Having "FOR gotham_residents" mean that only actions performed by role "gotham_residents" should fire the trigger is
useless,since Gotham residents don't log in as that, but as themselves.  Batman won't catch anybody this way. 

Naturally.  That doesn't mean that there couldn't be some other role
which all of those roles are made a member of though.  Either way,
there's a big list of "roles this event trigger runs for" and that has
to be managed.  That it happens to be "roles owned by batman", if we
went with your suggested approach, instead of other role membership
doesn't really 'fix' that because there'll be other roles in the system
which 'batman' doesn't own.  One nice thing of using roles for this is
that you end up being able to use the same role multiple ways- consider
this: I want to audit all roles who login to database mydb.  Ah-hah, now
I can say:

CREATE DATABASE mydb;
CREATE EVENT TRIGGER FOR gotham_residents audit_stuff();
REVOKE CONNECT ON DATABASE mydb FROM PUBLIC;
GRANT CONNECT ON DATABASE mydb TO gotham_residents;

Now the two are connected- if you can connect to that database, then
you're going to get audited, and if you manage access to the 'mydb'
database using membership in that role then there's no way for a role to
be able to connect to that database without being audited (except for a
true superuser, but that's always going to be an exception).

> Having to list each new resident to the trigger is tedious and error-prone.  Batman may not be able to pass a
complianceaudit. 

Agreed.  Also wouldn't be great since eventually the role list might
have to get TOAST'd and then you're doing an extra lookup to pull back
the list, yuck.

> Having Batman *own* all residents in Gotham city would work, if we can agree on a role ownership system.  It has the
downsidethat only a role's (direct or indirect) owner can do the auditing, though.  That's more flexible than what we
havetoday, where only superuser can do it, but maybe some people would want to argue for a different solution with even
moreflexibility?  A grantable privilege perhaps?  But whatever it is, the reasoning about who gets audited and who does
notmust be clear enough that Batman can pass a compliance audit. 

What about roles which Batman owns but which he *doesn't* want the event
trigger to fire for?

Note that event triggers are not strictly limited to the auditing case.
Viewing them through that lense masks other quite common use-cases which
are also importnat to consider (like preventing many users, but not all,
from being able to DROP objects as a clear example).

Thanks,

Stephen

> On Nov 1, 2021, at 12:44 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> I can generally get behind the idea that a user who has been allowed to
> create other roles should be able to do various other things with that
> role, but should also be limited by what rights they themselves have
> (unlike how CREATEROLE works today).

I intend to rearrange the role ownership patch set to have the 0004-Restrict-power-granted-via-CREATEROLE.patch come
before,and be independent of, the patches that introduce role ownership.  That would put the less controversial patch
first,and might get committed what we all agree. 

> That said, I have a hard time seeing why we're drawing this distinction
> of 'ownership' as being ultimately different from simple 'admin' rights
> on a role.  In other words, beyond the ability to actually create/drop
> roles, having 'admin' rights on a role already conveys just about
> everything 'ownership' would.  The things that are getting in the way
> there are:
>
> - Ability to actually create/alter/drop roles, this needs to be
>   addressed somehow but doesn't necessarily imply a need for
>   'ownership' as a concept.
>
> - Restriction of a role from being able to implicitly have 'admin'
>   rights on itself, as I started a discussion about elsewhere.
>
> - Some system for deciding who event triggers should fire for.  I don't
>   think this should really be tied into the question of who has admin
>   rights on whom except to the extent that maybe you can only cause
>   event triggers to fire for roles you've got admin rights on (or maybe
>   membership in).

You and I are not that far apart on this issue.  The reason I wanted to use "ownership" rather than ADMIN is that the
spechas a concept of ADMIN and I don't know that we can fix everything we want to fix and still be within compliance
withthe spec. 

> One thing that comes to mind is that event triggers aren't the only
> thing out there and I have to wonder if we should be thinking about
> other things.  As a thought exercise- how is an event trigger really
> different from a table-level trigger?  Anyone who has the ability to
> create objects is able to create tables, create functions, create
> operators, and a user logging in and running SQL can certainly end up
> running those things with their privileges.

The difference in my mind is that table triggers owned by non-superusers have been around for a long time and are in
heavyuse, so changing how that behaves is a huge backwards compatibility break.  Event triggers owned by non-superusers
areonly a fluke, not an intentional feature, and only occur when a superuser creates an event trigger and later has
superuserprivileges revoked.  We can expect that far fewer users are really depending on that to work compared with
tabletriggers. 

In a green field, I would not create table triggers to work as they do.

>  We've generally recognized
> that that's not great and there's been work to get it so that the
> 'public' schema that everyone has in their search_path by default won't
> be world-writable but that isn't exactly a cure-all for the general
> problem.

I agree.

> One of the interesting bits is that there's two sides to this.  On the
> one hand, as a user, maybe I don't want to run functions of people who I
> don't trust.  As an admin/superuser/landlord, maybe I want to require
> everyone who I have authority over to run these functions/event
> triggers.  I'm not sure that we can find a solution to everything with
> this but figure I'd share these thoughts.

If roles were not cluster-wide, I might not have such a problem with leaving things mostly as they are.  But there is
somethingreally objectionable to having two separate databases in a cluster intended for two separate purposes and with
twoseparate sets of roles, and the set of roles in one database can mess with the roles intended for the other
database. I think some kind of partitioning is needed, and I saw role ownership as the cleanest solution to it.  I
shareyour intuitions that perhaps the WITH ADMIN OPTION stuff could be used instead, but I don't see quite how. 

> Last thought I'll share is that I do believe we're going to want to
> provide flexibility when it comes to defining who event triggers run
> for, as a given admin may wish for that set to be different from the set
> of roles that they ultimately have control over.  I dislike tying these
> two things together at such a core level and therefore continue to feel
> that CREATE EVENT TRIGGER should be extended in some fashion to allow
> individuals who can create them to specify who they are to run for.

Within reason, sure.  It is fine by me if we support CREATE EVENT TRIGGER...AUTHORIZATION... in order to accomplish
that. But the role running that command still needs to be limited to just a (proper or otherwise) subset of their own
privileges.

I thought about this some when originally writing the event trigger patch.  The author of the event trigger is free to
adda preamble to the trigger that exits early if the user, time of day, phase of the moon, etc., are inappropriate per
thereasoning of the trigger author.  We only need the system to prevent the event trigger from casting too wide a net.
Theevent trigger author can limit the scope of the net further if desired. 

> Open to different ideas as to how a user could express that, but it
> feels to me like that should be a core part of the definition of a
> user-defined event trigger (ie: could be "FOR ALL ROLES I OWN" or
> whatever, and maybe that's the default, but having that be the only
> option isn't appealing).

I am not strongly against adding syntactic support for FOR ALL ROLES vs. FOR role, role, ..., so long as that syntax
cannotexpand the net.  It does seem a bit arbitrary to me, though, since you could also say FOR HOURS OF DAY 11PM
through3AM, and once you open the door to supporting all that in the syntax, and tracking it in the catalogs, you've
openeda can of worms. 


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Nov 1, 2021, at 1:13 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
>> Having Batman *own* all residents in Gotham city would work, if we can agree on a role ownership system.  It has the
downsidethat only a role's (direct or indirect) owner can do the auditing, though.  That's more flexible than what we
havetoday, where only superuser can do it, but maybe some people would want to argue for a different solution with even
moreflexibility?  A grantable privilege perhaps?  But whatever it is, the reasoning about who gets audited and who does
notmust be clear enough that Batman can pass a compliance audit. 
>
> What about roles which Batman owns but which he *doesn't* want the event
> trigger to fire for?

I think Batman just has the event trigger exit early for that.  There is nothing we can hardcode for filtering users
intoand out of the trigger that will be as flexible as the logic that Batman can implement in the trigger itself.  We
onlyneed to worry about Batman over stepping his authority.  It's not our job to filter further than that. 

> Note that event triggers are not strictly limited to the auditing case.
> Viewing them through that lense masks other quite common use-cases which
> are also importnat to consider (like preventing many users, but not all,
> from being able to DROP objects as a clear example).

Nothing in my proposal limits what superusers can do with event triggers they create.  The issue under discussion is
entirelyto do with what non-superusers are allowed to do with event triggers.  I see no reason why some ordinary role
"joe"should be allowed to thwart DROP commands issued on a table that "joe" doesn't own by roles that "joe" doesn't
own. Maybe "own" here should be "have ADMIN on", but it has to be something. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Nov 1, 2021, at 12:44 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > I can generally get behind the idea that a user who has been allowed to
> > create other roles should be able to do various other things with that
> > role, but should also be limited by what rights they themselves have
> > (unlike how CREATEROLE works today).
>
> I intend to rearrange the role ownership patch set to have the 0004-Restrict-power-granted-via-CREATEROLE.patch come
before,and be independent of, the patches that introduce role ownership.  That would put the less controversial patch
first,and might get committed what we all agree. 

I've not directly looked at that patch, but I like it based on the name,
if we think we can actually get folks to agree to is as it's quite a
change from the current situation.  Previously I've felt that we
wouldn't have support for breaking backwards compatibility in such a
manner but perhaps I'm wrong on that.  There's also something to be
said, in my view anyway, of having a predefined role be used for what we
want CREATEROLE to be rather than changing the existing CREATEROLE role
attribute.  Reason being that such a predefined role could then be
GRANT'd to some other role along with whatever other generally-relevant
privileges there are and then that GRANT'd to whomever should have those
rights.  That's not really possible with the current CREATEROLE role
attribute.

> > That said, I have a hard time seeing why we're drawing this distinction
> > of 'ownership' as being ultimately different from simple 'admin' rights
> > on a role.  In other words, beyond the ability to actually create/drop
> > roles, having 'admin' rights on a role already conveys just about
> > everything 'ownership' would.  The things that are getting in the way
> > there are:
> >
> > - Ability to actually create/alter/drop roles, this needs to be
> >   addressed somehow but doesn't necessarily imply a need for
> >   'ownership' as a concept.
> >
> > - Restriction of a role from being able to implicitly have 'admin'
> >   rights on itself, as I started a discussion about elsewhere.
> >
> > - Some system for deciding who event triggers should fire for.  I don't
> >   think this should really be tied into the question of who has admin
> >   rights on whom except to the extent that maybe you can only cause
> >   event triggers to fire for roles you've got admin rights on (or maybe
> >   membership in).
>
> You and I are not that far apart on this issue.  The reason I wanted to use "ownership" rather than ADMIN is that the
spechas a concept of ADMIN and I don't know that we can fix everything we want to fix and still be within compliance
withthe spec. 

There's no concept in the spec of event triggers, I don't believe
anyway, so I'm not really buying this particular argument.  Seems like
we'd be more likely to run afoul of some future spec by creating a
concept of role ownership and creating a definition around what that
means than using something existing in the spec as controlling what some
other not-in-spec thing does.

> > One thing that comes to mind is that event triggers aren't the only
> > thing out there and I have to wonder if we should be thinking about
> > other things.  As a thought exercise- how is an event trigger really
> > different from a table-level trigger?  Anyone who has the ability to
> > create objects is able to create tables, create functions, create
> > operators, and a user logging in and running SQL can certainly end up
> > running those things with their privileges.
>
> The difference in my mind is that table triggers owned by non-superusers have been around for a long time and are in
heavyuse, so changing how that behaves is a huge backwards compatibility break.  Event triggers owned by non-superusers
areonly a fluke, not an intentional feature, and only occur when a superuser creates an event trigger and later has
superuserprivileges revoked.  We can expect that far fewer users are really depending on that to work compared with
tabletriggers. 
>
> In a green field, I would not create table triggers to work as they do.

I don't think we're entirely beholden to having table-level triggers
work the way they do today.  I agree that we can't simply stop having
them fire for some users while letting things continue to happen on the
table but throwing an error and rolling back a transaction with an error
saying "you were about to run this trigger which runs this function with
your privileges and you don't trust the person who wrote it" seems
entirely within reason, were we to have such a concept.

> >  We've generally recognized
> > that that's not great and there's been work to get it so that the
> > 'public' schema that everyone has in their search_path by default won't
> > be world-writable but that isn't exactly a cure-all for the general
> > problem.
>
> I agree.
>
> > One of the interesting bits is that there's two sides to this.  On the
> > one hand, as a user, maybe I don't want to run functions of people who I
> > don't trust.  As an admin/superuser/landlord, maybe I want to require
> > everyone who I have authority over to run these functions/event
> > triggers.  I'm not sure that we can find a solution to everything with
> > this but figure I'd share these thoughts.
>
> If roles were not cluster-wide, I might not have such a problem with leaving things mostly as they are.  But there is
somethingreally objectionable to having two separate databases in a cluster intended for two separate purposes and with
twoseparate sets of roles, and the set of roles in one database can mess with the roles intended for the other
database. I think some kind of partitioning is needed, and I saw role ownership as the cleanest solution to it.  I
shareyour intuitions that perhaps the WITH ADMIN OPTION stuff could be used instead, but I don't see quite how. 

I agree that roles existing cluster-level is an issue in some instances
though I'm not quite sure what the concern here is (how could a role in
database A mess with roles in database B unless the first role had some
kind of access on those roles, in which case, what's the issue..?).

Another thread/patch under discussion is around role membership being
made to be able to be per-database, which could be pretty interesting,
though I don't think it directly helps with what you're suggesting
above, unfortunately.

> > Last thought I'll share is that I do believe we're going to want to
> > provide flexibility when it comes to defining who event triggers run
> > for, as a given admin may wish for that set to be different from the set
> > of roles that they ultimately have control over.  I dislike tying these
> > two things together at such a core level and therefore continue to feel
> > that CREATE EVENT TRIGGER should be extended in some fashion to allow
> > individuals who can create them to specify who they are to run for.
>
> Within reason, sure.  It is fine by me if we support CREATE EVENT TRIGGER...AUTHORIZATION... in order to accomplish
that. But the role running that command still needs to be limited to just a (proper or otherwise) subset of their own
privileges.
>
> I thought about this some when originally writing the event trigger patch.  The author of the event trigger is free
toadd a preamble to the trigger that exits early if the user, time of day, phase of the moon, etc., are inappropriate
perthe reasoning of the trigger author.  We only need the system to prevent the event trigger from casting too wide a
net. The event trigger author can limit the scope of the net further if desired. 

I don't know that all such event triggers will necessarily be able to be
modified by everyone who will want to use them in the way you're
suggesting.  Consider that there's things which require the event
trigger to be a C function as a simple example.

> > Open to different ideas as to how a user could express that, but it
> > feels to me like that should be a core part of the definition of a
> > user-defined event trigger (ie: could be "FOR ALL ROLES I OWN" or
> > whatever, and maybe that's the default, but having that be the only
> > option isn't appealing).
>
> I am not strongly against adding syntactic support for FOR ALL ROLES vs. FOR role, role, ..., so long as that syntax
cannotexpand the net.  It does seem a bit arbitrary to me, though, since you could also say FOR HOURS OF DAY 11PM
through3AM, and once you open the door to supporting all that in the syntax, and tracking it in the catalogs, you've
openeda can of worms. 

I disagree that it's a "can of worms" that one would be opening.  Sure,
folks can ask for all kinds of things and that's true today, but
ultimately we're the arbitrators of what is a sensible and common enough
use-case and what's not.  We seem to be in pretty clear agreement that
it's a sensible and reasonably common use-case for an event trigger
definer to wish for it to only be run for some subset of individuals and
that subset might not always be the exact subset of individuals that a
given role has 'ownership' or 'admin' rights over.  Your approach puts
the onus of limiting that on the trigger author, who might not even be
involved if it's some function that's provided from an extension and
written in C and distributed in a packaged form from PGDG.  There's also
no way to tie together privileges between who is allowed to do some
action and the individuals who the event trigger fires for, which seems
unfortuante to me.

Thanks,

Stephen

Greetings,

* Mark Dilger (mark.dilger@enterprisedb.com) wrote:
> > On Nov 1, 2021, at 1:13 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >> Having Batman *own* all residents in Gotham city would work, if we can agree on a role ownership system.  It has
thedownside that only a role's (direct or indirect) owner can do the auditing, though.  That's more flexible than what
wehave today, where only superuser can do it, but maybe some people would want to argue for a different solution with
evenmore flexibility?  A grantable privilege perhaps?  But whatever it is, the reasoning about who gets audited and who
doesnot must be clear enough that Batman can pass a compliance audit. 
> >
> > What about roles which Batman owns but which he *doesn't* want the event
> > trigger to fire for?
>
> I think Batman just has the event trigger exit early for that.  There is nothing we can hardcode for filtering users
intoand out of the trigger that will be as flexible as the logic that Batman can implement in the trigger itself.  We
onlyneed to worry about Batman over stepping his authority.  It's not our job to filter further than that. 

As noted in my other email you're likely currently reading, this
presumes that Batman is the author of the trigger and is able to make
such changes.  I'm also not thrilled with the presumption that, even if
batman is the author and maintainer, that batman would then have to
write in such exclusions for what strikes me as a pretty commonly wished
for use-case.

> > Note that event triggers are not strictly limited to the auditing case.
> > Viewing them through that lense masks other quite common use-cases which
> > are also importnat to consider (like preventing many users, but not all,
> > from being able to DROP objects as a clear example).
>
> Nothing in my proposal limits what superusers can do with event triggers they create.  The issue under discussion is
entirelyto do with what non-superusers are allowed to do with event triggers.  I see no reason why some ordinary role
"joe"should be allowed to thwart DROP commands issued on a table that "joe" doesn't own by roles that "joe" doesn't
own. Maybe "own" here should be "have ADMIN on", but it has to be something. 

I agree that we're talking about non-superuser event triggers.  I wasn't
suggesting that a non-superuser role 'joe' be able to create event
triggers that impact roles that 'joe' doesn't have rights of some kind
over.  I'm not quite following how your response here is addressing the
point that I brought up in what was quoted above it.

Thanks,

Stephen

> On Nov 1, 2021, at 2:00 PM, Stephen Frost <sfrost@snowman.net> wrote:

> I've not directly looked at that patch, but I like it based on the name,
> if we think we can actually get folks to agree to is as it's quite a
> change from the current situation.  Previously I've felt that we
> wouldn't have support for breaking backwards compatibility in such a
> manner but perhaps I'm wrong on that.

I am neutral on this.  I prefer not to break backward compatibility, but I also prefer to fix broken features rather
thanleave them as traps for the unwary.  The CREATEROLE attribute exists and is defined in a way that is broadly viewed
asa misfeature.  Fixing it has long term benefits, but short term compatibility concerns. 

> There's also something to be
> said, in my view anyway, of having a predefined role be used for what we
> want CREATEROLE to be rather than changing the existing CREATEROLE role
> attribute.

I don't see an additional benefit beyond preserving compatibility with how CREATEROLE has historically worked.

>  Reason being that such a predefined role could then be
> GRANT'd to some other role along with whatever other generally-relevant
> privileges there are and then that GRANT'd to whomever should have those
> rights.  That's not really possible with the current CREATEROLE role
> attribute.

I'm not seeing that.  If you create a role "role_admin" and give it CREATEROLE and other stuff, maybe CREATEDB,
pg_read_all_data,and so forth, then you grant "stephen" membership in "role_admin", doesn't that work?  Why would
"role_admin"being a member of some new role, say "pg_can_create_roles", work differently than "role_admin" having the
CREATEROLEattribute? 

>>> That said, I have a hard time seeing why we're drawing this distinction
>>> of 'ownership' as being ultimately different from simple 'admin' rights
>>> on a role.  In other words, beyond the ability to actually create/drop
>>> roles, having 'admin' rights on a role already conveys just about
>>> everything 'ownership' would.  The things that are getting in the way
>>> there are:
>>>
>>> - Ability to actually create/alter/drop roles, this needs to be
>>>  addressed somehow but doesn't necessarily imply a need for
>>>  'ownership' as a concept.
>>>
>>> - Restriction of a role from being able to implicitly have 'admin'
>>>  rights on itself, as I started a discussion about elsewhere.
>>>
>>> - Some system for deciding who event triggers should fire for.  I don't
>>>  think this should really be tied into the question of who has admin
>>>  rights on whom except to the extent that maybe you can only cause
>>>  event triggers to fire for roles you've got admin rights on (or maybe
>>>  membership in).
>>
>> You and I are not that far apart on this issue.  The reason I wanted to use "ownership" rather than ADMIN is that
thespec has a concept of ADMIN and I don't know that we can fix everything we want to fix and still be within
compliancewith the spec. 
>
> There's no concept in the spec of event triggers, I don't believe
> anyway, so I'm not really buying this particular argument.  Seems like
> we'd be more likely to run afoul of some future spec by creating a
> concept of role ownership and creating a definition around what that
> means than using something existing in the spec as controlling what some
> other not-in-spec thing does.

The WITH ADMIN OPTION feature has a really well defined meaning.  If you have ADMIN on a role, you can grant and revoke
thatrole to/from other roles.  That's it.  If we start tying a bunch of other stuff to that, we're breaking reasonable
expectationsabout how WITH ADMIN OPTION works, and since the spec defines how that works, we're then in violation of
thespec. 

CREATEROLE, on the other hand, has no defined meaning in the spec.  It's a postgres invention.  So if we change what it
means,we're not breaking compability with the spec, only backward compatibility with older version of postgres
vis-a-visthe CREATEROLE misfeature that most people presumably don't use.  I find that far preferable to breaking spec
compliance. It is strange to me that you view changing how WITH ADMIN OPTION functions as being motivated by spec
compliance,since I see it as going in the opposite direction. 

As you say above, we'd have to tie the ability to create, alter, and drop roles to the ADMIN option.  That already
soundslike a non-starter to me.  We'd further want to tie other stuff, like event triggers, to ADMIN option.  I don't
seehow this furthers spec compliance. 

Tying this stuff to CREATEROLE seems perfectly fair.  Nobody coming from another database vendor to postgres should
haveany spec-compatibility-based expectations about how CREATEROLE works. 

>>> One thing that comes to mind is that event triggers aren't the only
>>> thing out there and I have to wonder if we should be thinking about
>>> other things.  As a thought exercise- how is an event trigger really
>>> different from a table-level trigger?  Anyone who has the ability to
>>> create objects is able to create tables, create functions, create
>>> operators, and a user logging in and running SQL can certainly end up
>>> running those things with their privileges.
>>
>> The difference in my mind is that table triggers owned by non-superusers have been around for a long time and are in
heavyuse, so changing how that behaves is a huge backwards compatibility break.  Event triggers owned by non-superusers
areonly a fluke, not an intentional feature, and only occur when a superuser creates an event trigger and later has
superuserprivileges revoked.  We can expect that far fewer users are really depending on that to work compared with
tabletriggers. 
>>
>> In a green field, I would not create table triggers to work as they do.
>
> I don't think we're entirely beholden to having table-level triggers
> work the way they do today.  I agree that we can't simply stop having
> them fire for some users while letting things continue to happen on the
> table but throwing an error and rolling back a transaction with an error
> saying "you were about to run this trigger which runs this function with
> your privileges and you don't trust the person who wrote it" seems
> entirely within reason, were we to have such a concept.

You're pushing at an open door.  If the community doesn't object to fixing the security problems with table triggers,
I'mnot going to object either. 

>> If roles were not cluster-wide, I might not have such a problem with leaving things mostly as they are.  But there
issomething really objectionable to having two separate databases in a cluster intended for two separate purposes and
withtwo separate sets of roles, and the set of roles in one database can mess with the roles intended for the other
database. I think some kind of partitioning is needed, and I saw role ownership as the cleanest solution to it.  I
shareyour intuitions that perhaps the WITH ADMIN OPTION stuff could be used instead, but I don't see quite how. 
>
> I agree that roles existing cluster-level is an issue in some instances
> though I'm not quite sure what the concern here is (how could a role in
> database A mess with roles in database B unless the first role had some
> kind of access on those roles, in which case, what's the issue..?).

The problem is that superusers can act in any database, so role administration in database A must be done by a
non-superuserif you want that administrator to be unable to mess with the roles used in database B.  But what mechanism
dowe have for that?  WITH ADMIN OPTION is too narrow to do it, and I've already argued why I don't want to expand that
power,and CREATEROLE as currently implemented is too broad. 

> Another thread/patch under discussion is around role membership being
> made to be able to be per-database, which could be pretty interesting,
> though I don't think it directly helps with what you're suggesting
> above, unfortunately.

Yes, I took some interest in that conversation.  Like you, I'm not sure I see how it fixes the problems under
discussionhere. 

>>> Last thought I'll share is that I do believe we're going to want to
>>> provide flexibility when it comes to defining who event triggers run
>>> for, as a given admin may wish for that set to be different from the set
>>> of roles that they ultimately have control over.  I dislike tying these
>>> two things together at such a core level and therefore continue to feel
>>> that CREATE EVENT TRIGGER should be extended in some fashion to allow
>>> individuals who can create them to specify who they are to run for.
>>
>> Within reason, sure.  It is fine by me if we support CREATE EVENT TRIGGER...AUTHORIZATION... in order to accomplish
that. But the role running that command still needs to be limited to just a (proper or otherwise) subset of their own
privileges.
>>
>> I thought about this some when originally writing the event trigger patch.  The author of the event trigger is free
toadd a preamble to the trigger that exits early if the user, time of day, phase of the moon, etc., are inappropriate
perthe reasoning of the trigger author.  We only need the system to prevent the event trigger from casting too wide a
net. The event trigger author can limit the scope of the net further if desired. 
>
> I don't know that all such event triggers will necessarily be able to be
> modified by everyone who will want to use them in the way you're
> suggesting.  Consider that there's things which require the event
> trigger to be a C function as a simple example.

I don't care much about this.  You can implement that if you want, and I'm not going to have a reason to complain,
unlessit somehow allows people to cast too wide a net.  Narrowing the net is, to my mind, entirely orthogonal to this
discussion.

>>> Open to different ideas as to how a user could express that, but it
>>> feels to me like that should be a core part of the definition of a
>>> user-defined event trigger (ie: could be "FOR ALL ROLES I OWN" or
>>> whatever, and maybe that's the default, but having that be the only
>>> option isn't appealing).
>>
>> I am not strongly against adding syntactic support for FOR ALL ROLES vs. FOR role, role, ..., so long as that syntax
cannotexpand the net.  It does seem a bit arbitrary to me, though, since you could also say FOR HOURS OF DAY 11PM
through3AM, and once you open the door to supporting all that in the syntax, and tracking it in the catalogs, you've
openeda can of worms. 
>
> I disagree that it's a "can of worms" that one would be opening.  Sure,
> folks can ask for all kinds of things and that's true today, but
> ultimately we're the arbitrators of what is a sensible and common enough
> use-case and what's not.  We seem to be in pretty clear agreement that
> it's a sensible and reasonably common use-case for an event trigger
> definer to wish for it to only be run for some subset of individuals and
> that subset might not always be the exact subset of individuals that a
> given role has 'ownership' or 'admin' rights over.  Your approach puts
> the onus of limiting that on the trigger author, who might not even be
> involved if it's some function that's provided from an extension and
> written in C and distributed in a packaged form from PGDG.  There's also
> no way to tie together privileges between who is allowed to do some
> action and the individuals who the event trigger fires for, which seems
> unfortuante to me.

I understand why you want this, but I don't understand why you think it is tied to a security patch.  I'm not being
facetiouswhen I mention having syntax to support event triggers to fire only at certain times of day.  Plenty of
deploymentsI have encountered have exactly this type of restriction, limiting the time of day when certain sorts of
actionscan be performed.  Similarly, I have seen deployments which have their heaviest activity around the Christmas
shoppingseason.  They might want event triggers that fire between Black Friday and Boxing Day that prevent schema
changesduring such heavy load, but not the rest of the year.  And they might want them to fire for some roles and not
others.

The idea in the event trigger patch is to make it reasonable, from a security standpoint, to allow non-superusers to
createevent triggers.  The only thing that makes it *unreasonable* for them to do so is that an event trigger could
blockor alter actions performed by roles which the non-superuser trigger owner should not have the privilege to block
oralter.  So restrictions on when the event trigger fires to get around that problem seem on topic. Other filters, no
matterhow good the idea, are simply off topic.  Go and implement them with my blessing.  But I don't see why I should
haveto implement them as part of this patch set. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Nov 1, 2021, at 2:05 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> I'm not quite following how your response here is addressing the
> point that I brought up in what was quoted above it.

Could you clarify which question I didn't answer?  I fear I may have left something unanswered, but I don't know to
whatyou are referring. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company