Re: Non-superuser subscription owners

Hi,

On 2023-01-19 10:45:35 -0500, Robert Haas wrote:
> On Wed, Jan 18, 2023 at 3:58 PM Mark Dilger
> <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> > > On Jan 18, 2023, at 12:51 PM, Robert Haas <robertmhaas(at)gmail(dot)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 https://postgr.es/m/20220925232237.p6uskba2dw6fnwj2%40awork3.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