Basic auth extension by jon-wei · Pull Request #4823 · apache/druid

jon-wei · 2017-09-19T03:06:52Z

This PR adds an extension that provides implementations of the auth interfaces introduced in #4271 :

an Authenticator which supports HTTP Basic authentication
an Authorizer which implements basic role-based access control

fjy · 2017-09-19T17:05:46Z

👍

gianm · 2017-10-02T18:46:12Z

@jon-wei please merge from master if you haven't recently; I hope that will fix the VM crash we got during CI.

jon-wei · 2017-10-10T00:27:10Z

Making some modifications to this re: password initialization, marking WIP

jihoonson

Reviewed up to SQLBasicSecurityStorageConnector.

jihoonson · 2017-10-20T04:48:54Z

+    <dependency>
+      <groupId>mysql</groupId>
+      <artifactId>mysql-connector-java</artifactId>
+      <version>5.1.38</version>


mysql-metadata-storage also has this dependency of the same version. Did you intentionally specify these versions separately? Similar comment on the dependency on postgresql below.

Hm, moved the mysql/postgres versions to the main druid pom.xml, changed these to reference those versions

jihoonson · 2017-10-20T04:51:02Z

+layout: doc_page
+---
+
+# Druid-Basic-Security


Please removes the hyphens.

Removed hyphens

jihoonson · 2017-10-20T05:12:31Z

+import java.util.Map;
+import java.util.concurrent.Callable;
+
+public abstract class SQLBasicSecurityStorageConnector implements BasicSecurityStorageConnector


Please add some java doc and unit tests.

Added javadoc and tests

jihoonson · 2017-10-20T05:13:44Z

+    return dataSource;
+  }
+
+  protected boolean connectorIsTransientException(Throwable e)


This always returns false. Is this for any user-defined subclasses?

I copied this from SQLMetadataConnector initially, I think that was the intent, this is now in the common BaseSQLMetadataConnector class

jihoonson · 2017-10-20T05:13:59Z

+   *
+   * @return String representing the SQL type
+   */
+  protected String getPayloadType()


Please remove unused method.

jihoonson · 2017-10-20T07:04:17Z

+@JsonTypeName("basic")
+public class BasicRoleBasedAuthorizer implements Authorizer
+{
+  private static final Logger log = new Logger(BasicRoleBasedAuthorizer.class);


Please remove unused variable.

jihoonson · 2017-10-20T07:05:58Z

+  )
+  {
+    if (authenticationResult == null) {
+      return new Access(false);


Any error message like 'null authenticationResult'?

I changed this to throw an IAE, the authenticationResult should never be null here and indicates a bug if it is

jihoonson · 2017-10-20T07:07:25Z

+          authenticationResult.getIdentity()
+      );
+      if (authorizationName == null) {
+        return new Access(false);


Similar comment. 'Cannot find an authorization name for authenticationResult.getIdentity()'?

I decided to remove the authentication name -> authorization name features, felt like the benefit (possibly avoiding cases where two user accounts are essentially the same with a different name) wasn't worth the added complexity, so this block is gone now

jihoonson · 2017-10-20T07:07:48Z

+      }
+    }
+
+    return new Access(false);


Similar comment. Maybe 'permission denied'?

hm, I think that's a bit redundant in this case, since Access(false) already directly expresses an authorization denial

jihoonson · 2017-10-20T07:10:52Z

+    String permissionResourceName = permissionResource.getName();
+    Pattern resourceNamePattern = Pattern.compile(permissionResourceName);
+    Matcher resourceNameMatcher = resourceNamePattern.matcher(resource.getName());
+    return resourceNameMatcher.matches();


Hmm, do you have any performance benchmark result? I'm not sure how this is fast enough.

Not yet, I'll run some shortly

jihoonson · 2017-10-24T00:28:12Z

+        ImmutableList.of(
+            StringUtils.format(
+                "CREATE TABLE %1$s (\n"
+                + "  user_name INTEGER NOT NULL, \n"


VARCHAR(255)?

fixed, thanks

jihoonson · 2017-10-24T00:40:07Z

Maybe better to return a map containing the exception?

Made this propagate the exception since it should never happen under any normal conditions

jihoonson · 2017-10-24T00:43:29Z

Should be the camel case or simply return without defining a variable.

Changed this and similar places to return without defining a variable

jihoonson · 2017-10-24T00:43:37Z

Should be the camel case or simply return without defining a variable.

jihoonson · 2017-10-24T00:46:34Z

Please break the line.

This is method is deleted now since I removed the name mapping table

jihoonson · 2017-10-24T00:52:34Z

+import java.util.List;
+import java.util.Map;
+
+public interface BasicSecurityStorageConnector


What do you think about making all non get* methods to return the result indicating success or failure? For example, delete* methods return the number of deleted rows. It may be useful for operators to reduce mistakes.

jihoonson · 2017-10-24T00:55:26Z

+    }
+  }
+
+  public <T> T retryWithHandle(


Or maybe it's better to create a new super class which both SQLMetadataConnector and this class extend.

jihoonson · 2017-10-24T00:56:56Z

Looks not being used.

Removed unused class, the Derby implementation now uses MetadataStorage

jihoonson · 2017-10-24T00:58:34Z

Looks not being used.

It's not instantiated directly, but the class is bound in BasicSecurityDruidModule:

PolyBind.optionBinder(binder, Key.get(BasicSecurityStorageConnector.class)) .addBinding("derby") .to(DerbySQLBasicSecurityStorageConnector.class) .in(ManageLifecycle.class); PolyBind.optionBinder(binder, Key.get(SQLBasicSecurityStorageConnector.class)) .addBinding("derby") .to(DerbySQLBasicSecurityStorageConnector.class) .in(ManageLifecycle.class);

jihoonson · 2017-10-24T01:01:12Z

bail -> fail?

"bail" is used here in the sense of "abandon what it's doing"

jihoonson

The latest patch looks good to me. I have one question.

jihoonson · 2017-10-26T01:53:05Z

+    );
+  }
+
+  private int getUserCountInTransaction(Handle handle, String userName)


What does the "InTransaction" suffix mean in this and below methods?

It's referring to how they're called from within the DB transactions, for example:

getDBI().inTransaction( new TransactionCallback<Void>() { @Override public Void inTransaction(Handle handle, TransactionStatus transactionStatus) throws Exception {

Thanks for the explanation. I was confused with its name. It sounds to me like getUserCount() is executed in a transaction. I think just getUserCount() is fine.

Cool, I changed the names to remove "inTransaction"

jon-wei · 2017-10-26T03:59:04Z

@jihoonson I added a RegexMatchBenchmark for benchmarking regex pattern compilation and matching, these are the results I'm seeing on my macbook:

# Run complete. Total time: 00:06:22

Benchmark                                                   (numPatterns)  Mode  Cnt       Score      Error  Units
RegexMatchBenchmark.compileGranularityPathRegex                    100000  avgt   25  176488.968 ± 2731.548  us/op
RegexMatchBenchmark.compileGranularityPathRegexAndMatch            100000  avgt   25  196734.506 ± 6023.698  us/op
RegexMatchBenchmark.compileUUIDRegex                               100000  avgt   25   62557.928 ± 1232.438  us/op
RegexMatchBenchmark.compileUUIDRegexAndMatch                       100000  avgt   25   98763.608 ± 2052.479  us/op
RegexMatchBenchmark.compileUUIDsAsRegex                            100000  avgt   25   74987.953 ± 1011.895  us/op
RegexMatchBenchmark.compileUUIDsAsRegexAndMatchRandomUUID          100000  avgt   25   79894.899 ± 1000.825  us/op
RegexMatchBenchmark.deserializeGranularityPathRegex                100000  avgt   25  233961.684 ± 2421.426  us/op
RegexMatchBenchmark.deserializeUUIDRegex                           100000  avgt   25  107444.061 ± 1850.141  us/op
RegexMatchBenchmark.precompileGranularityPathRegexAndMatch         100000  avgt   25   19848.358 ±  312.136  us/op
RegexMatchBenchmark.precompileUUIDRegexAndMatch                    100000  avgt   25   29987.644 ±  407.776  us/op

Roughly, with the test regexes used in that benchmark (one for UUID strings, and a more complex one taken from Granularity), compile + match takes ~2us or lower.

Deserializing a byte[] representation of a Pattern using DefaultObjectMapper is also slower than recompiling the pattern from the regex string.

jihoonson · 2017-10-26T05:06:25Z

@jon-wei thanks for sharing the benchmark result. I was concerned with the performance because BasicRoleBasedAuthorizer.permissionCheck() always compile a new pattern for each permissionResourceName. This method may become slow as the number of resourceAction increases. Do you have any better idea? Maybe caching compiled patterns?

jon-wei · 2017-10-26T06:35:42Z

@jihoonson I added a cache for the compiled regex patterns

jihoonson · 2017-10-26T08:42:50Z

@jon-wei thanks for the quick fix!

gianm · 2017-10-27T18:23:29Z

+# Druid Basic Security
+
+This extension adds:
+- an Authenticator which supports [HTTP Basic authentication](https://en.wikipedia.org/wiki/Basic_access_authentication)


Link to the docs for Authenticator and Authorizer (druid's auth.md) -- otherwise, if a user comes in to this page first, they will be lost.

gianm · 2017-10-27T18:28:16Z

+### Properties
+|Property|Description|Default|required|
+|--------|-----------|-------|--------|
+|`druid.auth.basic.initialAdminPassword`|Password to assign when Druid automatically creates the default admin account. See [Default user accounts](#default-user-accounts) for more information.|"druid"|No|


This isn't related to the authenticator or authorizer? That seems strange to me.

gianm · 2017-11-02T03:04:42Z

+
+The configuration examples in the rest of this document will use "MyBasicAuthenticator" as the name of the authenticator being configured.
+
+Only one instance of a "basic" type authenticator should be created and used, multiple "basic" authenticator instances are not supported.


What happens if you configure more than one?

gianm · 2017-11-02T03:04:51Z

+
+The Basic authorizer has no additional configuration properties at this time.
+
+Only one instance of a "basic" type authorizer should be created and used, multiple "basic" authorizer instances are not supported.


What happens if you configure more than one?

gianm · 2017-11-02T03:40:42Z

+
+Cluster administrators should change the default passwords for these accounts before exposing a cluster to users.
+
+## Defining permissions


This section of the doc looks like it applies to all authorizers and should be part of Druid's auth.md.

gianm · 2017-11-02T09:06:18Z

+
+    String permissionResourceName = permissionResource.getName();
+
+    Pattern resourceNamePattern = permissionPatternCache.get(permissionResourceName);


The permissionPatternCache won't be needed if all permissions details are cached -- since the permission objects would be long-lived and could contain their own compiled Patterns.

gianm · 2017-11-02T09:07:55Z

+      return null;
+    }
+
+    if (dbConnector.checkCredentials(dbConfig.getDbPrefix(), user, password.toCharArray())) {


Same comment as in the Authorizer: we need to avoid hitting the DB here.

gianm · 2017-11-02T09:10:50Z

+      String user = splits[0];
+      char[] password = splits[1].toCharArray();
+
+      if (dbConnector.checkCredentials(dbConfig.getDbPrefix(), user, password)) {


Similar -- need to avoid hitting the DB.

gianm · 2017-11-02T09:12:09Z

+                createUserTable(dbConfig.getDbPrefix());
+                createUserCredentialsTable(dbConfig.getDbPrefix());
+
+                makeDefaultSuperuser(


IMO, this should be optional -- superuser may use a different authenticator.

gianm · 2017-11-02T09:12:13Z

+                    dbConfig.getInitialAdminPassword()
+                );
+
+                makeDefaultSuperuser(


IMO, this should be optional -- internal may use a different authenticator.

jon-wei · 2017-11-17T02:22:03Z

closing this in favor of #5099

jon-wei added the Feature label Sep 19, 2017

jon-wei force-pushed the basic_sec branch from e5a92b8 to 9a831df Compare September 19, 2017 03:08

gianm added the Security label Oct 2, 2017

jon-wei force-pushed the basic_sec branch from 1f2c2db to 32ab40c Compare October 9, 2017 21:20

jon-wei changed the title ~~Basic auth extension~~ [WIP] Basic auth extension Oct 10, 2017

jon-wei added the WIP label Oct 10, 2017

jon-wei force-pushed the basic_sec branch 2 times, most recently from d0c7577 to e9ba4ff Compare October 10, 2017 01:05

Basic security extension

5addb33

jon-wei force-pushed the basic_sec branch from e9ba4ff to 5addb33 Compare October 20, 2017 01:30

jon-wei changed the title ~~[WIP] Basic auth extension~~ Basic auth extension Oct 20, 2017

jihoonson requested changes Oct 20, 2017

View reviewed changes

jon-wei removed the WIP label Oct 20, 2017

jihoonson reviewed Oct 24, 2017

View reviewed changes

PR comments

aac0f92

jon-wei force-pushed the basic_sec branch from 9ec0622 to aac0f92 Compare October 26, 2017 00:22

jihoonson reviewed Oct 26, 2017

View reviewed changes

jon-wei added 2 commits October 25, 2017 19:57

Add regex matching benchmark

1792477

More PR comments

6f98b30

Checkstyle

dd89a13

Cache permission Patterns, update docs

bbbd43a

Add regex compile failure check

f1cd250

jihoonson approved these changes Oct 26, 2017

View reviewed changes

Allow multiple authenticator/authorizer instances

0707449

jon-wei force-pushed the basic_sec branch from cf0959c to 0707449 Compare November 2, 2017 02:14

gianm requested changes Nov 2, 2017

View reviewed changes

jon-wei mentioned this pull request Nov 17, 2017

Basic auth extension #5099

Merged

jon-wei closed this Nov 17, 2017

veotax mentioned this pull request Apr 19, 2018

Support jCasbin as the authorization backend #5663

Closed


		The configuration examples in the rest of this document will use "MyBasicAuthenticator" as the name of the authenticator being configured.

		Only one instance of a "basic" type authenticator should be created and used, multiple "basic" authenticator instances are not supported.


		The Basic authorizer has no additional configuration properties at this time.

		Only one instance of a "basic" type authorizer should be created and used, multiple "basic" authorizer instances are not supported.


		Cluster administrators should change the default passwords for these accounts before exposing a cluster to users.

		## Defining permissions


		String permissionResourceName = permissionResource.getName();

		Pattern resourceNamePattern = permissionPatternCache.get(permissionResourceName);

Conversation

jon-wei commented Sep 19, 2017

Uh oh!

fjy commented Sep 19, 2017

Uh oh!

gianm commented Oct 2, 2017

Uh oh!

jon-wei commented Oct 10, 2017

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

jihoonson Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jihoonson Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

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!

jihoonson Oct 20, 2017 •

edited

Loading

jihoonson Oct 20, 2017 •

edited

Loading