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

On Nov 17, 2021, at 1:10 PM, Jeff Davis <pgsql@j-davis.com> wrote:

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?

/*
* Grantable rights are encoded so that we can OR them together in a bitmask.
* The present representation of AclItem limits us to 16 distinct rights,
* even though AclMode is defined as uint32. See utils/acl.h.
*
* Caution: changing these codes breaks stored ACLs, hence forces initdb.
*/
typedef uint32 AclMode; /* a bitmask of privilege bits */

#define ACL_INSERT (1<<0) /* for relations */
#define ACL_SELECT (1<<1)
#define ACL_UPDATE (1<<2)
#define ACL_DELETE (1<<3)
#define ACL_TRUNCATE (1<<4)
#define ACL_REFERENCES (1<<5)
#define ACL_TRIGGER (1<<6)
#define ACL_EXECUTE (1<<7) /* for functions */
#define ACL_USAGE (1<<8) /* for languages, namespaces, FDWs, and
* servers */
#define ACL_CREATE (1<<9) /* for namespaces and databases */
#define ACL_CREATE_TEMP (1<<10) /* for databases */
#define ACL_CONNECT (1<<11) /* for databases */

We only have 4 values left in the bitmask, and I doubt that burning those slots for multiple new types of rights that only have meaning for subscriptions is going to be accepted. For full disclosure, I'm proposing adding ACL_SET and ACL_ALTER_SYSTEM in another patch and my proposal there could get shot down for the same reasons, but I think your argument would be even harder to defend. Maybe others feel differently.

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

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

We only have 4 values left in the bitmask, and I doubt that burning
those slots for multiple new types of rights that only have meaning
for subscriptions is going to be accepted. For full disclosure, I'm
proposing adding ACL_SET and ACL_ALTER_SYSTEM in another patch and my
proposal there could get shot down for the same reasons, but I think
your argument would be even harder to defend. Maybe others feel
differently.

Why not overload ACL_USAGE again, and say:

GRANT USAGE ON SUBSCRIPTION sub1 TO nonsuper;

would allow ENABLE/DISABLE and REFRESH.

Again, I don't really understand the use case behind "can use a
subscription but not create one", so I'm not making a proposal. But
assuming that the use case exists, GRANT seems like a much better
approach.

(Aside: for me to commit something like this I'd want to understand the
"can use a subscription but not create one" use case better.)

Regards,
Jeff Davis

On Nov 17, 2021, at 1:06 PM, Jeff Davis <pgsql@j-davis.com> wrote:

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.

Ok, fair enough. I think eventually we'll want that, but I'm also setting that aside for this patch.

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.

This patch has basically no value beyond the fact that it allows the replication to be *applied* as a user other than superuser. Throw that out, and there isn't any point. Everything else is window dressing. The real security problem with subscriptions is that they act with superuser power. That naturally means that they must be owned and operated by superuser, too, otherwise they serve as a privilege escalation attack vector. It really doesn't make any sense to think of subscriptions as operating under the permissions of multiple non-superusers. You must choose a single role you want the subscription to run under. What purpose would be served by GRANTing privileges on a subscription to more than one non-superuser? It still operates as just the one user. I agree you *could* give multiple users privileges to mess with it, but you'd still need to assign a single role as the one whose privileges matter for the purpose of applying replication changes. I'm using "owner" for that purpose, and I think that is consistent with how security definer functions work. They run as the owner, too. It's perfectly well-precedented to use "owner" for this.

I think the longer term plan is that non-superusers who have some privileged role will be allowed to create subscriptions, and naturally they will own the subscriptions that they create, at least until an ALTER SUBSCRIPTION..OWNER TO is successfully executed to transfer ownership. Once that longer term plan is complete, non-superusers will be able to create publications of their tables on one database, and subscriptions to those publications on another database, all without needing the help of a superuser. This patch doesn't get us all the way there, but it heads directly toward that goal.

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

* Why can there be only one low-power user per subscription?

Because the apply workers run as only one user. Currently it is always superuser. After this patch, it is always the owner, which amounts to the same thing for legacy subscriptions created and owned by superuser prior to upgrading to v15, but not necessarily for new ones or ones that have ownership transferred after upgrade.

We could think about subscriptions that act under multiple roles, perhaps taking role information as part of the data-stream from the publisher, but that's a pretty complicated proposal, and it is far from clear that we want it anyway. There is a security case to be made for *not* allowing the publisher to call all the shots, so such a proposal would at best be an alternate mode of operation, not the one and only mode.

* Why is RENAME a separate capability from CREATE/DROP?

I don't care enough to argue this point. If you want me to remove RENAME privilege from the owner, I can resubmit with it removed. It just doesn't seem like it's dangerous to allow a non-superuser to rename their subscriptions, so I saw no compelling reason to disallow it.

CREATE clearly must be disallowed since it gives the creator the ability to form network connections, set fsync modes, etc., and there is no reason to assume arbitrary non-superusers should be able to do that.

The argument against DROP is a bit weaker. It doesn't seem like a user who cannot bring subscriptions into existence should be able to drop them either. I was expecting to visit that issue in a follow-on patch which deals with non-superuser predefined roles that have some power to create and drop subscriptions. What that patch will propose to do is not obvious, since some of what you can do with subscriptions is so powerful we may not want non-superusers doing it, even with a privileged role. If you can't picture what I mean, consider that you might use a connection parameter that connects outside and embeds data into the connection string, with a server listening on the other end, not really to publish data, but to harvest the secret data that you are embedding in the network connection attempt.

* 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.

We can modify the patch to have the subscription owner have zero privileges on the subscription, not even the ability to see how it is defined, and just have "owner" mean the role under whose privileges the logical replication workers apply changes. Would that be better? I would expect people to find that odd.

The problem is that we want a setuid/setgid type behavior. Actual setuid/setgid programs act as the user/group of the executable. There's no reason that user/group needs to be one that any real human uses to log into the system. Likewise, we need the subscription to act under a role, and we're establishing which role that is by having that role own the subscription. That is like how setuid/setgid programs work by executing as the user/group that owns the executable, except that postgres doesn't have separate user/group concepts, just roles. Isn't this design pattern completely commonplace?

Maybe some people want RENAME to be a part of that, and
others don't.

Fair enough. Should I remove RENAME from what the patch allows the owner to do? On this particular point, I genuinely don't care. I think it can be reasonably argued either way.

GRANT seems to provide better answers here.

No, because we don't have infinite privilege bits to burn.

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).

There is a cart-before-the-horse problem here. If I propose a patch with a privileged role for creating and owning subscriptions *before* I tighten down how non-superuser-owned subscriptions work, that patch would surely be rejected. So I either propose this first, and only if/when it gets accepted, propose the other, or I propose them together. That's a damned-if-you-do--damned-if-you-dont situation, because if I propose them together, I'll get arguments that they are clearly separable and should be proposed separately, and if I do them one before the other, I'll get the argument that you are making now. I fully expect the privileged role proposal to be made (possibly by me), though it is unclear if there will be time left to do it in v15.

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.

Even if we had the privilege bits to burn, no spelling of that GRANT idea sounds all that great:

GRANT RUN AS ON subscription TO role;
GRANT RUN AS ON role TO subscription;
GRANT SUDO ON subscription TO role;
GRANT SETUID ON role TO subscription;
...

I just don't see how that really works. I'm not inclined to spend time being more clever, since I already know that privilege bits are in short supply, but if you want to propose something, go ahead. Elsewhere you proposed GRANT REFRESH or something, not looking at that email just now, but that's not the same thing as GRANT RUN AS, and burns another privilege bit, and still doesn't get us all the way there, because you presumably also want GRANT RENAME, GRANT ALTER CONNECTION SETTING, GRANT ALTER FSYNC SETTING, ..., and we're out of privilege bits before we're done.

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 that's because you're thinking of these settings as properties of the subscription. You may *own* the subscription, but the subscription doesn't *own* the right to make connections to arbitrary databases, nor *own* the right to change buffer cache settings, nor *own* the right to bring data from a publication on some other server which, if it existed on the local server, would violate site policy and possibly constitute a civil or criminal violation of data privacy laws. I may own my house, and the land it sits on, and my driveway, but that doesn't mean I own the ability to make my driveway go across my neighbor's field, down through town, and to the waterfront. But that's the kind of ownership definition you seem to be defending.

Some of what I perceive as the screwiness of your argument I must admin is not your fault. The properties of subscriptions are defined in ways that don't make sense to me. It would be far more sensible if connection strings were objects in their own right, and you could grant USAGE on a connection string to a role, and USAGE on a subscription to a role, and only if the subscription worker's role had privileges on the connection string could they use it as part of fulfilling their task of replicating the data, and otherwise they'd error out in the attempt. Likewise, fsync modes could be proper objects, and only if the subscription's role had privileges on the fsync mode they wanted to use would they be able to use it. But we don't have these things as proper objects, with acl lists on them, so we're stuck trying to design around that. To my mind, that means subscription owners *do not own* properties associated with the subscription. To your mind, that's not what "ownership" means. What to do?

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.

I don't see how. Can you please explain?

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.

Close your eyes and imagine that I have superuser on your database... really picture it in your mind. Now, do you want the revoke command you are issuing to work?

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".

Ok, I'll wait to hear back from you on the points above.

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

On Wed, Nov 17, 2021 at 11:56 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

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

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.

Don't we want to close this gap irrespective of the other part of the
feature? I mean if we take out the part of your 0003 patch that checks
whether the current user has permission to perform a particular
operation on the target table then the gap related to the owner losing
superuser privileges should be addressed.

--
With Regards,
Amit Kapila.

On Thu, Nov 4, 2021 at 1:20 AM Mark Dilger <mark.dilger@enterprisedb.com> 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.

Are you talking about the below change in the above paragraph?

@@ -2912,6 +2941,7 @@ maybe_reread_subscription(void)
  strcmp(newsub->slotname, MySubscription->slotname) != 0 ||
  newsub->binary != MySubscription->binary ||
  newsub->stream != MySubscription->stream ||
+ newsub->owner != MySubscription->owner ||
  !equal(newsub->publications, MySubscription->publications))
  {

If so, I am not sure how it will ensure that we check the ownership
change before applying each change? I think this will be invoked at
each transaction boundary, so, if there is a transaction with a large
number of changes, all the changes will be processed under the
previous owner.

--
With Regards,
Amit Kapila.

On Nov 18, 2021, at 2:50 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

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.

Don't we want to close this gap irrespective of the other part of the
feature? I mean if we take out the part of your 0003 patch that checks
whether the current user has permission to perform a particular
operation on the target table then the gap related to the owner losing
superuser privileges should be addressed.

I don't think there is a gap. The patch does the right thing, causing the subscription whose owner has had superuser revoked to itself no longer function with superuser privileges. Whether that causes the subscription to fail depends on whether the previously-superuser now non-superuser owner now lacks sufficient privileges on the target relation(s). I think removing that part of the patch would be a regression.

Let's compare two scenarios. In the first, we have a regular user "alice" who owns a subscription which replicates into table "accounting.receipts" for which she has been granted privileges by the table's owner. What would you expect to happen after the table's owner revokes privileges from alice? I would expect that the subscription can no longer function, and periodic attempts to replicate into that table result in permission denied errors in the logs.

In the second, we have a superuser "alice" who owns a subscription that replicates into table "accounting.receipts", and she only has sufficient privileges to modify "accounting.receipts" by virtue of being superuser. I would expect that when she has superuser revoked, the subscription can likewise no longer function.

Now, maybe I'm wrong in both cases, and both should continue to function. But I would find it really strange if the first situation behaved differently from the second.

I think intuitions about how subscriptions behave differ depending on the reason you expect the subscription to be owned by a particular user. If the reason the user owns the subscription is that the user just happens to be the user who created it, but isn't in your mind associated with the subscription, then having the subscription continue to function regardless of what happens to the user, even the user being dropped, is probably consistent with your expectations. In a sense, you think of the user who creates the subscription as having gifted it to the universe rather than continuing to own it. Or perhaps you think of the creator of the subscription as a solicitor/lawyer/agent working on behalf of client, and once that legal transaction is completed, you don't expect the lawyer being disbarred should impact the subscription which exists for the benefit of the client.

If instead you think about the subscription owner as continuing to be closely associated with the subscription (as I do), then you expect changes in the owner's permissions to impact the subscription.

I think the "gifted to the universe"/"lawyer" mental model is not consistent with how the system is already designed to work. You can't drop the subscription's owner without first running REASSIGN OWNED, or ALTER SUBSCRIPTION..OWNER TO, or simply dropping the subscription:

DROP ROLE regress_subscription_user;
ERROR: role "regress_subscription_user" cannot be dropped because some objects depend on it
DETAIL: owner of subscription regress_testsub

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

On Nov 18, 2021, at 3:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

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.

Are you talking about the below change in the above paragraph?

@@ -2912,6 +2941,7 @@ maybe_reread_subscription(void)
strcmp(newsub->slotname, MySubscription->slotname) != 0 ||
newsub->binary != MySubscription->binary ||
newsub->stream != MySubscription->stream ||
+ newsub->owner != MySubscription->owner ||
!equal(newsub->publications, MySubscription->publications))
{

If so, I am not sure how it will ensure that we check the ownership
change before applying each change? I think this will be invoked at
each transaction boundary, so, if there is a transaction with a large
number of changes, all the changes will be processed under the
previous owner.

Yes, your analysis appears correct. I was sloppy to say "before applying the next record". It will pick up the change before applying the next transaction.

The prior version of the patch only picked up the change if it happened to start a new worker, but could process multiple transactions without noticing the change. Now, it is limited to finishing the current transaction. Would you prefer that the worker noticed the change in ownership and aborted the transaction on the subscriber side? Or should the ALTER SUBSCRIPTION..OWNER TO block? I don't see much advantage to either of those options, but I also don't think I have any knock-down argument for my approach either. What do you think?

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

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

You must choose a single role you want the subscription to run
under.

I think that was the source of a lot of my confusion:

Your patches are creating the notion of a "run as" user by assigning
ownership-that-isn't-really-ownership. I got distracted wondering why
you would really care if some user could enable/disable/refresh/rename
a subscription, but the main point was to change who the subscription
runs as.

That's a more general idea: I could see how "run as" could apply to
subscriptions as well as functions (right now it can only run as the
owner or the invoker, not an arbitrary role). And I better understand
your analogy to security definer now.

But it's also not exactly a simple idea, and I think the current
patches oversimplify it and conflate it with ownership.

I think the longer term plan is that non-superusers who have some
privileged role will be allowed to create subscriptions,

You earlier listed some challenges with that:

/messages/by-id/CF56AC0D-7495-4E8D-A48F-FF38BD8074EB@enterprisedb.com

But it seems like it's really the right direction to go. Probably the
biggest concern is connection strings that read server files, but
dblink solved that by requiring password auth.

What are the reasonable steps to get there? Do you think anything is
doable for v15?

There is a cart-before-the-horse problem here.

I don't think we need to hold up v2-0003. It seems like a step in the
right direction, though I haven't looked closely yet.

Regards,
Jeff Davis

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

Some of what I perceive as the screwiness of your argument I must
admin is not your fault. The properties of subscriptions are defined
in ways that don't make sense to me. It would be far more sensible
if connection strings were objects in their own right, and you could
grant USAGE on a connection string to a role,

We sort of have that with CREATE SERVER, in fact dblink can use a
server instead of a string.

Regards,
Jeff Davis

On Thu, Nov 18, 2021 at 9:03 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Nov 18, 2021, at 2:50 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

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.

Don't we want to close this gap irrespective of the other part of the
feature? I mean if we take out the part of your 0003 patch that checks
whether the current user has permission to perform a particular
operation on the target table then the gap related to the owner losing
superuser privileges should be addressed.

I don't think there is a gap. The patch does the right thing, causing the subscription whose owner has had superuser revoked to itself no longer function with superuser privileges. Whether that causes the subscription to fail depends on whether the previously-superuser now non-superuser owner now lacks sufficient privileges on the target relation(s). I think removing that part of the patch would be a regression.

I think we are saying the same thing. I intend to say that your 0003*
patch closes the current gap in the code and we should consider
applying it irrespective of what we do with respect to changing the
... OWNER TO .. behavior. Is there a reason why 0003* patch (or
something on those lines) shouldn't be considered to be applied?

--
With Regards,
Amit Kapila.

On Thu, Nov 18, 2021 at 9:15 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

The prior version of the patch only picked up the change if it happened to start a new worker, but could process multiple transactions without noticing the change. Now, it is limited to finishing the current transaction. Would you prefer that the worker noticed the change in ownership and aborted the transaction on the subscriber side? Or should the ALTER SUBSCRIPTION..OWNER TO block? I don't see much advantage to either of those options, but I also don't think I have any knock-down argument for my approach either. What do you think?

How about allowing to change ownership only for disabled
subscriptions? Basically, users need to first disable the subscription
and then change its ownership. Now, disabling is an asynchronous
operation but we won't allow the ownership change command to proceed
unless the subscription is marked disabled and all the apply/sync
workers are not running. After the ownership is changed, users can
enable it. We already have 'slot_name' parameter's dependency on
whether the subscription is marked enabled or not.

This will add some steps in changing the ownership of a subscription
but I think it will be predictable.

--
With Regards,
Amit Kapila.

On Fri, Nov 19, 2021 at 12:00 AM Jeff Davis <pgsql@j-davis.com> wrote:

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

You must choose a single role you want the subscription to run
under.

I think that was the source of a lot of my confusion:

Your patches are creating the notion of a "run as" user by assigning
ownership-that-isn't-really-ownership. I got distracted wondering why
you would really care if some user could enable/disable/refresh/rename
a subscription, but the main point was to change who the subscription
runs as.

That's a more general idea: I could see how "run as" could apply to
subscriptions as well as functions (right now it can only run as the
owner or the invoker, not an arbitrary role). And I better understand
your analogy to security definer now.

I was thinking why not separate the ownership and "run as" privileges
for the subscriptions? We can introduce a new syntax in addition to
the current syntax for "Owner" for this as:

Create Subscription sub RUNAS <role_name> ...;
Alter Subscription sub RUNAS <role_name>

Now, RUNAS role will be used to apply changes and perform the initial
table sync. The owner will be tied to changing some of the other
properties (enabling, disabling, etc.) of the subscription. Now, we
still need a superuser to create subscription and change properties
like CONNECTION for a subscription but we can solve that by having
roles specific to it as being indicated by Mark in some of his
previous emails.

--
With Regards,
Amit Kapila.

On Nov 19, 2021, at 1:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I think we are saying the same thing. I intend to say that your 0003*
patch closes the current gap in the code and we should consider
applying it irrespective of what we do with respect to changing the
... OWNER TO .. behavior. Is there a reason why 0003* patch (or
something on those lines) shouldn't be considered to be applied?

Jeff Davis and I had a long conversation off-list yesterday and reached the same conclusion. I will be submitting a version of 0003 which does not depend on the prior two patches.

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

On Nov 19, 2021, at 1:56 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

How about allowing to change ownership only for disabled
subscriptions? Basically, users need to first disable the subscription
and then change its ownership.

There are some open issues about non-superuser owners that Jeff would like to address before allowing transfers of ownership to non-superusers. Your proposal about requiring the subscription to be disabled seems reasonable to me, but I'd like to see how it would interact with whatever Jeff proposes. So I think I will change the patch as you suggest, but consider it a WIP patch until then.

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

On Nov 19, 2021, at 2:23 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I was thinking why not separate the ownership and "run as" privileges
for the subscriptions? We can introduce a new syntax in addition to
the current syntax for "Owner" for this as:

Create Subscription sub RUNAS <role_name> ...;
Alter Subscription sub RUNAS <role_name>

Now, RUNAS role will be used to apply changes and perform the initial
table sync. The owner will be tied to changing some of the other
properties (enabling, disabling, etc.) of the subscription. Now, we
still need a superuser to create subscription and change properties
like CONNECTION for a subscription but we can solve that by having
roles specific to it as being indicated by Mark in some of his
previous emails.

I feel disquieted about the "runas" idea. I can't really say why yet. Maybe it is ok, but it feels like a larger design decision than just an implementation detail about how subscriptions work. We should consider if we won't soon be doing the same thing for other parts of the system. If so, we should choose a solution that makes sense when applied more broadly.

Security definer functions could benefit from splitting the owner from the runas role.

Event triggers might benefit from having a runas role. Currently, event triggers are always owned by superusers, but we've discussed allowing non-superuser owners. That proposal still has outstanding issues to be resolved, so I can't be sure if runas would be helpful, but it might.

Table triggers might benefit from having a runas role. I don't have a proposal here, just an intuition that we should think about this before designing how "runas" works.

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

On Nov 19, 2021, at 7:25 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

Jeff Davis and I had a long conversation off-list yesterday and reached the same conclusion. I will be submitting a version of 0003 which does not depend on the prior two patches.

Renamed as 0001 in version 3, as it is the only remaining patch. For anyone who reviewed the older patch set, please note that I made some changes to the src/test/subscription/t/026_nosuperuser.pl test case relative to the prior version.

On Fri, 2021-11-19 at 16:45 -0800, Mark Dilger wrote:

Renamed as 0001 in version 3, as it is the only remaining patch. For
anyone who reviewed the older patch set, please note that I made some
changes to the src/test/subscription/t/026_nosuperuser.pl test case
relative to the prior version.

We need to do permission checking for WITH CHECK OPTION and RLS. The
patch right now allows the subscription to write data that an RLS
policy forbids.

A couple other points:

* We shouldn't refer to the behavior of previous versions in the docs
unless there's a compelling reason
* Do we need to be smarter about partitioned tables, where an insert
can turn into an update?
* Should we refactor to borrow logic from ExecInsert so that it's less
likely that we miss something in the future?

Regards,
Jeff Davis

On Thu, Nov 25, 2021 at 6:00 AM Jeff Davis <pgsql@j-davis.com> wrote:

On Fri, 2021-11-19 at 16:45 -0800, Mark Dilger wrote:

Renamed as 0001 in version 3, as it is the only remaining patch. For
anyone who reviewed the older patch set, please note that I made some
changes to the src/test/subscription/t/026_nosuperuser.pl test case
relative to the prior version.

We need to do permission checking for WITH CHECK OPTION and RLS. The
patch right now allows the subscription to write data that an RLS
policy forbids.

Won't it be better to just check if the current user is superuser
before applying each change as a matter of this first patch? Sorry, I
was under impression that first, we want to close the current gap
where we allow to proceed with replication if the user's superuser
privileges were revoked during replication. To allow non-superusers
owners, I thought it might be better to first try to detect the change
of ownership as soon as possible instead of at the transaction
boundary.

--
With Regards,
Amit Kapila.

On Thu, 2021-11-25 at 09:51 +0530, Amit Kapila wrote:

Won't it be better to just check if the current user is superuser
before applying each change as a matter of this first patch? Sorry, I
was under impression that first, we want to close the current gap
where we allow to proceed with replication if the user's superuser
privileges were revoked during replication.

That could be a first step, and I don't oppose it. But it seems like a
very small first step that would be made obsolete when v3-0001 is
ready, which I think will be very soon.

To allow non-superusers
owners, I thought it might be better to first try to detect the
change
of ownership

In the case of revoked superuser privileges, there's no change in
ownership, just a change of privileges (SUPERUSER -> NOSUPERUSER). And
if we're detecting a change of privileges, why not just do it in
something closer to the right way, which is what v3-0001 is attempting
to do.

as soon as possible instead of at the transaction
boundary.

I don't understand why it's important to detect a loss of privileges
faster than a transaction boundary. Can you elaborate?

Regards,
Jeff Davis

On Nov 24, 2021, at 4:30 PM, Jeff Davis <pgsql@j-davis.com> wrote:

We need to do permission checking for WITH CHECK OPTION and RLS. The
patch right now allows the subscription to write data that an RLS
policy forbids.

Thanks for reviewing and for this observation! I can verify that RLS is not being honored on the subscriber side. I agree this is a problem for subscriptions owned by non-superusers.

The implementation of the table sync worker uses COPY FROM, which makes this problem hard to fix, because COPY FROM does not support row level security. We could do some work to honor the RLS policies during the apply workers' INSERT statements, but then some data would circumvent RLS during table sync and other data would honor RLS during worker apply, which would make the implementation not only wrong but inconsistently so.

I think a more sensible approach for v15 is to raise an ERROR if a non-superuser owned subscription is trying to replicate into a table which has RLS enabled. We might try to be more clever and check whether the RLS policies could possibly reject the operation (by comparing the TO and FOR clauses of the policies against the role and operation type) but that seems like a partial re-implementation of RLS. It would be simpler and more likely correct if we just unconditionally reject replicating into tables which have RLS enabled.

What do you think?

A couple other points:

* We shouldn't refer to the behavior of previous versions in the docs
unless there's a compelling reason

Fair enough.

* Do we need to be smarter about partitioned tables, where an insert
can turn into an update?

Do you mean an INSERT statement with an ON CONFLICT DO UPDATE clause that is running against a partitioned table? If so, I don't think we need to handle that on the subscriber side under the current logical replication design. I would expect the plain INSERT or UPDATE that ultimately executes on the publisher to be what gets replicated to the subscriber, and not the original INSERT .. ON CONFLICT DO UPDATE statement.

* Should we refactor to borrow logic from ExecInsert so that it's less
likely that we miss something in the future?

Hooking into the executor at a higher level, possibly ExecInsert or ExecModifyTable would do a lot more than what logical replication currently does. If we also always used INSERT/UPDATE/DELETE statements and never COPY FROM statements, we might solve several problems at once, including honoring RLS policies and honoring rules defined for the target table on the subscriber side.

Doing this would clearly be a major design change, and possibly one we do not want. Can we consider this out of scope?

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

On Fri, Nov 26, 2021 at 1:36 AM Jeff Davis <pgsql@j-davis.com> wrote:

as soon as possible instead of at the transaction
boundary.

I don't understand why it's important to detect a loss of privileges
faster than a transaction boundary. Can you elaborate?

The first reason is that way it would be consistent with what we can
see while doing the operations from the backend. For example, if we
revoke privileges from the user during the transaction, the results
will be reflected.
postgres=> Begin;
BEGIN
postgres=*> insert into t1 values(1);
INSERT 0 1
postgres=*> insert into t1 values(2);
ERROR: permission denied for table t1

In this case, after the first insert, I have revoked the privileges of
the user from table t1 and the same is reflected in the very next
operation. Another reason is to make behavior predictable as users can
always expect when exactly the privilege change will be reflected and
it won't depend on the number of changes in the transaction.

--
With Regards,
Amit Kapila.

On Sat, Nov 27, 2021 at 11:37 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Nov 24, 2021, at 4:30 PM, Jeff Davis <pgsql@j-davis.com> wrote:

We need to do permission checking for WITH CHECK OPTION and RLS. The
patch right now allows the subscription to write data that an RLS
policy forbids.

Thanks for reviewing and for this observation! I can verify that RLS is not being honored on the subscriber side. I agree this is a problem for subscriptions owned by non-superusers.

...

A couple other points:

* Do we need to be smarter about partitioned tables, where an insert
can turn into an update?

Do you mean an INSERT statement with an ON CONFLICT DO UPDATE clause that is running against a partitioned table? If so, I don't think we need to handle that on the subscriber side under the current logical replication design. I would expect the plain INSERT or UPDATE that ultimately executes on the publisher to be what gets replicated to the subscriber, and not the original INSERT .. ON CONFLICT DO UPDATE statement.

Yeah, that is correct but I think the update case is more relevant
here. In ExecUpdate(), we convert Update to DELETE+INSERT when the
partition constraint is failed whereas, on the subscriber-side, it
will simply fail in this case. It is not clear to me how that is
directly related to this patch but surely it will be a good
improvement on its own and might help if that requires us to change
some infrastructure here like hooking into executor at a higher level.

* Should we refactor to borrow logic from ExecInsert so that it's less
likely that we miss something in the future?

Hooking into the executor at a higher level, possibly ExecInsert or ExecModifyTable would do a lot more than what logical replication currently does. If we also always used INSERT/UPDATE/DELETE statements and never COPY FROM statements, we might solve several problems at once, including honoring RLS policies and honoring rules defined for the target table on the subscriber side.

Doing this would clearly be a major design change, and possibly one we do not want. Can we consider this out of scope?

I agree that if we want to do all of this then that would require a
lot of changes. However, giving an error for RLS-enabled tables might
also be too restrictive. The few alternatives could be that (a) we
allow subscription owners to be either have "bypassrls" attribute or
they could be superusers. (b) don't allow initial table_sync for rls
enabled tables. (c) evaluate/analyze what is required to allow Copy
From to start respecting RLS policies. (d) reject replicating any
changes to tables that have RLS enabled.

I see that you are favoring (d) which clearly has merits like lesser
code/design change but not sure if that is the best way forward or we
can do something better than that either by following one of (a), (b),
(c), or something less restrictive than (d).

--
With Regards,
Amit Kapila.

On Nov 28, 2021, at 9:56 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

In ExecUpdate(), we convert Update to DELETE+INSERT when the
partition constraint is failed whereas, on the subscriber-side, it
will simply fail in this case. It is not clear to me how that is
directly related to this patch but surely it will be a good
improvement on its own and might help if that requires us to change
some infrastructure here like hooking into executor at a higher level.

I would rather get a fix for non-superuser subscription owners committed than expand the scope of work and have this patch linger until the v16 development cycle. This particular DELETE+INSERT problem sounds important but unrelated and out of scope.

I agree that if we want to do all of this then that would require a
lot of changes. However, giving an error for RLS-enabled tables might
also be too restrictive. The few alternatives could be that (a) we
allow subscription owners to be either have "bypassrls" attribute or
they could be superusers. (b) don't allow initial table_sync for rls
enabled tables. (c) evaluate/analyze what is required to allow Copy
From to start respecting RLS policies. (d) reject replicating any
changes to tables that have RLS enabled.

I see that you are favoring (d) which clearly has merits like lesser
code/design change but not sure if that is the best way forward or we
can do something better than that either by following one of (a), (b),
(c), or something less restrictive than (d).

I was favoring option (d) only when RLS policies exist for one or more of the target relations.

Skipping the table_sync step while replicating tables that have RLS policies for subscriptions that are owned by users who lack bypassrls is interesting. If we make that work, it will be a more complete solution than option (d).

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

On Mon, 2021-11-29 at 08:26 -0800, Mark Dilger wrote:

On Nov 28, 2021, at 9:56 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

In ExecUpdate(), we convert Update to DELETE+INSERT when the
partition constraint is failed whereas, on the subscriber-side, it
will simply fail in this case.

Thank you, yes, that's the more important case.

This particular DELETE+INSERT problem sounds important but unrelated
and out of scope.

+1

I agree that if we want to do all of this then that would require a
lot of changes. However, giving an error for RLS-enabled tables
might
also be too restrictive. The few alternatives could be that (a) we
allow subscription owners to be either have "bypassrls" attribute
or
they could be superusers. (b) don't allow initial table_sync for
rls
enabled tables. (c) evaluate/analyze what is required to allow Copy
From to start respecting RLS policies. (d) reject replicating any
changes to tables that have RLS enabled.

Maybe a combination?

Allow subscriptions with copy_data=true iff the subscription owner is
bypassrls or superuser. And then enforce RLS+WCO during
insert/update/delete.

I don't think it's a big change (correct me if I'm wrong), and it
allows good functionality now, and room to improve in the future if we
want to bring in more of ExecInsert into logical replication.

Regards,
Jeff Davis

On Mon, 2021-11-29 at 09:43 +0530, Amit Kapila wrote:

The first reason is that way it would be consistent with what we can
see while doing the operations from the backend.

Logical replication is not interactive, so it doesn't seem quite the
same.

If you have a long running INSERT INTO SELECT or COPY FROM, the
permission checks just happen at the beginning. As a user, it wouldn't
surprise me if logical replication was similar.

operation. Another reason is to make behavior predictable as users
can
always expect when exactly the privilege change will be reflected and
it won't depend on the number of changes in the transaction.

This patch does detect ownership changes more quickly (at the
transaction boundary) than the current code (only when it reloads for
some other reason). Transaction boundary seems like a reasonable time
to detect the change to me.

Detecting faster might be nice, but I don't have a strong opinion about
it and I don't see why it necessarily needs to happen before this patch
goes in.

Also, do you think the cost of doing maybe_reread_subscription() per-
tuple instead of per-transaction would be detectable? If we lock
ourselves into semantics that detect changes quickly, it will be harder
to optimize the per-tuple path later.

Regards,
Jeff Davis

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

On Mon, 2021-11-29 at 08:26 -0800, Mark Dilger wrote:

I agree that if we want to do all of this then that would require a
lot of changes. However, giving an error for RLS-enabled tables
might
also be too restrictive. The few alternatives could be that (a) we
allow subscription owners to be either have "bypassrls" attribute
or
they could be superusers. (b) don't allow initial table_sync for
rls
enabled tables. (c) evaluate/analyze what is required to allow Copy
From to start respecting RLS policies. (d) reject replicating any
changes to tables that have RLS enabled.

Maybe a combination?

Allow subscriptions with copy_data=true iff the subscription owner is
bypassrls or superuser. And then enforce RLS+WCO during
insert/update/delete.

Yeah, that sounds reasonable to me.

I don't think it's a big change (correct me if I'm wrong),

Yeah, I also don't think it should be a big change.

--
With Regards,
Amit Kapila.

On Tue, Nov 30, 2021 at 12:56 AM Jeff Davis <pgsql@j-davis.com> wrote:

On Mon, 2021-11-29 at 09:43 +0530, Amit Kapila wrote:

The first reason is that way it would be consistent with what we can
see while doing the operations from the backend.

Logical replication is not interactive, so it doesn't seem quite the
same.

If you have a long running INSERT INTO SELECT or COPY FROM, the
permission checks just happen at the beginning. As a user, it wouldn't
surprise me if logical replication was similar.

operation. Another reason is to make behavior predictable as users
can
always expect when exactly the privilege change will be reflected and
it won't depend on the number of changes in the transaction.

This patch does detect ownership changes more quickly (at the
transaction boundary) than the current code (only when it reloads for
some other reason). Transaction boundary seems like a reasonable time
to detect the change to me.

Detecting faster might be nice, but I don't have a strong opinion about
it and I don't see why it necessarily needs to happen before this patch
goes in.

I think it would be better to do it before we allow subscription
owners to be non-superusers.

Also, do you think the cost of doing maybe_reread_subscription() per-
tuple instead of per-transaction would be detectable?

Yeah, it is possible that is why I suggested in one of the emails
above to allow changing the owners only for disabled subscriptions.

--
With Regards,
Amit Kapila.

On Tue, 2021-11-30 at 17:25 +0530, Amit Kapila wrote:

I think it would be better to do it before we allow subscription
owners to be non-superusers.

There are a couple other things to consider before allowing non-
superusers to create subscriptions anyway. For instance, a non-
superuser shouldn't be able to use a connection string that reads the
certificate file from the server unless they also have
pg_read_server_files privs.

Yeah, it is possible that is why I suggested in one of the emails
above to allow changing the owners only for disabled subscriptions.

The current patch detects the following cases at the transaction
boundary:

* ALTER SUBSCRIPTION ... OWNER TO ...
* ALTER ROLE ... NOSUPERUSER
* privileges revoked one way or another (aside from the RLS/WCO
problems, which will be fixed)

If we want to detect at row boundaries we need to capture all of those
cases too, or else we're being inconsistent. The latter two cannot be
tied to whether the subscription is disabled or not, so I don't think
that's a complete solution.

How about (as a separate patch) we just do maybe_reread_subscription()
every K operations within a transaction? That would speed up
permissions errors if a revoke happens.

Regards,
Jeff Davis

On Wed, Dec 1, 2021 at 2:12 AM Jeff Davis <pgsql@j-davis.com> wrote:

On Tue, 2021-11-30 at 17:25 +0530, Amit Kapila wrote:

I think it would be better to do it before we allow subscription
owners to be non-superusers.

There are a couple other things to consider before allowing non-
superusers to create subscriptions anyway. For instance, a non-
superuser shouldn't be able to use a connection string that reads the
certificate file from the server unless they also have
pg_read_server_files privs.

Isn't allowing to create subscriptions via non-superusers and allowing
to change the owner two different things? I am under the impression
that the latter one is more towards allowing the workers to apply
changes with a non-superuser role.

--
With Regards,
Amit Kapila.

On Dec 1, 2021, at 5:36 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Dec 1, 2021 at 2:12 AM Jeff Davis <pgsql@j-davis.com> wrote:

On Tue, 2021-11-30 at 17:25 +0530, Amit Kapila wrote:

I think it would be better to do it before we allow subscription
owners to be non-superusers.

There are a couple other things to consider before allowing non-
superusers to create subscriptions anyway. For instance, a non-
superuser shouldn't be able to use a connection string that reads the
certificate file from the server unless they also have
pg_read_server_files privs.

Isn't allowing to create subscriptions via non-superusers and allowing
to change the owner two different things? I am under the impression
that the latter one is more towards allowing the workers to apply
changes with a non-superuser role.

The short-term goal is to have logical replication workers respect the privileges of the role which owns the subscription.

The long-term work probably includes creating a predefined role with permission to create subscriptions, and the ability to transfer those subscriptions to roles who might be neither superuser nor members of any particular predefined role; the idea being that logical replication subscriptions can be established without any superuser involvement, and may thereafter run without any special privilege.

The more recent patches on this thread are not as ambitious as the earlier patch-sets. We are no longer trying to support transferring subscriptions to non-superusers.

Right now, on HEAD, if a subscription owner has superuser revoked, the subscription can continue to operate as superuser in so far as its replication actions are concerned. That seems like a pretty big security hole.

This patch mostly plugs that hole by adding permissions checks, so that a subscription owned by a role who has privileges revoked cannot (for the most part) continue to act under the old privileges.

There are two problematic edge cases that can occur after transfer of ownership. Remember, the new owner is required to be superuser for the transfer of ownership to occur.

1) A subscription is transferred to a new owner, and the new owner then has privilege revoked.

2) A subscription is transferred to a new owner, and then the old owner has privileges increased.

In both cases, a currently running logical replication worker may finish a transaction in progress acting with the current privileges of the old owner. The clearest solution is, as you suggest, to refuse transfer of ownership of subscriptions that are enabled.

Doing so will create a failure case for REASSIGN OWNED BY. Will that be ok?

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

On Thu, Dec 2, 2021 at 12:51 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Dec 1, 2021, at 5:36 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Dec 1, 2021 at 2:12 AM Jeff Davis <pgsql@j-davis.com> wrote:

On Tue, 2021-11-30 at 17:25 +0530, Amit Kapila wrote:

I think it would be better to do it before we allow subscription
owners to be non-superusers.

There are a couple other things to consider before allowing non-
superusers to create subscriptions anyway. For instance, a non-
superuser shouldn't be able to use a connection string that reads the
certificate file from the server unless they also have
pg_read_server_files privs.

Isn't allowing to create subscriptions via non-superusers and allowing
to change the owner two different things? I am under the impression
that the latter one is more towards allowing the workers to apply
changes with a non-superuser role.

The short-term goal is to have logical replication workers respect the privileges of the role which owns the subscription.

The long-term work probably includes creating a predefined role with permission to create subscriptions, and the ability to transfer those subscriptions to roles who might be neither superuser nor members of any particular predefined role; the idea being that logical replication subscriptions can be established without any superuser involvement, and may thereafter run without any special privilege.

The more recent patches on this thread are not as ambitious as the earlier patch-sets. We are no longer trying to support transferring subscriptions to non-superusers.

Right now, on HEAD, if a subscription owner has superuser revoked, the subscription can continue to operate as superuser in so far as its replication actions are concerned. That seems like a pretty big security hole.

This patch mostly plugs that hole by adding permissions checks, so that a subscription owned by a role who has privileges revoked cannot (for the most part) continue to act under the old privileges.

If we want to maintain the property that subscriptions can only be
owned by superuser for your first version then isn't a simple check
like ((!superuser()) for each of the operations is sufficient?

There are two problematic edge cases that can occur after transfer of ownership. Remember, the new owner is required to be superuser for the transfer of ownership to occur.

1) A subscription is transferred to a new owner, and the new owner then has privilege revoked.

2) A subscription is transferred to a new owner, and then the old owner has privileges increased.

In (2), I am not clear what do you mean by "the old owner has
privileges increased"? If the owners can only be superusers then what
does it mean to increase the privileges.

In both cases, a currently running logical replication worker may finish a transaction in progress acting with the current privileges of the old owner. The clearest solution is, as you suggest, to refuse transfer of ownership of subscriptions that are enabled.

Doing so will create a failure case for REASSIGN OWNED BY. Will that be ok?

I think so. Do we see any problem with that? I think we have some
failure cases currently as well like "All Tables Publication" can only
be owned by superusers whereas ownership for others can be to
non-superusers and similarly we can't change ownership for pinned
objects. I think the case being discussed is not exactly the same but
I am not able to see a problem with it.

--
With Regards,
Amit Kapila.

On Dec 2, 2021, at 1:29 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

If we want to maintain the property that subscriptions can only be
owned by superuser for your first version then isn't a simple check
like ((!superuser()) for each of the operations is sufficient?

As things stand today, nothing prevents a superuser subscription owner from having superuser revoked. The patch does nothing to change this.

In (2), I am not clear what do you mean by "the old owner has
privileges increased"? If the owners can only be superusers then what
does it mean to increase the privileges.

The old owner may have had privileges reduced (no superuser, only permission to write into a specific schema, etc.) and the subscription enabled only after those privilege reductions were put in place. This is a usage pattern this patch is intended to support, by honoring those privilege restrictions.

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

On Fri, Dec 3, 2021 at 10:37 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Dec 2, 2021, at 1:29 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

If we want to maintain the property that subscriptions can only be
owned by superuser for your first version then isn't a simple check
like ((!superuser()) for each of the operations is sufficient?

As things stand today, nothing prevents a superuser subscription owner from having superuser revoked. The patch does nothing to change this.

I understand that but won't that get verified when we look up the
information in pg_authid as part of superuser() check?

--
With Regards,
Amit Kapila.

On Dec 6, 2021, at 2:19 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

If we want to maintain the property that subscriptions can only be
owned by superuser

We don't want to maintain such a property, or at least, that's not what I want. I don't think that's what Jeff wants, either.

To clarify, I'm not entirely sure how to interpret the verb "maintain" in your question, since before the patch the property does not exist, and after the patch, it continues to not exist. We could *add* such a property, of course, though this patch does not attempt any such thing.

I understand that but won't that get verified when we look up the
information in pg_authid as part of superuser() check?

If we added a superuser() check, then yes, but that would take things in a direction I do not want to go.

As I perceive the roadmap:

1) Fix the current bug wherein subscription changes are applied with superuser force after the subscription owner has superuser privileges revoked.
2) Allow the transfer of subscriptions to non-superuser owners.
3) Allow the creation of subscriptions by non-superusers who are members of some as yet to be created predefined role, say "pg_create_subscriptions"

I may be wrong, but it sounds like you interpret the intent of this patch as enforcing superuserness. That's not so. This patch intends to correctly handle the situation where a subscription is owned by a non-superuser (task 1, above) without going so far as creating new paths by which that situation could arise (tasks 2 and 3, above).

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

Le lundi 6 décembre 2021, 16:56:56 CET Mark Dilger a écrit :

On Dec 6, 2021, at 2:19 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

If we want to maintain the property that subscriptions can only be
owned by superuser

We don't want to maintain such a property, or at least, that's not what I
want. I don't think that's what Jeff wants, either.

That's not what I want either: the ability to run and refresh subscriptions as
a non superuser is a desirable feature.

The REFRESH part was possible before PG 14, when it was allowed to run REFRESH
in a function, which could be made to run as security definer.

As I perceive the roadmap:

1) Fix the current bug wherein subscription changes are applied with
superuser force after the subscription owner has superuser privileges
revoked. 2) Allow the transfer of subscriptions to non-superuser owners.
3) Allow the creation of subscriptions by non-superusers who are members of
some as yet to be created predefined role, say "pg_create_subscriptions"

This roadmap seems sensible.

--
Ronan Dunklau

On Mon, Dec 6, 2021 at 9:26 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:

On Dec 6, 2021, at 2:19 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

If we want to maintain the property that subscriptions can only be
owned by superuser

We don't want to maintain such a property, or at least, that's not what I want. I don't think that's what Jeff wants, either.

To clarify, I'm not entirely sure how to interpret the verb "maintain" in your question, since before the patch the property does not exist, and after the patch, it continues to not exist. We could *add* such a property, of course, though this patch does not attempt any such thing.

Okay, let me try to explain again. Following is the text from docs
[1]: https://www.postgresql.org/docs/devel/logical-replication-security.html
The subscription apply process will run in the local database with the
privileges of a superuser. (c) Privileges are only checked once at the
start of a replication connection. They are not re-checked as each
change record is read from the publisher, nor are they re-checked for
each change when applied.

My understanding is that we want to improve what is written as (c)
which I think is the same as what you mentioned later as "Fix the
current bug wherein subscription changes are applied with superuser
force after the subscription owner has superuser privileges revoked.".
Am I correct till here? If so, I think what I am suggesting should fix
this with the assumption that we still want to follow (b) at least for
the first patch. One possibility is that our understanding of the
first problem is the same but you want to allow apply worker running
even when superuser privileges are revoked provided the user with
which it is running has appropriate privileges on the objects being
accessed by apply worker.

We will talk about other points of the roadmap you mentioned once our
understanding for the first one matches.

[1]: https://www.postgresql.org/docs/devel/logical-replication-security.html

--
With Regards,
Amit Kapila.

On Dec 7, 2021, at 2:29 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Okay, let me try to explain again. Following is the text from docs
[1]: " (a) To create a subscription, the user must be a superuser. (b)
The subscription apply process will run in the local database with the
privileges of a superuser. (c) Privileges are only checked once at the
start of a replication connection. They are not re-checked as each
change record is read from the publisher, nor are they re-checked for
each change when applied.

My understanding is that we want to improve what is written as (c)
which I think is the same as what you mentioned later as "Fix the
current bug wherein subscription changes are applied with superuser
force after the subscription owner has superuser privileges revoked.".
Am I correct till here? If so, I think what I am suggesting should fix
this with the assumption that we still want to follow (b) at least for
the first patch.

Ok, that's a point of disagreement. I was trying to fix both (b) and (c) in the first patch.

One possibility is that our understanding of the
first problem is the same but you want to allow apply worker running
even when superuser privileges are revoked provided the user with
which it is running has appropriate privileges on the objects being
accessed by apply worker.

Correct, that's what I'm trying to make safe.

We will talk about other points of the roadmap you mentioned once our
understanding for the first one matches.

I am happy to have an off-list phone call with you, if you like.

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

On Tue, Dec 7, 2021 at 8:25 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:

On Dec 7, 2021, at 2:29 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Okay, let me try to explain again. Following is the text from docs
[1]: " (a) To create a subscription, the user must be a superuser. (b)
The subscription apply process will run in the local database with the
privileges of a superuser. (c) Privileges are only checked once at the
start of a replication connection. They are not re-checked as each
change record is read from the publisher, nor are they re-checked for
each change when applied.

My understanding is that we want to improve what is written as (c)
which I think is the same as what you mentioned later as "Fix the
current bug wherein subscription changes are applied with superuser
force after the subscription owner has superuser privileges revoked.".
Am I correct till here? If so, I think what I am suggesting should fix
this with the assumption that we still want to follow (b) at least for
the first patch.

Ok, that's a point of disagreement. I was trying to fix both (b) and (c) in the first patch.

But, I think as soon as we are trying to fix (b), we seem to be
allowing non-superusers to apply changes. If we want to do that then
we should be even allowed to change the owners to non-superusers. I
was thinking of the below order:
1. First fix (c) from the above description "Privileges are only
checked once at the start of a replication connection."
2A. Allow the transfer of subscriptions to non-superuser owners. This
will be allowed only on disabled subscriptions to make this action
predictable.
2B. The apply worker should be able to apply the changes provided the
user has appropriate privileges on the objects being accessed by apply
worker.
3) Allow the creation of subscriptions by non-superusers who are
members of some as yet to be created predefined role, say
"pg_create_subscriptions"

We all seem to agree that (3) can be done later as an independent
project. 2A, 2B can be developed as separate patches but they need to
be considered together for commit. After 2A, 2B, the first one (1)
won't be required so, in fact, we can just ignore (1) but the only
benefit I see is that if we stuck with some design problem during the
development of 2A, 2B, we would have at least something better than
what we have now.

You seem to be indicating let's do 2B first as that will anyway be
used later after 2A and 1 won't be required if we do that. I see that
but I personally feel either we should follow 1, 2(A, B) or just do
2(A, B).

--
With Regards,
Amit Kapila.

On Tue, Nov 30, 2021 at 6:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

This patch does detect ownership changes more quickly (at the
transaction boundary) than the current code (only when it reloads for
some other reason). Transaction boundary seems like a reasonable time
to detect the change to me.

Detecting faster might be nice, but I don't have a strong opinion about
it and I don't see why it necessarily needs to happen before this patch
goes in.

I think it would be better to do it before we allow subscription
owners to be non-superusers.

I think it would be better not to ever do it at any time.

It seems like a really bad idea to me to change the run-as user in the
middle of a transaction. That seems prone to producing all sorts of
confusing behavior that's hard to understand, and hard to test. So
what are we to do if a change occurs mid-transaction? I think we can
either finish replicating the current transaction and then switch to
the new owner for the next transaction, or we could abort the current
attempt to replicate the transaction and retry the whole transaction
with the new run-as user. My guess is that most users would prefer the
former behavior to the latter.

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

On Wed, Dec 8, 2021 at 11:58 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

But, I think as soon as we are trying to fix (b), we seem to be
allowing non-superusers to apply changes. If we want to do that then
we should be even allowed to change the owners to non-superusers. I
was thinking of the below order:
1. First fix (c) from the above description "Privileges are only
checked once at the start of a replication connection."
2A. Allow the transfer of subscriptions to non-superuser owners. This
will be allowed only on disabled subscriptions to make this action
predictable.
2B. The apply worker should be able to apply the changes provided the
user has appropriate privileges on the objects being accessed by apply
worker.
3) Allow the creation of subscriptions by non-superusers who are
members of some as yet to be created predefined role, say
"pg_create_subscriptions"

We all seem to agree that (3) can be done later as an independent
project. 2A, 2B can be developed as separate patches but they need to
be considered together for commit. After 2A, 2B, the first one (1)
won't be required so, in fact, we can just ignore (1) but the only
benefit I see is that if we stuck with some design problem during the
development of 2A, 2B, we would have at least something better than
what we have now.

You seem to be indicating let's do 2B first as that will anyway be
used later after 2A and 1 won't be required if we do that. I see that
but I personally feel either we should follow 1, 2(A, B) or just do
2(A, B).

1 and 2B seem to require changing the same code, or related code. 1A
seems to require a completely different set of changes. If I'm right
about that, it seems like a good reason for doing 1+2B first and
leaving 2A for a separate patch.

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

On Dec 9, 2021, at 7:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Nov 30, 2021 at 6:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

This patch does detect ownership changes more quickly (at the
transaction boundary) than the current code (only when it reloads for
some other reason). Transaction boundary seems like a reasonable time
to detect the change to me.

Detecting faster might be nice, but I don't have a strong opinion about
it and I don't see why it necessarily needs to happen before this patch
goes in.

I think it would be better to do it before we allow subscription
owners to be non-superusers.

I think it would be better not to ever do it at any time.

It seems like a really bad idea to me to change the run-as user in the
middle of a transaction.

I agree. We allow SET ROLE inside transactions, but faking one on the subscriber seems odd. No such role change was performed on the publisher side, nor is there a principled reason for assuming the old run-as role has membership in the new run-as role, so we'd be pretending to do something that might otherwise be impossible.

There was some discussion off-list about having the apply worker take out a lock on its subscription, thereby blocking ownership changes mid-transaction. I coded that and it seems to work fine, but I have a hard time seeing how the lock traffic would be worth expending. Between (a) changing roles mid-transaction, and (b) locking the subscription for each transaction, I'd prefer to do neither, but (b) seems far better than (a). Thoughts?

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

On Dec 9, 2021, at 7:47 AM, Robert Haas <robertmhaas@gmail.com> wrote:

1 and 2B seem to require changing the same code, or related code. 1A
seems to require a completely different set of changes. If I'm right
about that, it seems like a good reason for doing 1+2B first and
leaving 2A for a separate patch.

There are unresolved problems with 2A and 3 which were discussed upthread. I don't want to include fixes for them in this patch, as it greatly expands the scope of this patch, and is a logically separate effort. We can come back to those problems after this first patch is committed.

Specifically, a non-superuser owner can perform ALTER SUBSCRIPTION and do things that are morally equivalent to creating a new subscription. This is problematic where things like the connection string are concerned, because it means the non-superuser owner can connect out to entirely different servers, without any access control checks to make sure the owner should be able to connect to these servers.

This problem already exists, right now. I'm not fixing it in this first patch, but I'm also not making it any worse.

The solution Jeff Davis proposed seems right to me. We change subscriptions to use a foreign server rather than a freeform connection string. When creating or altering a subscription, the role performing the action must have privileges on any foreign server they use.

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

On Thu, Dec 9, 2021 at 10:52 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Dec 9, 2021, at 7:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Nov 30, 2021 at 6:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

This patch does detect ownership changes more quickly (at the
transaction boundary) than the current code (only when it reloads for
some other reason). Transaction boundary seems like a reasonable time
to detect the change to me.

Detecting faster might be nice, but I don't have a strong opinion about
it and I don't see why it necessarily needs to happen before this patch
goes in.

I think it would be better to do it before we allow subscription
owners to be non-superusers.

I think it would be better not to ever do it at any time.

It seems like a really bad idea to me to change the run-as user in the
middle of a transaction.

I agree. We allow SET ROLE inside transactions, but faking one on the subscriber seems odd. No such role change was performed on the publisher side, nor is there a principled reason for assuming the old run-as role has membership in the new run-as role, so we'd be pretending to do something that might otherwise be impossible.

There was some discussion off-list about having the apply worker take out a lock on its subscription, thereby blocking ownership changes mid-transaction. I coded that and it seems to work fine, but I have a hard time seeing how the lock traffic would be worth expending. Between (a) changing roles mid-transaction, and (b) locking the subscription for each transaction, I'd prefer to do neither, but (b) seems far better than (a). Thoughts?

Yeah, to me also (b) sounds better than (a). However, a few points
that we might want to consider in that regard are as follows: 1.
locking the subscription for each transaction will add new blocking
areas considering we acquire AccessExclusiveLock to change any
property of subscription. But as Alter Subscription won't be that
frequent operation it might be acceptable. 2. It might lead to adding
some cost to small transactions but not sure if that will be
noticeable. 3. Tomorrow, if we want to make the apply-process parallel
(IIRC, we do have the patch for that somewhere in archives) especially
for large in-progress transactions then this locking will have
additional blocking w.r.t Altering Subscription. But again, this also
might be acceptable.

--
With Regards,
Amit Kapila.

On Thu, Dec 9, 2021 at 11:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Yeah, to me also (b) sounds better than (a). However, a few points
that we might want to consider in that regard are as follows: 1.
locking the subscription for each transaction will add new blocking
areas considering we acquire AccessExclusiveLock to change any
property of subscription. But as Alter Subscription won't be that
frequent operation it might be acceptable.

The problem isn't the cost of the locks taken by ALTER SUBSCRIPTION.
It's the cost of locking and unlocking the relation for every
transaction we apply. Suppose it's a pgbench-type workload with a
single UPDATE per transaction. You've just limited the maximum
possible apply speed to about, I think, 30,000 transactions per second
no matter how many parallel workers you use, because that's how fast
the lock manager is (or was, unless newer hardware or newer PG
versions have changed things in a way I don't know about). That seems
like a poor idea. There's nothing wrong with noticing changes at the
next transaction boundary, as long as we document it. So why would we
incur a possibly-significant performance cost to provide a stricter
guarantee?

I bet users wouldn't even like this behavior. It would mean that if
you are replicating a long-running transaction, an ALTER SUBSCRIPTION
command might block for a long time until replication of that
transaction completes. I have a hard time understanding why anyone
would consider that an improvement.

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

On 12/10/21 09:09, Robert Haas wrote:

On Thu, Dec 9, 2021 at 11:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Yeah, to me also (b) sounds better than (a). However, a few points
that we might want to consider in that regard are as follows: 1.
locking the subscription for each transaction will add new blocking
areas considering we acquire AccessExclusiveLock to change any
property of subscription. But as Alter Subscription won't be that
frequent operation it might be acceptable.

The problem isn't the cost of the locks taken by ALTER SUBSCRIPTION.
It's the cost of locking and unlocking the relation for every
transaction we apply. Suppose it's a pgbench-type workload with a
single UPDATE per transaction. You've just limited the maximum
possible apply speed to about, I think, 30,000 transactions per second
no matter how many parallel workers you use, because that's how fast
the lock manager is (or was, unless newer hardware or newer PG
versions have changed things in a way I don't know about). That seems
like a poor idea. There's nothing wrong with noticing changes at the
next transaction boundary, as long as we document it. So why would we
incur a possibly-significant performance cost to provide a stricter
guarantee?

I bet users wouldn't even like this behavior. It would mean that if
you are replicating a long-running transaction, an ALTER SUBSCRIPTION
command might block for a long time until replication of that
transaction completes. I have a hard time understanding why anyone
would consider that an improvement.

+1

I think noticing changes at the transaction boundary is perfectly
acceptable.

cheers

andrew

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

On Fri, Dec 10, 2021 at 7:39 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Dec 9, 2021 at 11:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Yeah, to me also (b) sounds better than (a). However, a few points
that we might want to consider in that regard are as follows: 1.
locking the subscription for each transaction will add new blocking
areas considering we acquire AccessExclusiveLock to change any
property of subscription. But as Alter Subscription won't be that
frequent operation it might be acceptable.

The problem isn't the cost of the locks taken by ALTER SUBSCRIPTION.
It's the cost of locking and unlocking the relation for every
transaction we apply.

This point is not clear to me as we are already locking the relation
while applying changes. I think here additional cost is to lock a
particular subscription as well in addition to the relation on which
we are going to perform apply. I agree that has a cost and that is why
I mentioned it as one of the points above and then also the
concurrency effect as you also noted could make this idea moot.

Suppose it's a pgbench-type workload with a
single UPDATE per transaction. You've just limited the maximum
possible apply speed to about, I think, 30,000 transactions per second
no matter how many parallel workers you use, because that's how fast
the lock manager is (or was, unless newer hardware or newer PG
versions have changed things in a way I don't know about). That seems
like a poor idea. There's nothing wrong with noticing changes at the
next transaction boundary, as long as we document it.

If we want to just document this then I think we should also keep in
mind that these could be N transactions as well if say tomorrow we
have N parallel apply workers applying the N transactions in parallel.
I think it might also be possible that RLS policies won't be applied
for initial table sync whereas those will be applied for later changes
even though the ownership has changed before both operations and one
of those happens to miss it. If that is possible, then it might be
better to avoid the same as it could appear inconsistent as mentioned
by Mark [1]/messages/by-id/FE7D7024-6723-4ACB-82AB-94F6A194BE0D@enterprisedb.com as well. Now, it might be possible to avoid this by
implementation or we can say that we don't care about this or just
document it. But it seems to me that if we have some way to detect the
change of ownership at each operation level then no such possibilities
would arise.

The other alternative we discussed was to allow a change of ownership
on disabled subscriptions that way the apply behavior will always be
predictable.

There is clearly a merit in noticing the change of ownership at
transaction boundary but just wanted to consider other possibilities.
It could be that detecting at transaction-boundary is the best we can
do but I think there is no harm in considering other possibilities.

So why would we
incur a possibly-significant performance cost to provide a stricter
guarantee?

I bet users wouldn't even like this behavior. It would mean that if
you are replicating a long-running transaction, an ALTER SUBSCRIPTION
command might block for a long time until replication of that
transaction completes.

Agreed and if we decide to lock the subscription during the initial
table sync phase then that could also take a long time for which again
users might not be happy.

[1]: /messages/by-id/FE7D7024-6723-4ACB-82AB-94F6A194BE0D@enterprisedb.com

--
With Regards,
Amit Kapila.

On Nov 24, 2021, at 4:30 PM, Jeff Davis <pgsql@j-davis.com> wrote:

We need to do permission checking for WITH CHECK OPTION and RLS. The
patch right now allows the subscription to write data that an RLS
policy forbids.

Version 4 of the patch, attached, no longer allows RLS to be circumvented, but does so in a course-grained fashion. If the target table has row-level security policies which are enforced against the subscription owner, the replication draws an error, much as with a permissions failure. This seems sufficient for now, as superusers, roles with bypassrls, and target table owners should be able to replicate as before. We may want to revisit this later, perhaps if/when we address your ExecInsert question, below.

A couple other points:

* We shouldn't refer to the behavior of previous versions in the docs
unless there's a compelling reason

Fixed.

* Do we need to be smarter about partitioned tables, where an insert
can turn into an update?

Indeed, the logic of apply_handle_tuple_routing() required a bit of refactoring. Fixed in v4.

* Should we refactor to borrow logic from ExecInsert so that it's less
likely that we miss something in the future?

Let's just punt on this for now.

On Thu, Dec 16, 2021 at 1:53 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Nov 24, 2021, at 4:30 PM, Jeff Davis <pgsql@j-davis.com> wrote:

We need to do permission checking for WITH CHECK OPTION and RLS. The
patch right now allows the subscription to write data that an RLS
policy forbids.

Version 4 of the patch, attached.

For Update/Delete, we do read the table first via
FindReplTupleInLocalRel(), so is there a need to check ACL_SELECT
before that?

--
With Regards,
Amit Kapila.

On Sat, 2022-01-08 at 12:27 +0530, Amit Kapila wrote:

For Update/Delete, we do read the table first via
FindReplTupleInLocalRel(), so is there a need to check ACL_SELECT
before that?

If it's logically an update/delete, then I think ACL_UPDATE/DELETE is
the right one to check. Do you have a different opinion?

Regards,
Jeff Davis

On Wed, 2021-12-15 at 12:23 -0800, Mark Dilger wrote:

On Nov 24, 2021, at 4:30 PM, Jeff Davis <pgsql@j-davis.com> wrote:

We need to do permission checking for WITH CHECK OPTION and RLS.
The
patch right now allows the subscription to write data that an RLS
policy forbids.

Version 4 of the patch, attached, no longer allows RLS to be
circumvented, but does so in a course-grained fashion.

Committed.

I tried to do some performance testing to see if there was any impact
of the extra catalog + ACL checks. Logical replication seems slow
enough -- something like 3X slower than local inserts -- that it didn't
seem to make a difference.

To test it, I did the following:
1. sent a SIGSTOP to the logical apply worker
2. loaded more data in publisher
3. made the subscriber a sync replica
4. timed the following:
a. sent a SIGCONT to the logical apply worker
b. insert a single tuple on the publisher side
c. wait for the insert to return, indicating that logical
replication is done up to that point

Does anyone have a better way to measure logical replication
performance?

Regards,
Jeff Davis

On Sat, Jan 8, 2022 at 1:01 PM Jeff Davis <pgsql@j-davis.com> wrote:

On Sat, 2022-01-08 at 12:27 +0530, Amit Kapila wrote:

For Update/Delete, we do read the table first via
FindReplTupleInLocalRel(), so is there a need to check ACL_SELECT
before that?

If it's logically an update/delete, then I think ACL_UPDATE/DELETE is
the right one to check. Do you have a different opinion?

But shouldn't we do it the first time before accessing the table?

--
With Regards,
Amit Kapila.

On Sat, 2022-01-08 at 15:35 +0530, Amit Kapila wrote:

On Sat, Jan 8, 2022 at 1:01 PM Jeff Davis <pgsql@j-davis.com> wrote:

On Sat, 2022-01-08 at 12:27 +0530, Amit Kapila wrote:

For Update/Delete, we do read the table first via
FindReplTupleInLocalRel(), so is there a need to check ACL_SELECT
before that?

If it's logically an update/delete, then I think ACL_UPDATE/DELETE
is
the right one to check. Do you have a different opinion?

But shouldn't we do it the first time before accessing the table?

I'm not sure I follow the reasoning. Are you saying that, to logically
replay a simple DELETE, the subscription owner should have SELECT
privileges on the destination table?

Is there a way that a subscription owner could somehow exploit a DELETE
privilege to see the contents of a table on which they have no SELECT
privileges? Or is it purely an internal read, which is necessary for
any ordinary local DELETE/UPDATE anyway?

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

I'm not sure I follow the reasoning. Are you saying that, to logically
replay a simple DELETE, the subscription owner should have SELECT
privileges on the destination table?

We consider that DELETE WHERE <condition> requires SELECT privilege
on the column(s) read by the <condition>. I suppose that the point
here is to enforce the same privilege checks that occur in normal
SQL operation, so yes.

Is there a way that a subscription owner could somehow exploit a DELETE
privilege to see the contents of a table on which they have no SELECT
privileges?

BEGIN;
DELETE FROM tab WHERE col = 'foo';
-- note deletion count
ROLLBACK;

Now you have some information about whether "col" contains 'foo'.
Admittedly, it might be a pretty low-bandwidth way to extract data,
but we still regard it as a privilege issue.

regards, tom lane

... btw, I'd like to complain that this new test script consumes
a completely excessive amount of time. On my fairly-new primary
workstation:

[12:48:00] t/027_nosuperuser.pl ............... ok 22146 ms ( 0.02 usr 0.00 sys + 1.12 cusr 0.95 csys = 2.09 CPU)

The previously-slowest script in the subscription suite is

[12:48:23] t/100_bugs.pl ...................... ok 7048 ms ( 0.00 usr 0.00 sys + 2.85 cusr 0.99 csys = 3.84 CPU)

and the majority of the scripts clock in at more like 2 to 4 seconds.
So I don't think I'm out of line in saying that this test is consuming
an order of magnitude more time than is justified. I do not wish to
see this much time added to every check-world run till kingdom come
for this one feature/issue.

regards, tom lane

On Sat, 2022-01-08 at 12:57 -0500, Tom Lane wrote:

... btw, I'd like to complain that this new test script consumes
a completely excessive amount of time.

Should be fixed now; I brought the number of tests down from 100 to 14.
It's not running in 2 seconds on my machine, but it's in line with the
other tests.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

On Sat, 2022-01-08 at 12:57 -0500, Tom Lane wrote:

... btw, I'd like to complain that this new test script consumes
a completely excessive amount of time.

Should be fixed now; I brought the number of tests down from 100 to 14.
It's not running in 2 seconds on my machine, but it's in line with the
other tests.

Thanks, I appreciate that.

regards, tom lane

On Sat, Jan 8, 2022 at 2:38 AM Jeff Davis <pgsql@j-davis.com> wrote:

Committed.

I was just noticing that what was committed here didn't actually fix
the problem implied by the subject line. That is, non-superuser still
can't own subscriptions. To put that another way, there's no way for
the superuser to delegate the setup and administration of logical
replication to a non-superuser. That's a bummer.

Reading the thread, I'm not quite sure why we seemingly did all the
preparatory work and then didn't actually fix the problem. It was
previously proposed that we introduce a new predefined role
pg_create_subscriptions and allow users who have the privileges of
that predefined role to create and alter subscriptions. There are a
few issues with that which, however, seem fairly solvable to me:

1. Jeff pointed out that if you supply a connection string that is
going to try to access local files, you'd better have
pg_read_server_files, or else we should not let you use that
connection string. I guess that's mostly a function of which
parameters you specify, e.g. passfile, sslcert, sslkey, though maybe
for host it depends on whether the value starts with a slash. We might
need to think a bit here to make sure we get the rules right but it
seems like a pretty solvable problem.

2. There was also quite a bit of discussion of what to do if a user
who was previously eligible to own a subscription ceases to be
eligible, in particular around a superuser who is made into a
non-superuser, but the same problem would apply if you had
pg_create_subscriptions or pg_read_server_files and then lost it. My
vote is to not worry about it too much. Specifically, I think we
should certainly check whether the user has permission to create a
subscription before letting them do so, but we could handle the case
where the user already owns a subscription and tries to modify it by
either allowing or denying the operation and I think either of those
would be fine. I even think we could do one of those in some cases and
the other in other cases and as long as there is some principle to the
thing, it's fine. I argue that it's not a normal configuration and
therefore it doesn't have to work in a particularly useful way. It
shouldn't break the world in some horrendous way, but that's about as
good as it needs to be. I'd argue for example that DROP SUBSCRIPTION
could just check whether you own the object, and that ALTER
SUBSCRIPTION could check whether you own the object and, if you're
changing the connection string, also whether you would have privileges
to set that new connection string on a new subscription.

3. There was a bit of discussion of maybe wanting to allow users to
create subscriptions with some connection strings but not others,
perhaps by having some kind of intermediate object that owns the
connection string and is owned by a superuser or someone with lots of
privileges, and then letting a less-privileged user point a
subscription at that object. I agree that might be useful to somebody,
but I don't see why it's a hard requirement to get anything at all
done here. Right now, a subscription contains a connection string
directly. If in the future someone wants to introduce a CREATE
REPLICATION DESTINATION command (or whatever) and have a way to point
a subscription at a replication destination rather than a connection
string directly, cool. Or if someone wants to wire this into CREATE
SERVER somehow, also cool. But if you don't care about restricting
which IPs somebody can try to access by providing a connection string
of their choice, then you would be happy if we just did something
simple here and left this problem for another day.

I am very curious to know (a) why work on this was abandoned (perhaps
the answer is just lack of round tuits, in which case there is no more
to be said), and (b) what people think of (1)-(3) above, and (c)
whether anyone knows of further problems that need to be considered
here.

Thanks,

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

On Jan 18, 2023, at 11:38 AM, Robert Haas <robertmhaas@gmail.com> wrote:

I was just noticing that what was committed here didn't actually fix
the problem implied by the subject line. That is, non-superuser still
can't own subscriptions.

Not so. They can. See src/test/subscription/027_nosuperuser.pl

To put that another way, there's no way for
the superuser to delegate the setup and administration of logical
replication to a non-superuser.

True.

That's a bummer.

Also true.

Reading the thread, I'm not quite sure why we seemingly did all the
preparatory work and then didn't actually fix the problem.

Prior to the patch, if a superuser created a subscription, then later was demoted to non-superuser, the subscription apply workers still applied the changes with superuser force. So creating a superuser Alice, letting Alice create a subscription, then revoking superuser from Alice didn't accomplish anything interesting. But after the patch, it does. The superuser can now create non-superuser subscriptions. (I'm not sure this ability is well advertised.) But the problem of non-superuser roles creating non-superuser subscriptions is not solved.

From a security perspective, the bit that was solved may be the more important part; from a usability perspective, perhaps not.

It was
previously proposed that we introduce a new predefined role
pg_create_subscriptions and allow users who have the privileges of
that predefined role to create and alter subscriptions. There are a
few issues with that which, however, seem fairly solvable to me:

1. Jeff pointed out that if you supply a connection string that is
going to try to access local files, you'd better have
pg_read_server_files, or else we should not let you use that
connection string. I guess that's mostly a function of which
parameters you specify, e.g. passfile, sslcert, sslkey, though maybe
for host it depends on whether the value starts with a slash. We might
need to think a bit here to make sure we get the rules right but it
seems like a pretty solvable problem.

2. There was also quite a bit of discussion of what to do if a user
who was previously eligible to own a subscription ceases to be
eligible, in particular around a superuser who is made into a
non-superuser, but the same problem would apply if you had
pg_create_subscriptions or pg_read_server_files and then lost it. My
vote is to not worry about it too much. Specifically, I think we
should certainly check whether the user has permission to create a
subscription before letting them do so, but we could handle the case
where the user already owns a subscription and tries to modify it by
either allowing or denying the operation and I think either of those
would be fine. I even think we could do one of those in some cases and
the other in other cases and as long as there is some principle to the
thing, it's fine. I argue that it's not a normal configuration and
therefore it doesn't have to work in a particularly useful way. It
shouldn't break the world in some horrendous way, but that's about as
good as it needs to be. I'd argue for example that DROP SUBSCRIPTION
could just check whether you own the object, and that ALTER
SUBSCRIPTION could check whether you own the object and, if you're
changing the connection string, also whether you would have privileges
to set that new connection string on a new subscription.

3. There was a bit of discussion of maybe wanting to allow users to
create subscriptions with some connection strings but not others,
perhaps by having some kind of intermediate object that owns the
connection string and is owned by a superuser or someone with lots of
privileges, and then letting a less-privileged user point a
subscription at that object. I agree that might be useful to somebody,
but I don't see why it's a hard requirement to get anything at all
done here. Right now, a subscription contains a connection string
directly. If in the future someone wants to introduce a CREATE
REPLICATION DESTINATION command (or whatever) and have a way to point
a subscription at a replication destination rather than a connection
string directly, cool. Or if someone wants to wire this into CREATE
SERVER somehow, also cool. But if you don't care about restricting
which IPs somebody can try to access by providing a connection string
of their choice, then you would be happy if we just did something
simple here and left this problem for another day.

I am very curious to know (a) why work on this was abandoned (perhaps
the answer is just lack of round tuits, in which case there is no more
to be said)

Mostly, it was a lack of round-tuits. After the patch was committed, I quickly switched my focus elsewhere.

, and (b) what people think of (1)-(3) above

There are different ways of solving (1), and Jeff and I discussed them in Dec 2021. My recollection was that idea (3) was the cleanest. Other ideas might be simpler than (3), or they may just appear simpler but in truth turn into a can of worms. I don't know, since I never went as far as trying to implement either approach.

Idea (2) seems to contemplate non-superuser subscription owners as a theoretical thing, but it's quite real already. Again, see 027_nosuperuser.pl.

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

On Wed, Jan 18, 2023 at 3:26 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

Prior to the patch, if a superuser created a subscription, then later was demoted to non-superuser, the subscription apply workers still applied the changes with superuser force. So creating a superuser Alice, letting Alice create a subscription, then revoking superuser from Alice didn't accomplish anything interesting. But after the patch, it does. The superuser can now create non-superuser subscriptions. (I'm not sure this ability is well advertised.) But the problem of non-superuser roles creating non-superuser subscriptions is not solved.

Ah, OK, thanks for the clarification!

There are different ways of solving (1), and Jeff and I discussed them in Dec 2021. My recollection was that idea (3) was the cleanest. Other ideas might be simpler than (3), or they may just appear simpler but in truth turn into a can of worms. I don't know, since I never went as far as trying to implement either approach.

Idea (2) seems to contemplate non-superuser subscription owners as a theoretical thing, but it's quite real already. Again, see 027_nosuperuser.pl.

I think the solution to the problem of a connection string trying to
access local files is to just look at the connection string, decide
whether it does that, and if yes, require the owner to have
pg_read_server_files as well as pg_create_subscription. (3) is about
creating some more sophisticated and powerful solution to that
problem, but that seems like a nice-to-have, not something essential,
and a lot more complicated to implement.

I guess what I listed as (2) is not relevant since I didn't understand
correctly what the current state of things is.

Unless I'm missing something, it seems like this could be a quite small patch.

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

On Jan 18, 2023, at 12:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Unless I'm missing something, it seems like this could be a quite small patch.

I didn't like the idea of the create/alter subscription commands needing to parse the connection string and think about what it might do, because at some point in the future we might extend what things are allowed in that string, and we have to keep everything that contemplates that string in sync. I may have been overly hesitant to tackle that problem. Or maybe I just ran short of round tuits.

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

On Wed, Jan 18, 2023 at 3:58 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Jan 18, 2023, at 12:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Unless I'm missing something, it seems like this could be a quite small patch.

I didn't like the idea of the create/alter subscription commands needing to parse the connection string and think about what it might do, because at some point in the future we might extend what things are allowed in that string, and we have to keep everything that contemplates that string in sync. I may have been overly hesitant to tackle that problem. Or maybe I just ran short of round tuits.

I wouldn't be OK with writing our own connection string parser for
this purpose, but using PQconninfoParse seems OK. We still have to
embed knowledge of which connection string parameters can trigger
local file access, but that doesn't seem like a massive problem to me.
If we already had (or have) that logic someplace else, it would
probably make sense to reuse it, but if we don't, writing new logic
doesn't seem prohibitively scary. I'm not 100% confident of my ability
to get those rules right on the first try, but I feel like whatever
problems are there are just bugs that can be fixed with a few lines of
code changes. The basic idea that by looking at which connection
string properties are set we can tell what kinds of things the
connection string is going to do seems sound to me.

If there's some reason that it isn't, that would be good to discover
now rather than later.

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

On Wed, 2023-01-18 at 14:38 -0500, Robert Haas wrote:

I was just noticing that what was committed here didn't actually fix
the problem implied by the subject line. That is, non-superuser still
can't own subscriptions. To put that another way, there's no way for
the superuser to delegate the setup and administration of logical
replication to a non-superuser. That's a bummer.

Right, though as Mark pointed out, it does accomplish something even if
it's a bit unsatisfying. We could certainly do better here.

2. There was also quite a bit of discussion of what to do if a user
who was previously eligible to own a subscription ceases to be
eligible, in particular around a superuser who is made into a
non-superuser, but the same problem would apply

Correct, that's not a new problem, but exists in only a few places now.
Our privilege system is focused on "what action can the user take right
now?", and gets weirder when it comes to object ownership, which is a
more permanent thing.

Extending that system to a subscription object, which has its own
capabilities including a long-lived process, is cause for some
hesitation. I agree it's not necessarily a blocker.

3. There was a bit of discussion of maybe wanting to allow users to
create subscriptions with some connection strings but not others,

This was an alternative to trying to sanitize connection strings,
because it's a bit difficult to reason about what might be "safe"
connection strings for a non-superuser, because it's environment-
dependent. But if we do identify a reasonable set of sanitization
rules, we can proceed without 3.

I am very curious to know (a) why work on this was abandoned (perhaps
the answer is just lack of round tuits, in which case there is no
more
to be said), and (b) what people think of (1)-(3) above, and (c)
whether anyone knows of further problems that need to be considered
here.

(a) Mostly round-tuits. There are problems and questions; but there are
with any work, and they could be solved. Or, if they don't turn out to
be terribly serious, we could ignore them.

(b) When I pick this up again I would be inclined towards the
following: try to solve 4-5 (listed below) first, which are
independently useful; then look at both 1 and 3 to see which one
presents an agreeable solution faster. I'll probably ignore 2 because I
couldn't get agreement the last time around (I think Mark objected to
the idea of denying a drop in privileges).

(c) Let me add:

4. There are still differences between the subscription worker applying
a change and going through the ordinary INSERT paths, for instance with
RLS. Also solvable.

5. Andres raised in another thread the idea of switching to the table
owner when applying changes (perhaps in a
SECURITY_RESTRICTED_OPERATION?):

/messages/by-id/20230112033355.u5tiyr2bmuoc4jf4@awork3.anarazel.de

That seems related, and I like the idea.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Thu, 2023-01-19 at 10:45 -0500, Robert Haas wrote:

I wouldn't be OK with writing our own connection string parser for
this purpose, but using PQconninfoParse seems OK. We still have to
embed knowledge of which connection string parameters can trigger
local file access, but that doesn't seem like a massive problem to
me.

Another idea (I discussed with Andres some time ago) was to have an
option to libpq to turn off file access entirely. That could be a new
API function or a new connection option.

That would be pretty valuable by itself. Though we might want to
support a way to pass SSL keys as values rather than file paths, so
that we can still do SSL.

So perhaps the answer is that it will be a small patch to get non-
superuser subscription owners, but we need three or four preliminary
patches first.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Thu, Jan 19, 2023 at 1:40 PM Jeff Davis <pgsql@j-davis.com> wrote:

On Thu, 2023-01-19 at 10:45 -0500, Robert Haas wrote:

I wouldn't be OK with writing our own connection string parser for
this purpose, but using PQconninfoParse seems OK. We still have to
embed knowledge of which connection string parameters can trigger
local file access, but that doesn't seem like a massive problem to
me.

Another idea (I discussed with Andres some time ago) was to have an
option to libpq to turn off file access entirely. That could be a new
API function or a new connection option.

That would be pretty valuable by itself. Though we might want to
support a way to pass SSL keys as values rather than file paths, so
that we can still do SSL.

Maybe all of that would be useful, but it doesn't seem that mandatory.

So perhaps the answer is that it will be a small patch to get non-
superuser subscription owners, but we need three or four preliminary
patches first.

I guess I'm not quite seeing it. Why can't we write a small patch to
get this working right now, probably in a few hours, and deal with any
improvements that people want at a later time?

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

On Thu, 2023-01-19 at 14:11 -0500, Robert Haas wrote:

I guess I'm not quite seeing it. Why can't we write a small patch to
get this working right now, probably in a few hours, and deal with
any
improvements that people want at a later time?

To me, it's worrisome when there are more than a few loose ends, and
here it seems like there are more like five. No single issue is a
blocker, but I believe we'd end up with a better user-facing solution
if we solved a couple of these lower-level issues (and think a little
more about the other ones) before we expose new functionality to the
user.

The predefined role is probably the biggest user-facing part of the
change. Does it mean that members can create any number of any kind of
subscription? If so it may be hard to tighten down later, because we
don't know what existing setups might break.

Perhaps we can just permit a superuser to "ALTER SUBSCRIPTION ... OWNER
TO <non-super>", which makes it simpler to use while still leaving the
responisbility with the superuser to get it right. Maybe we even block
the user from altering their own subscription (would be weird but not
much weirder than what we have now)? I don't know if that solves the
problem you're trying to solve, but it seems lower-risk.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

Hi,

On 2023-01-19 10:45:35 -0500, Robert Haas wrote:

On Wed, Jan 18, 2023 at 3:58 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Jan 18, 2023, at 12:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Unless I'm missing something, it seems like this could be a quite small patch.

I didn't like the idea of the create/alter subscription commands needing to parse the connection string and think about what it might do, because at some point in the future we might extend what things are allowed in that string, and we have to keep everything that contemplates that string in sync. I may have been overly hesitant to tackle that problem. Or maybe I just ran short of round tuits.

I wouldn't be OK with writing our own connection string parser for
this purpose, but using PQconninfoParse seems OK. We still have to
embed knowledge of which connection string parameters can trigger
local file access, but that doesn't seem like a massive problem to me.

If we already had (or have) that logic someplace else, it would
probably make sense to reuse it

We hve. See at least postgres_fdw's check_conn_params(), dblink's
dblink_connstr_check() and dblink_security_check().

As part of the fix for /messages/by-id/20220925232237.p6uskba2dw6fnwj2@awork3.anarazel.de
I am planning to introduce a bunch of server side helpers for dealing with
libpq (for establishing a connection while accepting interrupts). We could try
to centralize knowledge for those checks there.

The approach of checking, after connection establishment (see
dblink_security_check()), that we did in fact use the specified password,
scares me somewhat. See also below.

The basic idea that by looking at which connection string properties are set
we can tell what kinds of things the connection string is going to do seems
sound to me.

I don't think you *can* check it purely based on existing connection string
properties, unfortunately. Think of e.g. a pg_hba.conf line of "local all user
peer" (quite reasonable config) or "host all all 127.0.0.1/32 trust" (less so).

Hence the hack with dblink_security_check().

I think there might be a discussion somewhere about adding an option to force
libpq to not use certain auth methods, e.g. plaintext password/md5. It's
possible this could be integrated.

Greetings,

Andres Freund

Hi,

On 2023-01-19 17:16:20 -0800, Jeff Davis wrote:

The predefined role is probably the biggest user-facing part of the
change. Does it mean that members can create any number of any kind of
subscription?

I don't think we need to support complicated restriction schemes around this
now. I'm sure such needs exist, but I think there's more places where a simple
"allowed/not allowed" suffices.

You'd presumably just grant such a permission to "pseudo superuser"
users. They can typically do a lot of bad things already, so I don't really
see the common need to prevent them from creating many subscriptions.

If so it may be hard to tighten down later, because we don't know what
existing setups might break.

Presumably the unlimited number of subs case would still exist as an option
later - so I don't see the problem?

Perhaps we can just permit a superuser to "ALTER SUBSCRIPTION ... OWNER
TO <non-super>", which makes it simpler to use while still leaving the
responisbility with the superuser to get it right. Maybe we even block
the user from altering their own subscription (would be weird but not
much weirder than what we have now)? I don't know if that solves the
problem you're trying to solve, but it seems lower-risk.

That seems to not really get us very far. It's hard to use for users, and hard
to make secure for the hosted PG providers.

Greetings,

Andres Freund

On Thu, 2023-01-19 at 17:51 -0800, Andres Freund wrote:

I don't think we need to support complicated restriction schemes
around this
now. I'm sure such needs exist, but I think there's more places where
a simple
"allowed/not allowed" suffices.

If we did follow a path like 3 (having some kind of other object
represent the connection string), then it would create two different
kinds of subscriptions that might be controlled different ways, and
there might be some rough edges. Might also be fine, or we might never
pursue 3.

I feel like my words are being interpreted as though I don't want this
feature. I do, and I'm happy Robert re-raised it. I'm just trying to
answer his questions about why I set the work down, which is that I
felt some groundwork should be done before proceeding to a documented
feature, and I still feel that's the right thing.

But (a) that's not a very strong objection; and (b) my efforts are
better spent doing some of that groundwork than arguing about the order
in which the work should be done. So, time permitting, I may be able to
put out a patch or two for the next 'fest.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Thu, Jan 19, 2023 at 8:46 PM Andres Freund <andres@anarazel.de> wrote:

I wouldn't be OK with writing our own connection string parser for
this purpose, but using PQconninfoParse seems OK. We still have to
embed knowledge of which connection string parameters can trigger
local file access, but that doesn't seem like a massive problem to me.

If we already had (or have) that logic someplace else, it would
probably make sense to reuse it

We hve. See at least postgres_fdw's check_conn_params(), dblink's
dblink_connstr_check() and dblink_security_check().

That's not the same thing. It doesn't know anything about other
parameters that might try to consult a local file, like sslcert,
sslkey, sslrootcert, sslca, sslcrl, sslcrldir, and maybe service.
Maybe you want to argue we don't need that, but that's what the
earlier discussion was about.

As part of the fix for /messages/by-id/20220925232237.p6uskba2dw6fnwj2@awork3.anarazel.de
I am planning to introduce a bunch of server side helpers for dealing with
libpq (for establishing a connection while accepting interrupts). We could try
to centralize knowledge for those checks there.

Maybe. We could also add something into libpq, as Jeff proposed, e.g.
a new connection parameter
the_other_connection_parameters_might_try_to_trojan_the_local_host=1
blocks all that stuff from doing anything.

The approach of checking, after connection establishment (see
dblink_security_check()), that we did in fact use the specified password,
scares me somewhat. See also below.

Yes, I find that extremely dubious. It blocks things that you might
want to do for legitimate reasons, including things that might be more
secure than using a password. And there's no guarantee that it
accomplishes the intended objective either. The stated motivation for
that restriction was. I believe, that we don't want the outbound
connection to rely on the privileges available from the context in
which PostgreSQL itself is running -- but for all we know the remote
side has an IP filter that only allows the PostgreSQL host and no
others. Moreover, it relies on us knowing what the behavior of the
remote server is, even though we have no way of knowing that that
server shares our security interests.

Worse still, I have always felt that the security vulnerability that
led to these controls being installed is pretty much fabricated: it's
an imaginary problem. Today I went back and found the original CVE at
https://nvd.nist.gov/vuln/detail/CVE-2007-3278 and it seems that at
least one other person agrees. The Red Hat vendor statement on that
page says: "Red Hat does not consider this do be a security issue.
dblink is disabled in default configuration of PostgreSQL packages as
shipped with Red Hat Enterprise Linux versions 2.1, 3, 4 and 5, and it
is a configuration decision whether to grant local users arbitrary
access." I think whoever wrote that has an excellent point. I'm unable
to discern any legitimate security purpose for this restriction. What
I think it mostly does is (a) inconvenience users or (b) force them to
rely on a less-secure authentication method than they would otherwise
have chosen.

The basic idea that by looking at which connection string properties are set
we can tell what kinds of things the connection string is going to do seems
sound to me.

I don't think you *can* check it purely based on existing connection string
properties, unfortunately. Think of e.g. a pg_hba.conf line of "local all user
peer" (quite reasonable config) or "host all all 127.0.0.1/32 trust" (less so).

Hence the hack with dblink_security_check().

I think there might be a discussion somewhere about adding an option to force
libpq to not use certain auth methods, e.g. plaintext password/md5. It's
possible this could be integrated.

I still think you're talking about a different problem here. I'm
talking about the problem of knowing whether local files are going to
be accessed by the connection string.

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

On Fri, Jan 20, 2023 at 8:25 AM Robert Haas <robertmhaas@gmail.com> wrote:

I still think you're talking about a different problem here. I'm
talking about the problem of knowing whether local files are going to
be accessed by the connection string.

So here's a dumb patch for this. At least in my mind, the connection
string sanitization/validation is the major design problem here, and
I'm not at all sure that what I did in the attached patch is right.
But let's talk about that. This approach is inspired by Jeff's
comments about local file access upthread, but as Andres pointed out,
that's a completely different set of things than we worry about in
other places. I'm not quite sure what the right model is here.

This patch incidentally allows ALTER SUBSCRIPTION .. SKIP for any
subscription owner, removing the existing check that limits that
operation to superusers and replacing it with nothing. I can't really
see why this needs to be any more restricted than that, and
regrettably neither the check in the existing code nor the commit that
added it have any comments explaining the logic behind that check. If,
for example, skipping a subscription could lead to a server crash,
that would be a reason to restrict the feature to superusers (or
revert it). If it's just a case of the operation being maybe not the
right thing to do, that's not a sufficient reason to restrict it to
superusers. This change is really independent of the rest of the patch
and, if we want to do this, I will separate it into its own patch. But
since this is just for discussion, I didn't worry about that right
now.

Aside from the above, I don't yet see a problem here that I would
consider to be serious enough that we couldn't proceed. I'll try to
avoid too much repetition of what's already been said on this topic,
but I do want to add that I think that creating subscriptions is
properly viewed as a *slightly* scary operation, not a *very* scary
operation. It lets you do two things that you couldn't otherwise. One
is get background processes running that take up process slots and
consume resources -- but note that your ability to consume resources
with however many normal database connections you can make is
virtually unlimited. The other thing it lets you do is poke around the
network, maybe figure out whether some ports are open or closed, and
try to replicate data from any accessible servers you can find, which
could include ports or servers that you can't access directly. I think
that the superuser will be in a good position to evaluate whether that
is a risk in a certain environment or not, and I think many superusers
will conclude that it isn't a big risk. I think that the main
motivation for NOT handing out pg_create_subscription will turn out to
be administrative rather than security-related i.e. they'll want to be
something that falls under their authority rather than someone else's.

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

Hi,

On 2023-01-20 08:25:46 -0500, Robert Haas wrote:

Worse still, I have always felt that the security vulnerability that
led to these controls being installed is pretty much fabricated: it's
an imaginary problem. Today I went back and found the original CVE at
https://nvd.nist.gov/vuln/detail/CVE-2007-3278 and it seems that at
least one other person agrees. The Red Hat vendor statement on that
page says: "Red Hat does not consider this do be a security issue.
dblink is disabled in default configuration of PostgreSQL packages as
shipped with Red Hat Enterprise Linux versions 2.1, 3, 4 and 5, and it
is a configuration decision whether to grant local users arbitrary
access." I think whoever wrote that has an excellent point. I'm unable
to discern any legitimate security purpose for this restriction. What
I think it mostly does is (a) inconvenience users or (b) force them to
rely on a less-secure authentication method than they would otherwise
have chosen.

FWIW, I've certainly seen situations where having the checks prevented easy
paths to privilege escalations. That's not to say that I like the checks, but
I also don't think we can get away without them (or a better replacement, of
course).

There are good reasons to have 'peer' authentication set up for the user
running postgres, so admin scripts can connect without issues. Which
unfortunately then also means that postgres_fdw etc can connect to the current
database as superuser, without that check. Which imo clearly is an issue.

Why do you think this is a fabricated issue?

The solution we have is quite bad, of course. Just because the user isn't a
superuser "immediately" doesn't mean it doesn't have the rights to become
one somehow.

The basic idea that by looking at which connection string properties are set
we can tell what kinds of things the connection string is going to do seems
sound to me.

I don't think you *can* check it purely based on existing connection string
properties, unfortunately. Think of e.g. a pg_hba.conf line of "local all user
peer" (quite reasonable config) or "host all all 127.0.0.1/32 trust" (less so).

Hence the hack with dblink_security_check().

I think there might be a discussion somewhere about adding an option to force
libpq to not use certain auth methods, e.g. plaintext password/md5. It's
possible this could be integrated.

I still think you're talking about a different problem here. I'm
talking about the problem of knowing whether local files are going to
be accessed by the connection string.

Why is this only about local files, rather than e.g. also using the local
user?

Greetings,

Andres Freund

Hi,

On 2023-01-20 11:08:54 -0500, Robert Haas wrote:

/*
- * Validate connection info string (just try to parse it)
+ * Validate connection info string, and determine whether it might cause
+ * local filesystem access to be attempted.
+ *
+ * If the connection string can't be parsed, this function will raise
+ * an error and will not return. If it can, it will return true if local
+ * filesystem access may be attempted and false otherwise.
*/
-static void
+static bool
libpqrcv_check_conninfo(const char *conninfo)
{
PQconninfoOption *opts = NULL;
+	PQconninfoOption *opt;
char	   *err = NULL;
+	bool		result = false;

opts = PQconninfoParse(conninfo, &err);
if (opts == NULL)
@@ -267,7 +274,40 @@ libpqrcv_check_conninfo(const char *conninfo)
errmsg("invalid connection string syntax: %s", errcopy)));
}

+	for (opt = opts; opt->keyword != NULL; ++opt)
+	{
+		/* Ignore connection options that are not present. */
+		if (opt->val == NULL)
+			continue;
+
+		/* For all these parameters, the value is a local filename. */
+		if (strcmp(opt->keyword, "passfile") == 0 ||
+			strcmp(opt->keyword, "sslcert") == 0 ||
+			strcmp(opt->keyword, "sslkey") == 0 ||
+			strcmp(opt->keyword, "sslrootcert") == 0 ||
+			strcmp(opt->keyword, "sslcrl") == 0 ||
+			strcmp(opt->keyword, "sslcrldir") == 0 ||
+			strcmp(opt->keyword, "service") == 0)
+		{
+			result = true;
+			break;
+		}

Do we need to think about 'options' allowing anything bad? I don't
immediately* see a problem, but ...

+
+		/*
+		 * For the host parameter, the value might be a local filename.
+		 * It might also be a reference to the local host's abstract UNIX
+		 * socket namespace, which we consider equivalent to a local pathname
+		 * for security purporses.
+		 */
+		if (strcmp(opt->keyword, "host") == 0 && is_unixsock_path(opt->val))
+		{
+			result = true;
+			break;
+		}
+	}

Hm, what about kerberos / gss / SSPI? Aren't those essentially also tied to
the local filesystem / user?

Greetings,

Andres Freund

On Sat, 2023-01-21 at 14:01 -0800, Andres Freund wrote:

There are good reasons to have 'peer' authentication set up for the
user
running postgres, so admin scripts can connect without issues. Which
unfortunately then also means that postgres_fdw etc can connect to
the current
database as superuser, without that check. Which imo clearly is an
issue.

Perhaps we should have a way to directly turn on/off authentication
methods in libpq through API functions and/or options?

This reminds me of the "channel_binding=required" option. We considered
some similar alternatives for that feature.

Why is this only about local files, rather than e.g. also using the
local
user?

It's not, but we happen to already have pg_read_server_files, and it
makes sense to use that at least for files referenced directly in the
connection string. You're right that it's incomplete, and also that it
doesn't make a lot of sense for files accessed indirectly.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

Hi,

On 2023-01-22 09:05:27 -0800, Jeff Davis wrote:

On Sat, 2023-01-21 at 14:01 -0800, Andres Freund wrote:

There are good reasons to have 'peer' authentication set up for the
user
running postgres, so admin scripts can connect without issues. Which
unfortunately then also means that postgres_fdw etc can connect to
the current
database as superuser, without that check. Which imo clearly is an
issue.

Perhaps we should have a way to directly turn on/off authentication
methods in libpq through API functions and/or options?

Yes. There's an in-progress patch adding, I think, pretty much what is
required here:
/messages/by-id/9e5a8ccddb8355ea9fa4b75a1e3a9edc88a70cd3.camel@vmware.com

require_auth=a,b,c

I think an allowlist approach is the right thing for the subscription (and
postgres_fdw/dblink) use case, otherwise we'll add some auth method down the
line without updating what's disallowed in the subscription code.

Why is this only about local files, rather than e.g. also using the local
user?

It's not, but we happen to already have pg_read_server_files, and it
makes sense to use that at least for files referenced directly in the
connection string. You're right that it's incomplete, and also that it
doesn't make a lot of sense for files accessed indirectly.

I just meant that we need to pay attention to user-based permissions as well.

Greetings,

Andres Freund

On Sat, Jan 21, 2023 at 5:01 PM Andres Freund <andres@anarazel.de> wrote:

There are good reasons to have 'peer' authentication set up for the user
running postgres, so admin scripts can connect without issues. Which
unfortunately then also means that postgres_fdw etc can connect to the current
database as superuser, without that check. Which imo clearly is an issue.

Why do you think this is a fabricated issue?

Well, if I have a bunch of PostgreSQL machines on the network that all
allow each other to log in without requiring anything much in the way
of passwords or closely-guarded SSL certificates or anything, and then
I grant to the users on those machines the right to make connections
to the other machines using arbitrary connection strings, whose fault
is it when security is compromised? We seem to be taking the policy
that it's PostgreSQL's fault if it doesn't block something bad from
happening there, but it seems to me that if you gate incoming
PostgreSQL connections only by source IP address and then also give
unprivileged users the ability to choose their source IP address, you
should expect to have a problem.

I will admit that this is not an open-and-shut case, because a
passwordless login back to the bootstrap superuser account from the
local machine is a pretty common scenario and doesn't feel
intrinsically unreasonable to me, and I hadn't thought about that as a
potential attack vector.

However, I still think there's a problem with putting all the
responsibility on PostgreSQL. The problem, specifically, is that we're
speculating wildly as to the user's intent. If we say, as we currently
do, that we're only going to allow connections if they require a
password, then we're making a judgement that the superuser couldn't
have intended to allow the postgres_fdw to make a passwordless
connection. On the other hand, if we say, as we also currently do,
that the postgres_fdw user is free to set the sslcert parameter to
anything they like, then we're making a judgement that the superuser
is totally OK with that being set to any file on the local filesystem.
Neither of those conclusions seems sound to me. The superuser may, or
may not, have intended to allow passwordless logins, and they may, or
may not, have intended for any SSL certificates stored locally to be
usable by outbound connection attempts.

And that's what I really dislike about the you-must-use-a-password
rule that we have right now. It embeds a policy decision about what
users do or do not want to allow. We've uncritically copied that
policy decision around to more and more places, and we've added
workarounds in some places for the fact that, well, you know, it might
not actually be what everybody wants (6136e94d), but it doesn't seem
like we've ever really acknowledged that we *made* a policy decision.
And that means we haven't really had a debate about the merits of this
*particular* rule, which seems to me to be highly debatable. It looks
to me like there's both stuff you might not want to allow that this
rule does not block, and also stuff you might want to allow that this
rule does block, and also that different people can want different
things yet this rule applies uniformly to everyone.

I still think you're talking about a different problem here. I'm
talking about the problem of knowing whether local files are going to
be accessed by the connection string.

Why is this only about local files, rather than e.g. also using the local
user?

Because there's nothing you can do about the local-user case.

If I'm asked to attempt to connect to a PostgreSQL server, and I
choose to do that, and the connection succeeds, all I know is that the
connection actually succeeded. I do not know why the remote machine
chose to accept the connection. If I supplied a password or an SSL
certificate or some such thing, then it seems likely that the remote
machine accepted that connection because I supplied that particular
password or SSL certificate, but it could also be because the remote
machine accepts all connections from Robert, or all connections
whatsoever, or all connections on Mondays. I just don't know. If I'm
worried that the person is asking me to make the connection is trying
to trick me into doing something that they can't do themselves, I
could refuse to read a password from a local password store or an SSL
certificate from a local certificate file or otherwise refuse to do
anything special to try to get their connection request accepted, and
then if it does get accepted anyway, I know that they weren't relying
on any of those resources that I refused to use. But, if I attempt a
plain vanilla, totally password-less connection and it works, I have
no way of knowing whether that happened because I'm Robert or for some
other reason.

To put that another way, if I'm making a connection on behalf of an
untrusted party, I can choose not to supply an SSL certificate, or not
to supply a password. But I cannot choose to not be myself.

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

On Sun, Jan 22, 2023 at 8:52 PM Andres Freund <andres@anarazel.de> wrote:

Perhaps we should have a way to directly turn on/off authentication
methods in libpq through API functions and/or options?

Yes. There's an in-progress patch adding, I think, pretty much what is
required here:
/messages/by-id/9e5a8ccddb8355ea9fa4b75a1e3a9edc88a70cd3.camel@vmware.com

require_auth=a,b,c

I think an allowlist approach is the right thing for the subscription (and
postgres_fdw/dblink) use case, otherwise we'll add some auth method down the
line without updating what's disallowed in the subscription code.

So what would we do here, exactly? We could force a require_auth
parameter into the provided connection string, although I'm not quite
sure of the details there, but what value should we force? Is that
going to be something hard-coded, or something configurable? If
configurable, where does that configuration get stored?

Regardless, this only allows connection strings to be restricted along
one axis: authentication type. If you want to let people connect only
to a certain subnet or whatever, you're still out of luck.

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

On Sat, Jan 21, 2023 at 5:11 PM Andres Freund <andres@anarazel.de> wrote:

+             /* For all these parameters, the value is a local filename. */
+             if (strcmp(opt->keyword, "passfile") == 0 ||
+                     strcmp(opt->keyword, "sslcert") == 0 ||
+                     strcmp(opt->keyword, "sslkey") == 0 ||
+                     strcmp(opt->keyword, "sslrootcert") == 0 ||
+                     strcmp(opt->keyword, "sslcrl") == 0 ||
+                     strcmp(opt->keyword, "sslcrldir") == 0 ||
+                     strcmp(opt->keyword, "service") == 0)
+             {
+                     result = true;
+                     break;
+             }

Do we need to think about 'options' allowing anything bad? I don't
immediately* see a problem, but ...

If it is, it'd be a different kind of bad. What these parameters all
have in common is that they allow you to read some local file and
maybe benefit from that during the authentication process. options
doesn't let you to do anything like that, and by definition kind of
can't, because it's just a string to be sent to the remote server. As
I noted in my other responses, the local superuser could want to
impose any arbitrary restriction the connection strings users can
choose, and so it's just as plausible that they want to restrict
options as anything else; but this test is about something more
specific.

+             /*
+              * For the host parameter, the value might be a local filename.
+              * It might also be a reference to the local host's abstract UNIX
+              * socket namespace, which we consider equivalent to a local pathname
+              * for security purporses.
+              */
+             if (strcmp(opt->keyword, "host") == 0 && is_unixsock_path(opt->val))
+             {
+                     result = true;
+                     break;
+             }
+     }

Hm, what about kerberos / gss / SSPI? Aren't those essentially also tied to
the local filesystem / user?

Uh, I don't know. It doesn't seem so directly true as in these cases,
but what's your thought?

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

Hi,

On 2023-01-23 11:34:32 -0500, Robert Haas wrote:

I will admit that this is not an open-and-shut case, because a
passwordless login back to the bootstrap superuser account from the
local machine is a pretty common scenario and doesn't feel
intrinsically unreasonable to me, and I hadn't thought about that as a
potential attack vector.

I think it's 90% of the problem... There's IMO no particularly good
alternative to a passwordless login for the bootstrap superuser, and it's the
account you least want to expose...

I still think you're talking about a different problem here. I'm
talking about the problem of knowing whether local files are going to
be accessed by the connection string.

Why is this only about local files, rather than e.g. also using the local
user?

Because there's nothing you can do about the local-user case.

If I'm asked to attempt to connect to a PostgreSQL server, and I
choose to do that, and the connection succeeds, all I know is that the
connection actually succeeded.

Well, there is PQconnectionUsedPassword()... Not that it's a great answer.

Greetings,

Andres Freund

On Mon, Jan 23, 2023 at 8:35 AM Robert Haas <robertmhaas@gmail.com> wrote:

I will admit that this is not an open-and-shut case, because a
passwordless login back to the bootstrap superuser account from the
local machine is a pretty common scenario and doesn't feel
intrinsically unreasonable to me, and I hadn't thought about that as a
potential attack vector.

It seems to me like that's _the_ primary attack vector. I think I
agree with you that the password requirement is an overly large
hammer, but I don't think it's right (or safe/helpful to DBAs reading
along) to describe it as a manufactured concern.

If I'm asked to attempt to connect to a PostgreSQL server, and I
choose to do that, and the connection succeeds, all I know is that the
connection actually succeeded. I do not know why the remote machine
chose to accept the connection. If I supplied a password or an SSL
certificate or some such thing, then it seems likely that the remote
machine accepted that connection because I supplied that particular
password or SSL certificate, but it could also be because the remote
machine accepts all connections from Robert, or all connections
whatsoever, or all connections on Mondays. I just don't know.

As of SYSTEM_USER, I think this is no longer the case -- after
connection establishment, you can ask the server who was authenticated
and why. (It doesn't explain why you were authorized to be that
particular user, but that seems maybe less important wen you're trying
to disallow ambient authentication.)

If my require_auth patchset gets in, you'd be able to improve on this
by rejecting all ambient forms of authentication at the protocol level
(require_auth=password,md5,scram-sha-256). You could even go a step
further and disable ambient transport authentication
(sslcertmode=disable gssencmode=disable), which keeps a proxied
connection from making use of a client cert or a Kerberos cache. But
for postgres_fdw, at least, that carries a risk of disabling current
use cases. Stephen and I had a discussion about one such case in the
Kerberos delegation thread [1]/messages/by-id/23337c51-7a48-d5a8-569d-ef3ce6fe235f@timescale.com.

It doesn't help you if you want to differentiate one form of ambient
auth (trust/peer/etc.) from another, since they look the same to the
protocol. But for e.g. postgres_fdw I'm not sure why you would want to
differentiate between those cases, because they all seem bad.

To put that another way, if I'm making a connection on behalf of an
untrusted party, I can choose not to supply an SSL certificate, or not
to supply a password. But I cannot choose to not be myself.

(IMO, you're driving towards a separation of the proxy identity from
the user identity. Other protocols do that too.)

--Jacob

[1]: /messages/by-id/23337c51-7a48-d5a8-569d-ef3ce6fe235f@timescale.com

Hi,

On 2023-01-23 12:39:50 -0500, Robert Haas wrote:

On Sun, Jan 22, 2023 at 8:52 PM Andres Freund <andres@anarazel.de> wrote:

Perhaps we should have a way to directly turn on/off authentication
methods in libpq through API functions and/or options?

Yes. There's an in-progress patch adding, I think, pretty much what is
required here:
/messages/by-id/9e5a8ccddb8355ea9fa4b75a1e3a9edc88a70cd3.camel@vmware.com

require_auth=a,b,c

I think an allowlist approach is the right thing for the subscription (and
postgres_fdw/dblink) use case, otherwise we'll add some auth method down the
line without updating what's disallowed in the subscription code.

So what would we do here, exactly? We could force a require_auth
parameter into the provided connection string, although I'm not quite
sure of the details there

If we parse the connection string first, we can ensure that our values take
precedence, that shouldn't be an issue, I think.

, but what value should we force? Is that going to be something hard-coded,
or something configurable? If configurable, where does that configuration
get stored?

I would probably start with something hardcoded, perhaps with an adjusted
value depending on things like pg_read_server_files.

I'd say just allowing password (whichever submethod), ssl is a good start,
with something like your existing code to prevent file access for ssl unless
pg_read_server_files is granted.

I don't think kerberos, gss, peer, sspi would be safe.

Regardless, this only allows connection strings to be restricted along
one axis: authentication type. If you want to let people connect only
to a certain subnet or whatever, you're still out of luck.

True. But I think it'd get us a large percentage of the use cases.

Greetings,

Andres Freund

On Mon, Jan 23, 2023 at 1:26 PM Andres Freund <andres@anarazel.de> wrote:

If I'm asked to attempt to connect to a PostgreSQL server, and I
choose to do that, and the connection succeeds, all I know is that the
connection actually succeeded.

Well, there is PQconnectionUsedPassword()... Not that it's a great answer.

Sure, but that's making an inference about why the remote side did
what it did. It's not fantastic to have a security model that relies
on connecting to a server chosen by the user and having it tell us
truthfully whether or not it relied on the password. Granted, it won't
lie unless it's been hacked, and we're trying to protect it, not
ourselves, so the only thing that happens if it does lie is that it
gets hacked a second time, so I guess there's no real vulnerability?
But I feel like we'd be on far sounder footing if we our security
policy were based on deciding what we are willing to do (are we
willing to read that file? are we willing to attempt that
authentication method?) and before we actually do it, rather than on
trying to decide after-the-fact whether what we did is OK based on
what the remote side tells us about how things turned out.

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

Hi,

On 2023-01-23 10:27:27 -0800, Jacob Champion wrote:

On Mon, Jan 23, 2023 at 8:35 AM Robert Haas <robertmhaas@gmail.com> wrote:

I will admit that this is not an open-and-shut case, because a
passwordless login back to the bootstrap superuser account from the
local machine is a pretty common scenario and doesn't feel
intrinsically unreasonable to me, and I hadn't thought about that as a
potential attack vector.

It seems to me like that's _the_ primary attack vector. I think I
agree with you that the password requirement is an overly large
hammer, but I don't think it's right (or safe/helpful to DBAs reading
along) to describe it as a manufactured concern.

+1

If I'm asked to attempt to connect to a PostgreSQL server, and I
choose to do that, and the connection succeeds, all I know is that the
connection actually succeeded. I do not know why the remote machine
chose to accept the connection. If I supplied a password or an SSL
certificate or some such thing, then it seems likely that the remote
machine accepted that connection because I supplied that particular
password or SSL certificate, but it could also be because the remote
machine accepts all connections from Robert, or all connections
whatsoever, or all connections on Mondays. I just don't know.

As of SYSTEM_USER, I think this is no longer the case -- after
connection establishment, you can ask the server who was authenticated
and why. (It doesn't explain why you were authorized to be that
particular user, but that seems maybe less important wen you're trying
to disallow ambient authentication.)

There's not enough documentation for SYSTEM_USER imo.

You could even go a step further and disable ambient transport
authentication (sslcertmode=disable gssencmode=disable), which keeps a
proxied connection from making use of a client cert or a Kerberos cache. But
for postgres_fdw, at least, that carries a risk of disabling current use
cases. Stephen and I had a discussion about one such case in the Kerberos
delegation thread [1].

I did not find that very convincing for today's code. The likelihood of
something useful being prevented seems far far lower than preventing privilege
leakage...

It doesn't help you if you want to differentiate one form of ambient
auth (trust/peer/etc.) from another, since they look the same to the
protocol. But for e.g. postgres_fdw I'm not sure why you would want to
differentiate between those cases, because they all seem bad.

It might be possible to teach libpq to differentiate peer from trust (by
disabling passing the current user), or we could tell the server via an option
to disable peer. But as you say, I don't think it'd buy us much.

Greetings,

Andres Freund

On Mon, Jan 23, 2023 at 1:27 PM Jacob Champion <jchampion@timescale.com> wrote:

On Mon, Jan 23, 2023 at 8:35 AM Robert Haas <robertmhaas@gmail.com> wrote:

I will admit that this is not an open-and-shut case, because a
passwordless login back to the bootstrap superuser account from the
local machine is a pretty common scenario and doesn't feel
intrinsically unreasonable to me, and I hadn't thought about that as a
potential attack vector.

It seems to me like that's _the_ primary attack vector. I think I
agree with you that the password requirement is an overly large
hammer, but I don't think it's right (or safe/helpful to DBAs reading
along) to describe it as a manufactured concern.

First, sorry about the wording. I try to get it right, but sometimes I don't.

Second, the reason why I described it as a manufactured issue is
because it's a bit like asking someone to stand under a ladder and
then complaining when they get hit in the head by a falling object.
It's not that I think it's good for people to get a free exploit to
superuser, or to get hit in the head by falling objects. It's just
that you can't have the things that together lead to some outcome
without also getting the outcome. It seems to me that we basically let
the malicious connection to the target host succeed, and then say ...
oh, never mind, we may have made this connection under false
pretenses, so we shan't use it after all. What I was attempting to
argue is that we shouldn't let things get that far. Either the victim
should be able to protect itself from the malicious connection, or the
connection attempt shouldn't be allowed in the first place, or both.
Blocking the connection attempt after the fact feels like too little,
too late.

For instance, what if the connection string itself caused SQL to be
executed on the remote side, as in the case of target_session_attrs?
Or what if we got those logon triggers that people have been wanting
for years? Or what if the remote server speaks the PostgreSQL protocol
but isn't really PostgreSQL and does ... whatever ... when you just
connect to it?

As of SYSTEM_USER, I think this is no longer the case -- after
connection establishment, you can ask the server who was authenticated
and why. (It doesn't explain why you were authorized to be that
particular user, but that seems maybe less important wen you're trying
to disallow ambient authentication.)

I think this is too after-the-fact, as discussed above.

If my require_auth patchset gets in, you'd be able to improve on this
by rejecting all ambient forms of authentication at the protocol level
(require_auth=password,md5,scram-sha-256). You could even go a step
further and disable ambient transport authentication
(sslcertmode=disable gssencmode=disable), which keeps a proxied
connection from making use of a client cert or a Kerberos cache. But
for postgres_fdw, at least, that carries a risk of disabling current
use cases. Stephen and I had a discussion about one such case in the
Kerberos delegation thread [1].

Yes, this is why I think that the system administrator needs to have
some control over policy, instead of just having a hard-coded rule
that applies to everyone.

I'm not completely sure that this is good enough in terms of blocking
the attack as early as I think we should. This is all happening in the
midst of a connection attempt. If the remote server says, "hey, what's
your password?" and we refuse to answer that question, well that seems
somewhat OK. But what if we're hoping to be asked for a password and
the remote server doesn't ask? Then we don't find out that things
aren't right until after we've already logged in, and that gets back
to what I talk about above.

To put that another way, if I'm making a connection on behalf of an
untrusted party, I can choose not to supply an SSL certificate, or not
to supply a password. But I cannot choose to not be myself.

(IMO, you're driving towards a separation of the proxy identity from
the user identity. Other protocols do that too.)

Hmm, interesting.

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

On Mon, Jan 23, 2023 at 2:47 PM Robert Haas <robertmhaas@gmail.com> wrote:

Second, the reason why I described it as a manufactured issue is
because it's a bit like asking someone to stand under a ladder and
then complaining when they get hit in the head by a falling object.
It's not that I think it's good for people to get a free exploit to
superuser, or to get hit in the head by falling objects. It's just
that you can't have the things that together lead to some outcome
without also getting the outcome.

I left out a sentence here. What I meant to say was we can't both
allow passwordless loopback connections to the bootstrap superuser and
also allow postgres_fdw to connect to anything that the user requests
and then be surprised when that user can get into the superuser
account. The natural outcome of combining those two things is that
superuser gets hacked.

The password requirement just *barely* prevents that attack from
working, almost, maybe, while at the same time managing to block
things that people want to do for totally legitimate reasons. But
IMHO, the real problem is that combining those two things is extremely
dangerous.

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

On Fri, 2023-01-20 at 11:08 -0500, Robert Haas wrote:

On Fri, Jan 20, 2023 at 8:25 AM Robert Haas <robertmhaas@gmail.com>
wrote:

I still think you're talking about a different problem here. I'm
talking about the problem of knowing whether local files are going
to
be accessed by the connection string.

So here's a dumb patch for this. At least in my mind, the connection
string sanitization/validation is the major design problem here

I believe your patch conflates two use cases:

(A) Tightly-coupled servers that are managed by the administrator. In
this case, there are a finite number of connection strings to make, and
the admin knows about all of them. Validation is a poor solution for
this use case, because we get into the weeds trying to figure out
what's safe or not, overriding the admin's better judgement in some
cases and letting through connection strings that might be unsafe. A
much better solution is to simply declare the connection strings as
some kind of object (perhaps a SERVER object), and hand out privileges
or inherit them from a predefined role. Having connection string
objects is also just a better UI: it allows changes to connection
strings over time to adapt to changing security needs, and allows a
simple name that is much easier to type and read.

(B) Loosely-coupled servers that the admin doesn't know about, but
which might be perfectly safe to access. Validation is useful here, but
it's a long road of fine-grained privileges around acceptable hosts,
IPs, authentication types, file access, password sources, password
protocols, connection options, etc. The right solution here is to
identify the sub-usecases of loosely-coupled servers, and enable them
(with the appropriate controls) one at a time. Arguably, that's already
what's happened by demanding a password (even if we don't like the
mechanism, it does seem to work for some important cases).

Is your patch targeted at use case (A), (B), or both?

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On 1/23/23 11:05, Andres Freund wrote:

There's not enough documentation for SYSTEM_USER imo.

If we were to make use of SYSTEM_USER programmatically (and based on
what Robert wrote downthread, that's probably not what's desired), I
think we'd have to make more guarantees about how it can be parsed and
the values that you can expect. Right now it's meant mostly for human
consumption.

You could even go a step further and disable ambient transport
authentication (sslcertmode=disable gssencmode=disable), which keeps a
proxied connection from making use of a client cert or a Kerberos cache. But
for postgres_fdw, at least, that carries a risk of disabling current use
cases. Stephen and I had a discussion about one such case in the Kerberos
delegation thread [1].

I did not find that very convincing for today's code. The likelihood of
something useful being prevented seems far far lower than preventing privilege
leakage...

Fair enough. Preventing those credentials from being pulled in by
default would effectively neutralize my concern for the delegation
patchset, too.

--Jacob

On 1/23/23 11:52, Robert Haas wrote:

On Mon, Jan 23, 2023 at 2:47 PM Robert Haas <robertmhaas@gmail.com> wrote:

Second, the reason why I described it as a manufactured issue is
because it's a bit like asking someone to stand under a ladder and
then complaining when they get hit in the head by a falling object.
It's not that I think it's good for people to get a free exploit to
superuser, or to get hit in the head by falling objects. It's just
that you can't have the things that together lead to some outcome
without also getting the outcome.

I left out a sentence here. What I meant to say was we can't both
allow passwordless loopback connections to the bootstrap superuser and
also allow postgres_fdw to connect to anything that the user requests
and then be surprised when that user can get into the superuser
account. The natural outcome of combining those two things is that
superuser gets hacked.

The password requirement just *barely* prevents that attack from
working, almost, maybe, while at the same time managing to block
things that people want to do for totally legitimate reasons. But
IMHO, the real problem is that combining those two things is extremely
dangerous.

I don't disagree. I'm worried that the unspoken conclusion being
presented is "it's such an obvious problem that we should just leave it
to the DBAs," which I very much disagree with, but I may be reading too
much into it.

It seems to me that we basically let
the malicious connection to the target host succeed, and then say ...
oh, never mind, we may have made this connection under false
pretenses, so we shan't use it after all. What I was attempting to
argue is that we shouldn't let things get that far. Either the victim
should be able to protect itself from the malicious connection, or the
connection attempt shouldn't be allowed in the first place, or both.
Blocking the connection attempt after the fact feels like too little,
too late.

Expanding on my previous comment, you could give the client a way to say
"I am a proxy, and I'm connecting on behalf of this user, and here are
both my credentials and their credentials. So if you were planning to,
say, authorize me as superuser based on my IP address... maybe don't do
that?"

(You can sort of implement this today, by giving the proxy a client
certificate for transport authn, having it provide the in-band authn for
the user, and requiring both at the server. It's not very flexible.)

I think this has potential overlap with Magnus' PROXY proposal [1]/messages/by-id/CABUevExJ0ifpUEiX4uOREy0s2kHBrBrb=pXLEHhpMTR1vVR1XA@mail.gmail.com, and
also the case where we want pgbouncer to authenticate itself and then
perform actions on behalf of someone else [2]/messages/by-id/CAMT0RQR2fxeaPLHXappBCGEjHJiPCBJMPOHoDWiaYLjuieR0sg@mail.gmail.com, and maybe SASL's authzid
concept. I don't think one solution will hit all of the desired use
cases, but there are directions that can be investigated.

I'm not completely sure that this is good enough in terms of blocking
the attack as early as I think we should. This is all happening in the
midst of a connection attempt. If the remote server says, "hey, what's
your password?" and we refuse to answer that question, well that seems
somewhat OK. But what if we're hoping to be asked for a password and
the remote server doesn't ask?

require_auth should still successfully mitigate the target_session_attrs
case (going back to the examples you provided). It looks like the SQL is
initiated from the client side, so require_auth will notice that there
was no authentication performed and bail out before we get there.

For the hypothetical logon trigger, or any case where the server does
something on behalf of a user upon connection, I agree it doesn't help you.

--Jacob

[1]: /messages/by-id/CABUevExJ0ifpUEiX4uOREy0s2kHBrBrb=pXLEHhpMTR1vVR1XA@mail.gmail.com
/messages/by-id/CABUevExJ0ifpUEiX4uOREy0s2kHBrBrb=pXLEHhpMTR1vVR1XA@mail.gmail.com
[2]: /messages/by-id/CAMT0RQR2fxeaPLHXappBCGEjHJiPCBJMPOHoDWiaYLjuieR0sg@mail.gmail.com
/messages/by-id/CAMT0RQR2fxeaPLHXappBCGEjHJiPCBJMPOHoDWiaYLjuieR0sg@mail.gmail.com

On Mon, Jan 23, 2023 at 7:24 PM Jacob Champion <jchampion@timescale.com> wrote:

The password requirement just *barely* prevents that attack from
working, almost, maybe, while at the same time managing to block
things that people want to do for totally legitimate reasons. But
IMHO, the real problem is that combining those two things is extremely
dangerous.

I don't disagree. I'm worried that the unspoken conclusion being
presented is "it's such an obvious problem that we should just leave it
to the DBAs," which I very much disagree with, but I may be reading too
much into it.

To be honest, that was my first instinct here, but I see the problems
better now than I did at the beginning of this discussion.

Expanding on my previous comment, you could give the client a way to say
"I am a proxy, and I'm connecting on behalf of this user, and here are
both my credentials and their credentials. So if you were planning to,
say, authorize me as superuser based on my IP address... maybe don't do
that?"

(You can sort of implement this today, by giving the proxy a client
certificate for transport authn, having it provide the in-band authn for
the user, and requiring both at the server. It's not very flexible.)

I think this has potential overlap with Magnus' PROXY proposal [1], and
also the case where we want pgbouncer to authenticate itself and then
perform actions on behalf of someone else [2], and maybe SASL's authzid
concept. I don't think one solution will hit all of the desired use
cases, but there are directions that can be investigated.

I think this has some potential, but it's pretty complex, seeming to
require protocol extensions and having backward-compatibility problems
and so on. What do you think about something in the spirit of a
reverse-pg_hba.conf? The idea being that PostgreSQL facilities that
make outbound connections are supposed to ask it whether those
connections are OK to initiate. Then you could have a default
configuration that basically says "don't allow loopback connections"
or "require passwords all the time" or whatever we like, and the DBA
can change that as desired. We could teach dblink, postgres_fdw, and
CREATE SUBSCRIPTION to use this new thing, and third-party code could
adopt it if it likes.

Even if we do that, some kind of proxy protocol support might be very
desirable. I'm not against that. But I think that DBAs need better
control over what kind of outbound connections they want to permit,
too.

For the hypothetical logon trigger, or any case where the server does
something on behalf of a user upon connection, I agree it doesn't help you.

I don't think the logon trigger thing is all *that* hypothetical. We
don't have it yet, but there have been patches proposed repeatedly for
many years.

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

On 2023-01-24 Tu 08:50, Robert Haas wrote:

What do you think about something in the spirit of a
reverse-pg_hba.conf? The idea being that PostgreSQL facilities that
make outbound connections are supposed to ask it whether those
connections are OK to initiate. Then you could have a default
configuration that basically says "don't allow loopback connections"
or "require passwords all the time" or whatever we like, and the DBA
can change that as desired. We could teach dblink, postgres_fdw, and
CREATE SUBSCRIPTION to use this new thing, and third-party code could
adopt it if it likes.

I kinda like this idea, especially if we could specify the context that
rules are to apply in. e.g. postgres_fdw, mysql_fdw etc. I'd certainly
give it an outing in the redis_fdw if appropriate.

cheers

andrew

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

On Tue, Jan 24, 2023 at 5:50 AM Robert Haas <robertmhaas@gmail.com> wrote:

I think this has some potential, but it's pretty complex, seeming to
require protocol extensions and having backward-compatibility problems
and so on.

Yeah.

What do you think about something in the spirit of a
reverse-pg_hba.conf? The idea being that PostgreSQL facilities that
make outbound connections are supposed to ask it whether those
connections are OK to initiate. Then you could have a default
configuration that basically says "don't allow loopback connections"
or "require passwords all the time" or whatever we like, and the DBA
can change that as desired.

Well, I'll have to kick the idea around a little bit. Kneejerk reactions:

- It's completely reasonable to let a proxy operator restrict how that
proxy is used. I doubt very much that a typical DBA wants to be
operating an open proxy.

- I think the devil will be in the details of the configuration
design. Lists of allowed destination authorities (in the URI sense),
options that must be present/absent/overridden, those sound great. But
your initial examples of allow-loopback and require-passwords options
are in the "make the DBA deal with it" line of thinking, IMO. I think
it's difficult for someone to reason through those correctly the first
time, even for experts. I'd like to instead see the core problem --
that *any* ambient authentication used by a proxy is inherently risky
-- exposed as a highly visible concept in the config, so that it's
hard to make mistakes.

- I'm inherently skeptical of solutions that require all clients --
proxies, in this case -- to be configured correctly in order for a
server to be able to protect itself. (But I also have a larger
appetite for security options that break compatibility when turned on.
:D)

For the hypothetical logon trigger, or any case where the server does
something on behalf of a user upon connection, I agree it doesn't help you.

I don't think the logon trigger thing is all *that* hypothetical. We
don't have it yet, but there have been patches proposed repeatedly for
many years.

Okay. I think this thread has applicable lessons -- if connection
establishment itself leads to side effects, all actors in the
ecosystem (bouncers, proxies) have to be hardened against making those
connections passively. I know we're very different from HTTP, but it
feels similar to their concept of method safety and the consequences
of violating it.

--Jacob

[ Changing subject line to something more appropriate: This is
branched from the "Non-superuser subscription owners" thread, but the
topic has become connection security more generally for outbound
connections from a PostgreSQL instance, the inadequacies of just
trying to require that such connections always use a password, and
related problems. I proposed some kind of "reverse pg_hba.conf file"
as a way of allowing configurable limits on such outbound connections.
]

On Tue, Jan 24, 2023 at 2:18 PM Jacob Champion <jchampion@timescale.com> wrote:

- It's completely reasonable to let a proxy operator restrict how that
proxy is used. I doubt very much that a typical DBA wants to be
operating an open proxy.

That's very well put. It's precisely what I was thinking, but
expressed much more clearly.

- I think the devil will be in the details of the configuration
design. Lists of allowed destination authorities (in the URI sense),
options that must be present/absent/overridden, those sound great. But
your initial examples of allow-loopback and require-passwords options
are in the "make the DBA deal with it" line of thinking, IMO. I think
it's difficult for someone to reason through those correctly the first
time, even for experts. I'd like to instead see the core problem --
that *any* ambient authentication used by a proxy is inherently risky
-- exposed as a highly visible concept in the config, so that it's
hard to make mistakes.

I find the concept of "ambient authentication" problematic. I don't
know exactly what you mean by it. I hope you'll tell me, but I think
that I won't like it even after I know, because as I said before, it's
difficult to know why anyone else makes a decision, and asking an
untrusted third-party why they're deciding something is sketchy at
best. I think that the problems we have in this area can be solved by
either (a) restricting the open proxy to be less open or (b)
encouraging people to authenticate users in some way that won't admit
connections from an open proxy. The former needs to be configurable by
the DBA, and the latter is also a configuration choice by the DBA. We
can provide tools here that make it less likely that people will shoot
themselves in the foot, and we can ship default configurations that
reduce the chance of inadvertent foot-shooting, and we can write
documentation that says "don't shoot yourself in the foot," but we
cannot actually prevent people from shooting themselves in the foot
except, perhaps, by massively nerfing the capabilities of the system.

What I was thinking about in terms of a "reverse pg_hba.conf" was
something in the vein of, e.g.:

SOURCE_COMPONENT SOURCE_DATABASE SOURCE_USER DESTINATION_SUBNET
DESTINATION_DATABASE DESTINATION_USER OPTIONS ACTION

e.g.

all all all local all all - deny # block access through UNIX sockets
all all all 127.0.0.0/8 all all - deny # block loopback interface via IPv4

Or:

postgres_fdw all all all all all authentication=cleartext,md5,sasl
allow # allow postgres_fdw with password-ish authentication

Disallowing loopback connections feels quite tricky. You could use
127.anything.anything.anything, but you could also loop back via IPv6,
or you could loop back via any interface. But you can't use
subnet-based ACLs to rule out loop backs through IP/IPv6 interfaces
unless you know what all your system's own IPs are. Maybe that's an
argument in favor of having a dedicated deny-loopback facility built
into the system instead of relying on IP ACLs. But I am not sure that
really works either: how sure are we that we can discover all of the
local IP addresses? Maybe it doesn't matter anyway, since the point is
just to disallow anything that would be likely to use "trust" or
"ident" authentication, and that's probably not going to include any
non-loopback network interfaces. But ... is that true in general? What
about on Windows?

- I'm inherently skeptical of solutions that require all clients --
proxies, in this case -- to be configured correctly in order for a
server to be able to protect itself. (But I also have a larger
appetite for security options that break compatibility when turned on.
:D)

I (still) don't think that restricting the proxy is required, but you
can't both not restrict the proxy and also allow passwordless loopback
superuser connections. You have to pick one or the other. The reason I
keep harping on the role of the DBA is that I don't think we can make
that choice unilaterally on behalf of everyone. We've tried doing that
with the current rules and we've discussed the weaknesses of that
approach already.

I don't think the logon trigger thing is all *that* hypothetical. We
don't have it yet, but there have been patches proposed repeatedly for
many years.

Okay. I think this thread has applicable lessons -- if connection
establishment itself leads to side effects, all actors in the
ecosystem (bouncers, proxies) have to be hardened against making those
connections passively. I know we're very different from HTTP, but it
feels similar to their concept of method safety and the consequences
of violating it.

I am not familiar with that concept in detail but that sounds right to me.

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

On Mon, Jan 23, 2023 at 3:50 PM Jeff Davis <pgsql@j-davis.com> wrote:

I believe your patch conflates two use cases:

(A) Tightly-coupled servers that are managed by the administrator. In
this case, there are a finite number of connection strings to make, and
the admin knows about all of them. Validation is a poor solution for
this use case, because we get into the weeds trying to figure out
what's safe or not, overriding the admin's better judgement in some
cases and letting through connection strings that might be unsafe. A
much better solution is to simply declare the connection strings as
some kind of object (perhaps a SERVER object), and hand out privileges
or inherit them from a predefined role. Having connection string
objects is also just a better UI: it allows changes to connection
strings over time to adapt to changing security needs, and allows a
simple name that is much easier to type and read.

(B) Loosely-coupled servers that the admin doesn't know about, but
which might be perfectly safe to access. Validation is useful here, but
it's a long road of fine-grained privileges around acceptable hosts,
IPs, authentication types, file access, password sources, password
protocols, connection options, etc. The right solution here is to
identify the sub-usecases of loosely-coupled servers, and enable them
(with the appropriate controls) one at a time. Arguably, that's already
what's happened by demanding a password (even if we don't like the
mechanism, it does seem to work for some important cases).

Is your patch targeted at use case (A), (B), or both?

I suppose that I would say that the patch is a better fit for (B),
because I'm not proposing to add any kind of intermediate object of
the type you postulate in (A). However, I don't really agree with the
way you've split this up, either. It seems to me that the relevant
question isn't "are the servers tightly coupled?" but rather "could
some user make a mess if we let them use any arbitrary connection
string?".

If you're running all of the machines involved on a private network
that is well-isolated from the Internet and in which only trusted
actors operate, you could use what I'm proposing here for either (A)
or (B) and it would be totally fine. If your server is sitting out on
the public Internet and is adequately secured against malicious
loopback connections, you could also probably use it for either (A) or
(B), unless you've got users who are really shady and you're worried
that the outbound connections that they make from your machine might
get you into trouble, in which case you probably can't use it for
either (A) or (B). Basically, the patch is suitable for cases where
you don't really need to restrict what connection strings people can
use, and unsuitable for cases where you do, but that doesn't have much
to do with whether the servers involved are loosely or tightly
coupled.

I think that you're basically trying to make an argument that some
sort of complex outbound connection filtering is mandatory, and I
still don't really agree with that. We ship postgres_fdw with
something extremely minimal - just a requirement that the password get
used - and the same for dblink. I think those rules suck and are
probably bad and insecure in quite a number of cases, and overly
strict in others, but I can think of no reason why CREATE SUBSCRIPTION
should be held to a higher standard than anything else. The
connections that you can make using CREATE SUBSCRIPTION are strictly
weaker than the ones you can make with dblink, which permits arbitrary
SQL execution. It cannot be right to suppose that a less-exploitable
system needs to be held to a higher security standard than a similar
but more-exploitable system.

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

On 1/24/23 12:04, Robert Haas wrote:

I find the concept of "ambient authentication" problematic. I don't
know exactly what you mean by it. I hope you'll tell me,

Sure: Ambient authority [1]https://en.wikipedia.org/wiki/Ambient_authority means that something is granted access based
on some aspect of its existence that it can't remove (or even
necessarily enumerate). Up above, when you said "I cannot choose not to
be myself," that's a clear marker that ambient authority is involved.
Examples of ambient authn/z factors might include an originating IP
address, the user ID of a connected peer process, the use of a loopback
interface, a GPS location, and so on. So 'peer' and 'ident' are ambient
authentication methods.

And, because I think it's useful, I'll extend the definition to include
privileges that _could_ be dropped by a proxy, but in practice are
included because there's no easy way not to. Examples for libpq include
the automatic use of the client certificate in ~/.postgresql, or any
Kerberos credentials available in the local user cache. (Or even a
PGPASSWORD set up and forgotten by a DBA.)

Ambient authority is closely related to the confused deputy problem [2]https://en.wikipedia.org/wiki/Confused_deputy_problem,
and the proxy discussed here is a classic confused deputy. The proxy
doesn't know that a core piece of its identity has been used to
authenticate the request it's forwarding. It can't choose its IP
address, or its user ID.

I'm most familiar with this in the context of HTTP, cookie-/IP-based
authn, and cross-site request forgeries. Whenever someone runs a local
web server with no authentication and says "it's okay! we only respond
to requests from the local host!" they're probably about to be broken
open by the first person to successfully reflect a request through the
victim's (very local) web browser.

Ways to mitigate or solve this problem (that I know of) include

1) Forwarding the original ambient context along with the request, so
the server can check it too. HTTP has the Origin header, so a browser
can say, "This request is not coming from my end user; it's coming from
a page controlled by example.org. You can't necessarily treat attached
cookies like they're authoritative." The PROXY protocol lets a proxy
forward several ambient factors, including the originating IP address
(or even the use of a UNIX socket) and information about the original
TLS context.

2) Explicitly combining the request with the proof of authority needed
to make it, as in capability-based security [3]https://en.wikipedia.org/wiki/Capability-based_security. Some web frameworks
push secret "CSRF tokens" into URLs for this purpose, to tangle the
authorization into the request itself [4]https://www.rfc-editor.org/rfc/rfc6265#section-8.2. I'd argue that the "password
requirement" implemented by postgres_fdw and discussed upthread was an
attempt at doing this, to try to ensure that the authentication comes
from the user explicitly and not from the proxy. It's just not very strong.

(require_auth would strengthen it quite a bit; a major feature of that
patchset is to explicitly name the in-band authentication factors that a
server is allowed to pull out of a client. It's still not strong enough
to make a true capability, for one because it's client-side only. But as
long as servers don't perform actions on behalf of users upon
connection, that's pretty good in practice.)

3) Dropping as many implicitly-held privileges as possible before making
a request. This doesn't solve the problem but may considerably reduce
the practical attack surface. For example, if browsers didn't attach
their user's cookies to cross-origin requests, cross-site request
forgeries would probably be considerably less dangerous (and, in the
years since I left the space, it looks like browsers have finally
stopped doing this by default). Upthread, Andres suggested disabling the
default inclusion of client certs and GSS creds, and I would extend that
to include really *anything* pulled in from the environment. Make the
DBA explicitly allow those things.

but I think
that I won't like it even after I know, because as I said before, it's
difficult to know why anyone else makes a decision, and asking an
untrusted third-party why they're deciding something is sketchy at
best.

I think that's a red herring. Setting aside that you can, in fact, prove
that the server has authenticated you (e.g. require_auth=scram-sha-256
in my proposed patchset), I don't think "untrusted servers, that we
don't control, doing something stupid" is a very useful thing to focus
on. We're trying to secure the case where a server *is* authenticating
us, using known useful factors, but those factors have been co-opted by
an attacker via a proxy.

I think that the problems we have in this area can be solved by
either (a) restricting the open proxy to be less open or (b)
encouraging people to authenticate users in some way that won't admit
connections from an open proxy.

(a) is an excellent mitigation, and we should do it. (b) starts getting
shaky because I think peer auth is actually a very reasonable choice for
many people. So I hope we can also start solving the underlying problem
while we implement (a).

we
cannot actually prevent people from shooting themselves in the foot
except, perhaps, by massively nerfing the capabilities of the system.

But I thought we already agreed that most DBAs do not want a massively
capable proxy? I don't think we have to massively nerf the system, but
let's say we did. Would that really be unacceptable for this use case?

(You're still driving hard down the "it's impossible for us to securely
handle both cases at the same time" path. I don't think that's true from
a technical standpoint, because we hold nearly total control of the
protocol. I think we're in a much easier situation than HTTP was.)

What I was thinking about in terms of a "reverse pg_hba.conf" was
something in the vein of, e.g.:

SOURCE_COMPONENT SOURCE_DATABASE SOURCE_USER DESTINATION_SUBNET
DESTINATION_DATABASE DESTINATION_USER OPTIONS ACTION

e.g.

all all all local all all - deny # block access through UNIX sockets
all all all 127.0.0.0/8 all all - deny # block loopback interface via IPv4

Or:

postgres_fdw all all all all all authentication=cleartext,md5,sasl
allow # allow postgres_fdw with password-ish authentication

I think this style focuses on absolute configuration flexibility at the
expense of usability. It obfuscates the common use cases. (I have the
exact same complaint about our HBA and ident configs, so I may be
fighting uphill.)

How should a DBA decide what is correct, or audit a configuration they
inherited from someone else? What makes it obvious why a proxy should
require cleartext auth instead of peer auth (especially since peer auth
seems to be inherently better, until you've read this thread)?

I'd rather the configuration focus on the pieces of a proxy's identity
that can be assumed by a client. For example, if the config has an
option for "let a client steal the proxy's user ID", and it's off by
default, then we've given the problem a name. DBAs can educate
themselves on it.

And if that option is off, then the implementation knows that

1) If the client has supplied explicit credentials and we can force the
server to use them, we're safe.
2) If the DBA says they're not running an ident server, or we can force
the server not to use ident authn, or the DBA pinky-swears that that
server isn't using ident authn, all IP connections are additionally safe.
3) If we have a way to forward the client's "origin" and we know that
the server will pay attention to it, all UNIX socket connections are
additionally safe.
4) Any *future* authentication method we add later needs to be
restricted in the same way.

Should we allow the use of our default client cert? the Kerberos cache?
passwords from the environment? All these are named and off by default.
DBAs can look through those options and say "oh, yeah, that seems like a
really bad idea because we have this one server over here..." And we
(the experts) now get to make the best decisions we can, based on a
DBA's declared intent, so the implementation gets to improve over time.

Disallowing loopback connections feels quite tricky. You could use
127.anything.anything.anything, but you could also loop back via IPv6,
or you could loop back via any interface. But you can't use
subnet-based ACLs to rule out loop backs through IP/IPv6 interfaces
unless you know what all your system's own IPs are. Maybe that's an
argument in favor of having a dedicated deny-loopback facility built
into the system instead of relying on IP ACLs. But I am not sure that
really works either: how sure are we that we can discover all of the
local IP addresses?

Well, to follow you down that road a little bit, I think that a DBA that
has set up `samehost ... trust` in their HBA is going to expect a
corresponding concept here, and it seems important for us to use an
identical implementation of samehost and samenet.

But I don't really want to follow you down that road, because I think
you illustrated my point yourself. You're already thinking about making
Disallowing Loopback Connections a first-class concept, but then you
immediately said

Maybe it doesn't matter anyway, since the point is
just to disallow anything that would be likely to use "trust" or
"ident" authentication

I'd rather we enshrine that -- the point -- in the configuration, and
have the proxy disable everything that can't provably meet that intent.

Thanks,
--Jacob

[1]: https://en.wikipedia.org/wiki/Ambient_authority
[2]: https://en.wikipedia.org/wiki/Confused_deputy_problem
[3]: https://en.wikipedia.org/wiki/Capability-based_security
[4]: https://www.rfc-editor.org/rfc/rfc6265#section-8.2

On Tue, 2023-01-24 at 17:00 -0500, Robert Haas wrote:

It seems to me that the relevant
question isn't "are the servers tightly coupled?" but rather "could
some user make a mess if we let them use any arbitrary connection
string?".

The split I created is much easier for an admin to answer: is the list
of servers finite, or can users connect to new servers the admin isn't
even aware of? If it's a finite list, I feel there's a much better
solution with both security and UI benefits.

With your question, I'm not entirely clear if that's a question that we
already have an answer for (require a password parameter), or that we
will answer in this thread, or that the admin will answer.

unless you've got users who are really shady 

Or compromised. Unfortunately, a role that's creating subscriptions has
a lot of surface area for escalation-of-privilege attacks, because they
have to trust all the owners of all the tables the subscriptions write
to.

I think that you're basically trying to make an argument that some
sort of complex outbound connection filtering is mandatory

No, I'm not asking for the validation to be more complex.

I believe use case (A) is a substantial use case, and I'd like to leave
space in the user interface to solve it a much better way than
connection string validation can offer. But to solve use case (A), we
need to separate the ability to create a subscription from the ability
to create a connection string.

Right now you see those as the same because they are done at the same
time in the same command; but I don't see it that way, because I had
plans to allow a variant of CREATE SUBSCRIPTION that uses foreign
servers. That plan would be consistent with dblink and postgres_fdw,
which already allow specifying foreign servers.

I propose that we have two predefined roles: pg_create_subscription,
and pg_create_connection. If creating a subscription with a connection
string, you'd need to be a member of both roles. But to create a
subscription with a server object, you'd just need to be a member of
pg_create_subscription and have the USAGE privilege on the server
object.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Wed, Jan 25, 2023 at 10:45 PM Jeff Davis <pgsql@j-davis.com> wrote:

I propose that we have two predefined roles: pg_create_subscription,
and pg_create_connection. If creating a subscription with a connection
string, you'd need to be a member of both roles. But to create a
subscription with a server object, you'd just need to be a member of
pg_create_subscription and have the USAGE privilege on the server
object.

I have no issue with that as a long-term plan. However, I think that
for right now we should just introduce pg_create_subscription. It
would make sense to add pg_create_connection in the same patch that
adds a CREATE CONNECTION command (or whatever exact syntax we end up
with) -- and that patch can also change CREATE SUBSCRIPTION to require
both privileges where a connection string is specified directly.

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

On Thu, 2023-01-26 at 09:43 -0500, Robert Haas wrote:

I have no issue with that as a long-term plan. However, I think that
for right now we should just introduce pg_create_subscription. It
would make sense to add pg_create_connection in the same patch that
adds a CREATE CONNECTION command (or whatever exact syntax we end up
with) -- and that patch can also change CREATE SUBSCRIPTION to
require
both privileges where a connection string is specified directly.

I assumed it would be a problem to say that pg_create_subscription was
enough to create a subscription today, and then later require
additional privileges (e.g. pg_create_connection).

If that's not a problem, then this sounds fine with me.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Thu, Jan 26, 2023 at 12:36 PM Jeff Davis <pgsql@j-davis.com> wrote:

On Thu, 2023-01-26 at 09:43 -0500, Robert Haas wrote:

I have no issue with that as a long-term plan. However, I think that
for right now we should just introduce pg_create_subscription. It
would make sense to add pg_create_connection in the same patch that
adds a CREATE CONNECTION command (or whatever exact syntax we end up
with) -- and that patch can also change CREATE SUBSCRIPTION to
require
both privileges where a connection string is specified directly.

I assumed it would be a problem to say that pg_create_subscription was
enough to create a subscription today, and then later require
additional privileges (e.g. pg_create_connection).

If that's not a problem, then this sounds fine with me.

Wonderful! I'm working on a patch, but due to various distractions,
it's not done yet.

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

On Thu, Jan 19, 2023 at 8:46 PM Andres Freund <andres@anarazel.de> wrote:

If we already had (or have) that logic someplace else, it would
probably make sense to reuse it

We hve. See at least postgres_fdw's check_conn_params(), dblink's
dblink_connstr_check() and dblink_security_check().

In the patch I posted previously, I had some other set of checks, more
or less along the lines suggested by Jeff. I looked into revising that
approach and making the behavior match exactly what we do in those
places instead. I find that it breaks 027_nosuperuser.pl.
Specifically, where without the patch I get "ok 6 - nosuperuser admin
with all table privileges can replicate into unpartitioned", with the
patch it goes boom, because the subscription can't connect any more
due to the password requirement.

At first, I found it a bit tempting to see this as a further
indication that the force-a-password approach is not the right idea,
because the test case clearly memorializes a desire *not* to require a
password in this situation. However, the loopback-to-superuser attack
is just as viable for subscription as it in other cases, and my
previous patch would have done nothing to block it. So what I did
instead is add a password_required attribute, just like what
postgres_fdw has. As in the case of postgres_fdw, the actual rule is
that if the attribute is false, a password is not required, and if the
attribute is true, a password is required unless you are a superuser.
If you're a superuser, it still isn't. This is a slightly odd set of
semantics but it has precedent and practical advantages. Also, as in
the case of postgres_fdw, only a superuser can set
password_required=false, and a subscription that has that setting can
only be modified by a superuser, no matter who owns it.

Even though I hate the require-a-password stuff with the intensity of
a thousand suns, I think this is better than the previous patch,
because it's more consistent with what we do elsewhere and because it
blocks the loopback-connection-to-superuser attack. I think we
*really* need to develop a better system for restricting proxied
connections (no matter how proxied) and I hope that we do that soon.
But inventing something for this purpose that differs from what we do
elsewhere will make that task harder, not easier.

Thoughts?

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

On Wed, Jan 25, 2023 at 6:22 PM Jacob Champion <jchampion@timescale.com> wrote:

Sure: Ambient authority [1] means that something is granted access based
on some aspect of its existence that it can't remove (or even
necessarily enumerate). Up above, when you said "I cannot choose not to
be myself," that's a clear marker that ambient authority is involved.
Examples of ambient authn/z factors might include an originating IP
address, the user ID of a connected peer process, the use of a loopback
interface, a GPS location, and so on. So 'peer' and 'ident' are ambient
authentication methods.

OK.

1) Forwarding the original ambient context along with the request, so
the server can check it too.

Right, so a protocol extension. Reasonable idea, but a big lift. Not
only do you need everyone to be running a new enough version of
PostgreSQL, but existing proxies like pgpool and pgbouncer need
updates, too.

2) Explicitly combining the request with the proof of authority needed
to make it, as in capability-based security [3].

As far as I can see, that link doesn't address how you'd make this
approach work across a network.

3) Dropping as many implicitly-held privileges as possible before making
a request. This doesn't solve the problem but may considerably reduce
the practical attack surface.

Right. I definitely don't object to this kind of approach, but I don't
think it can ever be sufficient by itself.

e.g.

all all all local all all - deny # block access through UNIX sockets
all all all 127.0.0.0/8 all all - deny # block loopback interface via IPv4

Or:

postgres_fdw all all all all all authentication=cleartext,md5,sasl
allow # allow postgres_fdw with password-ish authentication

I think this style focuses on absolute configuration flexibility at the
expense of usability. It obfuscates the common use cases. (I have the
exact same complaint about our HBA and ident configs, so I may be
fighting uphill.)

That's probably somewhat true, but on the other hand, it also is more
powerful than what you're describing. In your system, is there some
way the DBA can say "hey, you can connect to any of the machines on
this list of subnets, but nothing else"? Or equally, "hey, you may NOT
connect to any machine on this list of subnets, but anything else is
fine"? Or "you can connect to these subnets without SSL, but if you
want to talk to anything else, you need to use SSL"? I would feel a
bit bad saying that those are just use cases we don't care about. Most
people likely wouldn't use that kind of flexibility, so maybe it
doesn't really matter, but it seems kind of nice to have. Your idea
seems to rely on us being able to identify all of the policies that a
user is likely to want and give names to each one, and I don't feel
very confident that that's realistic. But maybe I'm misinterpreting
your idea?

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

Hi,

On 2023-01-27 14:42:01 -0500, Robert Haas wrote:

At first, I found it a bit tempting to see this as a further
indication that the force-a-password approach is not the right idea,
because the test case clearly memorializes a desire *not* to require a
password in this situation. However, the loopback-to-superuser attack
is just as viable for subscription as it in other cases, and my
previous patch would have done nothing to block it.

Hm, compared to postgres_fdw, the user has far less control over what's
happening using that connection. Is there a way a subscription owner can
trigger evaluation of near-arbitrary SQL on the publisher side?

So what I did instead is add a password_required attribute, just like what
postgres_fdw has. As in the case of postgres_fdw, the actual rule is that if
the attribute is false, a password is not required, and if the attribute is
true, a password is required unless you are a superuser. If you're a
superuser, it still isn't. This is a slightly odd set of semantics but it
has precedent and practical advantages. Also, as in the case of
postgres_fdw, only a superuser can set password_required=false, and a
subscription that has that setting can only be modified by a superuser, no
matter who owns it.

I started out asking what benefits it provides to own a subscription one
cannot modify. But I think it is a good capability to have, to restrict the
set of relations that replication could target. Although perhaps it'd be
better to set the "replay user" as a separate property on the subscription?

Does owning a subscription one isn't allowed to modify useful outside of that?

Even though I hate the require-a-password stuff with the intensity of
a thousand suns, I think this is better than the previous patch,
because it's more consistent with what we do elsewhere and because it
blocks the loopback-connection-to-superuser attack. I think we
*really* need to develop a better system for restricting proxied
connections (no matter how proxied) and I hope that we do that soon.
But inventing something for this purpose that differs from what we do
elsewhere will make that task harder, not easier.

Thoughts?

I think it's reasonable to mirror behaviour from elsewhere, and it'd let us
have this feature relatively soon - I think it's a common need to do this as a
non-superuser. It's IMO a very good idea to not subscribe as a superuser, even
if set up by a superuser...

But I also would understand if you / somebody else chose to focus on
implementing a less nasty connection model.

Subject: [PATCH v2] Add new predefined role pg_create_subscriptions.

Maybe a daft question:

Have we considered using a "normal grant", e.g. on the database, instead of a
role? Could it e.g. be useful to grant a user the permission to create a
subscription in one database, but not in another?

@@ -1039,6 +1082,16 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,

sub = GetSubscription(subid, false);

+	/*
+	 * Don't allow non-superuser modification of a subscription with
+	 * password_required=false.
+	 */
+	if (!sub->passwordrequired && !superuser())
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+						 errmsg("password_required=false is superuser-only"),
+						 errhint("Subscriptions with the password_required option set to false may only be created or modified by the superuser.")));
+
/* Lock the subscription so nobody else can do anything with it. */
LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);

The subscription code already does ownership checks before locking and now
there's also the passwordrequired before. Is it possible that this could open
up some sort of race? Could e.g. the user change the ownership to the
superuser in one session, do an ALTER in the other?

It looks like your change won't increase the danger of that, as the
superuser() check just checks the current users permissions.

@@ -180,6 +180,13 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
if (PQstatus(conn->streamConn) != CONNECTION_OK)
goto bad_connection_errmsg;

+	if (must_use_password && !PQconnectionUsedPassword(conn->streamConn))
+		ereport(ERROR,
+				(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+				 errmsg("password is required"),
+				 errdetail("Non-superuser cannot connect if the server does not request a password."),
+				 errhint("Target server's authentication method must be changed.")));
+

The documentation of libpqrcv_connect() says that:
* Returns NULL on error and fills the err with palloc'ed error message.

and throwing an error like that will at the very least leak the connection,
fd, fd reservation. Which I just had fixed :). At the very least you'd need to
copy the stuff that "bad_connection:" does.

I did wonder whether we should make libpqrcv_connect() use errsave() to return
errors. Or whether we should make libpqrcv register a memory context reset
callback that'd close the libpq connection.

/*
- * Validate connection info string (just try to parse it)
+ * Validate connection info string, and determine whether it might cause
+ * local filesystem access to be attempted.
+ *
+ * If the connection string can't be parsed, this function will raise
+ * an error and will not return. If it can, it will return true if this
+ * connection string specifies a password and false otherwise.
*/
-static void
+static bool
libpqrcv_check_conninfo(const char *conninfo)

That is a somewhat odd API. Why does it throw for some things, but not
others? Seems a bit cleaner to pass in a parameter indicating whether it
should throw when not finding a password? Particularly because you already
pass that to walrcv_connect().

Greetings,

Andres Freund

On Fri, Jan 27, 2023 at 4:09 PM Andres Freund <andres@anarazel.de> wrote:

Hm, compared to postgres_fdw, the user has far less control over what's
happening using that connection. Is there a way a subscription owner can
trigger evaluation of near-arbitrary SQL on the publisher side?

I'm not aware of one, but what I think it would let you do is
exfiltrate data you're not entitled to see.

I started out asking what benefits it provides to own a subscription one
cannot modify. But I think it is a good capability to have, to restrict the
set of relations that replication could target. Although perhaps it'd be
better to set the "replay user" as a separate property on the subscription?

That's been proposed previously, but for reasons I don't quite
remember it seems not to have happened. I don't think it achieved
consensus.

Does owning a subscription one isn't allowed to modify useful outside of that?

Uh, possibly that's a question for Mark or Jeff. I don't know. I can't
see what they would be, but I just work here.

Maybe a daft question:

Have we considered using a "normal grant", e.g. on the database, instead of a
role? Could it e.g. be useful to grant a user the permission to create a
subscription in one database, but not in another?

Potentially, but I didn't think we'd want to burn through permissions
bits that fast, even given 7b378237aa805711353075de142021b1d40ff3b0.
Still, if the consensus is otherwise, I can change it. Then I guess
we'd end up with GRANT CREATE ON DATABASE and GRANT CREATE
SUBSCRIPTION ON DATABASE, which I'm sure wouldn't be confusing at all.

Or, another thought, maybe this should be checking for CREATE on the
current database + also pg_create_subscription. That seems like it
might be the right idea, actually.

The subscription code already does ownership checks before locking and now
there's also the passwordrequired before. Is it possible that this could open
up some sort of race? Could e.g. the user change the ownership to the
superuser in one session, do an ALTER in the other?

It looks like your change won't increase the danger of that, as the
superuser() check just checks the current users permissions.

I'm not entirely clear whether there's a hazard there. If there is, I
think we could fix it by moving the LockSharedObject call up higher,
above object_ownercheck. The only problem with that is it lets you
lock an object on which you have no permissions: see
2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd
need an analogue of RangeVarGetRelidExtended.

and throwing an error like that will at the very least leak the connection,
fd, fd reservation. Which I just had fixed :). At the very least you'd need to
copy the stuff that "bad_connection:" does.

OK.

I did wonder whether we should make libpqrcv_connect() use errsave() to return
errors. Or whether we should make libpqrcv register a memory context reset
callback that'd close the libpq connection.

Yeah. Using errsave() might be better, but not sure I want to tackle
that just now.

That is a somewhat odd API. Why does it throw for some things, but not
others? Seems a bit cleaner to pass in a parameter indicating whether it
should throw when not finding a password? Particularly because you already
pass that to walrcv_connect().

Will look into that.

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

Hi,

On 2023-01-27 16:35:11 -0500, Robert Haas wrote:

Maybe a daft question:

Have we considered using a "normal grant", e.g. on the database, instead of a
role? Could it e.g. be useful to grant a user the permission to create a
subscription in one database, but not in another?

Potentially, but I didn't think we'd want to burn through permissions
bits that fast, even given 7b378237aa805711353075de142021b1d40ff3b0.
Still, if the consensus is otherwise, I can change it.

I don't really have an opinion on what's better. I looked briefly whether
there was discussion around ithis but I didn't see anything.

pg_create_subcription feels a bit different than most of the other pg_*
roles. For most of those there is no schema object to tie permissions to. But
here there is.

But I think there's good arguments against a GRANT approach, too. GRANT ALL ON
DATABASE would suddenly be dangerous. How does it interact with database
ownership? Etc.

Then I guess we'd end up with GRANT CREATE ON DATABASE and GRANT CREATE
SUBSCRIPTION ON DATABASE, which I'm sure wouldn't be confusing at all.

Heh. I guess it could just be GRANT SUBSCRIBE.

Or, another thought, maybe this should be checking for CREATE on the
current database + also pg_create_subscription. That seems like it
might be the right idea, actually.

Yes, that seems like a good idea.

The subscription code already does ownership checks before locking and now
there's also the passwordrequired before. Is it possible that this could open
up some sort of race? Could e.g. the user change the ownership to the
superuser in one session, do an ALTER in the other?

It looks like your change won't increase the danger of that, as the
superuser() check just checks the current users permissions.

I'm not entirely clear whether there's a hazard there.

I'm not at all either. It's just a code pattern that makes me anxious - I
suspect there's a few places it makes us more vulnerable.

If there is, I think we could fix it by moving the LockSharedObject call up
higher, above object_ownercheck. The only problem with that is it lets you
lock an object on which you have no permissions: see
2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd need an
analogue of RangeVarGetRelidExtended.

Yea, we really should have something like RangeVarGetRelidExtended() for other
kinds of objects. It'd take a fair bit of work / time to use it widely, but
it'll take even longer if we start in 5 years ;)

Perhaps the bulk of RangeVarGetRelidExtended() could be generalized by having
a separate name->oid lookup callback?

Greetings,

Andres Freund

On Jan 27, 2023, at 1:35 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I started out asking what benefits it provides to own a subscription one
cannot modify. But I think it is a good capability to have, to restrict the
set of relations that replication could target. Although perhaps it'd be
better to set the "replay user" as a separate property on the subscription?

That's been proposed previously, but for reasons I don't quite
remember it seems not to have happened. I don't think it achieved
consensus.

Does owning a subscription one isn't allowed to modify useful outside of that?

Uh, possibly that's a question for Mark or Jeff. I don't know. I can't
see what they would be, but I just work here.

If the owner cannot modify the subscription, then the owner degenerates into a mere "run-as" user. Note that this isn't how things work now, and even if we disallowed owners from modifying the connection string, there would still be other attributes the owner could modify, such as the set of publications subscribed.

More generally, my thinking on this thread is that there needs to be two nosuperuser roles: A higher privileged role which can create a subscription, and a lower privileged role serving the "run-as" function. Those shouldn't be the same, because the "run-as" concept doesn't logically need to have subscription creation power, and likely *shouldn't* have that power. Depending on which sorts of attributes a subscription object has, such as the connection string, the answer differs for whether the owner/"run-as" user should get to change those attributes. One advantage of Jeff's idea of using a server object rather than a string is that it becomes more plausibly safe to allow the subscription owner to make changes to that attribute of the subscription.

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

On Fri, Jan 27, 2023 at 5:56 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

If the owner cannot modify the subscription, then the owner degenerates into a mere "run-as" user. Note that this isn't how things work now, and even if we disallowed owners from modifying the connection string, there would still be other attributes the owner could modify, such as the set of publications subscribed.

The proposed patch blocks every form of ALTER SUBSCRIPTION if
password_required false is set and you aren't a superuser. Is there
some other DML command that could be used to modify the set of
publications subscribed?

More generally, my thinking on this thread is that there needs to be two nosuperuser roles: A higher privileged role which can create a subscription, and a lower privileged role serving the "run-as" function. Those shouldn't be the same, because the "run-as" concept doesn't logically need to have subscription creation power, and likely *shouldn't* have that power. Depending on which sorts of attributes a subscription object has, such as the connection string, the answer differs for whether the owner/"run-as" user should get to change those attributes. One advantage of Jeff's idea of using a server object rather than a string is that it becomes more plausibly safe to allow the subscription owner to make changes to that attribute of the subscription.

There's some question in my mind about what these different mechanisms
are intended to accomplish.

On a technical level, I think that the idea of having a separate
objection for the connection string vs. the subscription itself is
perfectly sound, and to repeat what I said earlier, if someone wants
to implement that, cool. I also agree that it has the advantage that
you specify, namely, that someone can have rights to modify one of
those objects but not the other. What that lets you do is define a
short list of known systems and say, hey, you can replicate whatever
tables you want with whatever options you want, but only between these
systems. I'm not quite sure what problem that solves, though.

From my point of view, the two things that the superuser is most
likely to want to do are (1) control the replication setup themselves
and delegate nothing to any non-superuser or (2) give a non-superuser
pretty much complete control over replication with just enough
restrictions to avoid letting them do things that would compromise
security, such as hacking the local superuser account. In other words,
I expect that delegation of the logical replication configuration is
usually going to be all or nothing. Jeff's system allows for a
situation where you want to delegate some stuff but not everything,
and specifically where you want to dedicate control over the
subscription options and the tables being replicated, but not the
connection strings. To me, that feels like a bit of an awkward
configuration; I don't really understand in what situation that
division of responsibility would be particularly useful. I trust that
Jeff is proposing it because he knows of such a situation, but I don't
know what it is. I feel like, even if I wanted to let people use some
connection strings and not others, I'd probably want that control in
some form other than listing a specific list of allowable connection
strings -- I'd want to say things like "you have to use SSL" or "no
connecting back to the local host," because that lets me enforce some
general organizational policy without having to care specifically
about how each subscription is being set up.

Unfortunately, I have even less of an idea about what the run-as
concept is supposed to accomplish. I mean, at one level, I see it
quite clearly: the user creating the subscription wants replication to
have restricted privileges when it's running, and so they make the
run-as user some role with fewer privileges than their own. Brilliant.
But then I get stuck: against what kind of attack does that actually
protect us? If I'm a high privilege user, perhaps even a superuser,
and it's not safe to have logical replication running as me, then it
seems like the security model of logical replication is fundamentally
busted and we need to fix that. It can't be right to say that if you
have 263 users in a database and you want to replicate the whole
database to some other node, you need 263 different subscriptions with
a different run-as user for each. You need to be able to run all of
that logical replication as the superuser or some other high-privilege
user and not end up with a security compromise. And if we suppose that
that already works and is safe, well then what's the case where I do
need a run-as user?

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

On Jan 30, 2023, at 7:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:

And if we suppose that
that already works and is safe, well then what's the case where I do
need a run-as user?

A) Alice publishes tables, and occasionally adds new tables to existing publications.

B) Bob manages subscriptions, and periodically runs "refresh publication". Bob also creates new subscriptions for people when a row is inserted into the "please create a subscription for me" table which Bob owns, using a trigger that Bob created on that table.

C) Alice creates a "please create a subscription for me" table on the publishing database, adds lots of malicious requests, and adds that table to the publication.

D) Bob replicates the table, fires the trigger, creates the malicious subscriptions, and starts replicating all that stuff, too.

I think that having Charlie, not Bob, as the "run-as" user helps somewhere right around (D).

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

On Mon, Jan 30, 2023 at 11:11 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

On Jan 30, 2023, at 7:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:

And if we suppose that
that already works and is safe, well then what's the case where I do
need a run-as user?

A) Alice publishes tables, and occasionally adds new tables to existing publications.

B) Bob manages subscriptions, and periodically runs "refresh publication". Bob also creates new subscriptions for people when a row is inserted into the "please create a subscription for me" table which Bob owns, using a trigger that Bob created on that table.

C) Alice creates a "please create a subscription for me" table on the publishing database, adds lots of malicious requests, and adds that table to the publication.

D) Bob replicates the table, fires the trigger, creates the malicious subscriptions, and starts replicating all that stuff, too.

I think that having Charlie, not Bob, as the "run-as" user helps somewhere right around (D).

I suppose it does, but I have some complaints.

First, it doesn't seem to make a lot of sense to have one person
managing the publications and someone else managing the subscriptions,
and especially if those parties are mutually untrusting. I can't think
of any real reason to set things up that way. Sure, you could, but why
would you? You could, equally, decide that one member of your
household was going to decide what's for dinner every night, and some
other member of your household was going to decide what gets purchased
at the grocery store each week. If those two people exercise their
responsibilities without tight coordination, or with hostile intent
toward each other, things are going to go badly, but that's not an
argument for putting a combination lock on the flour canister. It's an
argument for getting along better, or not having such a dumb system in
the first place. I don't quite see how the situation you postulate in
(A) and (B) is any different. Publications and subscriptions are as
closely connected as food purchases and meals. The point of a
publication is for it to connect up to a subscription. In what
circumstances would be it be reasonable to give responsibility for
those objects to different and especially mutually untrusting users?

Second, in step (B), we may ask why Bob is doing this with a trigger.
If he's willing to create any subscription for which Alice asks, we
could have just given Alice the authority to do those actions herself.
Presumably, therefore, Bob is willing to create some subscriptions for
which Alice may ask and not others. Perhaps this whole arrangement is
just a workaround for the lack of a sensible system for controlling
which connection strings Alice can use, in which case what is really
needed here might be something like the separate connection object
which Jeff postulated or my idea of a reverse pg_hba.conf. That kind
of approach would give a better user interface to Alice, who wouldn't
have to rephrase all of her CREATE SUBSCRIPTION commands as insert
statements. Conversely, if Alice and Bob are truly dedicated to this
convoluted system of creating subscriptions, then Bob needs to put
logic into his trigger that's smart enough to block any malicious
requests that Alice may make. He really brought this problem on
himself by not doing that.

Third, in step (C), it seems to me that whoever set up Alice's
permissions has really messed up. Either the schema Bob is using for
his create-me-a-subscription table exists on the primary and Alice has
permission to create tables in that schema, or else that schema does
not exist on the primary and Alice has permission to create it. Either
way, that's a bad setup. Bob's table should be located in a schema for
which Alice has only USAGE permissions and shouldn't have excess
permissions on the table, either. Then this step can't happen. This
step could also be blocked if, instead of using a table with a
trigger, Bob wrote a security definer function or procedure and
granted EXECUTE permission on that function or procedure to Alice.
He's still going to need sanity checks, though, and if the function or
procedure inserts into a logging table or something, he'd better make
sure that table is adequately secured rather than being, say, a table
owned by Alice with malicious triggers on it.

So basically this doesn't really feel like a valid scenario to me.
We're supposed to believe that Alice is hostile to Bob, but the
superuser doesn't seem to have thought very carefully about how Bob is
supposed to defend himself against Alice, and Bob doesn't even seem to
be trying. Maybe we should rename the users to Samson and Delilah? :-)

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

On Jan 30, 2023, at 9:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:

First, it doesn't seem to make a lot of sense to have one person
managing the publications and someone else managing the subscriptions,
and especially if those parties are mutually untrusting. I can't think
of any real reason to set things up that way. Sure, you could, but why
would you? You could, equally, decide that one member of your
household was going to decide what's for dinner every night, and some
other member of your household was going to decide what gets purchased
at the grocery store each week. If those two people exercise their
responsibilities without tight coordination, or with hostile intent
toward each other, things are going to go badly, but that's not an
argument for putting a combination lock on the flour canister. It's an
argument for getting along better, or not having such a dumb system in
the first place. I don't quite see how the situation you postulate in
(A) and (B) is any different. Publications and subscriptions are as
closely connected as food purchases and meals. The point of a
publication is for it to connect up to a subscription.

I have a grim view of the requirement that publishers and subscribers trust each other. Even when they do trust each other, they can firewall attacks by acting as if they do not.

In what
circumstances would be it be reasonable to give responsibility for
those objects to different and especially mutually untrusting users?

When public repositories of data, such as the IANA whois database, publish their data via postgres publications.

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

On Jan 30, 2023, at 9:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:

So basically this doesn't really feel like a valid scenario to me.
We're supposed to believe that Alice is hostile to Bob, but the
superuser doesn't seem to have thought very carefully about how Bob is
supposed to defend himself against Alice, and Bob doesn't even seem to
be trying. Maybe we should rename the users to Samson and Delilah? :-)

No, Atahualpa and Pizarro.

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

On Mon, Jan 30, 2023 at 1:46 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

I have a grim view of the requirement that publishers and subscribers trust each other. Even when they do trust each other, they can firewall attacks by acting as if they do not.

I think it's OK if the CREATE PUBLICATION user doesn't particularly
trust the CREATE SUBSCRIPTION user, because the publication is just a
grouping of tables to which somebody can pay attention or not. The
CREATE PUBLICATION user isn't compromised either way. But, at least as
things stand, I don't see how the CREATE SUBSCRIPTION user get away
with not trusting the CREATE PUBLICATION user. CREATE SUBSCRIPTION
provides no tools at all for filtering the data that the subscriber
chooses to send.

Now that can be changed, I suppose, and a run-as user would be one way
to make progress in that direction. But I'm not sure how viable that
is, because...

In what
circumstances would be it be reasonable to give responsibility for
those objects to different and especially mutually untrusting users?

When public repositories of data, such as the IANA whois database, publish their data via postgres publications.

... for that to work, IANA would need to set up the database so that
untrusted parties can create logical replication slots on their
PostgreSQL server. And I think that granting REPLICATION privilege on
your database to random people on the Internet is not really viable,
nor intended to be viable. As the CREATE ROLE documentation says, "A
role having the REPLICATION attribute is a very highly privileged
role."

Concretely, this kind of setup would have the problem that you could
kill the IANA database by just creating a replication slot and then
not using it (or replicating from it only very very slowly).
Eventually, the replication slot would either hold back xmin enough
that you got a lot of bloat, or cause enough WAL to be retained that
you ran out of disk space. Maybe you could protect yourself against
that kind of problem by cutting off users who get too far behind, but
that also cuts off people who just have an outage for longer than your
cutoff.

Also, anyone who can connection to a replication slot can also connect
to any other replication slot, and drop any replication slot. So if
IANA did grant REPLICATION privilege to random people on the Internet,
one of them could jump into the system and screw things up for all the
others.

This kind of setup just doesn't seem viable to me.

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

On Jan 30, 2023, at 11:30 AM, Robert Haas <robertmhaas@gmail.com> wrote:

CREATE SUBSCRIPTION
provides no tools at all for filtering the data that the subscriber
chooses to send.

Now that can be changed, I suppose, and a run-as user would be one way
to make progress in that direction. But I'm not sure how viable that
is, because...

In what
circumstances would be it be reasonable to give responsibility for
those objects to different and especially mutually untrusting users?

When public repositories of data, such as the IANA whois database, publish their data via postgres publications.

... for that to work, IANA would need to set up the database so that
untrusted parties can create logical replication slots on their
PostgreSQL server. And I think that granting REPLICATION privilege on
your database to random people on the Internet is not really viable,
nor intended to be viable.

That was an aspirational example in which there's infinite daylight between the publisher and subscriber. I, too, doubt that's ever going to be possible. But I still think we should aspire to some extra daylight between the two. Perhaps IANA doesn't publish to the whole world, but instead publishes only to subscribers who have a contract in place, and have agreed to monetary penalties should they abuse the publishing server. Whatever. There's going to be some amount of daylight possible if we design for it, and none otherwise.

My real argument here isn't against your goal of having non-superusers who can create subscriptions. That part seems fine to me.

Given that my work last year made it possible for subscriptions to run as somebody other than the subscription creator, it annoys me that you now want the subscription creator's privileges to be what the subscription runs as. That seems to undo what I worked on. In my mental model of a (superuser-creator, non-superuser-owner) pair, it seems you're logically only touching the lefthand side, so you should then have a (nonsuperuser-creator, nonsuperuser-owner) pair. But you don't. You go the apparently needless extra step of just squashing them together. I just don't see why it needs to be like that.

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

On Fri, Jan 27, 2023 at 5:00 PM Andres Freund <andres@anarazel.de> wrote:

Or, another thought, maybe this should be checking for CREATE on the
current database + also pg_create_subscription. That seems like it
might be the right idea, actually.

Yes, that seems like a good idea.

Done in this version. I also changed check_conninfo to take an extra
argument instead of returning a Boolean, as per your suggestion.

I had a long think about what to do with ALTER SUBSCRIPTION ... OWNER
TO in terms of permissions checks. The previous version required that
the new owner have permissions of pg_create_subscription, but there
seems to be no particular reason for that rule except that it happens
to be what I made the code do. So I changed it to say that the current
owner must have CREATE privilege on the database, and must be able to
SET ROLE to the new owner. This matches the rule for CREATE SCHEMA.
Possibly we should *additionally* require that the person performing
the rename still have pg_create_subscription, but that shouldn't be
the only requirement. This change means that you can't just randomly
give your subscription to the superuser (with or without concurrently
attempting some other change as per your other comments) which is good
because you can't do that with other object types either.

There seems to be a good deal of inconsistency here. If you want to
give someone a schema, YOU need CREATE on the database. But if you
want to give someone a table, THEY need CREATE on the containing
schema. It make sense that we check permissions on the containing
object, which could be a database or a schema depending on what you're
renaming, but it's unclear to me why we sometimes check on the person
performing the ALTER command and at other times on the recipient. It's
also somewhat unclear to me why we are checking CREATE in the first
place, especially on the donor. It might make sense to have a rule
that you can't own an object in a place where you couldn't have
created it, but there is no such rule, because you can give someone
CREATE on a schema, they can create an object, and they you can take
CREATE a way and they still own an object there. So it kind of looks
to me like we made it up as we went along and that the result isn't
very consistent, but I'm inclined to follow CREATE SCHEMA here unless
there's some reason to do otherwise.

Another question around ALTER SUBSCRIPTION ... OWNER TO and also ALTER
SUBSCRIPTION .. RENAME is whether they ought to fail if you're not a
superuser and password_required false is set. They are separate code
paths from the rest of the ALTER SUBSCRIPTION cases, so if we want
that to be a rule we need dedicated code for it. I'm not quite sure
what's right. There's no comparable case for ALTER USER MAPPING
because a user mapping doesn't have an owner and so can't be
reassigned to a new owner. I don't see what the harm is, especially
for RENAME, but I might be missing something, and it certainly seems
arguable.

I'm not entirely clear whether there's a hazard there.

I'm not at all either. It's just a code pattern that makes me anxious - I
suspect there's a few places it makes us more vulnerable.

It looks likely to me that it was cut down from the CREATE SCHEMA code, FWIW.

If there is, I think we could fix it by moving the LockSharedObject call up
higher, above object_ownercheck. The only problem with that is it lets you
lock an object on which you have no permissions: see
2ad36c4e44c8b513f6155656e1b7a8d26715bb94. To really fix that, we'd need an
analogue of RangeVarGetRelidExtended.

Yea, we really should have something like RangeVarGetRelidExtended() for other
kinds of objects. It'd take a fair bit of work / time to use it widely, but
it'll take even longer if we start in 5 years ;)

We actually have something sort of like that in the form of
get_object_address(). It doesn't allow for a callback, but it does
have a retry loop.

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

On Fri, Jan 27, 2023 at 1:08 PM Robert Haas <robertmhaas@gmail.com> wrote:

1) Forwarding the original ambient context along with the request, so
the server can check it too.

Right, so a protocol extension. Reasonable idea, but a big lift. Not
only do you need everyone to be running a new enough version of
PostgreSQL, but existing proxies like pgpool and pgbouncer need
updates, too.