Require Datasource WRITE authorization for Supervisor and Task access by kfaraz · Pull Request #11718 · apache/druid

kfaraz · 2021-09-17T10:27:48Z

Follow up PR for #11680

Description

Supervisor and Task APIs are related to ingestion and must always require Datasource WRITE
authorization even if they are purely informative.

Changes

Check Datasource WRITE in SystemSchema for tables "supervisors" and "tasks"
Check Datasource WRITE for APIs /supervisor/history and /supervisor/{id}/history
Check Datasource for all Indexing Task APIs

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
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 or updated version, license, or notice information in licenses.yaml
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.

….tasks

clintropolis

makes sense to me that these should be WRITE permissions, but I think we need to tag this design review and release notes (incompatible?) since based on the state of things, it seems like these were all maybe intentionally using READ permissions for the 'reading' ingestion APIs that use HTTP GET, and WRITE to run or otherwise actually control jobs.

clintropolis · 2021-09-21T10:21:44Z

  )
  {
-    IndexTaskUtils.datasourceAuthorizationCheck(req, Action.READ, getDataSource(), authorizerMapper);
+    IndexTaskUtils.datasourceAuthorizationCheck(req, Action.WRITE, getDataSource(), authorizerMapper);


IndexTaskUtils.datasourceAuthorizationCheck should be modified to no longer accept an Action since I think after this PR every caller will be using Action.WRITE, and it can just be hard coded in the method.

I've also noticed that no callers seem to ever be using the return value, so it could probably be changed to void since we seem to mostly be using it to explode if not authorized and otherwise ignore the return value.

clintropolis · 2021-09-21T10:29:05Z

   * @return authorization result
   */
-  private Access authorizationCheck(final HttpServletRequest req, Action action)
+  private Access datasourceWriteAuthCheck(final HttpServletRequest req)


nearly this same method is also in ParallelIndexSupervisorTask, maybe consider moving it to AbstractTask and calling it authorizeRequest, then here can just return task.authorizeRequest(req); (or dropping this wrapper method and calling the new task.authorizeRequest(req); method directly in the http api methods of SeekableStreamIndexTaskRunner)

Done. Thanks for the suggestion. 👍

clintropolis

cool, lgtm 👍

suneet-s

Code changes LGTM - but can we add some tests to catch any changes in behavior around authorization for these endpoints? At least for the endpoints where the authorization requirements have changed.

It looks like there are some integration tests for things like this in ITBasicAuthConfigurationTest. If you think integration tests are overkill for this, I am open to unit tests being written to cover these cases instead.

kfaraz · 2021-09-22T10:09:56Z

Thanks for the review, @suneet-s . I will add some UTs/ITs as applicable today.

…ervisor_api_auth

clintropolis · 2021-10-06T01:36:28Z

I think we also need to update the system table docs here https://github.com/apache/druid/blob/master/docs/operations/security-user-auth.md#sql-permissions to indicate that some of these system tables now require write permissions. I haven't found any other places yet in docs where a READ should be changed to a WRITE, yet at least...

clintropolis

👍

suneet-s

None of my comments are blocking - it would be nice to get a better understanding of what the lockGranularity if statement in IndexTaskTest#testAuthorizeResource does

suneet-s · 2021-10-05T23:23:57Z

+  public void testAuthorizeRequest() throws Exception
+  {
+    // Need to run this only once
+    if (lockGranularity == LockGranularity.SEGMENT) {


I don't understand this part of the test.

This is a parameterized unit test class. So every test method is run once for SEGMENT granularity and once for TIME_CHUNK. Since this test method does not depend on granularity, figured we should run it only once.

We could explain this part better in the comment or just remove the check.
Please let me know what you think would be better.

suneet-s · 2021-10-05T23:25:56Z

+              case Datasources.WIKIPEDIA:
+                // All users can read wikipedia but only writer can write
+                return new Access(
+                    action == Action.READ || Users.WIKI_WRITER.equals(username)


Suggested change

action == Action.READ || Users.WIKI_WRITER.equals(username)

action == Action.READ || (action == Action.WRITE && Users.WIKI_WRITER.equals(username))

I think a similar change is needed on line 132

Should we include this for readability? Because the logic would remain the same.

Primarily, yes.

Since there are only 2 actions, I suppose this is equivalent. If a new action was added later, this logic may not match what is described in the comment above.

…equestForDatasourceWrite()

kfaraz · 2021-10-06T12:13:20Z

@clintropolis , I have made some modifications to AbstractAuthConfigurationTestand the corresponding bootstrap.ldif as some auth integration tests were failing and I had missed updating them. Please review these changes.

techdocsmith

I'm changing the language around those docs in another PR. Otherwise LGTM.

Co-authored-by: Charles Smith <techdocsmith@gmail.com>

suneet-s

+1 after CI

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

…k access (apache#11718)" This reverts commit f2d6100.

* Revert "Require Datasource WRITE authorization for Supervisor and Task access (#11718)" This reverts commit f2d6100. * Revert "Require DATASOURCE WRITE access in SupervisorResourceFilter and TaskResourceFilter (#11680)" This reverts commit 6779c46. * Fix docs for the reverted commits * Fix and restore deleted tests * Fix and restore SystemSchemaTest

…k access (apache#11718)" This reverts commit f2d6100.

kfaraz added 3 commits September 17, 2021 15:14

Require Datasource WRITE access when fetching supervisor history

b97e9a3

Require Datasource WRITE for all Task APIs and sys.supervisor and sys…

860f295

….tasks

Remove unused import

caa70bf

kfaraz changed the title ~~[WIP] Require Datasource WRITE authorization for Supervisor and Task access~~ Require Datasource WRITE authorization for Supervisor and Task access Sep 17, 2021

suneet-s added the Security label Sep 17, 2021

clintropolis reviewed Sep 21, 2021

View reviewed changes

clintropolis added Design Review Release Notes labels Sep 21, 2021

Move authorizeRequest() to AbstractTask

bb06216

clintropolis approved these changes Sep 21, 2021

View reviewed changes

suneet-s reviewed Sep 21, 2021

View reviewed changes

kfaraz added 9 commits October 1, 2021 08:39

Add auth tests for SystemSchema

05b0080

Merge branch 'master' of https://github.com/apache/druid into fix_sup…

868b9d3

…ervisor_api_auth

Add auth tests in OverlordResourceTest

2c6a46e

temp stuff

bfef053

Add SeekableStreamIndexTaskRunnerAuthTest

1a987ac

Add Datasource Write check in SupervisorResourceTest

e3131cd

Merge branch 'master' of https://github.com/apache/druid into fix_sup…

f55e1f4

…ervisor_api_auth

Remove usage of forbidden APIs

6a15181

Fix checkstyle

b5b1881

Update permission requirements for system schema tables in docs

3913d80

clintropolis approved these changes Oct 6, 2021

View reviewed changes

suneet-s reviewed Oct 6, 2021

View reviewed changes

kfaraz added 4 commits October 6, 2021 10:59

Rename method AbstractTask.authorizeRequestForDatasourceWrite()

975dc3d

Add ForbiddenException to method signature of AbstractTask.authorizeR…

aa9616a

…equestForDatasourceWrite()

Fix auth integration test and add case for datasource write user

d41a22b

Add comments, rename fields

0a961e1

kfaraz requested review from clintropolis and suneet-s October 6, 2021 12:13

Fix auth integration tests

22cb309

techdocsmith approved these changes Oct 7, 2021

View reviewed changes

Comment thread docs/operations/security-user-auth.md Outdated

techdocsmith mentioned this pull request Oct 7, 2021

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

Merged

Update doc security-user-auth.md to match language used in other places

27f3f4f

Co-authored-by: Charles Smith <techdocsmith@gmail.com>

suneet-s approved these changes Oct 7, 2021

View reviewed changes

abhishekagarwal87 merged commit f2d6100 into apache:master Oct 8, 2021

kfaraz added a commit to kfaraz/druid that referenced this pull request Oct 20, 2021

Revert "Require Datasource WRITE authorization for Supervisor and Tas…

92e716f

…k access (apache#11718)" This reverts commit f2d6100.

kfaraz mentioned this pull request Oct 20, 2021

Revert permission changes to Supervisor and Task APIs #11819

Merged

9 tasks

jon-wei pushed a commit to jon-wei/druid that referenced this pull request Nov 22, 2021

Revert "Require Datasource WRITE authorization for Supervisor and Tas…

a82e28b

…k access (apache#11718)" This reverts commit f2d6100.

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

kfaraz deleted the fix_supervisor_api_auth branch August 1, 2023 04:51

	action == Action.READ \|\| Users.WIKI_WRITER.equals(username)
	action == Action.READ \|\| (action == Action.WRITE && Users.WIKI_WRITER.equals(username))

Conversation

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

Uh oh!

Description

Changes

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

kfaraz commented Sep 22, 2021

Uh oh!

clintropolis commented Oct 6, 2021

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

techdocsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kfaraz commented Sep 17, 2021 •

edited

Loading

kfaraz Oct 6, 2021 •

edited

Loading

kfaraz commented Oct 6, 2021 •

edited

Loading