Re: Non-superuser subscription owners

On Fri, Jan 27, 2023 at 4:09 PM Andres Freund <andres(at)anarazel(dot)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