Skip to content

Revert permission changes to Supervisor and Task APIs#11819

Merged
abhishekagarwal87 merged 5 commits intoapache:masterfrom
kfaraz:revert_permission_changes
Oct 25, 2021
Merged

Revert permission changes to Supervisor and Task APIs#11819
abhishekagarwal87 merged 5 commits intoapache:masterfrom
kfaraz:revert_permission_changes

Conversation

@kfaraz
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz commented Oct 20, 2021

Reverts #11718 and #11680

Description

Changing the required permission level for ALL Supervisor and Task APIs may break existing
automation that users might have set up.

The motivation behind the above mentioned changes were to prevent leakage of sensitive
information through these APIs. But there are other workarounds to prevent that, such as
using EnvironmentVariableDynamicConfigProvider to provide passwords and secrets.

While the above mentioned changes do prevent such leakage of information, they do so
at the cost of unreasonable restrictions on even purely informative APIs. The original changes
also remove the possibility of having a user who needs to view Supervisor and Task information
but should not have permission to ingest data, modify compaction configuration etc. These are
typically support users managing a cluster.

Keeping these arguments in mind, the changes are being reverted. A better solution to these
problems would be a more granular auth model in Druid.

Changes being reverted

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

@abhishekagarwal87 abhishekagarwal87 merged commit abac9e3 into apache:master Oct 25, 2021
@abhishekagarwal87
Copy link
Copy Markdown
Contributor

Merged since CI failure is unrelated.

@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
@kfaraz kfaraz deleted the revert_permission_changes branch September 30, 2022 16:07
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.

3 participants