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

jon-wei · 2017-11-17T02:20:32Z

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

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

With the large amount of changes (almost a rewrite) from the last round of PR reviews on #4823, I felt like it would be easier to close the old PR and start a fresh one.

Some design notes:

The user/role information is stored in the metadata store, using the "druid_config" table as a key-value store, user/role maps are stored as serialized byte arrays
On the coordinator a *MetadataStorageUpdater class is responsible for handling writes to the user/role DB and holds a deserialized cached copy of the latest DB state
Each node has a *CacheManager instance that polls the coordinator for the latest DB state periodically, this also syncs the auth DB state at startup time
The coordinator can also notify and push updates to remote nodes, using the *CacheNotifier classes
The BasicHTTPAuthenticator and BasicRoleBasedAuthorizer perform their checks using cached info from the *CacheManager instance, to avoid overloading the coordinator
The coordinator nodes have a jersey resource for configuring the auth DB, which also holds the endpoint that nodes poll to get the latest DB state
A jersey resource is also installed on non-coordinator nodes, containing an endpoint that listens for DB state updates

jihoonson · 2017-11-19T03:28:40Z

@jon-wei thanks for raising a new PR, but it looks that this single patch contains quite large changes. Is it possible to split into several smaller PRs? It would be much helpful for reviewers.

jon-wei · 2017-11-20T23:46:29Z

@jihoonson

I can split the PR into two if really needed, but the changes are pretty self contained in an extension with few changes to core druid. Within the extension the authentication and authorization portions are also pretty self-contained. I'd rather keep this as one PR if possible since the features are closely related and to avoid the churn/overhead of having two PRs to go through the review process.

gianm · 2017-11-28T17:54:34Z

I'm reviewing this now.

jihoonson · 2017-12-05T05:35:07Z

@jon-wei ok. I'm also reviewing.

jihoonson

@jon-wei thank you for the nice work! I did a first round of review. Also please let me know what do you think about the below.

There is the sync issue between the coordinator and other nodes. So, an API to check the load status will be much helpful. See the lookup document as an example.
I think credential data is usually not much big. What do you think about adding a coordinator API to sync over the whole cluster immediately?

jihoonson · 2017-12-06T01:16:58Z

Looks unnecessary.

deleted, thanks

jihoonson · 2017-12-06T01:29:24Z

How does this relate to druid.auth.authenticator.MyBasicAuthenticator.initialInternalClientPassword?

It should be set to the same value, unless the user changes the password later on

jihoonson · 2017-12-06T01:33:12Z

What is the meaning of initial? Is it possible to change the password using other APIs?

What happens if the password is changed and then the cluster is restarted? The adminPassword is reset to the initial one?

initial is the password these users will have when the user is first automatically created.

The password can be changed afterwards and won't be reset after cluster restart, I added a comment about that.

jihoonson · 2017-12-06T01:36:39Z

Should these configurations added to common.runtime.properties?

Added a note about common.runtime.properties here

jihoonson · 2017-12-06T01:37:35Z

Same with initialAdminPassword.

jihoonson · 2017-12-06T12:54:52Z

+public class DefaultBasicAuthorizerResourceHandler implements BasicAuthorizerResourceHandler
+{
+  private static final Logger log = new Logger(DefaultBasicAuthorizerResourceHandler.class);
+  private static final Response NOT_FOUND_RESPONSE = Response.status(Response.Status.NOT_FOUND).build();


Maybe FORBIDDEN

jihoonson · 2017-12-06T12:55:45Z

+      this.resourceNamePattern = Pattern.compile(resourceAction.getResource().getName());
+    }
+    catch (PatternSyntaxException pse) {
+      throw new BasicSecurityDBResourceException(


pse should be included.

Included exception

jihoonson · 2017-12-06T12:56:14Z

+
+    BasicAuthorizerPermission that = (BasicAuthorizerPermission) o;
+
+    if (getResourceAction() != null


You can use Objects.equals().

This was autogenerated by IntelliJ

jihoonson · 2017-12-06T12:56:24Z

+  public int hashCode()
+  {
+    int result = getResourceAction() != null ? getResourceAction().hashCode() : 0;
+    result = 31 * result + (getResourceNamePattern().pattern() != null


You can use Objects.hash().

Also autogenerated, they're not used outside of tests

jihoonson · 2017-12-06T12:59:48Z

+    connector = derbyConnectorRule.getConnector();
+    tablesConfig = derbyConnectorRule.metadataTablesConfigSupplier().get();
+    connector.createConfigTable();
+    //injector = setupInjector();


Please remove this line.

removed line

gianm · 2017-12-06T23:18:18Z

Thanks @jon-wei for working on the extension and @jihoonson for reviewing.

Beyond what @jihoonson has said (especially an API to check the load status -- it's always helpful) I think we should cache the credential db on the local disk of each node, so when they start up, they can use their cache if the coordinator is not accessible. Maybe they should use their cache if they can't contact the coordinator within some amount of time. That way the coordinator being down doesn't torch the cluster completely.

jon-wei · 2017-12-09T00:48:14Z

@jihoonson @gianm

Thanks for the reviews, I addressed the comments and added the following features:

Disk snapshots of the caches, these will be used during initialization if a service cannot sync with the coordinator
A "refresh all" cache syncing API on the coordinator
loadStatus API (I also added "/db/" to some of the API paths to prevent path conflicts")

jihoonson

@jon-wei thanks for the update. Left more comments.

jihoonson · 2017-12-12T09:42:47Z

+      return objectMapper.writeValueAsBytes(userMap);
+    }
+    catch (IOException ioe) {
+      throw new ISE("WTF? Couldn't serialize userMap!");


Please add ioe to the exception.

jihoonson · 2017-12-12T09:44:33Z

+      return objectMapper.writeValueAsBytes(userMap);
+    }
+    catch (IOException ioe) {
+      throw new ISE("WTF? Couldn't serialize userMap!");


Please add ioe to the exception.

jihoonson · 2017-12-12T09:45:03Z

+      return objectMapper.writeValueAsBytes(roleMap);
+    }
+    catch (IOException ioe) {
+      throw new ISE("WTF? Couldn't serialize roleMap!");


Please add ioe to the exception.

jihoonson · 2017-12-12T11:56:03Z

+  private final EmittingLogger logger;
+  private final String threadName;
+
+  private Thread notifierThread;


Thread should be stopped on stop(). Suggest to use ExecutorService for easy maintenance.

Added stop(), changed to use ExecutorService

jihoonson · 2017-12-12T12:05:41Z

+    synchronized (itemsToUpdate) {
+      itemsToUpdate.add(updatedItemName);
+      serializedMaps.put(updatedItemName, updatedItemData);
+      itemsToUpdate.notify();


It looks to the notifier notifies each update every time. Maybe it's better to collect updates for a given time (like 100 ms) and send them in bulk to reduce API calls. I'm not sure about this because I guess credentials are not frequently updated.

If you want to keep the current behavior, we don't have to keep itemsToUpdate and serializedMaps because there will be only a single element in them. Instead, I think it's better to use AtomicReference<Pair<String, byte[]>> or simply put a Pair<String, byte[]> to the blockingQueue if you decide to use it.

I think the current behavior is okay for now, since the updates should be pretty infrequent, I changed this to use Pair<String, byte[]> and a BlockingQueue

jihoonson · 2017-12-12T14:00:04Z

+  )
+  {
+    this.storageUpdater = storageUpdater;
+    this.objectMapper = new ObjectMapper(new SmileFactory());


Looks that objectMapper can be injected using @Smile annotation.

Changed to use @Smile annotation

jihoonson · 2017-12-12T14:04:16Z

+      }
+    }
+    catch (Exception e) {
+      LOG.makeAlert(e, "WTF? Could not deserialize user/role map received from coordinator.").emit();


Is this intended to does nothing for all kinds of exceptions but emitting an alert?

Yes, see the previous comment

jihoonson · 2017-12-12T14:05:20Z

+  }
+
+  @Nullable
+  private UserAndRoleMap loadUserAndRoleMapFromDisk(String prefix) throws Exception


Can be reduced to throws IOException.

Reduced to IOException

jihoonson · 2017-12-12T14:05:43Z

+    );
+  }
+
+  private void writeMapToDisk(String prefix, byte[] userMapBytes) throws Exception


Can be reduced to IOException.

Reduced to IOException

jihoonson · 2017-12-12T14:06:03Z

+    File cacheDir = new File(commonCacheConfig.getCacheDirectory());
+    cacheDir.mkdirs();
+    File userMapFile = new File(commonCacheConfig.getCacheDirectory(), getUserRoleMapFilename(prefix));
+    Files.write(userMapBytes, userMapFile);


Similar comment. What if the write fails?

jihoonson · 2017-12-14T01:38:24Z

+  private final Map<String, BasicAuthDBConfig> itemConfigMap;
+  private final String baseUrl;
+  private final EmittingLogger logger;
+  private final String threadName;


Unused variable.

jihoonson · 2017-12-14T01:42:59Z

+      HttpClient httpClient,
+      String baseUrl,
+      String threadName,
+      EmittingLogger logger


Ah it's needed. But I'm still concerned with that it's difficult to find where the logging code is from the log if the logging class is different from the actual class. What do you think about passing a caller name in constructor and adding it to all logs?

jihoonson · 2017-12-14T01:48:58Z

+            return loadUserMapFromDisk(prefix);
+          }
+          catch (Exception e2) {
+            LOG.makeAlert(e2, "Encountered exception while loading user map snapshot for authenticator [%s]", prefix);


Please add e to e2 like e2.addSuppressed(e).

jihoonson · 2017-12-14T01:50:05Z

+            return loadUserAndRoleMapFromDisk(prefix);
+          }
+          catch (Exception e2) {
+            LOG.makeAlert(e2, "Encountered exception while loading user-role map snapshot for authorizer [%s]", prefix);


Please add e to e2.

jihoonson · 2017-12-14T03:41:36Z

@jon-wei thanks for the quick update! +1 after CI.

jon-wei · 2017-12-14T07:49:47Z

@jihoonson thanks for the review!

@JsonProperty

* Basic auth extension * Add auth configuration integration test * Fix missing authorizerName property * PR comments * Fix missing @JsonProperty annotation * PR comments * more PR comments

leventov · 2018-09-25T11:11:01Z

+{
+  private static final EmittingLogger LOG = new EmittingLogger(CommonCacheNotifier.class);
+
+  private static final List<String> NODE_TYPES = Arrays.asList(


@jon-wei does it purposely skip the Coordinator type?

@leventov Yes, the coordinator gets its information about the auth state directly from metadata storage, this update notification is for other service types

zhouyuanchao · 2018-10-31T09:29:24Z

Why basic security can't use EnvironmentVariablePasswordProvider, such as initialAdminPassword?

jon-wei · 2018-10-31T17:10:37Z

@zhouyuanchao It can, check the master branch

zhouyuanchao · 2018-11-21T02:49:41Z

@jon-wei internalClientPassword can not use EnvironmentVariablePasswordProvider in master branch

jon-wei added Feature Security labels Nov 17, 2017

jon-wei mentioned this pull request Nov 17, 2017

Basic auth extension #4823

Closed

jon-wei force-pushed the new_basic_sec2 branch 2 times, most recently from 754b0b3 to b3557b7 Compare November 17, 2017 02:34

jon-wei force-pushed the new_basic_sec2 branch from f077814 to 82f2e28 Compare November 20, 2017 23:07

jon-wei added 3 commits December 5, 2017 20:30

Basic auth extension

4c5f606

Add auth configuration integration test

6c8c4f1

Fix missing authorizerName property

ac419d3

jon-wei force-pushed the new_basic_sec2 branch from bc64372 to ac419d3 Compare December 6, 2017 04:31

jihoonson requested changes Dec 6, 2017

View reviewed changes

jon-wei force-pushed the new_basic_sec2 branch 4 times, most recently from 1301600 to c937dc8 Compare December 9, 2017 00:08

jon-wei force-pushed the new_basic_sec2 branch from c937dc8 to 4f762cd Compare December 9, 2017 00:50

PR comments

1b1f022

jon-wei force-pushed the new_basic_sec2 branch from 4f762cd to 1b1f022 Compare December 9, 2017 01:10

Fix missing @JsonProperty annotation

7ae438f

jihoonson reviewed Dec 12, 2017

View reviewed changes

PR comments

ff244cc

jon-wei force-pushed the new_basic_sec2 branch from 12e5a0e to a8ae159 Compare December 12, 2017 22:41

Merge remote-tracking branch 'upstream/master' into new_basic_sec2

08a4e8f

jon-wei force-pushed the new_basic_sec2 branch from a8ae159 to 08a4e8f Compare December 13, 2017 01:15

jihoonson reviewed Dec 14, 2017

View reviewed changes

more PR comments

895ce3e

jon-wei force-pushed the new_basic_sec2 branch from d81f604 to 895ce3e Compare December 14, 2017 02:13

jihoonson approved these changes Dec 14, 2017

View reviewed changes

jon-wei merged commit f48c9d7 into apache:master Dec 14, 2017

jon-wei added this to the 0.12.0 milestone Jan 5, 2018

jon-wei mentioned this pull request Jan 5, 2018

[WIP] Druid 0.12.0 release notes #5211

Closed

leventov reviewed Sep 25, 2018

View reviewed changes

leventov mentioned this pull request Sep 25, 2018

Use NodeType enum instead of Strings #6377

Merged


		BasicAuthorizerPermission that = (BasicAuthorizerPermission) o;

		if (getResourceAction() != null

Conversation

jon-wei commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jihoonson commented Nov 19, 2017

Uh oh!

jon-wei commented Nov 20, 2017

Uh oh!

gianm commented Nov 28, 2017

Uh oh!

jihoonson commented Dec 5, 2017

Uh oh!

jihoonson left a comment • 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!

gianm commented Dec 6, 2017

Uh oh!

jon-wei commented Dec 9, 2017

Uh oh!

jihoonson left a comment

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

jon-wei commented Nov 17, 2017 •

edited

Loading

jihoonson left a comment •

edited

Loading