Re: pg_auth_members.grantor is bunk - Mailing list pgsql-hackers
| From | Stephen Frost |
|---|---|
| Subject | Re: pg_auth_members.grantor is bunk |
| Date | |
| Msg-id | CAOuzzgqhfXYd6jF7N1egGkrshkuWADvdNN8SAE13UHdnYueoew@mail.gmail.com Whole thread Raw |
| In response to | Re: pg_auth_members.grantor is bunk ("David G. Johnston" <david.g.johnston@gmail.com>) |
| List | pgsql-hackers |
Greetings,
On Sun, Jul 31, 2022 at 11:44 David G. Johnston <david.g.johnston@gmail.com> wrote:
On Sun, Jul 31, 2022 at 11:18 AM Stephen Frost <sfrost@snowman.net> wrote:Greetings,
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, Jul 26, 2022 at 12:46 PM Robert Haas <robertmhaas@gmail.com> wrote:
+ }
+
+ /*
+ * Disallow attempts to grant ADMIN OPTION back to a user who granted it
+ * to you, similar to what check_circularity does for ACLs. We want the
+ * chains of grants to remain acyclic, so that it's always possible to use
+ * REVOKE .. CASCADE to clean up all grants that depend on the one being
+ * revoked.
+ *
+ * NB: This check might look redundant with the check for membership
+ * loops above, but it isn't. That's checking for role-member loop (e.g.
+ * A is a member of B and B is a member of A) while this is checking for
+ * a member-grantor loop (e.g. A gave ADMIN OPTION to X to B and now B, who
+ * has no other source of ADMIN OPTION on X, tries to give ADMIN OPTION
+ * on X back to A).
+ */
With this exact scenario, wouldn't it just be a no-op as A must have
ADMIN OPTION already on X? The spec says that no cycles of role
authorizations are allowed.
I’ve realized that what I hadn’t been contemplating here is actually that the GRANT from B to A for X wouldn’t be redundant because grantor is part of the key (A got the right from someone else, but this would be giving it to A from B and therefore would be distinct and would also create a loop which is no good). Haven’t got a good idea on how to improve on the comment based off of that though it still feels like it could be clearer. If I think of something, I’ll share it.
Role A must have admin option for X to grant membership in X (with or without admin option) to B. But that doesn't preclude A from getting another admin option from someone else. That someone else cannot be someone to whom they gave admin option to however. So B cannot grant admin option back to A but role P could if it was basically a sibling of A (i.e., both getting their initial admin option from someone else).
Right but that wasn’t what I had been trying to get at above.
If they do have admin option twice it should be possible to drop one of them, the prohibition should be on dropping the only admin option permission a role has for some other role. The commit message for 2 contemplates this though I haven't gone through the revocation code in detail.
Yes, think I agree with this also- if A has been given the WITH ADMIN right from Q and P to GRANT X to other roles, and uses that to GRANT X to B, then the GRANT of X to B should be retained even if Q decides to revoke their GRANT as A still has the right from P. If both remove the right, however, either B should lose the right (if CASCADE was passed in) or an error should be returned saying that there’s a dependent GRANT and CASCADE wasn’t given.
I'm trying to get this review out sooner than later and so I might be
missing something, but looking at the regression test for this and these
error messages, feels like the 'circular' error message makes more sense
than the 'your own grantor' message that actually ends up being
returned in that regression test.Having a more specific error seems reasonable, faster to track down what the problem is.
Yeah, but also making sure that all the error messages we have in this area are in the regression test output would be good.
Makes me wonder if we might try to figure out a way to globally check for that. I suppose one could review coverage.p.o for any ereport() calls that aren’t ever called. I wonder what that would turn up.
I think that the whole graph dynamic of this might need some presentation work (messages and/or psql and/or functions) ; but assuming the errors are handled improved messages and/or presentation of graphs can be a separate enhancement.
Yes, we can further improve this later too but that doesn’t mean we should just commit this as-is when some deficiencies have been pointed out. If the only comments were “would be good to improve this error message but I haven’t got a great idea how”, then sure, but there were other items pointed out which were clear corrections and we should make sure to cover in the regression tests all these scenarios that we are checking for and erroring on, lest we end up breaking them unintentionally later.
Thanks,
Stephen
pgsql-hackers by date: