sql: Update role membership for PostgreSQL v16 #18780

jkosh44 · 2023-04-15T22:09:37Z

Previously, role membership was modeled after the implementation of role membership in PostgreSQL v15. Specifically the grantor was set as the role ID of the session executing the GRANT statement. However, PostgreSQL v16, which has not been released yet as of this commit, updated role membership to be more consistent with the SQL standard and with object privileges. Specifically the grantor could only be set to someone with ADMIN OPTION on the role being granted or the bootstrap superuser.

This commit set's the grantor of all role membership to the mz_system role. We haven't implemented ADMIN OPTION yet, and the 'mz_system' role is our version of the bootstrap superuser.

The PostgreSQL commit that made this change can be found here: postgres/postgres@ce6b672

Part of #11579

Motivation

Tips for reviewer

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

Previously, role membership was modeled after the implementation of role membership in PostgreSQL v15. Specifically the grantor was set as the role ID of the session executing the `GRANT` statement. However, PostgreSQL v16, which has not been released yet as of this commit, updated role membership to be more consistent with the SQL standard and with object privileges. Specifically the grantor could only be set to someone with ADMIN OPTION on the role being granted or the bootstrap superuser. This commit set's the grantor of all role membership to the mz_system role. We haven't implemented ADMIN OPTION yet, and the 'mz_system' role is our version of the bootstrap superuser. The PostgreSQL commit that made this change can be found here: postgres/postgres@ce6b672 Part of MaterializeInc#11579

src/sql/src/plan.rs

maddyblue · 2023-04-17T06:42:12Z

src/sql/src/plan/statement/ddl.rs

+    // equivalent of the bootstrap superuser. Therefore the grantor is always 'mz_system'.
+    // For more details see:
+    // https://github.com/postgres/postgres/blob/064eb89e83ea0f59426c92906329f1e6c423dfa4/src/backend/commands/user.c#L2180-L2238
+    let grantor_id = scx


Will this make the audit log useless for looking up which human typed grant or revoke? If so, we should delay this PR until ADMIN OPTION is implemented somehow. Can ADMIN OPTION be implied for superusers?

Will this make the audit log useless for looking up which human typed grant or revoke?

Pretty much yes.

If so, we should delay this PR until ADMIN OPTION is implemented somehow.

Even when ADMIN OPTION is implemented, the grantor role does not always reflect the role that executed the GRANT statement. It reflects the role chosen as the best grantor.

Can ADMIN OPTION be implied for superusers.

Yes in the sense that superusers can always execute GRANT. No in the sense that the superuser will not be chosen as the grantor unless they have ADMIN OPTION explicitly.

Maybe the audit log entry should contain an additional field of something like "session role membership".

I updated the audit log to also contain the role of the session that execute the command.

If you're curious the pg mailing thread that discusses this change can be found here: https://www.postgresql.org/message-id/flat/1102925.1654199458%40sss.pgh.pa.us#505bfe4947d7a86ee07d5fdad85f34d1

It's very long and mostly irrelevant to this change (I haven't finished reading the whole thing yet). Scroll down to where the email subject changes from "CREATEROLE and role ownership hierarchies" to "pg_auth_members.grantor is bunk" for the relevant emails.

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>

…ize into role-member-grantor-fix

…e-member-grantor-fix # Conflicts: # src/adapter/src/catalog/storage.rs

def-

What is our guidance on compatibility with Postgres? Should it always be with the latest version, if that is a breaking change?

jkosh44 · 2023-04-17T16:08:50Z

What is our guidance on compatibility with Postgres? Should it always be with the latest version, if that is a breaking change?

In general we try and target the most recent version of PostgreSQL, though I'm not sure if that's written down anywhere. For breaking changes we can address them on a case-by-case basis. However, PostgreSQL is also reluctant to make breaking changes, so if they're making the change then it's probably an important change.

…e-member-grantor-fix # Conflicts: # src/adapter/src/catalog/storage.rs

maddyblue

I haven't read the postgres thread to understand their reasoning, but I trust them. The executed_by fields in the audit log seem good. Question: if the grantor fields were bunk before this change, are the executed_by fields also bunk?

jkosh44 · 2023-04-17T19:54:32Z

I haven't read the postgres thread to understand their reasoning, but I trust them. The executed_by fields in the audit log seem good. Question: if the grantor fields were bunk before this change, are the executed_by fields also bunk?

I haven't read every single message, but my understanding of "bunk" in this context is that it means:

Not compliant with the SQL spec.
May refer to a role that's been deleted.
Does not form a tree with the builtin superuser as the root. The tree allows you to trace the lineage of all grants.
Is inconsistent with object privilege grantors.
Doesn't allow duplicate privileges with different grantors.

None of these really apply to executed_by which just allows us to audit who actually typed the command.

jkosh44 force-pushed the role-member-grantor-fix branch from 7fa1e56 to 60d76d6 Compare April 15, 2023 22:32

jkosh44 requested a review from def- April 15, 2023 22:56

jkosh44 mentioned this pull request Apr 15, 2023

sql: Ensure role member grantor is always valid #18781

Merged

5 tasks

jkosh44 force-pushed the role-member-grantor-fix branch from 60d76d6 to feabcba Compare April 15, 2023 23:07

jkosh44 force-pushed the role-member-grantor-fix branch from feabcba to 95116e7 Compare April 15, 2023 23:22

jkosh44 marked this pull request as ready for review April 15, 2023 23:22

jkosh44 requested a review from a team as a code owner April 15, 2023 23:22

maddyblue reviewed Apr 17, 2023

View reviewed changes

jkosh44 and others added 4 commits April 17, 2023 10:35

Update src/sql/src/plan.rs

9badcac

Co-authored-by: Matt Jibson <matt.jibson@gmail.com>

Merge branch 'role-member-grantor-fix' of github.com:jkosh44/material…

0ab283e

…ize into role-member-grantor-fix

Merge branch 'main' of github.com:MaterializeInc/materialize into rol…

c31cfe5

…e-member-grantor-fix # Conflicts: # src/adapter/src/catalog/storage.rs

Update audit log entries

bbafcb1

def- approved these changes Apr 17, 2023

View reviewed changes

Merge branch 'main' of github.com:MaterializeInc/materialize into rol…

f27434a

…e-member-grantor-fix # Conflicts: # src/adapter/src/catalog/storage.rs

maddyblue approved these changes Apr 17, 2023

View reviewed changes

jkosh44 merged commit 2a0d4f5 into MaterializeInc:main Apr 17, 2023

jkosh44 deleted the role-member-grantor-fix branch April 17, 2023 19:55

jkosh44 mentioned this pull request Apr 18, 2023

platform-checks: Add privileges validation to owners check #18758

Merged

5 tasks

materialize-bot mentioned this pull request Apr 21, 2023

release: v0.52.0 required reviews #18907

Closed

12 tasks

jkosh44 mentioned this pull request Apr 24, 2023

[Epic] Role Based Access Control #11579

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Update role membership for PostgreSQL v16 #18780

sql: Update role membership for PostgreSQL v16 #18780

jkosh44 commented Apr 15, 2023

maddyblue Apr 17, 2023

jkosh44 Apr 17, 2023

jkosh44 Apr 17, 2023

jkosh44 Apr 17, 2023

def- left a comment

jkosh44 commented Apr 17, 2023

maddyblue left a comment

jkosh44 commented Apr 17, 2023

sql: Update role membership for PostgreSQL v16 #18780

sql: Update role membership for PostgreSQL v16 #18780

Conversation

jkosh44 commented Apr 15, 2023

Motivation

Tips for reviewer

Checklist

maddyblue Apr 17, 2023

Choose a reason for hiding this comment

jkosh44 Apr 17, 2023

Choose a reason for hiding this comment

jkosh44 Apr 17, 2023

Choose a reason for hiding this comment

jkosh44 Apr 17, 2023

Choose a reason for hiding this comment

def- left a comment

Choose a reason for hiding this comment

jkosh44 commented Apr 17, 2023

maddyblue left a comment

Choose a reason for hiding this comment

jkosh44 commented Apr 17, 2023