Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

any user can access pg_catalog by default #11282

Merged

Conversation

jeeminso
Copy link
Contributor

@jeeminso jeeminso commented Apr 16, 2021

Summary of the changes / Why this improves CrateDB

This will allow the user can access generic system data from pg_catalog schema and only the data that the user has the privileges for. ex) From a fresh cluster, super user can see 71 rows from pg_class table. A new user should be able to see them as well without any privileges granted.

1) allow all users to access pg_catalog to handle below observations.

original behaviours on different clients:

crash

cr> show search_path;
SchemaUnknownException[Schema 'pg_catalog' unknown]
cr> select current_schemas(true);
+-----------------------+
| current_schemas       |
+-----------------------+
| ["pg_catalog", "doc"] |
+-----------------------+
SELECT 1 row in set (0.081 sec)

pgcli (cannot log on since it tries to access pg_catalog.pg_settings while logging in)

pgcli postgresql://nuser2@localhost:5432/
Schema 'pg_catalog' unknown

2) protect any rows that users do not have permissions for.

Checklist

  • Added an entry in CHANGES.txt for user facing changes
  • Updated documentation & sql_features table for user facing changes
  • Touched code is covered by tests
  • CLA is signed
  • This does not contain breaking changes, or if it does:
    • It is released within a major release
    • It is recorded in CHANGES.txt
    • It was marked as deprecated in an earlier release if possible
    • You've thought about the consequences and other components are adapted
      (E.g. AdminUI)

@jeeminso jeeminso force-pushed the enable-users-to-access-pg_catalog-by-default branch from 3624ebf to 87f74ad Compare April 16, 2021 19:51
@jeeminso jeeminso marked this pull request as draft April 18, 2021 20:07
@jeeminso
Copy link
Contributor Author

handled both 1) and 2).

Regarding 2), there are 17 tables under pg_catalog schema, where 5 will reflect on users' creations and the rest 12 are either empty or fixed tables. The 5 are pg_class, pg_proc, pg_namespace, pg_attribute, and pg_constraint where each has its own privileged access test now.

@jeeminso jeeminso force-pushed the enable-users-to-access-pg_catalog-by-default branch 3 times, most recently from b2b388b to 37768bb Compare April 21, 2021 01:05
@jeeminso
Copy link
Contributor Author

it would be better if I handle the remainder of #11111 (sys schema) and information_schema if necessary separately.

@jeeminso jeeminso requested a review from seut April 21, 2021 01:44
@seut seut requested a review from mfussenegger April 21, 2021 07:58
Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Left some minor comments.

@jeeminso jeeminso force-pushed the enable-users-to-access-pg_catalog-by-default branch 7 times, most recently from 83bb087 to 359ce2e Compare April 21, 2021 23:01
@jeeminso
Copy link
Contributor Author

jeeminso commented Apr 21, 2021

Thank you @mfussenegger. I have applied all your suggestions.

I have created isBuiltin() on FunctionName which checks no schema and the system schemas.
Replaced isUnderSystemSchemas(..) to a static method from Schemas.

Also, made few fixups. Although blob support will be dropped, I kept the blob related code for now so it can be handled all together.

@jeeminso jeeminso force-pushed the enable-users-to-access-pg_catalog-by-default branch from 359ce2e to 4f6a6e0 Compare April 21, 2021 23:05
@jeeminso jeeminso requested review from mfussenegger and removed request for seut April 22, 2021 01:10
@jeeminso jeeminso marked this pull request as ready for review April 22, 2021 01:13
Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jeeminso jeeminso force-pushed the enable-users-to-access-pg_catalog-by-default branch from 4f6a6e0 to 7426809 Compare April 22, 2021 11:20
@jeeminso jeeminso added the ready-to-merge Let Mergify merge the PR once approved and checks pass label Apr 22, 2021
@mergify mergify bot merged commit 760b405 into crate:master Apr 22, 2021
@jeeminso jeeminso deleted the enable-users-to-access-pg_catalog-by-default branch April 22, 2021 17:45
@jeeminso jeeminso restored the enable-users-to-access-pg_catalog-by-default branch April 23, 2021 13:32
@jeeminso
Copy link
Contributor Author

Hello Jordi, @mfussenegger @seut
I had a chance to talk to Basti about this PR and in summary, the concern is with the users being able to access sys by default.
I would like to request you for suggestions before proceeding further.

To briefly explain with an example, if a new user performs select * from pg_catalog.pg_class, sys table names are returned (without DQL to sys schema). I think it makes sense that users should NOT be able to see sys schema related information without the privileges granted.

In my opinion, one way to handle is by backing out this PR except for the part that handles the login issue. Then it would be consistent with how information_schema is protected in CrateDB. Another option could be to allow default accesses to pg_catalog and information_schema (and block sys) which is the behaviour of Postgres.

@mfussenegger
Copy link
Member

the concern is with the users being able to access sys by default.

To clarify: You mean that users see entries related to the sys schema within pg_catalog tables? Users still can't access sys tables

In my opinion, one way to handle is by backing out this PR except for the part that handles the login issue. Then it would be consistent with how information_schema is protected in CrateDB. Another option could be to allow default accesses to pg_catalog and information_schema (and block sys) which is the behaviour of Postgres.

I'd opt for the latter option, which was what #11111 was outlined. So I think a follow up that replaces the isSystemSchema checks with a check for only pg_catalog or information_schema should be good enough.

To make things consistent we should probably later on (separately) also make some changes to how the information_schema tables are handled. For example information_schema.tables should probably return the information_schema and pg_catalog tables as well for a user who doesn't have explicit permissions on those tables.

@jeeminso
Copy link
Contributor Author

jeeminso commented Apr 26, 2021

To clarify: You mean that users see entries related to the sys schema within pg_catalog tables? Users still can't access sys tables

Yes, users still cannot access sys tables.

I will make the changes and then open an issue to track information_schema to be consistent. Thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Let Mergify merge the PR once approved and checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants