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: use follower reads for internal authentication-related queries #58497

Closed
rafiss opened this issue Jan 6, 2021 · 8 comments
Closed

sql: use follower reads for internal authentication-related queries #58497

rafiss opened this issue Jan 6, 2021 · 8 comments
Labels
A-security C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@rafiss
Copy link
Collaborator

rafiss commented Jan 6, 2021

Is your feature request related to a problem? Please describe.
When authenticating a non-root user, CockroachDB must query the system.users and system.role_options tables. See

func retrieveUserAndPassword(

In a multi-region cluster, the leaseholder for these system ranges may live in a different region than the node that is being connected to. This causes multiple cross-region network hops, thereby increasing the time it takes to establish the connection. Some applications configure aggressive connection timeouts, so in the worst case, this could prevent an application from connecting.

Describe the solution you'd like
Use follower reads for the above queries, and any other queries that are performed during authentication.

Describe alternatives you've considered
A cache could be added to avoid the cross-region hop. This could become hard to manage, since we'd need to make sure to invalidate stale data correctly.

Additional context
Customers have reported concerns about connection latency being too high for non-root users in multi-region clusters. See https://github.com/cockroachlabs/support/issues/742 and https://github.com/cockroachlabs/support/issues/736

@rafiss rafiss added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-security labels Jan 6, 2021
@ajwerner
Copy link
Contributor

ajwerner commented Jan 6, 2021

cc @aaron-crl regarding the implications of the semantics of only lazily realizing password changes.

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 6, 2021

Yeah that would be my main concern. At best, it seems like a pretty confusing user experience, and at worst a big security hole.

@ajwerner
Copy link
Contributor

ajwerner commented Jan 6, 2021

Another option that I think I prefer is to using a leasing protocol similar to how we do with system.role_membership that allows nodes to cache the password hash.

@ajwerner
Copy link
Contributor

ajwerner commented Jan 6, 2021

This has come up before.

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 11, 2021

Note that #58671 #58674 and #58677 eliminate much of the latency of the internal auth-queries, so this issue is lower in priority.

@aaron-crl
Copy link

cc @aaron-crl regarding the implications of the semantics of only lazily realizing password changes.

Checking auth against anything other than the current state of the database introduces the potential for authentication race conditions in certain circumstances. We should avoid performing any auth checks against data or permissions that are or may be out of date.

@ajwerner
Copy link
Contributor

In principle we could get authn down to 0 RTTs without too much lift. I'm inclined to close this issue to instead open an enhancement about caching hashes and using leasing rather than anything to do with follower reads.

@ajwerner
Copy link
Contributor

Closing in favor of #58869.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

3 participants