Skip to content

Require DATASOURCE WRITE access in SupervisorResourceFilter and TaskResourceFilter#11680

Merged
clintropolis merged 4 commits intoapache:masterfrom
kfaraz:require_write_access_for_supervisor
Sep 9, 2021
Merged

Require DATASOURCE WRITE access in SupervisorResourceFilter and TaskResourceFilter#11680
clintropolis merged 4 commits intoapache:masterfrom
kfaraz:require_write_access_for_supervisor

Conversation

@kfaraz
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz commented Sep 9, 2021

Description

Users with DATASOURCE READ access can currently call GET and HEAD APIs
on Supervisor and Task resources. These resources respond with ingestion related
information, which should be visible only to DATASOURCE WRITE users.

Changes

  • Always require DATASOURCE WRITE access in SupervisorResourceFilter and TaskResourceFilter

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

@kfaraz kfaraz marked this pull request as ready for review September 9, 2021 03:53
Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 makes sense to me

@suneet-s
Copy link
Copy Markdown
Contributor

suneet-s commented Sep 9, 2021

@kfaraz Is it possible to add integration tests that call all the different endpoints that are available on the ingest path and validate that a user needs DATASOURCE WRITE permission to see .

I skimmed the code and noticed users with DATASOURCE READ can see metadata about the segments. I think this seems ok since a DATASOURCE READ user may need to know which segments were available when a query was executed so they would have a need.

I dug a little bit and it seems like the segment metadata could include the loadSpec. Do you think that should also require DATASOURCE WRITE permissions?

Copy link
Copy Markdown
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change LGTM with 2 comments

  1. Can we add integration tests - maybe piggybacking off some of the security based integration tests?
  2. What should we do about segment metadata - should that require DATASOURCE WRITE?

Neither of these comments are blockers to this PR, so I'm approving the PR - but we might want fast follow ups to address these points

@kfaraz
Copy link
Copy Markdown
Contributor Author

kfaraz commented Sep 9, 2021

Thanks for the review, @suneet-s . I will add an integration test in a follow up PR (hopefully within a couple of days). I will also spend some time on the access of segment metadata and propose required changes, if any.

@clintropolis clintropolis merged commit 6779c46 into apache:master Sep 9, 2021
clintropolis pushed a commit to clintropolis/druid that referenced this pull request Sep 9, 2021
…esourceFilter (apache#11680)

* Require DATASOURCE WRITE access in SupervisorResourceFilter and TaskResourceFilter

* Remove unused imports

* Add SupervisorResourceFilterTest

* Verify mocks in test
@clintropolis clintropolis added this to the 0.22.0 milestone Sep 9, 2021
@clintropolis
Copy link
Copy Markdown
Member

I missed a couple of things in my initial review, SQL queries to sys.tasks and sys.supervisors still use read permissions and a handful of task report APIs also still use read instead of write

Screen Shot 2021-09-09 at 1 53 58 PM
Screen Shot 2021-09-09 at 1 54 08 PM

so this change isn't quite yet complete I think. (I do still think WRITE makes more sense for the changes here and all of these APIs). I'm not sure if I missed any others, probably worth having another look.

@kfaraz
Copy link
Copy Markdown
Contributor Author

kfaraz commented Sep 13, 2021

Thanks for checking, @clintropolis . I am taking another pass at the APIs and will share the results of my findings here.

@suneet-s
Copy link
Copy Markdown
Contributor

Removed milestone 0.22.0 since the backport PR was not merged

abhishekagarwal87 pushed a commit that referenced this pull request Oct 8, 2021
…#11718)

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
kfaraz added a commit to kfaraz/druid that referenced this pull request Oct 20, 2021
abhishekagarwal87 pushed a commit that referenced this pull request Oct 25, 2021
* 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
jon-wei pushed a commit to jon-wei/druid that referenced this pull request Nov 22, 2021
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
@kfaraz kfaraz deleted the require_write_access_for_supervisor branch August 1, 2023 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants