Skip to content

Add config to allow setting up custom unsecured paths for druid nodes. - #5614

Merged
jon-wei merged 6 commits into
apache:masterfrom
nishantmonu51:unsecured-paths
Apr 12, 2018
Merged

jon-wei merged 6 commits into
apache:masterfrom
nishantmonu51:unsecured-paths

Conversation

@nishantmonu51

@nishantmonu51 nishantmonu51 commented Apr 9, 2018 •

Copy link
Copy Markdown
Member

This PR adds a new config druid.auth.unsecuredPaths which allows users to set custom unsecured paths.

Comment thread docs/content/configuration/auth.md Outdated

@jon-wei jon-wei Apr 9, 2018 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest adding this to AuthConfig instead using druid.auth.unsecuredPaths, so that all of the general auth-related parameters are in one place

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done.

@jon-wei

jon-wei commented Apr 9, 2018

Copy link
Copy Markdown
Contributor

There are some checks in AuthorizationUtils that check for bugs where the code authorizes a request multiple times:

    if (request.getAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED) != null) {
      throw new ISE("Request already had authorization check.");
    }

Prior to this patch, UNSECURED_PATHS only contained paths that did not have authorization checks.

With this patch, this check would need to be adjusted so that these functions no-op when the path is already allowed through the unsecured paths config.

Perhaps UnsecuredResourceFilter could add a new flag that indicates the request was allowed through the unsecured paths configuration.

@nishantmonu51 nishantmonu51 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@jon-wei : Made changes are per your comments.


AuthenticationResult result = (AuthenticationResult) request.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT);
if (authInfoChecked != null || result != null) {
if (authInfoChecked != null || result != null || allowUnsecured != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's remove the "allowUnsecured" condition here, it's sufficient to check for just authInfoChecked since UnsecuredResourceFilter sets that attribute

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah sorry, misread this, thought this was another area, please ignore the above comment

@jon-wei

jon-wei commented Apr 10, 2018 •

Copy link
Copy Markdown
Contributor

Had one comment, latest changes look good aside from that, there are some real unit test failures with the mocks not expecting the new attribute

Can you also add something to the integration tests? There's already a test set up in ITBasicAuthConfigurationTest, I think you can add a normally secure path to this new config and call makeRequest to that path with an uncredentialed http client for each node (like in checkNodeAccess)

@nishantmonu51

Copy link
Copy Markdown
Member Author

@jon-wei done.

@jon-wei

jon-wei commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

👍 after travis

@jon-wei
jon-wei merged commit e6efd75 into apache:master Apr 12, 2018
sathishsri88 pushed a commit to sathishs/druid that referenced this pull request May 8, 2018
apache#5614)

* Add config to allow setting up custom unsecured paths for druid nodes.

* return all resources for Unsecured paths

* review comment - Add test

* fix tests

* fix test
jon-wei pushed a commit to implydata/druid-public that referenced this pull request Sep 20, 2018
apache#5614)

* Add config to allow setting up custom unsecured paths for druid nodes.

* return all resources for Unsecured paths

* review comment - Add test

* fix tests

* fix test
@dclim dclim added this to the 0.13.0 milestone Oct 8, 2018
riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
apache#5614)

* Add config to allow setting up custom unsecured paths for druid nodes.

* return all resources for Unsecured paths

* review comment - Add test

* fix tests

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