Conversation
…ter user of managed PostgreSQL cloud services
…` status The catch block around the HMAC computation also caught every error from the load of the key Secret, such as a 403 from the Kubernetes API, and reported it as "HmacSHA256 not available". The key is now loaded before the try block, and the catch covers only the two checked exceptions of the HMAC API. The new test covers the creation of the key Secret with a 32 byte key, the reuse of an existing key by a second operator process, a new key after the Secret is lost, the error for a Secret without the `key` entry, and the exact HMAC construction that existing `Role` statuses depend on.
…atabase commit The fingerprint was written to the status inside the transaction. When the commit failed, the error handler still patched the status with the new fingerprint, and the next reconcile saw no password change. The fingerprint and the server password are now computed before the transaction, and the status receives the fingerprint after the transaction returns. The new test covers the state of every `Role` after the upgrade to the version that introduced the fingerprint. It changes the password in PostgreSQL, removes the fingerprint from the status, and asserts that the Secret password is applied once and left alone afterwards.
…amespace The `create` verb was part of the ClusterRole of the `Role` controller, which allowed the operator to create Secrets in every namespace. It is only needed for the password fingerprint key Secret in the operator namespace. The Quarkus Kubernetes extension now generates a namespaced Role and RoleBinding for it. The default RoleBinding to the `view` ClusterRole is kept explicitly, because configured role bindings replace it.
The flag test now covers `superuser`, `replication`, and `bypassrls`. Two tests run `passwordEncryption: server` and a pre-hashed SCRAM-SHA-256 verifier under the non-superuser admin.
`buildAlterRole` added `PASSWORD NULL` only on the transition from `LOGIN` to `NOLOGIN`. A role that was already `NOLOGIN` but still held a password kept it. `pg_roles` masks `rolpassword` with a constant, so the operator cannot tell whether such a role has a password. It therefore always clears the password when the spec expects no login. This costs nothing in the steady state. `RoleReconciler` calls `alterRole` only when the login state, the flags, or the password differ from the spec.
The fingerprint is a keyed hash, not a password hash with a work factor. Without the key it reveals nothing about the password. With the key an attacker can test guesses offline at `HMAC-SHA256` speed, so the key Secret is a credential.
|
Very excited about this. Do you guys think there'll be a release soon? |
stplasim
left a comment
There was a problem hiding this comment.
Really nice, very well done! I just found one bug withRoleService.fetchCurrentFlags. I think this is also not a new issue. The query logic itself isn't new. This PR only switched it from pg_authid to pg_roles. On main, a non superuser admin couldn't run the query, since pg_authid was denied, so the problem couldn't show up.
The other two things are just small docs..
And sorry for the long wait.
| .join(parent).on(parent.OID.eq(PG_AUTH_MEMBERS.ROLEID)) | ||
| .join(member).on(member.OID.eq(PG_AUTH_MEMBERS.MEMBER)) | ||
| .where(parent.OID.eq(PG_AUTHID.OID)) | ||
| .where(parent.OID.eq(PG_ROLES.OID)) |
There was a problem hiding this comment.
I think this list picks up a membership the operator didn't create, and on PostgreSQL 16+ that keeps every Role permanently out of sync when the admin isn't a superuser.
Since PG 16, a non-superuser with CREATEROLE is always implicitly granted ADMIN OPTION on the roles it creates (createrole_self_grant docs). The PR's docs rely on that grant (docs/role.md#L96). That grant is a row in pg_auth_members, so the admin shows up here as a member of every role it creates. Because Flags uses @EqualsAndHashCode, flagsMatch never becomes true.
I replayed the statements the operator sends on PG 16.15 as a LOGIN NOSUPERUSER CREATEDB CREATEROLE admin:
create role "grp1";
-- this query returns: inRole = {} role = {admin}
revoke "grp1" from "admin"; -- from reconcileRoleMembership
-- WARNING: role "admin" has not been granted membership in role "grp1" by role "admin"The REVOKE does nothing, because a role can only revoke grants it made itself (REVOKE docs), and this one was made by the bootstrap superuser. So every reconcile logs "Updating Role", runs the no-op REVOKE (plus PASSWORD NULL for NOLOGIN roles), and still reports READY. A superuser creating the same role gets no membership row, which is why the existing tests never hit this.
It gets worse with createrole_self_grant = 'set, inherit', which is the usual way to let a non-superuser admin act as the roles it creates. PG then adds a second row with the admin as grantor, and that one the operator does revoke:
set createrole_self_grant = 'set, inherit';
create role "grp2";
-- member | grantor | admin_option | set_option
-- admin | postgres | t | f
-- admin | admin | f | t
revoke "grp2" from "admin";
-- only the first row is leftAfter that, the admin can no longer manage databases owned by that role. DROP DATABASE requires ownership, and ALTER DATABASE … OWNER requires being able to SET ROLE to the owner:
drop database d1; -- ERROR: must be owner of database d1
alter database d1 owner to grp1; -- ERROR: must be owner of database d1RoleReconcilerNonSuperuserTest runs on postgres:18, but it doesn't catch this: nothing errors, and no test checks that a second reconcile is a no-op or looks at getRole().
We could maybe exclude the connecting admin (current_user) from this list unless the spec names it explicitly, and add a test that reconcils twice under the non superuser admin and asserts that nothing changes the second time.
There was a problem hiding this comment.
Don't ask me how long I sat wondering what happened to the admin user 😅. It could no longer drop tables.
| |---------------------------------------|-----------------------------------------------------------------| | ||
| | `ClusterConnection` | `LOGIN` | | ||
| | `Role` | `CREATEROLE` | | ||
| | `Database` | `CREATEDB` | |
There was a problem hiding this comment.
This row isn't quite enough on PostgreSQL 16+ when the Database has an owner. createDatabase runs alter database … owner to <owner>, which requires being able to SET ROLE to the new owner (ALTER DATABASE docs). The implicit grant a CREATEROLE admin gets on roles it creates doesn't include SET:
-- PG 16.15, as LOGIN NOSUPERUSER CREATEDB CREATEROLE
create database d1; -- OK
alter database d1 owner to grp1; -- ERROR: must be able to SET ROLE "grp1"I think the table should mention that the admin also needs SET on the owner role, for example through ALTER ROLE <admin> SET createrole_self_grant = 'set, inherit' or GRANT <owner> TO <admin> WITH SET TRUE.
| - With the key, an attacker can test password guesses offline at `HMAC-SHA256` speed. Treat the key Secret as a credential. Keep the number of principals with `get` on Secrets in the operator namespace small. | ||
| - An attacker who reads the key Secret can usually also read the password Secrets that the `Role` resources reference. In that case the fingerprint adds no exposure that the attacker does not already have. | ||
| - The operator never writes the password, its `SCRAM-SHA-256` verifier, or the key into the `Role` status. | ||
| - To retire a key, delete the key Secret. The operator generates a new key and re-applies every `Role` password once. Every old fingerprint then becomes meaningless. |
There was a problem hiding this comment.
This doesn't match what the code does. getKey() caches the key in a field on first use and never invalidates it. After the Secret is deleted, a running operator keeps using the old key and only creates a new Secret once it restarts. The same applies to the "lost Secret" sentence on L65 and in the class Javadoc.
whenSecretLost_shouldCreateNewKey passes because it creates a fresh service instance for the second call. I think either the docs should say the operator has to be restarted, or the service should watch the Secret and drop the cached key when it changes or is deleted.
Fixes #70
The Role controller failed on AWS RDS with
permission denied for table pg_authid.The same happens on every managed PostgreSQL service and on any vanilla PostgreSQL where the admin/management user is usually not a superuser, not do the cloud offerings support crating a superuser.
This PR makes the Role controller work with an admin role that has only
LOGIN,CREATEDB, andCREATEROLE.What changed
pg_rolesinstead ofpg_authid. Role state and membership come from the public viewpg_rolesand the public catalogpg_auth_members.pg_authidstays in the generated jOOQ sources for the test helper only.ALTER ROLEnames only the options that differ from the current state. PostgreSQL rejectsNOSUPERUSER,NOREPLICATION, andNOBYPASSRLSfrom a non-superuser even when the value does not change, so the previous full statement failed on every update of an existing role.pg_authidcannot be read without superuser rights. The operator now stores an HMAC-SHA256 of the Secret password instatus.passwordFingerprintand compares against it on each reconcile. The key is random and lives in a Secret in the operator namespace (postgresql-operator.password-fingerprint.secret-name). A reader of the Role status learns nothing about the password without that key. The RBAC rule for secrets gainscreatefor this one Secret.passwordEncryption: serveropts out for MD5-only clients. CloudNativePG does the same since v1.29.2.RoleReconcilerNonSuperuserTestruns the controller with aLOGIN CREATEDB CREATEROLEadmin. It covers create, update, password rotation, login toggle, membership, drop, and the error path forsuperuser: true.Documented consequences
docs/cluster-connection.md.superuser,replication, orbypassrls.ADMIN OPTION.credcheckor the Cloud SQL password policy.Verification against a non-superuser admin
Run on PostgreSQL 15 and 18 as
LOGIN NOSUPERUSER CREATEDB CREATEROLE, which mirrors the master user of the managed services.SELECTonpg_authidSELECTonpg_shadowSELECTonpg_roles, all flag columnsSELECTonpg_auth_membersshobj_description(oid, 'pg_authid')with the oid frompg_rolesbuildAlterRolestatementALTER ROLE ... NOSUPERUSER/NOREPLICATION/NOBYPASSRLSaloneALTER ROLEwith login, password, createdb, createrole, inherit, connection limit, valid untilGRANT/REVOKEmembership on a role the admin createdALTER ROLE ... PASSWORD '<SCRAM verifier>'Managed PostgreSQL offerings
pg_authidrds_superusercloudsqlsuperuser,alloydbsuperusercloudsql.pg_authid_select_rolecan grant itazure_pg_adminneon_superuserpostgrespostgresThe change works on all of them, because it uses only public catalogs and never names superuser-only options without a change.
Tests
javac -Xlintsettings of Replace Checkstyle with Error Prone #66 and Enable thejavac -Xlintcategories that Error Prone cannot see #67.