Druid authentication/authorization on router's side by esevastyanov · Pull Request #73 · metamx/druid

esevastyanov · 2019-05-23T12:09:03Z

Updated the druid-basic-security module to make it work on any Druid component as a user/role manager:

Added dependency on MetadataStorage because it is used as a storage for users/roles entities
Authenticator/authorizer cache/handler/notifier take the class composition from a config

…nent as user/role manager: 1. Added dependency on MetadataStorage because it is used as a storage for users/roles 2. Authenticator/authorizer cache/handler/notifier take class composition form a config

esevastyanov · 2019-05-24T14:58:49Z

@egor-ryashin @leventov could you please run integration test locally using 0.12.1-mmx branch?
I tried several times but two integration tests continuously fail on before stage: ITTwitterQueryTest and ITWikipediaQueryTest. Travis checks also failed at the same places.

[ERROR] Failures:
[ERROR]   ITTwitterQueryTest.before:47 » ISE Max number of retries[10] exceeded for Task...
[ERROR]   ITWikipediaQueryTest.before:47 » ISE Max number of retries[10] exceeded for Ta...

esevastyanov · 2019-05-24T19:58:18Z

I got the same result for the branch 0.12.1 of incubator-druid

leventov · 2019-05-27T16:17:13Z

...id-basic-security/src/main/java/io/druid/security/basic/BasicAuthClassCompositionConfig.java

+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+public class BasicAuthClassCompositionConfig


This class has a lot of fields, but seems that only one of them is used, why is that?

Actually, all of them are in use (see BasicSecurityDruidModule)

leventov · 2019-05-27T16:17:34Z

...id-basic-security/src/main/java/io/druid/security/basic/BasicAuthClassCompositionConfig.java

+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+public class BasicAuthClassCompositionConfig


Please add a class-level comment describing something or pointing somewhere.

leventov · 2019-05-27T16:19:12Z

...ore/druid-basic-security/src/main/java/io/druid/security/basic/BasicSecurityDruidModule.java

+      BasicAuthClassCompositionConfig config
+  ) throws ClassNotFoundException
  {
+    if (config.getAuthenticatorMetadataStorageUpdater() != null) {


This block of code can be extracted as a method.

Please add some explanation. What is going on here?

Refactored and added description

leventov · 2019-05-27T16:20:05Z

...ava/io/druid/security/basic/authentication/db/cache/NoopBasicAuthenticatorCacheNotifier.java

+
+package io.druid.security.basic.authentication.db.cache;
+
+public class NoopBasicAuthenticatorCacheNotifier implements BasicAuthenticatorCacheNotifier


Why this class is needed? It's not used in this PR.

It is needed on a config level
https://github.com/metamx/druid-config/pull/99

Would be nice to specify whether "noop" means "no auth" or "always rejecting auth".

leventov · 2019-05-27T16:20:18Z

...in/java/io/druid/security/basic/authorization/db/cache/NoopBasicAuthorizerCacheNotifier.java

+
+package io.druid.security.basic.authorization.db.cache;
+
+public class NoopBasicAuthorizerCacheNotifier implements BasicAuthorizerCacheNotifier


I also removed the same classes from test scope so this class is used in tests now

leventov · 2019-05-27T16:23:38Z

There are errors in Travis log: io.druid.server.AsyncQueryForwardingServlet - Can not find Authenticator with Name [allowAll]

esevastyanov · 2019-05-28T13:37:51Z

Also updated AWS S3 access and secret keys as they were changed last year so that's why integration tests were failing @leventov

leventov · 2019-05-28T16:52:39Z

...id-basic-security/src/main/java/io/druid/security/basic/BasicAuthClassCompositionConfig.java

Typo, handler

leventov · 2019-05-28T16:53:52Z

...id-basic-security/src/main/java/io/druid/security/basic/BasicAuthClassCompositionConfig.java

I would put it "BasicAuthClassCompositionConfig provides options to specify authenticator classes..."

leventov · 2019-05-28T16:55:04Z

...id-basic-security/src/main/java/io/druid/security/basic/BasicAuthClassCompositionConfig.java

Not very clear, I would put it "If a field in this class is non-null then ..."

leventov · 2019-05-28T16:56:52Z

...ore/druid-basic-security/src/main/java/io/druid/security/basic/BasicSecurityDruidModule.java

Maybe call the param classRunByCoordinator

leventov · 2019-05-28T16:58:15Z

...ava/io/druid/security/basic/authentication/db/cache/NoopBasicAuthenticatorCacheNotifier.java

+
+package io.druid.security.basic.authentication.db.cache;
+
+public class NoopBasicAuthenticatorCacheNotifier implements BasicAuthenticatorCacheNotifier


Would be nice to specify whether "noop" means "no auth" or "always rejecting auth".

leventov · 2019-05-28T16:58:23Z

...in/java/io/druid/security/basic/authorization/db/cache/NoopBasicAuthorizerCacheNotifier.java

+
+package io.druid.security.basic.authorization.db.cache;
+
+public class NoopBasicAuthorizerCacheNotifier implements BasicAuthorizerCacheNotifier


leventov · 2019-05-29T16:53:56Z

...id-basic-security/src/main/java/io/druid/security/basic/BasicAuthClassCompositionConfig.java

 * Basic authentication storage/cache/resource handler config.
- * The config is an option to specify classes of user/role managers, caches and notifiers.
- * If a config field is specified then the corresponding class is instantiated
+ * BasicAuthClassCompositionConfig provides options to specify authenticator/authorizer classes.


I intended that "of user/role managers, caches and notifiers" is retained - please fix the upstream PR.

egor-ryashin · 2019-06-08T21:27:32Z

integration-tests/docker/middlemanager.conf

  -Ddruid.indexer.fork.server.http.numThreads=100
-  -Ddruid.s3.accessKey=AKIAIMKECRUYKDQGR6YQ
-  -Ddruid.s3.secretKey=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
+  -Ddruid.s3.accessKey=AKIAJI7DG7CDECGBQ6NA


Just wonder what s3 storage is accessed by those credentials, is it safe to keep those credentials in github repo?

This is not our s3 account
https://github.com/apache/incubator-druid/blob/master/integration-tests/docker/middlemanager.conf#L19

I don't understand why we change them?

Since we depend on third-party's s3 account in integration-tests (and it was updated). As a solution, we might duplicate that data into our s3.

egor-ryashin · 2019-06-08T21:41:51Z

...ore/druid-basic-security/src/main/java/io/druid/security/basic/BasicSecurityDruidModule.java

+    return getInstance(
+        injector,
+        config.getAuthorizerResourceHandler(),
+        CoordinatorBasicAuthorizerResourceHandler.class,


I think we should rename CoordinatorBasicAuthorizerResourceHandler to something else, as it is not bound to Coordinator now.

Let me update names and some logic around centralized user management in the next PR

Could you create a ticket for that?

BTW, I wonder what's the purpose of doing it in a separate PR?

As we decided that this feature might be helpful for the community the PR into upstream druid was created and merged. So it would be great to follow this strategy and pull new changes into the upstream before pulling into our fork.

Should I create a ticket in the upstream?

Original PR into Druid
apache#7789

Should I create a ticket in the upstream?

Yes.

egor-ryashin · 2019-06-08T21:51:25Z

...ore/druid-basic-security/src/main/java/io/druid/security/basic/BasicSecurityDruidModule.java

+    if (configClassName != null) {
+      // ClassCastException is thrown in case of a mismatch, configuration fix is required.
+      @SuppressWarnings("unchecked")
+      final T instance = (T) injector.getInstance(Class.forName(configClassName));


I think we exposing too much internals in configs propagating the class names for each sub-component. In fact, we would like a switch like ...authorization.management.enabled=true/false which can effectively select required classes for us.

This will require refactoring current DI of the module

That works for testing, but it's awkward for production.

Agree, this might be improved

Let's add it to the issue we create for #73 (comment)

esevastyanov · 2019-06-28T14:46:02Z

@egor-ryashin I moved everything from merged incubator-druid PR
apache#7789
At first, I intended to do cherry-pick but unfortunately, this anyway requires some code changes as the master branch has different license and package names.
If you have some thoughts on how to make this logic better then let's discuss them separately (in slack channel #d-druid-auth) and implement them in the next PR. It would be great to merge this PR and start a new one with improvements.

egor-ryashin · 2019-06-28T20:36:33Z

integration-tests/docker/middlemanager.conf

  -Ddruid.indexer.fork.server.http.numThreads=100
-  -Ddruid.s3.accessKey=AKIAIMKECRUYKDQGR6YQ
-  -Ddruid.s3.secretKey=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
+  -Ddruid.s3.accessKey=AKIAJI7DG7CDECGBQ6NA


I don't understand why we change them?

egor-ryashin · 2019-06-28T20:39:07Z

...ore/druid-basic-security/src/main/java/io/druid/security/basic/BasicSecurityDruidModule.java

+    if (configClassName != null) {
+      // ClassCastException is thrown in case of a mismatch, configuration fix is required.
+      @SuppressWarnings("unchecked")
+      final T instance = (T) injector.getInstance(Class.forName(configClassName));


That works for testing, but it's awkward for production.

egor-ryashin · 2019-06-28T20:40:48Z

...ore/druid-basic-security/src/main/java/io/druid/security/basic/BasicSecurityDruidModule.java

+    return getInstance(
+        injector,
+        config.getAuthorizerResourceHandler(),
+        CoordinatorBasicAuthorizerResourceHandler.class,


Could you create a ticket for that?

druid-basic-security module update to make it work on any Druid compo…

0af5d8c

…nent as user/role manager: 1. Added dependency on MetadataStorage because it is used as a storage for users/roles 2. Authenticator/authorizer cache/handler/notifier take class composition form a config

esevastyanov marked this pull request as ready for review May 23, 2019 12:16

esevastyanov requested review from egor-ryashin and leventov May 23, 2019 12:16

leventov reviewed May 27, 2019

View reviewed changes

leventov reviewed May 28, 2019

View reviewed changes

esevastyanov force-pushed the druid-router-auth branch from afc14cb to e7e4eab Compare May 29, 2019 09:15

Refactored and added some comments

d4237cf

esevastyanov force-pushed the druid-router-auth branch from e7e4eab to d4237cf Compare May 29, 2019 10:12

Eugene Sevastianov added 3 commits May 29, 2019 13:27

Added comments

517b714

Reduced nulls

2ad064e

Removed nulls from BasicSecurityDruidModule completely

945d752

leventov reviewed May 30, 2019

View reviewed changes

egor-ryashin requested changes Jun 8, 2019

View reviewed changes

Eugene Sevastianov added 2 commits June 24, 2019 11:59

Added metadata storage to broker for authentication/authorization

d047b2c

Updated new class comments

5619b2f

esevastyanov requested a review from egor-ryashin June 28, 2019 14:46

egor-ryashin requested changes Jun 28, 2019

View reviewed changes

egor-ryashin approved these changes Jul 4, 2019

View reviewed changes


		package io.druid.security.basic.authentication.db.cache;

		public class NoopBasicAuthenticatorCacheNotifier implements BasicAuthenticatorCacheNotifier


		package io.druid.security.basic.authorization.db.cache;

		public class NoopBasicAuthorizerCacheNotifier implements BasicAuthorizerCacheNotifier

Conversation

esevastyanov commented May 23, 2019

Uh oh!

esevastyanov commented May 24, 2019

Uh oh!

esevastyanov commented May 24, 2019

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!

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!

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!

leventov commented May 27, 2019

Uh oh!

esevastyanov commented May 28, 2019

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!

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!

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!

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!

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