Re: Non-superuser subscription owners
От | Andres Freund |
---|---|
Тема | Re: Non-superuser subscription owners |
Дата | |
Msg-id | 20230201210229.cgr3byvvd3vvhi6i@alap3.anarazel.de обсуждение исходный текст |
Ответ на | Re: Non-superuser subscription owners (Robert Haas <robertmhaas@gmail.com>) |
Ответы |
Re: Non-superuser subscription owners
(Robert Haas <robertmhaas@gmail.com>)
|
Список | pgsql-hackers |
Hi, On 2023-01-30 15:32:34 -0500, Robert Haas wrote: > 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. As long as owner and run-as are the same, I think it's strongly preferrable to *not* require pg_create_subscription. > 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. Yuck. No idea what the best policy around this is. > 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. I don't really see a benefit in allowing it, so I'm inclined to go for the more restrictive option. But this is a really weakly held opinion. > > > 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. Hm, sure looks like that code doesn't do any privilege checking... > @@ -1269,13 +1270,19 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos) > slotname, > NAMEDATALEN); > > + /* Is the use of a password mandatory? */ > + must_use_password = MySubscription->passwordrequired && > + !superuser_arg(MySubscription->owner); There's a few repetitions of this - perhaps worth putting into a helper? > @@ -180,6 +181,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. or set password_required=falsein the subscription attributes\ ."))); > + > if (logical) > { > PGresult *res; This still leaks the connection on error, no? Greetings, Andres Freund
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Sergey DudoladovДата:
Сообщение: Re: Add connection active, idle time to pg_stat_activity