sql: grant CONNECT privilege to PUBLIC role on new databases

[nvanbenschoten]

This is somewhere between a bug and a Postgres-incompatability.

From https://www.postgresql.org/docs/current/ddl-priv.html:

PostgreSQL grants privileges on some types of objects to PUBLIC by default when the objects are created. No privileges are granted to PUBLIC by default on tables, table columns, sequences, foreign data wrappers, foreign servers, large objects, schemas, or tablespaces. For other types of objects, the default privileges granted to PUBLIC are as follows: CONNECT and TEMPORARY (create temporary tables) privileges for databases; EXECUTE privilege for functions and procedures; and USAGE privilege for languages and data types (including domains). The object owner can, of course, REVOKE both default and expressly granted privileges. (For maximum security, issue the REVOKE in the same transaction that creates the object; then there is no window in which another user can use the object.) Also, these default privilege settings can be overridden using the ALTER DEFAULT PRIVILEGES command.

Note specifically the "CONNECT and TEMPORARY (create temporary tables) privileges for databases" part.

I don't think we'll be able to solve the TEMPORARY privilege part right now, because we don't separate the CREATE and TEMPORARY privileges. However, we should be able to resolve the CONNECT part, one way or another.

In PostgreSQL:

nathan=# CREATE ROLE newrole LOGIN;
CREATE ROLE

nathan=# SELECT has_database_privilege('newrole', 'postgres', 'CONNECT');
 has_database_privilege
------------------------
 t

In CockroachDB:

root@127.0.0.1:26257/postgres> CREATE ROLE newrole LOGIN;
CREATE ROLE

root@127.0.0.1:26257/postgres> SELECT has_database_privilege('newrole', 'postgres', 'CONNECT');
  has_database_privilege
--------------------------
          false
(1 row)

root@127.0.0.1:26257/postgres> SELECT * FROM "".crdb_internal.cluster_database_privileges WHERE database_name = 'postgres';
  database_name | grantee | privilege_type
----------------+---------+-----------------
  postgres      | admin   | ALL
  postgres      | root    | ALL
(2 rows)

It looks like this might actually be an introspection issue, as we do allow the new role to connect to each database. So it's not clear to me exactly what's wrong here. Should PUBLIC be represented in crdb_internal.cluster_database_privileges? Should has_database_privilege assume that all roles have CONNECT privileges to all databases?

@RichardJCai are you the right person to evaluate what we should do here?

Epic CRDB-10300

[RichardJCai]

This is somewhere between a bug and a Postgres-incompatability.

From https://www.postgresql.org/docs/current/ddl-priv.html:

PostgreSQL grants privileges on some types of objects to PUBLIC by default when the objects are created. No privileges are granted to PUBLIC by default on tables, table columns, sequences, foreign data wrappers, foreign servers, large objects, schemas, or tablespaces. For other types of objects, the default privileges granted to PUBLIC are as follows: CONNECT and TEMPORARY (create temporary tables) privileges for databases; EXECUTE privilege for functions and procedures; and USAGE privilege for languages and data types (including domains). The object owner can, of course, REVOKE both default and expressly granted privileges. (For maximum security, issue the REVOKE in the same transaction that creates the object; then there is no window in which another user can use the object.) Also, these default privilege settings can be overridden using the ALTER DEFAULT PRIVILEGES command.

Note specifically the "CONNECT and TEMPORARY (create temporary tables) privileges for databases" part.

I don't think we'll be able to solve the TEMPORARY privilege part right now, because we don't separate the CREATE and TEMPORARY privileges. However, we should be able to resolve the CONNECT part, one way or another.

In PostgreSQL:

nathan=# CREATE ROLE newrole LOGIN;
CREATE ROLE

nathan=# SELECT has_database_privilege('newrole', 'postgres', 'CONNECT');
 has_database_privilege
------------------------
 t

In CockroachDB:

root@127.0.0.1:26257/postgres> CREATE ROLE newrole LOGIN;
CREATE ROLE

root@127.0.0.1:26257/postgres> SELECT has_database_privilege('newrole', 'postgres', 'CONNECT');
  has_database_privilege
--------------------------
          false
(1 row)

root@127.0.0.1:26257/postgres> SELECT * FROM "".crdb_internal.cluster_database_privileges WHERE database_name = 'postgres';
  database_name | grantee | privilege_type
----------------+---------+-----------------
  postgres      | admin   | ALL
  postgres      | root    | ALL
(2 rows)

It looks like this might actually be an introspection issue, as we do allow the new role to connect to each database. So it's not clear to me exactly what's wrong here. Should PUBLIC be represented in crdb_internal.cluster_database_privileges? Should has_database_privilege assume that all roles have CONNECT privileges to all databases?

@RichardJCai are you the right person to evaluate what we should do here?

Thanks for filing this, I'll check it out.

[rafiss]

It looks like this might actually be an introspection issue, as we do allow the new role to connect to each database

The CONNECT privilege is in a slightly odd spot right now. It exists because some applications rely on it to allow a role to see metadata (pg_catalog) from a database, but we don't actually use it when checking if a role can connect to a database: #59875

[nvanbenschoten]

Thanks for the background Rafi. Until we address #59875, should we be considering all users to have the CONNECT privilege in has_database_privilege? I think the effect of that would be reverting this one small part of the PR that introduced the CONNECT privilege on databases.

[RichardJCai]

Thanks for the background Rafi. Until we address #59875, should we be considering all users to have the CONNECT privilege in has_database_privilege? I think the effect of that would be reverting this one small part of the PR that introduced the CONNECT privilege on databases.

I'm just curious, is there a specific reason you're looking into this? Does PostgREST rely on checking has_database_privilege for CONNECT?

The has_database_privilege behaviour is expected, it's just that CONNECT isn't actually enforced for connecting to databases yet.

If anything, we should investigate where CONNECT should be granted when creating a database.

[rafiss]

the issue i see with always returning true for has_database_privilege(... CONNECT) is that the CONNECT privilege is still a real entity that gives users the ability to see metadata.

If anything, we should investigate where CONNECT should be granted when creating a database.

i agree with that. the first and least invasive step here would be to start granting CONNECT automatically. it also sounds like that's what postgres does. perhaps we should also include a long-running migration that gives every existing user CONNECT on all the databases.

[nvanbenschoten]

I'm just curious, is there a specific reason you're looking into this? Does PostgREST rely on checking has_database_privilege for CONNECT?

I thought it did here, but upon a re-read, it looks like I was wrong. Instead, it queries the pg_class and pg_namespace tables. pg_namespace doesn't return any of a database's schemas unless the user/role querying the table has CONNECT privileges on the table because it passes true as the requiresPrivileges argument to forEachDatabaseDesc:

cockroach/pkg/sql/pg_catalog.go

Line 1947 in d6195d4

return forEachDatabaseDesc(ctx, p, dbContext, true, /* requiresPrivileges */

the first and least invasive step here would be to start granting CONNECT automatically. it also sounds like that's what postgres does.

I think this would be the preferred long-term solution, as it would resolve this entire class of incompatibility.

perhaps we should also include a long-running migration that gives every existing user CONNECT on all the databases.

Would we want to grant CONNECT to each existing user for all databases, or just grant CONNECT to PUBLIC for all databases?

[otan]

given the number of surprise test changes in #72595, i'm a little hesitant to do the migration in #72603 and rather just have "new things have connect privileges" by default.

[RichardJCai]

given the number of surprise test changes in #72595, i'm a little hesitant to do the migration in #72603 and rather just have "new things have connect privileges" by default.

By surprise test changes are you mainly referring to the TestAuthenticationAndHBARules tests?

[otan]

More just like now people have visibility into other databases when they didn't before, and I'm not sure we should retroactively store that as the default

[rafiss]

I'll close this, and if we ever want the migration, it should be done as part of #59875