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

sql: add pg_catalog.pg_authid virtual table #43437

Merged
merged 1 commit into from
Dec 24, 2019

Conversation

jasobrown
Copy link
Contributor

This patch adds support for the pg_authid table virtual table.
This table displays authentication identifiers (users/roles),
and is similar to pg_catalog.pg_roles.

The Postgres table requires admin privileges as it will display
hashed passwords. We do not display passwords (hashed or otherwise),
so no admin privileges are required for access.

Fixes #27767

Release note (sql): clients can retrieve system user information
in another Postgres-compatible manner.

This patch adds support for the pg_authid table virtual table.
This table displays authentication identifiers (users/roles),
and is similar to pg_catalog.pg_roles.

The Postgres table requires admin privileges as it will display
hashed passwords. We do not display passwords (hashed or otherwise),
so no admin privileges are required for access.

Fixes cockroachdb#27767

Release note (sql): clients can retrieve system user information
in another Postgres-compatible manner.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jasobrown
Copy link
Contributor Author

tagging @jordanlewis as he worked with me on putting this together

Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @jasobrown! This looks great. I just have one inline nitpick.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jasobrown)


pkg/sql/pg_catalog.go, line 535 at r1 (raw file):

				tree.DBoolFalse,              // rolbypassrls
				negOneVal,                    // rolconnlimit
				passwdStarString,             // rolpassword

I think this wants to be tree.DNull - unlike pg_user, which in Postgres is supposed to actually have ***** for a password, this column is expected to either be a hashed password or NULL.

@jasobrown
Copy link
Contributor Author

jasobrown commented Dec 24, 2019

Should we return NULL in the case there is no password, or should we NULL on all cases (if we aren't displaying (hashed or starred) passwords)? The latter former feels closer in spirit to the original pg_authid.

@jasobrown
Copy link
Contributor Author

Also, I looked at the failed tests on TeamCity, and they do not seem related to this patch. Can you confirm?

@jordanlewis
Copy link
Member

That failure is an unrelated flaky test.

I think and hope that trying to mimic rolpassword is not that important. Any tool that actually expects something correct won't work here in any event, so we should be free to do whatever we want. You're right that NULL seems to indicate no password. But the docs also say that if the password doesn't follow the md5 scheme they lay out, it's assumed to be unencrypted raw password?

I think on consideration that whatever we do here will be fine, so I think we'll just merge as-is unless you have any other thoughts here.

@jasobrown
Copy link
Contributor Author

Any tool that actually expects something correct won't work here in any event

I think that is probably the most salient point. If lack of password becomes a significant problem in the future, we can always revisit.

Let's go ahead and merge.

@jordanlewis
Copy link
Member

Thanks Jason!

bors r+

craig bot pushed a commit that referenced this pull request Dec 24, 2019
43437: sql: add pg_catalog.pg_authid virtual table r=jordanlewis a=jasobrown

This patch adds support for the pg_authid table virtual table.
This table displays authentication identifiers (users/roles),
and is similar to pg_catalog.pg_roles.

The Postgres table requires admin privileges as it will display
hashed passwords. We do not display passwords (hashed or otherwise),
so no admin privileges are required for access.

Fixes #27767

Release note (sql): clients can retrieve system user information
in another Postgres-compatible manner.

Co-authored-by: Jason Brown <jasbrown@netflix.com>
@craig
Copy link
Contributor

craig bot commented Dec 24, 2019

Build succeeded

@craig craig bot merged commit f073d53 into cockroachdb:master Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: pg_catalog.pg_authid needs to be implemented and populated
3 participants