Non-superuser subscription owners

Le mercredi 20 octobre 2021, 20:40:39 CEST Mark Dilger a écrit :

These patches have been split off the now deprecated monolithic "Delegating
superuser tasks to new security roles" thread at [1].

The purpose of these patches is to allow non-superuser subscription owners
without risk of them overwriting tables they lack privilege to write
directly. This both allows subscriptions to be managed by non-superusers,
and protects servers with subscriptions from malicious activity on the
publisher side.

Thank you Mark for splitting this.

This patch looks good to me, and provides both better security (by closing the
"dropping superuser role" loophole) and usefule features.

--
Ronan Dunklau

On 10/20/21 14:40, Mark Dilger wrote:

These patches have been split off the now deprecated monolithic "Delegating superuser tasks to new security roles" thread at [1].

The purpose of these patches is to allow non-superuser subscription owners without risk of them overwriting tables they lack privilege to write directly. This both allows subscriptions to be managed by non-superusers, and protects servers with subscriptions from malicious activity on the publisher side.

[1] /messages/by-id/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC@enterprisedb.com

These patches look good on their face. The code changes are very
straightforward.

w.r.t. this:

+   On the subscriber, the subscription owner's privileges are
re-checked for
+   each change record when applied, but beware that a change of
ownership for a
+   subscription may not be noticed immediately by the replication workers.
+   Changes made on the publisher may be applied on the subscriber as
+   the old owner.  In such cases, the old owner's privileges will be
the ones
+   that matter.  Worse still, it may be hard to predict when replication
+   workers will notice the new ownership.  Subscriptions created
disabled and
+   only enabled after ownership has been changed will not be subject to
this
+   race condition.

maybe we should disable the subscription before making such a change and
then re-enable it?

cheers

andrew

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

On Nov 1, 2021, at 7:18 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

w.r.t. this:

+   On the subscriber, the subscription owner's privileges are
re-checked for
+   each change record when applied, but beware that a change of
ownership for a
+   subscription may not be noticed immediately by the replication workers.
+   Changes made on the publisher may be applied on the subscriber as
+   the old owner.  In such cases, the old owner's privileges will be
the ones
+   that matter.  Worse still, it may be hard to predict when replication
+   workers will notice the new ownership.  Subscriptions created
disabled and
+   only enabled after ownership has been changed will not be subject to
this
+   race condition.

maybe we should disable the subscription before making such a change and
then re-enable it?

Right. I commented the code that way because there is a clear concern, but I was uncertain which way around the problem was best.

ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers. The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that. Disabling and enabling the subscription as part of the OWNER TO would not reliably accomplish anything.

The attached patch demonstrates the race condition. It sets up a publisher and subscriber, and toggles the subscription on and off on the subscriber node, interleaved with inserts and deletes on the publisher node. If the ALTER SUBSCRIPTION commands were synchronous, the test results would be deterministic, with only the inserts performed while the subscription is enabled being replicated, but because the ALTER commands are asynchronous, the results are nondeterministic.

It is unclear that I can make ALTER SUBSCRIPTION..OWNER TO synchronous without redesigning the way workers respond to pg_subscription catalog updates generally. That may be a good project to eventually tackle, but I don't see that it is more important to close the race condition in an OWNER TO than for a DISABLE.

Thoughts?

On Nov 1, 2021, at 10:58 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers. The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that. Disabling and enabling the subscription as part of the OWNER TO would not reliably accomplish anything.

Having discussed this with Andrew off-list, we've concluded that updating the documentation for logical replication to make this point more clear is probably sufficient, but I wonder if anyone thinks otherwise?

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

On Mon, Nov 1, 2021 at 6:44 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:

ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers. The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that. Disabling and enabling the subscription as part of the OWNER TO would not reliably accomplish anything.

Having discussed this with Andrew off-list, we've concluded that updating the documentation for logical replication to make this point more clear is probably sufficient, but I wonder if anyone thinks otherwise?

The question in my mind is whether there's some reasonable amount of
time that a user should expect to have to wait for the changes to take
effect. If it could easily happen that the old permissions are still
in use a month after the change is made, I think that's probably not
good. If there's reason to think that, barring unusual circumstances,
changes will be noticed within a few minutes, I think that's fine.

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

On Nov 1, 2021, at 10:58 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers. The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that. Disabling and enabling the subscription as part of the OWNER TO would not reliably accomplish anything.

I have rethought my prior analysis. The problem in the previous patch was that the subscription apply workers did not check for a change in ownership the way they checked for other changes, instead only picking up the new ownership information when the worker restarted for some other reason. This next patch set fixes that. The application of a change record may continue under the old ownership permissions when a concurrent command changes the ownership of the subscription, but the worker will pick up the new permissions before applying the next record. I think that is consistent enough with reasonable expectations.

The first two patches are virtually unchanged. The third updates the behavior of the apply workers, and updates the documentation to match.

On Mon, 2021-11-01 at 10:58 -0700, Mark Dilger wrote:

It is unclear that I can make ALTER SUBSCRIPTION..OWNER TO
synchronous without redesigning the way workers respond to
pg_subscription catalog updates generally. That may be a good
project to eventually tackle, but I don't see that it is more
important to close the race condition in an OWNER TO than for a
DISABLE.

Thoughts?

What if we just say that OWNER TO must be done by a superuser, changing
from one superuser to another, just like today? That would preserve
backwards compatibility, but people with non-superuser subscriptions
would need to drop/recreate them.

When we eventually do tackle the problem, we can lift the restriction.

Regards,
Jeff Davis

On Nov 16, 2021, at 10:08 AM, Jeff Davis <pgsql@j-davis.com> wrote:

On Mon, 2021-11-01 at 10:58 -0700, Mark Dilger wrote:

It is unclear .....

Thoughts?

What if we just say that OWNER TO must be done by a superuser, changing
from one superuser to another, just like today? That would preserve
backwards compatibility, but people with non-superuser subscriptions
would need to drop/recreate them.

The paragraph I wrote on 11/01 and you are responding to is no longer relevant. The patch submission on 11/03 tackled the problem. Have you had a chance to take a look at the new design?

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

On 11/3/21 15:50, Mark Dilger wrote:

On Nov 1, 2021, at 10:58 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers. The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that. Disabling and enabling the subscription as part of the OWNER TO would not reliably accomplish anything.

I have rethought my prior analysis. The problem in the previous patch was that the subscription apply workers did not check for a change in ownership the way they checked for other changes, instead only picking up the new ownership information when the worker restarted for some other reason. This next patch set fixes that. The application of a change record may continue under the old ownership permissions when a concurrent command changes the ownership of the subscription, but the worker will pick up the new permissions before applying the next record. I think that is consistent enough with reasonable expectations.

The first two patches are virtually unchanged. The third updates the behavior of the apply workers, and updates the documentation to match.

I'm generally happier about this than the previous patch set. With the
exception of some slight documentation modifications I think it's
basically committable. There doesn't seem to be a CF item for it but I'm
inclined to commit it in a couple of days time.

cheers

andrew

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

On Nov 16, 2021, at 12:06 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

There doesn't seem to be a CF item for it but I'm
inclined to commit it in a couple of days time.

https://commitfest.postgresql.org/36/3414/

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

On 11/16/21 15:08, Mark Dilger wrote:

On Nov 16, 2021, at 12:06 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

There doesn't seem to be a CF item for it but I'm
inclined to commit it in a couple of days time.

https://commitfest.postgresql.org/36/3414/

OK, got it, thanks.

cheers

andrew

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

On Wed, 2021-11-03 at 12:50 -0700, Mark Dilger wrote:

The first two patches are virtually unchanged. The third updates the
behavior of the apply workers, and updates the documentation to
match.

v2-0001 corrects some surprises, but may create others. Why is renaming
allowed, but not changing the options? What if we add new options, and
some of them seem benign for a non-superuser to change?

The commit message part of the patch says that it's to prevent non-
superusers from being able to (effectively) create subscriptions, but
don't we want privileged non-superusers to be able to create
subscriptions?

Regards,
Jeff Davis

On Nov 16, 2021, at 8:11 PM, Jeff Davis <pgsql@j-davis.com> wrote:

On Wed, 2021-11-03 at 12:50 -0700, Mark Dilger wrote:

The first two patches are virtually unchanged. The third updates the
behavior of the apply workers, and updates the documentation to
match.

v2-0001 corrects some surprises, but may create others. Why is renaming
allowed, but not changing the options? What if we add new options, and
some of them seem benign for a non-superuser to change?

The patch cannot anticipate which logical replication options may be added to the project in some later commit. We can let that commit adjust the behavior to allow the option if we agree it is sensible for non-superusers to do so.

The commit message part of the patch says that it's to prevent non-
superusers from being able to (effectively) create subscriptions, but
don't we want privileged non-superusers to be able to create
subscriptions?

Perhaps, but I don't think merely owning a subscription should entitle a role to create new subscriptions. Administrators may quite intentionally create low-power users, ones without access to anything but a single table, or a single schema, as a means of restricting the damage that a subscription might do (or more precisely, what the publisher might do via the subscription.) It would be surprising if that low-power user was then able to recreate the subscription into something different.

We should probably come back to this topic in a different patch, perhaps a patch that introduces a new pg_manage_subscriptions role or such.

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

On Wed, 2021-11-17 at 07:44 -0800, Mark Dilger wrote:

Administrators may quite
intentionally create low-power users, ones without access to anything
but a single table, or a single schema, as a means of restricting the
damage that a subscription might do (or more precisely, what the
publisher might do via the subscription.) It would be surprising if
that low-power user was then able to recreate the subscription into
something different.

I am still trying to understand this use case. It doesn't feel like
"ownership" to me, it feels more like some kind of delegation.

Is GRANT a better fit here? That would allow more than one user to
REFRESH, or ENABLE/DISABLE the same subscription. It wouldn't allow
RENAME, but I don't see why we'd separate privileges for
CREATE/DROP/RENAME anyway.

This would not address the weirdness of the existing code where a
superuser loses their superuser privileges but still owns a
subscription. But perhaps we can solve that a different way, like just
performing a check when someone loses their superuser privileges that
they don't own any subscriptions.

Regards,
Jeff Davis

On Nov 17, 2021, at 9:33 AM, Jeff Davis <pgsql@j-davis.com> wrote:

I am still trying to understand this use case. It doesn't feel like
"ownership" to me, it feels more like some kind of delegation.

Is GRANT a better fit here? That would allow more than one user to
REFRESH, or ENABLE/DISABLE the same subscription. It wouldn't allow
RENAME, but I don't see why we'd separate privileges for
CREATE/DROP/RENAME anyway.

We may eventually allow non-superusers to create subscriptions, but there are lots of details to work out. Should there be limits on how many subscriptions they can create? Should there be limits to the number of simultaneously open connections they can create out to other database servers (publishers)? Should they need to be granted USAGE on a database publisher in order to use the connection string for that publisher in a subscription they create? Should they need to be granted USAGE on a publication in order to replicate it? Yes, there may be restrictions on the publisher side, too, but the user model on subscriber and publisher might differ, and the connection string used might not match the subscription owner, so some restriction on the subscriber side may be needed.

The implementation of [CREATE | ALTER] SUBSCRIPTION was designed at a time when only superusers could execute them, and as far as I can infer from the design, no effort to constrain the effects of those commands was made. Since we're trying to make subscriptions into things that non-superusers can use, we have to deal with some things in those functions. For example, ALTER SUBSCRIPTION can change the database connection parameter, or the publication subscribed, or whether synchronous_commit is used. I don't see that a subscription owner should necessarily be allowed to mess with that, at least not without some other privilege checks beyond mere ownership.

I think this is pretty analogous to how security definer functions work. You might call those "delegation" also, but the basic idea is that the function will run under the privileges of the function's owner, who might be quite privileged if you want the function to do highly secure things for you, but who could also intentionally be limited in privilege. It wouldn't make much sense to say the owner of a security definer function can arbitrarily escalate their privileges to do things like open connections to other database servers, or have the transactions in which they run have a different setting of synchronous_commit. Yet with subscriptions, if the subscription owner can run all forms of ALTER SUBSCRIPTION, that's what they can do.

I took a conservative position in the design of the patch to avoid giving away too much. I suspect that we'll come back to these design decisions and relax them at some point, but the exact way in which we relax them is not obvious. We could just agree to remove them (as you seem to propose), or we might agree to create predefined roles and say that the subscription owner can change certain aspects of the subscription if and only if they are members of one or more of those roles, or we may create new grantable privileges. Each of those debates may be long and hard fought, so I don't want to invite that as part of this thread, or this patch will almost surely miss the cutoff for v15.

This would not address the weirdness of the existing code where a
superuser loses their superuser privileges but still owns a
subscription. But perhaps we can solve that a different way, like just
performing a check when someone loses their superuser privileges that
they don't own any subscriptions.

I gave that a slight amount of thought during the design of this patch, but didn't think we could refuse to revoke superuser on such a basis, and didn't see what we should do with the subscription other than have it continue to be owned by the recently-non-superuser. If you have a better idea, we can discuss it, but to some degree I think that is also orthogonal to the purpose of this patch. The only sense in which this patch depends on that issue is that this patch proposes that non-superuser subscription owners are already an issue, and therefore that this patch isn't creating a new issue, but rather making more sane something that already can happen.

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

On Nov 17, 2021, at 9:33 AM, Jeff Davis <pgsql@j-davis.com> wrote:

Is GRANT a better fit here? That would allow more than one user to
REFRESH, or ENABLE/DISABLE the same subscription. It wouldn't allow
RENAME, but I don't see why we'd separate privileges for
CREATE/DROP/RENAME anyway.

I don't think I answered this directly in my last reply.

GRANT *might* be part of some solution, but it is unclear to me how best to do it. The various configuration parameters on subscriptions entail different security concerns. We might take a fine-grained approach and create a predefined role for each, or we might take a course-grained approach and create a single pg_manage_subscriptions role which can set any parameter on any subscription, or maybe just parameters on subscriptions that the role also owns, or we might do something else, like burn some privilege bits and define new privileges that can be granted per subscription rather than globally. (I think that last one is a non-starter, but just mention it as an example of another approach.)

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

On 11/16/21 15:06, Andrew Dunstan wrote:

On 11/3/21 15:50, Mark Dilger wrote:

On Nov 1, 2021, at 10:58 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers. The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that. Disabling and enabling the subscription as part of the OWNER TO would not reliably accomplish anything.

I have rethought my prior analysis. The problem in the previous patch was that the subscription apply workers did not check for a change in ownership the way they checked for other changes, instead only picking up the new ownership information when the worker restarted for some other reason. This next patch set fixes that. The application of a change record may continue under the old ownership permissions when a concurrent command changes the ownership of the subscription, but the worker will pick up the new permissions before applying the next record. I think that is consistent enough with reasonable expectations.

The first two patches are virtually unchanged. The third updates the behavior of the apply workers, and updates the documentation to match.

I'm generally happier about this than the previous patch set. With the
exception of some slight documentation modifications I think it's
basically committable. There doesn't seem to be a CF item for it but I'm
inclined to commit it in a couple of days time.

Given there is some debate about the patch set I will hold off any
action for the time being.

cheers

andrew

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

On Wed, 2021-11-17 at 10:25 -0800, Mark Dilger wrote:

We may eventually allow non-superusers to create subscriptions, but
there are lots of details to work out.

I am setting aside the idea of subscriptions created by non-superusers.

My comments were about your idea for "low-power users" that can still
do things with subscriptions. And for that, GRANT seems like a better
fit than ownership.

With v2-0001, there are several things that seem weird to me:

* Why can there be only one low-power user per subscription?
* Why is RENAME a separate capability from CREATE/DROP?
* What if you want to make the privileges more fine-grained, or make
changes in the future? Ownership is a single bit, so it requires that
everyone agree. Maybe some people want RENAME to be a part of that, and
others don't.

GRANT seems to provide better answers here.

Since we're trying to make subscriptions into things that non-
superusers can use, we have to deal with some things in those
functions.

I understand the use case where a superuser isn't required anywhere in
the process, and some special users can create and own subscriptions. I
also understand that's not what these patches are trying to accomplish
(though v2-0003 seems like a good step in that direction).

I don't understand the use case as well where a non-superuser can
merely "use" a subscription. I'm sure such use cases exist and I'm fine
to go along with that idea, but I'd like to understand why ownership
(partial ownership?) is the right way to do this and GRANT is the wrong
way.

For example, ALTER SUBSCRIPTION can change the database connection
parameter, or the publication subscribed, or whether
synchronous_commit is used. I don't see that a subscription owner
should necessarily be allowed to mess with that, at least not without
some other privilege checks beyond mere ownership.

That violates my expectations of what "ownership" means.

I think this is pretty analogous to how security definer functions
work.

The analogy to SECURITY DEFINER functions seems to support my
suggestion for GRANT at least as much as your modified definition of
ownership.

This would not address the weirdness of the existing code where a
superuser loses their superuser privileges but still owns a
subscription. But perhaps we can solve that a different way, like
just
performing a check when someone loses their superuser privileges
that
they don't own any subscriptions.

I gave that a slight amount of thought during the design of this
patch, but didn't think we could refuse to revoke superuser on such a
basis,

I don't necessarily see a problem there, but I could be missing
something.

and didn't see what we should do with the subscription other than
have it continue to be owned by the recently-non-superuser. If you
have a better idea, we can discuss it, but to some degree I think
that is also orthogonal to the purpose of this patch. The only sense
in which this patch depends on that issue is that this patch proposes
that non-superuser subscription owners are already an issue, and
therefore that this patch isn't creating a new issue, but rather
making more sane something that already can happen.

By introducing and documenting a way to get non-superusers to own a
subscription, it makes it more likely that people will do it, and
harder for us to change. That means the standard should be "this is
what we really want", rather than just "more sane than before".

Regards,
Jeff Davis

On Wed, 2021-11-17 at 10:48 -0800, Mark Dilger wrote:

GRANT *might* be part of some solution, but it is unclear to me how
best to do it. The various configuration parameters on subscriptions
entail different security concerns. We might take a fine-grained
approach and create a predefined role for each

I think you misunderstood the idea: not using predefined roles, just
plain old ordinary GRANT on a subscription object to ordinary roles.

GRANT REFRESH ON SUBSCRIPTION sub1 TO nonsuper;

This should be easy enough because the subscription is a real object,
right?

Regards,
Jeff Davis