release-19.1: server: properly authenticate some admin RPC req… #42727
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
Prior to this patch, all the admin RPCs would perform whatever SQL operations they needed to do using the root user. See issue cockroachdb#42567. Moreover, because of this the "UI data" payload containing all the UI configuration settings was shared between all users. This was possibly a security flaw, as it could enable any authenticated user (eg via a certificate on the CLI, or via the web UI) to access SQL data otherwise only available to root. This patch fixes **part of** issue cockroachdb#42567 by authorizing the SQL execution of admin RPC endpoints properly. It also introduces a new SQL built-in function `crdb_internal.is_admin()` used internally by the admin user. **The following problem from cockroachdb#42567 is not fixed: RPCs that do not use SQL are still allowed to proceed as root regardless of the authenticated user.** This will be addressed by a subsequent commit. Release note (admin ui change): Certain web UI pages (like the list of databases, tables) now restrict their content to match the privileges of the logged-in user. Release note (admin ui change): The event log now presents all cluster settings changes, unredacted, when an admin user uses the page. Release note (admin ui change): Customization of the UI by users is only properly saved if the user has write privilege to `system.ui` (i.e. admin users). Also, all authenticated users share the same customizations. This is a known limitation and should be lifted in a future version.
The Drain() (`cockroach quit`) RPC is used over gRPC by `cockroach quit`. There is no requirement to have it exposed via HTTP, and moreover it is a security problem (DoS) waiting to happen since the request is not fully authorized. Release note: none
Previously, admin/status RPCs available over HTTP but that did not perform SQL operations would not verify authorization from clients (the only authorization was that performed by SQL privilege checks). This patch fixes it. The following services are now blocked from non-admin users: - `TableStats`. This is a temporary limitation, until this RPC learns how to verify privileges on the specific table for which stats are requested. - `NonTableStats` - `DataDistribution` - `EnqueueRange` - `RangeLog` - `Gossip` - `EngineStats` - `Allocator` - `AllocatorRange` - `Certificates` - `Details` - `GetFiles` - `LogFilesList` - `LogFile` - `Logs` - `Stacks` - `Profile` - `RaftDebug` - `Ranges` - `HotRanges` - `Range` - `SpanStats` - `Stores` For reference, the following services were already properly authorized by SQL privilege checks: - `Databases` - `DatabaseDetails` - `TableDetails` - `Users` - `Events` - `Jobs` - `Locations` - `QueryPlan` The following were already authorized using a custom check: - `ListSessions` (user must be either admin or can only list their own sessions) - `ListLocalSessions` (ditto) The following are now authorized using a custom check: - `CancelSession` (user must be either admin or operate on their own sessions) - `CancelQuery` (user must be either admin or operate on their own queries) The following RPCs are not authorized but do not leak sensitive information: - `Health` - `Liveness` - `Settings` Release note (admin ui change): Access to table statistics are temporary blocked from access by non-admin users until further notice, for security reasons. Release note (admin ui change): Certain debug pages have been blocked from non-admin users for security reasons.
knz
force-pushed
the
backport19.1-42726
branch
from
d815049 to
04b08aa
Compare
|
@rolandcrosby could you nominate someone to do QA on the UI after these security changes, to see what functionality becomes different for non-admin users? We'll need to document this as known limitations. |
bdarnell
approved these changes
Nov 25, 2019
knz
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 3/3 commits from #42726.
/cc @cockroachdb/release
Backport 3/3 commits from #42563.
/cc @cockroachdb/release
Fixes the vulnerability described in #42567.
The the individual commits for details.