Re: Non-superuser subscription owners

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