add optional system schema authorization by clintropolis · Pull Request #11720 · apache/druid

clintropolis · 2021-09-17T18:48:35Z

Description

Builds on top of #11692 to add optional authorization to tables of the sys in SQL as SYSTEM_TABLE resources, if
the newly added druid.sql.planner.authorizeSystemTablesDirectly is true. For backwards compatibility, this feature is off by default. This allows cluster operators to selectively enable access to sys schema tables, or even disallow completely while still allowing SQL access to Druid datasources (DATASOURCE resources).

Because of the smooth way things were done in #11692, the actual code change here is pretty small, NamedSystemSchema now accepts the PlannerConfig so it can optionally return ResourceType.SYSTEM_TABLE. The
integration 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:

been self-reviewed.
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

clintropolis · 2021-09-17T18:54:38Z

 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:


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

Optional style changes

techdocsmith · 2021-09-17T20:47:15Z

 |`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|  


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|

techdocsmith · 2021-09-17T20:49:02Z

 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:


Suggested change

There are four resource types:

Druid uses the following resource types:

avoid specific number

techdocsmith · 2021-09-17T20:50:20Z

 * DATASOURCE &ndash; Each Druid table (i.e., `tables` in the `druid` schema in SQL) is a resource.
 * CONFIG &ndash; Configuration resources exposed by the cluster components. 
 * STATE &ndash; Cluster-wide state resources.
+* SYSTEM_TABLE &ndash; if `druid.sql.planner.authorizeSystemTablesDirectly` is enabled, then system tables (the `sys` schema in SQL) are authorized as this type of resource.


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 · 2021-09-20T06:13:25Z

 druid_coordinator_kill_supervisor_period=PT10S
 druid_coordinator_kill_supervisor_durationToRetain=PT0M
 druid_coordinator_period_metadataStoreManagementPeriod=PT10S
+druid_sql_planner_authorizeSystemTablesDirectly=true


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?

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

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.

clintropolis · 2021-09-21T20:28:31Z

thanks for review @techdocsmith and @zachjsh 👍

* security recommendation * Update docs/operations/security-overview.md Co-authored-by: Victoria Lim <vtlim@users.noreply.github.com> * Update docs/operations/security-user-auth.md Co-authored-by: Victoria Lim <vtlim@users.noreply.github.com> * Update docs/operations/security-user-auth.md Co-authored-by: Victoria Lim <vtlim@users.noreply.github.com> * Update security-user-auth.md add newline * Update docs/operations/security-overview.md Co-authored-by: Victoria Lim <vtlim@users.noreply.github.com> * Update security-overview.md add suggestion for environment variable dynamic config provider Co-authored-by: Victoria Lim <vtlim@users.noreply.github.com> Co-authored-by: Clint Wylie <cwylie@apache.org>

clintropolis added 2 commits September 17, 2021 11:36

add optional system schema authorization

99d56af

remove unused

273c1a5

clintropolis added Area - Operations Release Notes Design Review Area - SQL Security labels Sep 17, 2021

clintropolis commented Sep 17, 2021

View reviewed changes

adjust docs

3eee6ff

techdocsmith approved these changes Sep 17, 2021

View reviewed changes

clintropolis added 2 commits September 17, 2021 16:35

doc fixes, missing ldap config change for integration tests

9edc683

style

768c634

zachjsh self-requested a review September 20, 2021 06:09

zachjsh reviewed Sep 20, 2021

View reviewed changes

zachjsh approved these changes Sep 20, 2021

View reviewed changes

clintropolis merged commit 5de26cf into apache:master Sep 21, 2021

clintropolis deleted the authorize-system-schema branch September 21, 2021 20:28

techdocsmith mentioned this pull request Oct 7, 2021

docs for changes to authorization in #11718 and #11720 #11779

Merged

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

abhishekagarwal87 mentioned this pull request May 25, 2022

[Draft] 0.23.0 Release notes #12510

Closed

suneet-s mentioned this pull request Jul 1, 2022

ACL for lookups #12728

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add optional system schema authorization#11720

add optional system schema authorization#11720
clintropolis merged 5 commits intoapache:masterfrom
clintropolis:authorize-system-schema

clintropolis commented Sep 17, 2021

Uh oh!

clintropolis Sep 17, 2021

Uh oh!

techdocsmith left a comment

Uh oh!

techdocsmith Sep 17, 2021

Uh oh!

techdocsmith Sep 17, 2021

Uh oh!

techdocsmith Sep 17, 2021

Uh oh!

zachjsh Sep 20, 2021 •

edited

Loading

Uh oh!

clintropolis Sep 20, 2021

Uh oh!

zachjsh left a comment

Uh oh!

clintropolis commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	\|`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\|

	There are four resource types:
	Druid uses the following resource types:

	* 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.

Conversation

clintropolis commented Sep 17, 2021

Description

Uh oh!

clintropolis Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

techdocsmith left a comment

Choose a reason for hiding this comment

Uh oh!

techdocsmith Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

techdocsmith Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

techdocsmith Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

zachjsh Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

zachjsh left a comment

Choose a reason for hiding this comment

Uh oh!

clintropolis commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zachjsh Sep 20, 2021 •

edited

Loading