> On Nov 17, 2021, at 1:06 PM, Jeff Davis <pg...@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
