Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set DRUID_AUTHORIZATION_CHECKED attribute for router endpoints #8026

Merged
merged 2 commits into from
Jul 9, 2019
Merged

set DRUID_AUTHORIZATION_CHECKED attribute for router endpoints #8026

merged 2 commits into from
Jul 9, 2019

Conversation

pjain1
Copy link
Member

@pjain1 pjain1 commented Jul 4, 2019

Fixes #6786

Description

Router does not perform any authorization checks and depends on requests forwarded to other nodes to perform authorization and set DRUID_AUTHORIZATION_CHECKED header. It works fine in most cases, however for requests that are not forwarded to other nodes and are not in unsecured path list, this header is not set, thus PreResponseAuthorizationCheckFilter throws exception. Example of this would be /druid/router/v1/brokers end point on router.

To fix this added appropriate resource filter to router endpoint.

@gianm gianm added the Security label Jul 4, 2019
@gianm
Copy link
Contributor

gianm commented Jul 4, 2019

I would think the druid/router/v1/brokers endpoint should at least require STATE READ permissions.

How about adding that check to that endpoint, instead of skipping authorization for all router endpoints? (And also adding appropriate checks to any other router endpoints that might be missing them)

@pjain1
Copy link
Member Author

pjain1 commented Jul 5, 2019

Changed the code and PR description as per your recommendation. Thanks

@pjain1 pjain1 added this to the 0.16.0 milestone Jul 5, 2019
@egor-ryashin
Copy link
Contributor

@pjain1 I wonder if that can be unit-tested easily?

@pjain1
Copy link
Member Author

pjain1 commented Jul 5, 2019

@egor-ryashin added RouterResource to ResourceFilter test framework

@fjy fjy removed this from the 0.16.0 milestone Jul 7, 2019
Copy link
Member

@nishantmonu51 nishantmonu51 left a comment

Choose a reason for hiding this comment

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

LGTM

@gianm gianm added the Bug label Jul 9, 2019
@gianm gianm added this to the 0.16.0 milestone Jul 9, 2019
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM - thx!

@gianm gianm merged commit 027291a into apache:master Jul 9, 2019
@pjain1 pjain1 deleted the fix_router_auth branch July 9, 2019 07:56
gianm pushed a commit to implydata/druid-public that referenced this pull request Jul 9, 2019
…e#8026)

* add state resource filter to router endpoints

* add RouterResource to ResourceFilter test framework
clintropolis pushed a commit that referenced this pull request Jul 24, 2019
* add state resource filter to router endpoints

* add RouterResource to ResourceFilter test framework
@clintropolis clintropolis modified the milestones: 0.16.0, 0.15.1 Jul 24, 2019
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.

apache-druid-0.13.0-incubating router /druid/router/v1/brokers
6 participants