Allow for Input source security in SQL layer #13989
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
* fix checkstyle errors
* fix stuff
zachjsh
commented
Mar 28, 2023
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalTableMacro.java
Outdated
Show resolved
Hide resolved
zachjsh
changed the title
* Add javadocs
paul-rogers
requested changes
Mar 28, 2023
.../s3-extensions/src/test/java/org/apache/druid/catalog/model/table/S3InputSourceDefnTest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/catalog/model/table/BaseInputSourceDefn.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/catalog/model/table/ExternalTableSpec.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/apache/druid/server/security/AuthConfig.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/external/DruidExternTableMacro.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalTableMacro.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/external/Externals.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java
Show resolved
Hide resolved
paul-rogers
approved these changes
Mar 28, 2023
.../s3-extensions/src/test/java/org/apache/druid/catalog/model/table/S3InputSourceDefnTest.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
| // this shouldn't happen, as the sqlCall should have been validated by this point, |
There was a problem hiding this comment.
nit: sqlCall -> call or if you're referring to the type SqlCall
| testIngestionQuery() | ||
| .sql("INSERT INTO dst SELECT * FROM %s PARTITIONED BY ALL TIME", externSql(externalDataSource)) | ||
| .authentication(CalciteTests.REGULAR_USER_AUTH_RESULT) | ||
| .authConfig(AuthConfig.newBuilder().setEnableInputSourceSecurity(true).build()) |
There was a problem hiding this comment.
All the tests either implicitly disable the input source security (via a default) or explicitly enable it. It'll be nice also to have a test where we explicitly disable it.
Perhaps a unit test similar to this, where setEnableInputSourceSecurity is set to false, and we shouldn't get a ForbiddenException.
|
|
||
| public SqlResourceCollectorShuttle(SqlValidator validator, PlannerContext plannerContext) | ||
| { | ||
| this.validator = validator; | ||
| this.resourceActions = new HashSet<>(); | ||
| this.plannerContext = plannerContext; | ||
| inputSourceTypeSecurityEnabled = plannerContext.getPlannerToolbox().getAuthConfig().isEnableInputSourceSecurity(); |
There was a problem hiding this comment.
This class already holds plannerContext state, so we could just reference this line inline directly in the visit() call?
|
thanks @abhishekrb19 ! I will take care of the minor comments in follow up pr. |
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.
Partially Fixes #13837.
Description
This change introduces the concept of input source type security model, proposed in #13837.. With this change, this feature is only available at the SQL layer, but we will expand to native layer in a follow up PR.
To enable this feature, the user must set the following property to true:
druid.auth.enableInputSourceSecurity=trueThe default value for this property is false, which will continue the existing functionality of having the usage all external sources being authorized against the hardcoded resource action
new ResourceAction(new Resource(ResourceType.EXTERNAL, ResourceType.EXTERNAL), Action.READWhen this config is enabled, the users will be required to be authorized for the following resource action
new ResourceAction(new Resource(ResourceType.EXTERNAL, {INPUT_SOURCE_TYPE}, Action.READwhere
{INPUT_SOURCE_TYPE}is the type of the input source being used;, http, inline, s3, etc..Documentation has not been added for the feature as it is not complete at the moment, as we still need to enable this for the native layer in a follow up pr.
This PR has: