add optional system schema authorization #11720
Merged
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
clintropolis
commented
Sep 17, 2021
| @@ -29,11 +29,12 @@ This document describes the Druid security model that extensions use to enable u | |||
|
|
|||
| At the center of the Druid user authentication and authorization model are _resources_ and _actions_. A resource is something that authenticated users are trying to access or modify. An action is something that users are trying to do. | |||
|
|
|||
| There are three resource types: | |||
| There are four resource types: | |||
There was a problem hiding this comment.
heh this is actually a lie, there is also VIEW, but since there is no view implementation I don't think we need to call it out yet
techdocsmith
approved these changes
Sep 17, 2021
docs/configuration/index.md
Outdated
| @@ -1763,6 +1763,7 @@ The Druid SQL server is configured through the following properties on the Broke | |||
| |`druid.sql.planner.sqlTimeZone`|Sets the default time zone for the server, which will affect how time functions and timestamp literals behave. Should be a time zone name like "America/Los_Angeles" or offset like "-08:00".|UTC| | |||
| |`druid.sql.planner.metadataSegmentCacheEnable`|Whether to keep a cache of published segments in broker. If true, broker polls coordinator in background to get segments from metadata store and maintains a local cache. If false, coordinator's REST API will be invoked when broker needs published segments info.|false| | |||
| |`druid.sql.planner.metadataSegmentPollPeriod`|How often to poll coordinator for published segments list if `druid.sql.planner.metadataSegmentCacheEnable` is set to true. Poll period is in milliseconds. |60000| | |||
| |`druid.sql.planner.authorizeSystemTablesDirectly`|If true, queries against any of the system schema tables (`sys` in SQL) will be authorized as `SYSTEM_TABLE` resources which require `READ` access, on top of their current permissions based filtering.|false| | |||
There was a problem hiding this comment.
Suggested change
| |`druid.sql.planner.authorizeSystemTablesDirectly`|If true, queries against any of the system schema tables (`sys` in SQL) will be authorized as `SYSTEM_TABLE` resources which require `READ` access, on top of their current permissions based filtering.|false| | |
| |`druid.sql.planner.authorizeSystemTablesDirectly`|If true, Druid authorizes queries against any of the system schema tables (`sys` in SQL) as `SYSTEM_TABLE` resources which require `READ` access, in addition to permissions based filtering.|false| |
| @@ -29,11 +29,12 @@ This document describes the Druid security model that extensions use to enable u | |||
|
|
|||
| At the center of the Druid user authentication and authorization model are _resources_ and _actions_. A resource is something that authenticated users are trying to access or modify. An action is something that users are trying to do. | |||
|
|
|||
| There are three resource types: | |||
| There are four resource types: | |||
There was a problem hiding this comment.
Suggested change
| There are four resource types: | |
| Druid uses the following resource types: |
avoid specific number
|
|
||
| * DATASOURCE – Each Druid table (i.e., `tables` in the `druid` schema in SQL) is a resource. | ||
| * CONFIG – Configuration resources exposed by the cluster components. | ||
| * STATE – Cluster-wide state resources. | ||
| * SYSTEM_TABLE – if `druid.sql.planner.authorizeSystemTablesDirectly` is enabled, then system tables (the `sys` schema in SQL) are authorized as this type of resource. |
There was a problem hiding this comment.
Suggested change
| * SYSTEM_TABLE – if `druid.sql.planner.authorizeSystemTablesDirectly` is enabled, then system tables (the `sys` schema in SQL) are authorized as this type of resource. | |
| * SYSTEM_TABLE – if `druid.sql.planner.authorizeSystemTablesDirectly` is enabled, then Druid authorizes system tables,`sys` schema in SQL, using this resource type. |
zachjsh
reviewed
Sep 20, 2021
| @@ -72,6 +72,7 @@ druid_coordinator_kill_supervisor_on=true | |||
| druid_coordinator_kill_supervisor_period=PT10S | |||
| druid_coordinator_kill_supervisor_durationToRetain=PT0M | |||
| druid_coordinator_period_metadataStoreManagementPeriod=PT10S | |||
| druid_sql_planner_authorizeSystemTablesDirectly=true | |||
There was a problem hiding this comment.
Seems like it might be wasteful to have integration tests for both true and false, but since the default is disabled here, I think we may still want to have ITs for when this feature is disabled. What do you think?
There was a problem hiding this comment.
I considered this, I think it is ok to only test enabled because it just layers additional checks on top of the disabled code path, so with the newly added user we should still be covering all of the code paths that were previously tested, just distributed a bit differently.
zachjsh
reviewed
Sep 20, 2021
There was a problem hiding this comment.
LGTM. thanks for all the test refactoring @clintropolis . Just had one question about whether to keep the ITs with the new feature disabled in addition to it enabled as that is the default behavior.
zachjsh
approved these changes
Sep 20, 2021
|
thanks for review @techdocsmith and @zachjsh 👍 |
clintropolis
added a commit
that referenced
this pull request
Oct 8, 2021
* security recommendation * Update docs/operations/security-overview.md Co-authored-by: Victoria Lim <[email protected]> * Update docs/operations/security-user-auth.md Co-authored-by: Victoria Lim <[email protected]> * Update docs/operations/security-user-auth.md Co-authored-by: Victoria Lim <[email protected]> * Update security-user-auth.md add newline * Update docs/operations/security-overview.md Co-authored-by: Victoria Lim <[email protected]> * Update security-overview.md add suggestion for environment variable dynamic config provider Co-authored-by: Victoria Lim <[email protected]> Co-authored-by: Clint Wylie <[email protected]>
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.
Description
Builds on top of #11692 to add optional authorization to tables of the
sysin SQL asSYSTEM_TABLEresources, ifthe newly added
druid.sql.planner.authorizeSystemTablesDirectlyis true. For backwards compatibility, this feature is off by default. This allows cluster operators to selectively enable access tosysschema tables, or even disallow completely while still allowingSQLaccess to Druid datasources (DATASOURCEresources).Because of the smooth way things were done in #11692, the actual code change here is pretty small,
NamedSystemSchemanow accepts thePlannerConfigso it can optionally returnResourceType.SYSTEM_TABLE. Theintegration tests have been configured to now run with this new option enabled, and new authorization tests to cover this new permission. The majority of the changes in this PR are a slight refactoring of the authorization integration tests to share code a bit more effectively (and so I only had to add new tests once), and in fact right now at least this PR removes more lines than it adds even though it is adding new stuff.
This PR has: