Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Fri, May 24, 2024 at 4:00 PM Tom Lane wrote: > > Oh! That does seem like it would make what I said wrong, but how would > > it even know who the original owner was? Shouldn't we be recreating > > the object with the owner it had at dump time? > > Keep in mind that the whole point here is for the pg_dump script to > just say "CREATE EXTENSION foo", not to mess with the individual > objects therein. So the objects are (probably) going to be owned by > the user that issued CREATE EXTENSION. > > In the original conception, that was the end of it: what you got for > the member objects was whatever state CREATE EXTENSION left behind. > The idea of pg_init_privs is to support dump/reload of subsequent > manual alterations of privileges for extension-created objects. > I'm not, at this point, 100% certain that that's a fully realizable > goal. But I definitely think it's insane to expect that to work > without also tracking changes in the ownership of said objects. > > Maybe forbidding ALTER OWNER on extension-owned objects isn't > such a bad idea? I think the root of my confusion, or at least one of the roots of my confusion, was whether we were talking about altering the extension or the objects within the extension. If somebody creates an extension as user nitin and afterwards changes the ownership to user lucia, then I would hope for the same pg_init_privs contents as if user lucia had created the extension in the first place. I think that might be hard to achieve in complex cases, if the extension changes the ownership of some objects during the creation process, or if some ownership has been changed afterward. But in the simple case where nothing like that happens, it sounds possible. If, on the other hand, somebody creates the extension and modifies the ownership of an object in the extension, then I agree that pg_init_privs shouldn't be updated. However, it also seems pretty clear that pg_init_privs is not recording enough state for pg_dump to mimic the ownership change, because it only records the original privileges, not the original ownership. So, can we recreate the original privileges without recreating the original ownership? It doesn't really seem like this is going to work in general. If nitin creates the extension and grants privileges to lucia, and then the ownership of the extension is changed to swara, then nitin is no longer a valid grantor. Even if we could fix that by modifying the grants to substitute swara for nitin, that could create impermissible circularities in the permissions graph. Maybe there are some scenarios where we can fix things up, but it doesn't seem possible in general. In a perfect world, the fix here is probably to have pg_init_privs or something similar record the ownership as well as the permissions, but that is not back-patchable and nobody's on the hook to fix up somebody else's feature. So what do we do? Imposing a constraint that you can't change the ownership of an extension-owned object, as you propose, seems fairly likely to break a few existing extension scripts, and also won't fix existing instances that are already broken, but maybe it's still worth considering if we don't have a better idea. Another line of thinking might be to somehow nerf pg_init_privs, or the use of it, so that we don't even try to cover this case e.g. if the ownership is changed, we nuke the pg_init_privs entry or resnap it to the current state, and the dump fails to recreate the original state but we get out from under that by declaring the case unsupported (with appropriate documentation changes, hopefully). pg_init_privs seems adequate for normal system catalog entries where the ownership shouldn't ever change from the bootstrap superuser, but extensions seem like they require more infrastructure. I think the only thing we absolutely have to fix here is the dangling ACL references. Those are a hazard, because the OID could be reused by an unrelated role, which seems like it could even give rise to security concerns. Making the whole pg_init_privs system actually work for this case case would be even better, but that seems to require filling in at least one major hole the original design, which is a lot to ask for (1) a back-patched fix or (2) a patch that someone who is not the original author has to try to write and be responsible for despite not being the one who created the problem. -- Robert Haas EDB: http://www.enterprisedb.com
Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Tue, May 28, 2024 at 9:06 PM Hannu Krosing wrote: > We should definitely also fix pg_dump, likely just checking that the > role exists when generating REVOKE commands (may be a good practice > for other cases too so instead of casting to ::regrole do the actual > join) > > ## here is the fix for pg_dump > > While flying to Vancouver I looked around in pg_dump code, and it > looks like the easiest way to mitigate the dangling pg_init_priv > entries is to replace the query in pg_dump with one that filters out > invalid entries +1 for this approach. I agree with Tom that fixing this in REVOKE is a bad plan; REVOKE is used by way too many things other than pg_dump, and the behavior change is not in general desirable. -- Robert Haas EDB: http://www.enterprisedb.com
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Hi Daniel,
pg_upgrade is just one important user of pg_dump which is the one that
generates REVOKE for a non-existent role.
We should definitely also fix pg_dump, likely just checking that the
role exists when generating REVOKE commands (may be a good practice
for other cases too so instead of casting to ::regrole do the actual
join)
## here is the fix for pg_dump
While flying to Vancouver I looked around in pg_dump code, and it
looks like the easiest way to mitigate the dangling pg_init_priv
entries is to replace the query in pg_dump with one that filters out
invalid entries
The current query is at line 9336:
/* Fetch initial-privileges data */
if (fout->remoteVersion >= 90600)
{
printfPQExpBuffer(query,
"SELECT objoid, classoid, objsubid, privtype, initprivs "
"FROM pg_init_privs");
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
And we need the same but filtering out invalid aclitems from initprivs
something like this
WITH q AS (
SELECT objoid, classoid, objsubid, privtype, unnest(initprivs) AS
initpriv FROM saved_init_privs
)
SELECT objoid, classoid, objsubid, privtype, array_agg(initpriv) as initprivs
FROM q
WHERE is_valid_value_for_type(initpriv::text, 'aclitem')
GROUP BY 1,2,3,4;
### The proposed re[placement query:
Unfortunately we do not have an existing
is_this_a_valid_value_for_type(value text, type text, OUT res boolean)
function, so for a read-only workaround the following seems to work:
Here I first collect the initprivs array elements which fail the
conversion to text and back into an array and store it in GUC
pg_dump.bad_aclitems
Then I use this stored list to filter out the bad ones in the actual query.
DO $$
DECLARE
aclitem_text text;
bad_aclitems text[] = '{}';
BEGIN
FOR aclitem_text IN
SELECT DISTINCT unnest(initprivs)::text FROM pg_init_privs
LOOP
BEGIN /* try to convert back to aclitem */
PERFORM aclitem_text::aclitem;
EXCEPTION WHEN OTHERS THEN /* collect bad aclitems */
bad_aclitems := bad_aclitems || ARRAY[aclitem_text];
END;
END LOOP;
IF bad_aclitems != '{}' THEN
RAISE WARNING 'Ignoring bad aclitems "%" in pg_init_privs', bad_aclitems;
END IF;
PERFORM set_config('pg_dump.bad_aclitems', bad_aclitems::text,
false); -- true for trx-local
END;
$$;
WITH q AS (
SELECT objoid, classoid, objsubid, privtype, unnest(initprivs) AS
initpriv FROM pg_init_privs
)
SELECT objoid, classoid, objsubid, privtype, array_agg(initpriv) AS initprivs
FROM q
WHERE NOT initpriv::text = ANY
(current_setting('pg_dump.bad_aclitems')::text[])
GROUP BY 1,2,3,4;
--
Hannu
On Sun, May 26, 2024 at 11:27 PM Daniel Gustafsson wrote:
>
> > On 26 May 2024, at 23:25, Tom Lane wrote:
> >
> > Hannu Krosing writes:
> >> Attached is a minimal patch to allow missing roles in REVOKE command
> >
> > FTR, I think this is a very bad idea.
>
> Agreed, this is papering over a bug. If we are worried about pg_upgrade it
> would be better to add a check to pg_upgrade which detects this case and
> advices the user how to deal with it.
>
> --
> Daniel Gustafsson
>
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 26 May 2024, at 23:25, Tom Lane wrote: > > Hannu Krosing writes: >> Attached is a minimal patch to allow missing roles in REVOKE command > > FTR, I think this is a very bad idea. Agreed, this is papering over a bug. If we are worried about pg_upgrade it would be better to add a check to pg_upgrade which detects this case and advices the user how to deal with it. -- Daniel Gustafsson
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Hannu Krosing writes: > Attached is a minimal patch to allow missing roles in REVOKE command FTR, I think this is a very bad idea. It might be OK if we added some kind of IF EXISTS option, but I'm not eager about that concept either. The right thing here is to fix the backend so that pg_dump doesn't see these bogus ACLs. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Attached is a minimal patch to allow missing roles in REVOKE command
This should fix the pg_upgrade issue and also a case where somebody
has dropped a role you are trying to revoke privileges from :
smalltest=# create table revoketest();
CREATE TABLE
smalltest=# revoke select on revoketest from bob;
WARNING: ignoring REVOKE FROM a missing role "bob"
REVOKE
smalltest=# create user bob;
CREATE ROLE
smalltest=# grant select on revoketest to bob;
GRANT
smalltest=# \du
List of roles
Role name | Attributes
---+
bob |
hannuk| Superuser, Create role, Create DB, Replication, Bypass RLS
smalltest=# \dp
Access privileges
Schema |Name| Type | Access privileges| Column
privileges | Policies
++---++---+--
public | revoketest | table | hannuk=arwdDxtm/hannuk+| |
|| | bob=r/hannuk | |
public | vacwatch | table || |
(2 rows)
smalltest=# revoke select on revoketest from bob, joe;
WARNING: ignoring REVOKE FROM a missing role "joe"
REVOKE
smalltest=# \dp
Access privileges
Schema |Name| Type | Access privileges| Column
privileges | Policies
++---++---+--
public | revoketest | table | hannuk=arwdDxtm/hannuk | |
public | vacwatch | table || |
(2 rows)
On Sun, May 26, 2024 at 12:05 AM Hannu Krosing wrote:
>
> On Sat, May 25, 2024 at 4:48 PM Tom Lane wrote:
> >
> > Hannu Krosing writes:
> > > Having an pg_init_privs entry referencing a non-existing user is
> > > certainly of no practical use.
> >
> > Sure, that's not up for debate. What I think we're discussing
> > right now is
> >
> > 1. What other cases are badly handled by the pg_init_privs
> > mechanisms.
> >
> > 2. How much of that is practical to fix in v17, seeing that
> > it's all long-standing bugs and we're already past beta1.
> >
> > I kind of doubt that the answer to #2 is "all of it".
> > But perhaps we can do better than "none of it".
>
> Putting the fix either in pg_dump or making REVOKE tolerate
> non-existing users would definitely be most practical / useful fixes,
> as these would actually allow pg_upgrade to v17 to work without
> changing anything in older versions.
>
> Currently one already can revoke a privilege that is not there in the
> first place, with the end state being that the privilege (still) does
> not exist.
> This does not even generate a warning.
>
> Extending this to revoking from users that do not exist does not seem
> any different on conceptual level, though I understand that
> implementation would be very different as it needs catching the user
> lookup error from a very different part of the code.
>
> That said, it would be better if we can have something that would be
> easy to backport something that would make pg_upgrade work for all
> supported versions.
> Making REVOKE silently ignore revoking from non-existing users would
> improve general robustness but could conceivably change behaviour if
> somebody relies on it in their workflows.
>
> Regards,
> Hannu
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..aff91582d7 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -444,6 +444,9 @@ ExecuteGrantStmt(GrantStmt *stmt)
* Convert the RoleSpec list into an Oid list. Note that at this point we
* insert an ACL_ID_PUBLIC into the list if appropriate, so downstream
* there shouldn't be any additional work needed to support this case.
+*
+* Allow missing grantees in case of REVOKE (!istmt.is_grant)
+* if now valid roles found return immediately
*/
foreach(cell, stmt->grantees)
{
@@ -456,12 +459,20 @@ ExecuteGrantStmt(GrantStmt *stmt)
grantee_uid = ACL_ID_PUBLIC;
break;
default:
- grantee_uid = get_rolespec_oid(grantee, false);
+ grantee_uid = get_rolespec_oid(grantee, !istmt.is_grant);
break;
}
- istmt.grantees = lappend_oid(istmt.grantees, grantee_uid);
+ if (OidIsValid(grantee_uid))
+ istmt.grantees = lappend_oid(istmt.grantees, grantee_uid);
+ else
+ ereport(WARNING,
+ (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
+ errmsg("ignoring REVOKE FROM a missing role \"%s\"",
Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Sat, May 25, 2024 at 4:48 PM Tom Lane wrote: > > Hannu Krosing writes: > > Having an pg_init_privs entry referencing a non-existing user is > > certainly of no practical use. > > Sure, that's not up for debate. What I think we're discussing > right now is > > 1. What other cases are badly handled by the pg_init_privs > mechanisms. > > 2. How much of that is practical to fix in v17, seeing that > it's all long-standing bugs and we're already past beta1. > > I kind of doubt that the answer to #2 is "all of it". > But perhaps we can do better than "none of it". Putting the fix either in pg_dump or making REVOKE tolerate non-existing users would definitely be most practical / useful fixes, as these would actually allow pg_upgrade to v17 to work without changing anything in older versions. Currently one already can revoke a privilege that is not there in the first place, with the end state being that the privilege (still) does not exist. This does not even generate a warning. Extending this to revoking from users that do not exist does not seem any different on conceptual level, though I understand that implementation would be very different as it needs catching the user lookup error from a very different part of the code. That said, it would be better if we can have something that would be easy to backport something that would make pg_upgrade work for all supported versions. Making REVOKE silently ignore revoking from non-existing users would improve general robustness but could conceivably change behaviour if somebody relies on it in their workflows. Regards, Hannu
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Hannu Krosing writes: > Having an pg_init_privs entry referencing a non-existing user is > certainly of no practical use. Sure, that's not up for debate. What I think we're discussing right now is 1. What other cases are badly handled by the pg_init_privs mechanisms. 2. How much of that is practical to fix in v17, seeing that it's all long-standing bugs and we're already past beta1. I kind of doubt that the answer to #2 is "all of it". But perhaps we can do better than "none of it". regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Fri, May 24, 2024 at 10:00 PM Tom Lane wrote: > > Robert Haas writes: > > On Fri, May 24, 2024 at 2:57 PM Tom Lane wrote: > >> Doesn't seem right to me. That will give pg_dump the wrong idea > >> of what the initial privileges actually were, and I don't see how > >> it can construct correct delta GRANT/REVOKE on the basis of false > >> information. During the dump reload, the extension will be > >> recreated with the original owner (I think), causing its objects' > >> privileges to go back to the original pg_init_privs values. > > > Oh! That does seem like it would make what I said wrong, but how would > > it even know who the original owner was? Shouldn't we be recreating > > the object with the owner it had at dump time? > > Keep in mind that the whole point here is for the pg_dump script to > just say "CREATE EXTENSION foo", not to mess with the individual > objects therein. So the objects are (probably) going to be owned by > the user that issued CREATE EXTENSION. > > In the original conception, that was the end of it: what you got for > the member objects was whatever state CREATE EXTENSION left behind. > The idea of pg_init_privs is to support dump/reload of subsequent > manual alterations of privileges for extension-created objects. > I'm not, at this point, 100% certain that that's a fully realizable > goal. The issue became visible because pg_dump issued a bogus REVOKE ALL ON TABLE public.pg_stat_statements FROM "16390"; Maybe the right place for a fix is in pg_dump and the fix would be to *not* issue REVOKE ALL ON FROM ? Or alternatively change REVOKE to treat non-existing users as a no-op ? Also, the pg_init_privs entry should either go away or at least be changed at the point when the user referenced in init-privs is dropped. Having an pg_init_privs entry referencing a non-existing user is certainly of no practical use. Or maybe we should change the user at that point to NULL or some special non-existing-user-id ? > But I definitely think it's insane to expect that to work > without also tracking changes in the ownership of said objects. > > Maybe forbidding ALTER OWNER on extension-owned objects isn't > such a bad idea?
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Robert Haas writes: > On Fri, May 24, 2024 at 2:57 PM Tom Lane wrote: >> Doesn't seem right to me. That will give pg_dump the wrong idea >> of what the initial privileges actually were, and I don't see how >> it can construct correct delta GRANT/REVOKE on the basis of false >> information. During the dump reload, the extension will be >> recreated with the original owner (I think), causing its objects' >> privileges to go back to the original pg_init_privs values. > Oh! That does seem like it would make what I said wrong, but how would > it even know who the original owner was? Shouldn't we be recreating > the object with the owner it had at dump time? Keep in mind that the whole point here is for the pg_dump script to just say "CREATE EXTENSION foo", not to mess with the individual objects therein. So the objects are (probably) going to be owned by the user that issued CREATE EXTENSION. In the original conception, that was the end of it: what you got for the member objects was whatever state CREATE EXTENSION left behind. The idea of pg_init_privs is to support dump/reload of subsequent manual alterations of privileges for extension-created objects. I'm not, at this point, 100% certain that that's a fully realizable goal. But I definitely think it's insane to expect that to work without also tracking changes in the ownership of said objects. Maybe forbidding ALTER OWNER on extension-owned objects isn't such a bad idea? regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Fri, May 24, 2024 at 2:57 PM Tom Lane wrote: > Doesn't seem right to me. That will give pg_dump the wrong idea > of what the initial privileges actually were, and I don't see how > it can construct correct delta GRANT/REVOKE on the basis of false > information. During the dump reload, the extension will be > recreated with the original owner (I think), causing its objects' > privileges to go back to the original pg_init_privs values. Oh! That does seem like it would make what I said wrong, but how would it even know who the original owner was? Shouldn't we be recreating the object with the owner it had at dump time? > Although ... this is tickling a recollection that pg_dump doesn't > try very hard to run CREATE EXTENSION with the same owner that > the extension had originally. That's a leftover from the times > when basically all extensions required superuser to install, > and of course one superuser is as good as the next. There might > be some work we have to do on that side too if we want to up > our game in this area. Hmm, yeah. > Another case that's likely not handled well is what if the extension > really shouldn't have its original owner (e.g. you're using > --no-owner). If it's restored under a new owner then the > pg_init_privs data certainly doesn't apply, and it feels like it'd > be mostly luck if the precomputed delta GRANT/REVOKEs lead to a > state you like. I'm not sure exactly how this computation works, but if tgl granted nmisch privileges on an object and the extension is now owned by rhaas, it would seem like the right thing to do would be for rhaas to grant nmisch those same privileges. Conversely if tgl started with privileges to do X and Y and later was granted privileges to do Z and we dump and restore such that the extension is owned by rhaas, I'd presume rhaas would end up with those same privileges. I'm probably too far from the code to give terribly useful advice here, but I think the expected behavior is that the new owner replaces the old one for all purposes relating to the owned object(s). At least, I can't currently see what else makes any sense. -- Robert Haas EDB: http://www.enterprisedb.com
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Robert Haas writes: > On Fri, May 24, 2024 at 11:59 AM Tom Lane wrote: >> So this goal seems to >> mean that neither ALTER OWNER nor REASSIGN OWNED should touch >> pg_init_privs at all, as that would break its function of recording >> a historical state. Only DROP OWNED should get rid of pg_init_privs >> grants, and that only because there's no choice -- if the role is >> about to go away, we can't hang on to a reference to its OID. > But I would have thought that the right thing to do to pg_init_privs > here would be essentially s/$OLDOWNER/$NEWOWNER/g. Doesn't seem right to me. That will give pg_dump the wrong idea of what the initial privileges actually were, and I don't see how it can construct correct delta GRANT/REVOKE on the basis of false information. During the dump reload, the extension will be recreated with the original owner (I think), causing its objects' privileges to go back to the original pg_init_privs values. Applying a delta that starts from some other state seems pretty questionable in that case. It could be that if we expect pg_dump to issue an ALTER OWNER to move ownership of the altered extension object to its new owner, and only then apply its computed delta GRANT/REVOKEs, then indeed the right thing is for the original ALTER OWNER to apply s/$OLDOWNER/$NEWOWNER/g to pg_init_privs. I've not thought this through in complete detail, but it feels like that might work, because the reload-time ALTER OWNER would apply exactly that change to both the object's ACL and its pg_init_privs, and then the delta is starting from the right state. Of course, pg_dump can't do that right now because it lacks the information that such an ALTER is needed. Although ... this is tickling a recollection that pg_dump doesn't try very hard to run CREATE EXTENSION with the same owner that the extension had originally. That's a leftover from the times when basically all extensions required superuser to install, and of course one superuser is as good as the next. There might be some work we have to do on that side too if we want to up our game in this area. Another case that's likely not handled well is what if the extension really shouldn't have its original owner (e.g. you're using --no-owner). If it's restored under a new owner then the pg_init_privs data certainly doesn't apply, and it feels like it'd be mostly luck if the precomputed delta GRANT/REVOKEs lead to a state you like. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Fri, May 24, 2024 at 11:59 AM Tom Lane wrote: > Thinking about this some more: the point of pg_init_privs is to record > an object's privileges as they stood at the end of CREATE EXTENSION > (or extension update), with the goal that pg_dump should be able to > compute the delta between that and the object's current privileges > and emit GRANT/REVOKE commands to restore those current privileges > after a fresh extension install. (We slide gently past the question > of whether the fresh extension install is certain to create privileges > matching the previous pg_init_privs entry.) +1 to all of this. > So this goal seems to > mean that neither ALTER OWNER nor REASSIGN OWNED should touch > pg_init_privs at all, as that would break its function of recording > a historical state. Only DROP OWNED should get rid of pg_init_privs > grants, and that only because there's no choice -- if the role is > about to go away, we can't hang on to a reference to its OID. But I would have thought that the right thing to do to pg_init_privs here would be essentially s/$OLDOWNER/$NEWOWNER/g. I know I'm late to the party here, but why is that idea wrong? -- Robert Haas EDB: http://www.enterprisedb.com
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Daniel Gustafsson writes: > On 24 May 2024, at 16:20, Tom Lane wrote: >> Another point: shdepReassignOwned explicitly does not touch grants >> or default ACLs. It feels like the same should be true of >> pg_init_privs entries, > Agreed, I can't see why pg_init_privs should be treated differently. Thinking about this some more: the point of pg_init_privs is to record an object's privileges as they stood at the end of CREATE EXTENSION (or extension update), with the goal that pg_dump should be able to compute the delta between that and the object's current privileges and emit GRANT/REVOKE commands to restore those current privileges after a fresh extension install. (We slide gently past the question of whether the fresh extension install is certain to create privileges matching the previous pg_init_privs entry.) So this goal seems to mean that neither ALTER OWNER nor REASSIGN OWNED should touch pg_init_privs at all, as that would break its function of recording a historical state. Only DROP OWNED should get rid of pg_init_privs grants, and that only because there's no choice -- if the role is about to go away, we can't hang on to a reference to its OID. However ... then what are the implications of doing ALTER OWNER on an extension-owned object? Is pg_dump supposed to recognize that that's happened and replay it too? If not, is it sane at all to try to restore the current privileges, which are surely dependent on the current owner? I kind of doubt that that's possible at all, and even if it is it might result in security issues. It seems like pg_init_privs has missed a critical thing, which is to record the original owner not only the original privileges. (Alternatively, maybe we should forbid ALTER OWNER on extension-owned objects? Or at least on those having pg_init_privs entries?) I'm wondering too about this scenario: 1. CREATE EXTENSION installs an object and sets some initial privileges. 2. DBA manually modifies the object's privileges. 3. ALTER EXTENSION UPDATE further modifies the object's privileges. I think what will happen is that at the end of ALTER EXTENSION, we'll store the object's current ACL verbatim in pg_init_privs, therefore including the effects of step 2. This seems undesirable, but I'm not sure how to get around it. Anyway, this is starting to look like the sort of can of worms best not opened post-beta1. v17 has made some things better in this area, and I don't think it's made anything worse; so maybe we should declare victory for the moment and hope to address these additional concerns later. I've added an open item though. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 24 May 2024, at 16:20, Tom Lane wrote: > I've tentatively concluded that I shouldn't have modeled > SHARED_DEPENDENCY_INITACL so closely on SHARED_DEPENDENCY_ACL, > in particular the decision that we don't need such an entry if > there's also SHARED_DEPENDENCY_OWNER. +1, in light of this report I think we need to go back on that. > I can see two routes to a solution: > > 1. Create SHARED_DEPENDENCY_INITACL, if applicable, whether the > role is the object's owner or not. Then, clearing out the > pg_shdepend entry cues us to go delete the pg_init_privs entry. > > 2. Just always search pg_init_privs for relevant entries > when dropping an object. > > I don't especially like #2 on performance grounds, but it has > a lot fewer moving parts than #1. #1 is more elegant, but admittedly also more complicated. An unscientific guess is that a majority of objects dropped won't have init privs, making the extra scan in #2 quite possibly more than academic. #2 could however be backported and solve the issue in existing clusters. > Another point: shdepReassignOwned explicitly does not touch grants > or default ACLs. It feels like the same should be true of > pg_init_privs entries, Agreed, I can't see why pg_init_privs should be treated differently. > Another thing I'm wondering about right now is privileges on global > objects (roles, databases, tablespaces). The fine manual says > "Although an extension script is not prohibited from creating such > objects, if it does so they will not be tracked as part of the > extension". Presumably, that also means that no pg_init_privs > entries are made; but do we do that correctly? I'm away from a tree to check, but that does warrant investigation. If we don't have a test for it already then it might be worth constructing something to catch that. -- Daniel Gustafsson
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Daniel Gustafsson writes: > I had a look, but I didn't beat you to a fix since it's not immediately clear > to me how this should work for REASSING OWNED (DROP OWNED seems a simpler > case). Should REASSIGN OWNED alter the rows in pg_shdepend matching init > privs > from SHARED_DEPENDENCY_OWNER to SHARED_DEPENDENCY_INITACL, so that these can > be > mopped up with a later DROP OWNED? Trying this in a POC patch it fails with > RemoveRoleFromInitPriv not removing the rows, shortcircuiting that for a test > seems to make it work but is it the right approach? I've tentatively concluded that I shouldn't have modeled SHARED_DEPENDENCY_INITACL so closely on SHARED_DEPENDENCY_ACL, in particular the decision that we don't need such an entry if there's also SHARED_DEPENDENCY_OWNER. I think one reason we can get away with omitting a SHARED_DEPENDENCY_ACL entry for the owner is that the object's normal ACL is part of its primary catalog row, so it goes away automatically if the object is dropped. But obviously that's not true for a pg_init_privs entry. I can see two routes to a solution: 1. Create SHARED_DEPENDENCY_INITACL, if applicable, whether the role is the object's owner or not. Then, clearing out the pg_shdepend entry cues us to go delete the pg_init_privs entry. 2. Just always search pg_init_privs for relevant entries when dropping an object. I don't especially like #2 on performance grounds, but it has a lot fewer moving parts than #1. In particular, there's some handwaving in changeDependencyOnOwner() about why we should drop SHARED_DEPENDENCY_ACL when changing owner, and I've not wrapped my head around how those concerns map to INITACL if we treat it in this different way. Another point: shdepReassignOwned explicitly does not touch grants or default ACLs. It feels like the same should be true of pg_init_privs entries, or at least if not, why not? In that case there's nothing to be done in shdepReassignOwned (although maybe its comments should be adjusted to mention this explicitly). The bug is just that DROP OWNED isn't getting rid of the entries because there's no INITACL entry to cue it to do so. Another thing I'm wondering about right now is privileges on global objects (roles, databases, tablespaces). The fine manual says "Although an extension script is not prohibited from creating such objects, if it does so they will not be tracked as part of the extension". Presumably, that also means that no pg_init_privs entries are made; but do we do that correctly? Anyway, -ENOCAFFEINE for the moment. I'll look more later. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 24 May 2024, at 01:01, Tom Lane wrote: > > Hannu Krosing writes: >> While the 'DROP OWNED BY fails to clean out pg_init_privs grants' >> issue is now fixed,we have a similar issue with REASSIGN OWNED BY that >> is still there: > > Ugh, how embarrassing. I'll take a look tomorrow, if no one > beats me to it. I had a look, but I didn't beat you to a fix since it's not immediately clear to me how this should work for REASSING OWNED (DROP OWNED seems a simpler case). Should REASSIGN OWNED alter the rows in pg_shdepend matching init privs from SHARED_DEPENDENCY_OWNER to SHARED_DEPENDENCY_INITACL, so that these can be mopped up with a later DROP OWNED? Trying this in a POC patch it fails with RemoveRoleFromInitPriv not removing the rows, shortcircuiting that for a test seems to make it work but is it the right approach? -- Daniel Gustafsson
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Hannu Krosing writes: > While the 'DROP OWNED BY fails to clean out pg_init_privs grants' > issue is now fixed,we have a similar issue with REASSIGN OWNED BY that > is still there: Ugh, how embarrassing. I'll take a look tomorrow, if no one beats me to it. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
While the 'DROP OWNED BY fails to clean out pg_init_privs grants'
issue is now fixed,we have a similar issue with REASSIGN OWNED BY that
is still there:
Tested on fresh git checkout om May 20th
test=# create user privtestuser superuser;
CREATE ROLE
test=# set role privtestuser;
SET
test=# create extension pg_stat_statements ;
CREATE EXTENSION
test=# select * from pg_init_privs where privtype ='e';
objoid | classoid | objsubid | privtype |
initprivs
+--+--+--+--
16405 | 1259 |0 | e|
{privtestuser=arwdDxtm/privtestuser,=r/privtestuser}
16422 | 1259 |0 | e|
{privtestuser=arwdDxtm/privtestuser,=r/privtestuser}
16427 | 1255 |0 | e| {privtestuser=X/privtestuser}
(3 rows)
test=# reset role;
RESET
test=# reassign owned by privtestuser to hannuk;
REASSIGN OWNED
test=# select * from pg_init_privs where privtype ='e';
objoid | classoid | objsubid | privtype |
initprivs
+--+--+--+--
16405 | 1259 |0 | e|
{privtestuser=arwdDxtm/privtestuser,=r/privtestuser}
16422 | 1259 |0 | e|
{privtestuser=arwdDxtm/privtestuser,=r/privtestuser}
16427 | 1255 |0 | e| {privtestuser=X/privtestuser}
(3 rows)
test=# drop user privtestuser ;
DROP ROLE
test=# select * from pg_init_privs where privtype ='e';
objoid | classoid | objsubid | privtype |initprivs
+--+--+--+-
16405 | 1259 |0 | e| {16390=arwdDxtm/16390,=r/16390}
16422 | 1259 |0 | e| {16390=arwdDxtm/16390,=r/16390}
16427 | 1255 |0 | e| {16390=X/16390}
(3 rows)
This will cause pg_dump to produce something that cant be loaded back
into the database:
CREATE EXTENSION IF NOT EXISTS pg_stat_statements WITH SCHEMA public;
...
REVOKE ALL ON TABLE public.pg_stat_statements FROM "16390";
...
And this will, among other things, break pg_upgrade.
-
Hannu
On Tue, Apr 30, 2024 at 6:40 AM David G. Johnston
wrote:
>
> On Monday, April 29, 2024, Tom Lane wrote:
>>
>> "David G. Johnston" writes:
>> > My solution to this was to rely on the fact that the bootstrap superuser is
>> > assigned OID 10 regardless of its name.
>>
>> Yeah, I wrote it that way to start with too, but reconsidered
>> because
>>
>> (1) I don't like hard-coding numeric OIDs. We can avoid that in C
>> code but it's harder to do in SQL.
>
>
> If the tests don’t involve, e.g., the predefined role pg_monitor and its
> grantor of the memberships in the other predefined roles, this indeed can be
> avoided. So I think my test still needs to check for 10 even if some other
> superuser is allowed to produce the test output since a key output in my case
> was the bootstrap superuser and the initdb roles.
>
>>
>> (2) It's not clear to me that this test couldn't be run by a
>> non-bootstrap superuser. I think "current_user" is actually
>> the correct thing for the role executing the test.
>
>
> Agreed, testing against current_role is correct if the things being queried
> were created while executing the test. I would need to do this as well to
> remove the current requirement that my tests be run by the bootstrap
> superuser.
>
> David J.
>
Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Monday, April 29, 2024, Tom Lane wrote: > "David G. Johnston" writes: > > My solution to this was to rely on the fact that the bootstrap superuser > is > > assigned OID 10 regardless of its name. > > Yeah, I wrote it that way to start with too, but reconsidered > because > > (1) I don't like hard-coding numeric OIDs. We can avoid that in C > code but it's harder to do in SQL. If the tests don’t involve, e.g., the predefined role pg_monitor and its grantor of the memberships in the other predefined roles, this indeed can be avoided. So I think my test still needs to check for 10 even if some other superuser is allowed to produce the test output since a key output in my case was the bootstrap superuser and the initdb roles. > (2) It's not clear to me that this test couldn't be run by a > non-bootstrap superuser. I think "current_user" is actually > the correct thing for the role executing the test. > Agreed, testing against current_role is correct if the things being queried were created while executing the test. I would need to do this as well to remove the current requirement that my tests be run by the bootstrap superuser. David J.
Re: DROP OWNED BY fails to clean out pg_init_privs grants
"David G. Johnston" writes: > My solution to this was to rely on the fact that the bootstrap superuser is > assigned OID 10 regardless of its name. Yeah, I wrote it that way to start with too, but reconsidered because (1) I don't like hard-coding numeric OIDs. We can avoid that in C code but it's harder to do in SQL. (2) It's not clear to me that this test couldn't be run by a non-bootstrap superuser. I think "current_user" is actually the correct thing for the role executing the test. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Monday, April 29, 2024, Tom Lane wrote: > Daniel Gustafsson writes: > >> On 28 Apr 2024, at 20:52, Tom Lane wrote: > > > >> This is of course not bulletproof: with a sufficiently weird > >> bootstrap superuser name, we could get false matches to parts > >> of "regress_dump_test_role" or to privilege strings. That > >> seems unlikely enough to live with, but I wonder if anybody has > >> a better idea. > > > I think that will be bulletproof enough to keep it working in the > buildfarm and > > among 99% of hackers. > > It occurred to me to use "aclexplode" to expand the initprivs, and > then we can substitute names with simple equality tests. The test > query is a bit more complicated, but I feel better about it. > My solution to this was to rely on the fact that the bootstrap superuser is assigned OID 10 regardless of its name. David J.
Re: DROP OWNED BY fails to clean out pg_init_privs grants
I wrote: > Pushed, thanks for reviewing! Argh, I forgot I'd meant to push b0c5b215d first not second. Oh well, it was only neatnik-ism that made me want to see those other animals fail --- and a lot of the buildfarm is red right now for $other_reasons anyway. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Daniel Gustafsson writes: > On 29 Apr 2024, at 21:15, Tom Lane wrote: >> v3 attached also has a bit more work on code comments. > LGTM. Pushed, thanks for reviewing! regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 29 Apr 2024, at 21:15, Tom Lane wrote: > It occurred to me to use "aclexplode" to expand the initprivs, and > then we can substitute names with simple equality tests. The test > query is a bit more complicated, but I feel better about it. Nice, I didn't even remember that function existed. I agree that it's an improvement even at the increased query complexity. > v3 attached also has a bit more work on code comments. LGTM. -- Daniel Gustafsson
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Daniel Gustafsson writes:
>> On 28 Apr 2024, at 20:52, Tom Lane wrote:
>> ... It's a little bit
>> nasty to look at the ACL column of pg_init_privs, because that text
>> involves the bootstrap superuser's name which is site-dependent.
>> What I did to try to make the test stable is
>> replace(initprivs::text, current_user, 'postgres') AS initprivs
> Maybe that part warrants a small comment in the testfile to keep it from
> sending future readers into rabbitholes?
Agreed.
>> This is of course not bulletproof: with a sufficiently weird
>> bootstrap superuser name, we could get false matches to parts
>> of "regress_dump_test_role" or to privilege strings. That
>> seems unlikely enough to live with, but I wonder if anybody has
>> a better idea.
> I think that will be bulletproof enough to keep it working in the buildfarm
> and
> among 99% of hackers.
It occurred to me to use "aclexplode" to expand the initprivs, and
then we can substitute names with simple equality tests. The test
query is a bit more complicated, but I feel better about it.
v3 attached also has a bit more work on code comments.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..c8cb46c5b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7184,6 +7184,20 @@ SCRAM-SHA-256$iteration count:
+
+ SHARED_DEPENDENCY_INITACL (i)
+
+
+ The referenced object (which must be a role) is mentioned in a
+ pg_init_privs
+ entry for the dependent object.
+ (A SHARED_DEPENDENCY_INITACL entry is not made for
+ the owner of the object, since the owner will have
+ a SHARED_DEPENDENCY_OWNER entry anyway.)
+
+
+
+
SHARED_DEPENDENCY_POLICY (r)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..e6cc720579 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -165,9 +165,9 @@ static AclMode pg_type_aclmask_ext(Oid type_oid, Oid roleid,
AclMode mask, AclMaskHow how,
bool *is_missing);
static void recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
static void recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
/*
@@ -1447,7 +1447,19 @@ SetDefaultACL(InternalDefaultACL *iacls)
/*
* RemoveRoleFromObjectACL
*
- * Used by shdepDropOwned to remove mentions of a role in ACLs
+ * Used by shdepDropOwned to remove mentions of a role in ACLs.
+ *
+ * Notice that this doesn't accept an objsubid parameter, which is a bit bogus
+ * since the pg_shdepend record that caused us to call it certainly had one.
+ * If, for example, pg_shdepend records the existence of a permission on
+ * mytable.mycol, this function will effectively issue a REVOKE ALL ON TABLE
+ * mytable. That gets the job done because (per SQL spec) such a REVOKE also
+ * revokes per-column permissions. We could not recreate a situation where
+ * the role has table-level but not column-level permissions; but it's okay
+ * (for now anyway) because this is only used when we're dropping the role
+ * and so all its permissions everywhere must go away. At worst it's a bit
+ * inefficient if the role has column permissions on several columns of the
+ * same table.
*/
void
RemoveRoleFromObjectACL(Oid roleid, Oid classid, Oid objid)
@@ -1790,7 +1802,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
CatalogTupleUpdate(attRelation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, attnum,
+ recordExtensionInitPriv(relOid, RelationRelationId, attnum, ownerId,
ACL_NUM(new_acl) > 0 ? new_acl : NULL);
/* Update the shared dependency ACL info */
@@ -2050,7 +2062,8 @@ ExecGrant_Relation(InternalGrant *istmt)
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
+ recordExtensionInitPriv(relOid, RelationRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(RelationRelationId, relOid, 0,
@@ -2251,7 +2264,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(objectid, classid, 0, new_acl);
+ recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(classid,
@@ -2403,7 +2416,8 @@ ExecGrant_Largeobject(InternalGrant *istmt)
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 28 Apr 2024, at 20:52, Tom Lane wrote: > > I wrote: >> Here's a draft patch that attacks that. It seems to fix the >> problem with test_pg_dump: no dangling pg_init_privs grants >> are left behind. Reading this I can't find any sharp edges, and I prefer your changes to recordExtensionInitPriv over the version I had half-baked by the time you had this finished. Trying to break it with the testcases I had devised also failed, so +1. > Here's a v2 that attempts to add some queries to test_pg_dump.sql > to provide visual verification that pg_shdepend and pg_init_privs > are updated correctly during DROP OWNED BY. It's a little bit > nasty to look at the ACL column of pg_init_privs, because that text > involves the bootstrap superuser's name which is site-dependent. > What I did to try to make the test stable is > > replace(initprivs::text, current_user, 'postgres') AS initprivs Maybe that part warrants a small comment in the testfile to keep it from sending future readers into rabbitholes? > This is of course not bulletproof: with a sufficiently weird > bootstrap superuser name, we could get false matches to parts > of "regress_dump_test_role" or to privilege strings. That > seems unlikely enough to live with, but I wonder if anybody has > a better idea. I think that will be bulletproof enough to keep it working in the buildfarm and among 99% of hackers. -- Daniel Gustafsson
Re: DROP OWNED BY fails to clean out pg_init_privs grants
I wrote:
> Here's a draft patch that attacks that. It seems to fix the
> problem with test_pg_dump: no dangling pg_init_privs grants
> are left behind.
Here's a v2 that attempts to add some queries to test_pg_dump.sql
to provide visual verification that pg_shdepend and pg_init_privs
are updated correctly during DROP OWNED BY. It's a little bit
nasty to look at the ACL column of pg_init_privs, because that text
involves the bootstrap superuser's name which is site-dependent.
What I did to try to make the test stable is
replace(initprivs::text, current_user, 'postgres') AS initprivs
This is of course not bulletproof: with a sufficiently weird
bootstrap superuser name, we could get false matches to parts
of "regress_dump_test_role" or to privilege strings. That
seems unlikely enough to live with, but I wonder if anybody has
a better idea.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..c8cb46c5b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7184,6 +7184,20 @@ SCRAM-SHA-256$iteration count:
+
+ SHARED_DEPENDENCY_INITACL (i)
+
+
+ The referenced object (which must be a role) is mentioned in a
+ pg_init_privs
+ entry for the dependent object.
+ (A SHARED_DEPENDENCY_INITACL entry is not made for
+ the owner of the object, since the owner will have
+ a SHARED_DEPENDENCY_OWNER entry anyway.)
+
+
+
+
SHARED_DEPENDENCY_POLICY (r)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..04c41c0c14 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -165,9 +165,9 @@ static AclMode pg_type_aclmask_ext(Oid type_oid, Oid roleid,
AclMode mask, AclMaskHow how,
bool *is_missing);
static void recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
static void recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
/*
@@ -1790,7 +1790,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
CatalogTupleUpdate(attRelation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, attnum,
+ recordExtensionInitPriv(relOid, RelationRelationId, attnum, ownerId,
ACL_NUM(new_acl) > 0 ? new_acl : NULL);
/* Update the shared dependency ACL info */
@@ -2050,7 +2050,8 @@ ExecGrant_Relation(InternalGrant *istmt)
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
+ recordExtensionInitPriv(relOid, RelationRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(RelationRelationId, relOid, 0,
@@ -2251,7 +2252,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(objectid, classid, 0, new_acl);
+ recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(classid,
@@ -2403,7 +2404,8 @@ ExecGrant_Largeobject(InternalGrant *istmt)
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(loid, LargeObjectRelationId, 0, new_acl);
+ recordExtensionInitPriv(loid, LargeObjectRelationId, 0,
+ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(LargeObjectRelationId,
@@ -2575,7 +2577,7 @@ ExecGrant_Parameter(InternalGrant *istmt)
/* Update initial privileges for extensions */
recordExtensionInitPriv(parameterId, ParameterAclRelationId, 0,
-new_acl);
+ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(ParameterAclRelationId, parameterId, 0,
@@ -4463,6 +4465,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
}
recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ pg_class_tuple->relowner,
DatumGetAclP(attaclDatum));
ReleaseSysCache(attTuple);
@@ -4475,6 +4478,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
/* Add the record, if any, for the top-level object */
if (!isNull)
recordExtensionInitPrivWorker(objoid, classoid, 0,
+ pg_class_tuple->relowner,
DatumGetAclP(aclDatum));
ReleaseSysCache(tuple);
@@ -4485,6 +4489,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
Datum aclDatum;
bool isNull;
HeapTuple tuple;
+ Form_pg_largeobject_metadata form_lo_meta;
ScanKeyData
Re: DROP OWNED BY fails to clean out pg_init_privs grants
I wrote:
> A bigger problem though is that I think you are addressing the
> original complaint from the older thread, which while it's a fine
> thing to fix seems orthogonal to the failure we're seeing in the
> buildfarm. The buildfarm's problem is not that we're recording
> incorrect pg_init_privs entries, it's that when we do create such
> entries we're failing to show their dependency on the grantee role
> in pg_shdepend. We've missed spotting that so far because it's
> so seldom that pg_init_privs entries reference any but built-in
> roles (or at least roles that'd likely outlive the extension).
Here's a draft patch that attacks that. It seems to fix the
problem with test_pg_dump: no dangling pg_init_privs grants
are left behind.
A lot of the changes here are just involved with needing to pass the
object's owner OID to recordExtensionInitPriv so that it can be passed
to updateAclDependencies. One thing I'm a bit worried about is that
some of the new code assumes that all object types that are of
interest here will have catcaches on OID, so that it's possible to
fetch the owner OID for a generic object-with-privileges using the
catcache and objectaddress.c's tables of object properties. That
assumption seems to exist already, eg ExecGrant_common also assumes
it, but it's not obvious that it must be so.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 2907079e2a..c8cb46c5b9 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -7184,6 +7184,20 @@ SCRAM-SHA-256$iteration count:
+
+ SHARED_DEPENDENCY_INITACL (i)
+
+
+ The referenced object (which must be a role) is mentioned in a
+ pg_init_privs
+ entry for the dependent object.
+ (A SHARED_DEPENDENCY_INITACL entry is not made for
+ the owner of the object, since the owner will have
+ a SHARED_DEPENDENCY_OWNER entry anyway.)
+
+
+
+
SHARED_DEPENDENCY_POLICY (r)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..04c41c0c14 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -165,9 +165,9 @@ static AclMode pg_type_aclmask_ext(Oid type_oid, Oid roleid,
AclMode mask, AclMaskHow how,
bool *is_missing);
static void recordExtensionInitPriv(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
static void recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid,
- Acl *new_acl);
+ Oid ownerId, Acl *new_acl);
/*
@@ -1790,7 +1790,7 @@ ExecGrant_Attribute(InternalGrant *istmt, Oid relOid, const char *relname,
CatalogTupleUpdate(attRelation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, attnum,
+ recordExtensionInitPriv(relOid, RelationRelationId, attnum, ownerId,
ACL_NUM(new_acl) > 0 ? new_acl : NULL);
/* Update the shared dependency ACL info */
@@ -2050,7 +2050,8 @@ ExecGrant_Relation(InternalGrant *istmt)
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(relOid, RelationRelationId, 0, new_acl);
+ recordExtensionInitPriv(relOid, RelationRelationId, 0,
+ ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(RelationRelationId, relOid, 0,
@@ -2251,7 +2252,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs,
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(objectid, classid, 0, new_acl);
+ recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(classid,
@@ -2403,7 +2404,8 @@ ExecGrant_Largeobject(InternalGrant *istmt)
CatalogTupleUpdate(relation, >t_self, newtuple);
/* Update initial privileges for extensions */
- recordExtensionInitPriv(loid, LargeObjectRelationId, 0, new_acl);
+ recordExtensionInitPriv(loid, LargeObjectRelationId, 0,
+ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(LargeObjectRelationId,
@@ -2575,7 +2577,7 @@ ExecGrant_Parameter(InternalGrant *istmt)
/* Update initial privileges for extensions */
recordExtensionInitPriv(parameterId, ParameterAclRelationId, 0,
-new_acl);
+ownerId, new_acl);
/* Update the shared dependency ACL info */
updateAclDependencies(ParameterAclRelationId, parameterId, 0,
@@ -4463,6 +4465,7 @@ recordExtObjInitPriv(Oid objoid, Oid classoid)
}
recordExtensionInitPrivWorker(objoid, classoid, curr_att,
+ pg_class_tuple->relowner,
DatumGetAclP(attaclDatum));
ReleaseSysCache(attTuple);
@@ -4475,6
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Daniel Gustafsson writes: > On 21 Apr 2024, at 23:08, Tom Lane wrote: >> So the meson animals are not running the test that sets up the >> problematic data. > I took a look at this, reading code and the linked thread. My gut feeling is > that Stephen is right in that the underlying bug is these privileges ending up > in pg_init_privs to begin with. That being said, I wasn't able to fix that in > a way that doesn't seem like a terrible hack. Hmm, can't we put the duplicate logic inside recordExtensionInitPriv? Even if these calls need a different result from others, adding a flag parameter seems superior to having N copies of the logic. A bigger problem though is that I think you are addressing the original complaint from the older thread, which while it's a fine thing to fix seems orthogonal to the failure we're seeing in the buildfarm. The buildfarm's problem is not that we're recording incorrect pg_init_privs entries, it's that when we do create such entries we're failing to show their dependency on the grantee role in pg_shdepend. We've missed spotting that so far because it's so seldom that pg_init_privs entries reference any but built-in roles (or at least roles that'd likely outlive the extension). regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 21 Apr 2024, at 23:08, Tom Lane wrote: > > Daniel Gustafsson writes: >>> On 6 Apr 2024, at 01:10, Tom Lane wrote: >>> (So now I'm wondering why *only* copperhead has shown this so far. >>> Are our other cross-version-upgrade testing animals AWOL?) > >> Clicking around searching for Xversion animals I didn't spot any, but without >> access to the database it's nontrivial to know which animal does what. > > I believe I see why this is (or isn't) happening. The animals > currently running xversion tests are copperhead, crake, drongo, > and fairywren. copperhead is using the makefiles while the others > are using meson. And I find this in > src/test/modules/test_pg_dump/meson.build (from 3f0e786cc): > ># doesn't delete its user >'runningcheck': false, > > So the meson animals are not running the test that sets up the > problematic data. ugh =/ > I think we should remove the above, since (a) the reason to have > it is gone, and (b) it seems really bad that the set of tests > run by meson is different from that run by the makefiles. Agreed. > However, once we do that, those other three animals will presumably go > red, greatly complicating detection of any Windows-specific problems. > So I'm inclined to not do it till just before we intend to commit > a fix for the underlying problem. (Enough before that we can confirm > that they do go red.) Agreed, we definitely want that but compromising the ability to find Windows issues at this point in the cycle seems bad. > ... were you going to look at it? I can take a whack if it's > too far down your priority list. I took a look at this, reading code and the linked thread. My gut feeling is that Stephen is right in that the underlying bug is these privileges ending up in pg_init_privs to begin with. That being said, I wasn't able to fix that in a way that doesn't seem like a terrible hack. The attached POC hack fixes it for me but I'm not sure how to fix it properly. Your wisdom would be much appreciated. Clusters which already has such entries aren't helped by a fix for this though, fixing that would either require pg_dump to skip them, or pg_upgrade to have a check along with instructions for fixing the issue. Not sure what's the best strategy here, the lack of complaints could indicate this isn't terribly common so spending cycles on it for every pg_dump might be excessive compared to a pg_upgrade check? -- Daniel Gustafsson init_privs.diff Description: Binary data
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 21 Apr 2024, at 23:08, Tom Lane wrote: > ... were you going to look at it? I can take a whack if it's too far down > your priority list. Yeah, I’m working on a patchset right now. ./daniel
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Daniel Gustafsson writes: >> On 6 Apr 2024, at 01:10, Tom Lane wrote: >> (So now I'm wondering why *only* copperhead has shown this so far. >> Are our other cross-version-upgrade testing animals AWOL?) > Clicking around searching for Xversion animals I didn't spot any, but without > access to the database it's nontrivial to know which animal does what. I believe I see why this is (or isn't) happening. The animals currently running xversion tests are copperhead, crake, drongo, and fairywren. copperhead is using the makefiles while the others are using meson. And I find this in src/test/modules/test_pg_dump/meson.build (from 3f0e786cc): # doesn't delete its user 'runningcheck': false, So the meson animals are not running the test that sets up the problematic data. I think we should remove the above, since (a) the reason to have it is gone, and (b) it seems really bad that the set of tests run by meson is different from that run by the makefiles. However, once we do that, those other three animals will presumably go red, greatly complicating detection of any Windows-specific problems. So I'm inclined to not do it till just before we intend to commit a fix for the underlying problem. (Enough before that we can confirm that they do go red.) Speaking of which ... >> I doubt this is something we'll have fixed by Monday, so I will >> go add an open item for it. > +1. Having opened the can of worms I'll have a look at it next week. ... were you going to look at it? I can take a whack if it's too far down your priority list. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
> On 6 Apr 2024, at 01:10, Tom Lane wrote: > (So now I'm wondering why *only* copperhead has shown this so far. > Are our other cross-version-upgrade testing animals AWOL?) Clicking around searching for Xversion animals I didn't spot any, but without access to the database it's nontrivial to know which animal does what. > I doubt this is something we'll have fixed by Monday, so I will > go add an open item for it. +1. Having opened the can of worms I'll have a look at it next week. -- Daniel Gustafsson
Re: DROP OWNED BY fails to clean out pg_init_privs grants
Noah Misch writes: > On Fri, Apr 05, 2024 at 07:10:59PM -0400, Tom Lane wrote: >> The fact that the DROP ROLE added by 936e3fa37 succeeded indicates >> that these role references weren't captured in pg_shdepend. >> I imagine that we also lack code that would allow DROP OWNED BY to >> follow up on such entries if they existed, but I've not checked that >> for sure. In any case, there's probably a nontrivial amount of code >> to be written to make this work. >> >> Given the lack of field complaints, I suspect that extension scripts >> simply don't grant privileges to random roles that aren't the >> extension's owner. So I wonder a little bit if this is even worth >> fixing, as opposed to blocking off somehow. But probably we should >> first try to fix it. > This sounds closely-related to the following thread: > https://www.postgresql.org/message-id/flat/1573808483712.96817%40Optiver.com Oh, interesting, I'd forgotten that thread completely. So Stephen was pushing back against dealing with the case because he thought that the SQL commands issued in that example should not have produced pg_init_privs entries in the first place. Which nobody else wanted to opine on, so the thread stalled. However, in the case of the test_pg_dump extension, the test_pg_dump--1.0.sql script absolutely did grant those privileges so it's very hard for me to think that they shouldn't be listed in pg_init_privs. Hence, I think we've accidentally stumbled across a case where we do need all that mechanism --- unless somebody wants to argue that what test_pg_dump--1.0.sql is doing should be disallowed. regards, tom lane
Re: DROP OWNED BY fails to clean out pg_init_privs grants
On Fri, Apr 05, 2024 at 07:10:59PM -0400, Tom Lane wrote:
> I wondered why buildfarm member copperhead has started to fail
> xversion-upgrade-HEAD-HEAD tests. I soon reproduced the problem here:
>
> pg_restore: creating ACL "regress_pg_dump_schema.TYPE "test_type""
> pg_restore: while PROCESSING TOC:
> pg_restore: from TOC entry 4355; 0 0 ACL TYPE "test_type" buildfarm
> pg_restore: error: could not execute query: ERROR: role "74603" does not
> exist
> Command was: SELECT pg_catalog.binary_upgrade_set_record_init_privs(true);
> GRANT ALL ON TYPE "regress_pg_dump_schema"."test_type" TO "74603";
> SELECT pg_catalog.binary_upgrade_set_record_init_privs(false);
> REVOKE ALL ON TYPE "regress_pg_dump_schema"."test_type" FROM "74603";
>
> (So now I'm wondering why *only* copperhead has shown this so far.
> Are our other cross-version-upgrade testing animals AWOL?)
>
> I believe this is a longstanding problem that was exposed by accident
> by commit 936e3fa37. If you run "make installcheck" in HEAD's
> src/test/modules/test_pg_dump, and then poke around in the leftover
> contrib_regression database, you can find dangling grants in
> pg_init_privs:
>
> contrib_regression=# table pg_init_privs;
> objoid | classoid | objsubid | privtype |
> initprivs
>
> +--+--+--+--
> ---
> ...
> es}
> 43134 | 1259 |0 | e|
> {postgres=rwU/postgres,43125=U/postgr
> es}
> 43128 | 1259 |0 | e|
> {postgres=arwdDxtm/postgres,43125=r/p
> ostgres}
> ...
>
> The fact that the DROP ROLE added by 936e3fa37 succeeded indicates
> that these role references weren't captured in pg_shdepend.
> I imagine that we also lack code that would allow DROP OWNED BY to
> follow up on such entries if they existed, but I've not checked that
> for sure. In any case, there's probably a nontrivial amount of code
> to be written to make this work.
>
> Given the lack of field complaints, I suspect that extension scripts
> simply don't grant privileges to random roles that aren't the
> extension's owner. So I wonder a little bit if this is even worth
> fixing, as opposed to blocking off somehow. But probably we should
> first try to fix it.
This sounds closely-related to the following thread:
https://www.postgresql.org/message-id/flat/1573808483712.96817%40Optiver.com
DROP OWNED BY fails to clean out pg_init_privs grants
I wondered why buildfarm member copperhead has started to fail
xversion-upgrade-HEAD-HEAD tests. I soon reproduced the problem here:
pg_restore: creating ACL "regress_pg_dump_schema.TYPE "test_type""
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 4355; 0 0 ACL TYPE "test_type" buildfarm
pg_restore: error: could not execute query: ERROR: role "74603" does not exist
Command was: SELECT pg_catalog.binary_upgrade_set_record_init_privs(true);
GRANT ALL ON TYPE "regress_pg_dump_schema"."test_type" TO "74603";
SELECT pg_catalog.binary_upgrade_set_record_init_privs(false);
REVOKE ALL ON TYPE "regress_pg_dump_schema"."test_type" FROM "74603";
(So now I'm wondering why *only* copperhead has shown this so far.
Are our other cross-version-upgrade testing animals AWOL?)
I believe this is a longstanding problem that was exposed by accident
by commit 936e3fa37. If you run "make installcheck" in HEAD's
src/test/modules/test_pg_dump, and then poke around in the leftover
contrib_regression database, you can find dangling grants in
pg_init_privs:
contrib_regression=# table pg_init_privs;
objoid | classoid | objsubid | privtype |initprivs
+--+--+--+--
---
...
es}
43134 | 1259 |0 | e| {postgres=rwU/postgres,43125=U/postgr
es}
43128 | 1259 |0 | e| {postgres=arwdDxtm/postgres,43125=r/p
ostgres}
...
The fact that the DROP ROLE added by 936e3fa37 succeeded indicates
that these role references weren't captured in pg_shdepend.
I imagine that we also lack code that would allow DROP OWNED BY to
follow up on such entries if they existed, but I've not checked that
for sure. In any case, there's probably a nontrivial amount of code
to be written to make this work.
Given the lack of field complaints, I suspect that extension scripts
simply don't grant privileges to random roles that aren't the
extension's owner. So I wonder a little bit if this is even worth
fixing, as opposed to blocking off somehow. But probably we should
first try to fix it.
I doubt this is something we'll have fixed by Monday, so I will
go add an open item for it.
regards, tom lane
