Support LDAP authentication/authorization by mohammadjkhan · Pull Request #6972 · apache/druid

mohammadjkhan · 2019-01-31T19:19:08Z

Proposal for LDAP authentication/authorization within Druid

Issues/limitations with the existing Druid Basic Security extension:

Inability to authenticate requests using basic authentication with LDAP as the credentials store. Basic Security extension limits clients to use the internal database as the only credentials store option.
Basic Security extension does more than just providing the mechanism to transport/process credentials supplied in http requests
Basic Security extension authorization limits clients to use internal database as the only source to manage and lookup user roles. Inability to retrieve user role information from an external source like LDAP. Inability to map user groups, retrieved from LDAP for example, to roles.

Goals:

Expose the ability to authenticate HTTP requests with basic authentication using LDAP as the credentials store to validate against.
Refactor the existing basic security extension authentication and authorization implementation to be a bit more pluggable (database vs ldap or something else, with database being the default)
Expose a LDAP role-based authorizer that allows druid users to be authorized by enumerating user group/s fetched in LDAP, and group/s to role mappings configured in the internal database

Proposal:

Refactor the existing basic security extension authenticator to make it a bit more pluggable/configurable as to how to validate requests with basic authentication credentials. Abstract out a CredentialsValidator interface for use by BasicHTTPAuthenticator that will be used to select and configure the credential store/s to use (database, ldap, etc).
CredentialsValidator interface will expose a validate method that takes a username and password and give you back whether a user is valid (return an AuthenticationResult).
Support multiple credential validators. Authenticate a local user to the internal database and a ldap user at the same time, it’s common to have a local user in the database that’s used as the system user within the cluster, and ldap user for external access.
Refactor basis security extension authorizer (BasicRoleBasedAuthroizer) and make it configurable for multiple sources of truth for assigning roles to users/groups
Provide the ability to assign and lookup roles to groups instead of just users in the database. Check local database first for user permissions. If not, then check ldap and enumerate all groups and then follow set of rules dynamically configured in database for how to map those set of groups on to set of roles
Groups to role mappings in the database will be stored similar to the current structure of how user to role mappings are stored.

Fixes #6416

gianm · 2019-01-31T22:56:44Z

Hi @mohammadjkhan - thanks for the contribution!!

We are in the midst of discussing a template for proposals, that we haven't settled on yet but it seems that we will be able to soon. Would you mind writing one up to help reviewers understand what your patch is doing?

I don't know what the final template will be, so feel free to make up your own template for now. Or if you want a suggested template, try this one:

Motivation: Describe the problem and why you want to solve it.

Proposed changes: Describe your solution to the problem. This may be fairly extensive or it may just be a sentence or two, depending on the scope of the change. This should include any changes that are proposed to user-facing interfaces (configuration parameters, JSON query/ingest specs, SQL language, emitted metrics, and so on).

Rationale: Discuss why your proposed solution is the best one. The goal of this section is to help people understand why this is the best solution now, and also to prevent churn in the future when old alternatives are reconsidered. This should include any alternative solutions that you considered and decided against.

Operational impact: Is anything going to be deprecated or removed by this change? Is there a migration path that cluster operators need to be aware of? Will there be any effect on the ability to do a rolling upgrade, or to do a rolling downgrade if an operator wants to switch back to a previous version?

Future work: A discussion of things that you believe are out of scope for the particular proposal but would be nice follow-ups. It helps show where a particular change could be leading us. There isn't any commitment that you will actually work on this stuff, and it's okay if this section is empty.

gianm · 2019-01-31T23:19:26Z

Oops, nevermind, you did it already!!!

I just saw it in: #6416. Thanks, I'll go check that out. In the meantime, it looks like the integration tests are having trouble passing if you would like to take a look at that.

mohammadjkhan · 2019-02-06T15:33:34Z

Thanks @gianm. I'm taking a look at failing integration tests right now. I see them falling for me with the same errors/issue even for the master branch that doesn't have my changes. I'm following steps described in integration-tests and executing mvn verify -P integration-tests under the integration-tests module (with docker installed on my machine).

nishantmonu51 · 2019-02-06T15:51:57Z

A very useful feature, Thanks for the contribution @mohammadjkhan
Can you add a description to the PR and also link to the proposal there.

mohammadjkhan · 2019-02-07T18:11:02Z

I'm also looking into the Travis CI build errors related to the druid-security module

mohammadjkhan · 2019-02-21T22:17:53Z

I unknowingly did pull -rebase on branch 6972 from upstream master while my PR was open, that resulted in my PR tracking other/unrelated changes from upstream master. So I had to reset and force push to clean it up.

# Conflicts: # docs/content/development/extensions-core/druid-basic-security.md

mohammadjkhan · 2019-03-20T02:01:46Z

Hi @jon-wei
I'm following up to see if you had the chance to look and review the changes in this PR yet? It's currently assigned to you for review.

Thanks,
Mohammad

jon-wei · 2019-03-20T02:17:05Z

@mohammadjkhan Sorry for the delay, I will start reviewing this week.

jon-wei · 2019-03-27T20:57:36Z

I am still reviewing and testing out this patch, I will have more comments on it this week.

nishantmonu51 · 2019-03-28T16:30:34Z

  {
-    cacheNotifier = new CommonCacheNotifier(
+    userCacheNotifier = new CommonCacheNotifier(
        initAuthenticatorConfigMap(authenticatorMapper),


should this be initAuthenticatorUserMap instead of configMap ?

No, initAuthenticatorConfigMap has nothing to do with user map per say. initAuthenticatorConfigMap is unchanged and just returns the map of authenticator prefix and authenticator configuration (BasicAuthDBConfig) that the CommonCacheNotifier (userCacheNotifier) needs too reference configuration properties like isEnableCacheNotifications

nishantmonu51

did an initial pass through the code, left few comments, Havn't tested the feature myself yet.

few more general comments, not a blocker for this PR and can also be done in subsequent PRs -

Would be nice to add a tutorial on how to get this feature working would be helpful for new users.
Druid has integration-tests that run on docker containers, would like to see some basic test for ldap security added as well, so as to ensure this feature does not break with subsequent changes.

nishantmonu51 · 2019-03-28T19:14:14Z

+  private final Set<BasicAuthorizerRole> roles;
+
+  @JsonCreator
+  public BasicAuthorizerGroupMappingFull(


this class looks almost duplicate to BasicAuthorizerGroupMapping,
Can we remove duplicates ? Or are there any differences ?

I modeled BasicAuthorizerGroupMapping and BasicAuthorizerGroupMappingFull entities similar to already existing BasicAuthorizerUser and BasicAuthorizerUserFull entities.
BasicAuthorizerGroupMapping contains a set of role names whereas BasicAuthorizerGroupMappingFull contains a set of role objects, which contains the role name and list of permission objects associated with the role name.
BasicAuthorizerResource getUser and getGroupMapping rest endpoints allows for query parameter "full" that's used to decide whether to just return the role names or the "full" role object in the response.
Please let me know what you think.

jon-wei · 2019-03-28T21:28:54Z

@mohammadjkhan @nishantmonu51

Given that the metadata store-backed and LDAP implementations have such different configurations, I would structure this as follows:

Create an abstract PasswordBasedAuthenticator, and override the credentials validation in separate BasicHTTPAuthenticator and LDAPAuthenticator classes
Create an abstract RoleBasedAuthorizer, and override the rolemap calculation in separate classes for metadata-backed and LDAP-backed authorizer (The PR currently preferentially checks the metadata-backed store and then LDAP, I think these should be separate since the incoming request would be authenticated against only one or the other)

jon-wei · 2019-04-05T00:46:42Z

@mohammadjkhan

Regarding my earlier comment (#6972 (comment)), after more thought I now feel it would be better to split LDAP into a separate contrib extension and leave the existing basic auth extension unchanged.

The authenticator only shares the minimal "check password" logic
The authorizer is currently written to shared the role definitions with the basic auth extension, but I think it would be better to have LDAP be the sole source of truth for users/groups/roles/permissions for the LDAP implementation, instead of a mixed model where some information is kept in the Druid metadata store.
I think it's possible that the LDAP implementations will evolve over time with more features (maybe some are very specific to LDAP), and separating it into its own extension will give us more freedom to build upon it without affecting the basic auth extension.

mohammadjkhan · 2019-04-11T19:21:19Z

@jon-wei

LDAP is supposed to be source of truth for users/groups and not roles/permissions. The standard way to implement LDAP is to manage roles and permissions at the application/framework level as rules for managing roles/permissions can (and most likely will) be different from one application to another within an enterprise, and so they aren't managed within LDAP. Also, they are not 1:1. So it's normal/common to have a mix model of where some information is kept/managed at the application level and some information is stored in LDAP
The basic authenticator shares logic related to how basic auth credentials are retrieved from the http request; how those credentials are authenticated can be different, and the CredentialsValidator interface now provides a way support for more ways of validating basic auth credentials down the road. As described in our proposal the current basic security extension limits clients to use druid internal metadata store for authentication, and one of the goals/proposals is to refactor basic security extension authenticator to make it a bit more pluggable/configurable as to how to validate requests with basic authentication credentials. Moving in to separate contrib extension will result in duplication of logic related to how basic auth credentials are processed from the http request.
How druid authorizes a user in a basic auth credential request can be managed the same way regardless of how they got authenticated (db, ldap, etc). Basically, they differ in authentication methods, but not authorization. Also, it provides a way to reuse roles/permissions. For instance, an admin user and a user belonging to an admin group can be mapped to same role in the metastore. So, it makes sense to share the role definitions
If LDAP implementations evolve over time, the changes will be only limited to LDAPCredentialsValidator in how we interact with ldap and authenticate the user. How we retrieve/process basic auth credentails from the http request and authorize the user should stay the same. I think the druid basic auth security module as a whole will be evolved over time to support more ways of authenticating HTTP Basic Authentication requests, not just against db or ldap. How Spring security has achieved that is a really good example.
Some of the design decisions were adopted to preserve backward compatibility and not introduce any breaking changes. Leaving the BasicHTTPAuthenticator mostly as is to continue leveraging and make use of the "basic" json type name and introduced CredentialsValidator as a way authenticate a basic auth credential request either against db or ldap, default is db

jon-wei · 2019-09-11T00:24:15Z

    this.enableCacheNotifications = enableCacheNotifications;
    this.cacheNotificationTimeout = cacheNotificationTimeout;
-    this.iterations = iterations;
+    this.iterations = credentialIterations;


nit: Suggest renaming iterations and its getter as well here

Done, renamed

jon-wei · 2019-09-11T00:55:31Z

+    this.itemConfig = null;
+  }
+
+  public CommonCacheNotifier(


This constructor is unused

Done, removed

jon-wei · 2019-09-11T00:58:20Z

  }

-  private List<ListenableFuture<StatusResponseHolder>> sendUpdate(String updatedAuthorizerPrefix, byte[] serializedUserMap)
+  public void addUpdate(byte[] updatedItemData)


This addUpdate is unused

Done, removed

jon-wei · 2019-09-11T00:59:35Z

-              BasicAuthDBConfig authorizerConfig = itemConfigMap.get(update.lhs);
-              if (!authorizerConfig.isEnableCacheNotifications()) {
+
+              BasicAuthDBConfig dbConfig = itemConfigMap == null ? itemConfig : itemConfigMap.get(update.lhs);


This line should be reverted back to BasicAuthDBConfig authorizerConfig = itemConfigMap.get(update.lhs); since the constructor that sets itemConfig is not used anymore

Done, reverted

jon-wei · 2019-09-11T01:00:09Z

-                  serializedMap
-              );
+              List<ListenableFuture<StatusResponseHolder>> futures;
+              if (authorizer != null) {


The authorizer will always be non-null here, the addUpdate method that would have allowed it to be null is not used

Done, updated

jon-wei · 2019-09-11T01:00:59Z

  }

-  private URL getListenerURL(DruidNode druidNode, String baseUrl, String itemName)
+  private List<ListenableFuture<StatusResponseHolder>> sendUpdate(byte[] serializedEntity)


This sendUpdate method should be removed as a result of deleting the unused codepaths in this class

Done, removed

jon-wei · 2019-09-11T01:21:34Z

+import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
+
+public class BasicAuthenticatorUserPrincipal implements Principal


Suggest LdapUserPrincipal classname instead since only the LDAP validator uses this

Done, renamed

jon-wei · 2019-09-11T01:23:30Z

+  {
+    long now = System.currentTimeMillis();
+    long cutoff = now - (duration * 1000L);
+    if (this.lastVerified.get().toEpochMilli() < cutoff) {


From the docs, the maxDuration expiring should override the result of any last verified checks, the maxCutoff check should always happen and only if it passes should last verified time be checked

Done, fixed. Changes are in the new renamed LdapUserPrincipal class

jon-wei · 2019-09-11T01:27:01Z

+
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = DBCredentialsValidator.class)
+@JsonSubTypes(value = {
+    @JsonSubTypes.Type(name = "db", value = DBCredentialsValidator.class),


Suggest renaming the type here to metadata and the credentials validator to MetadataStoreCredentialsValidator

Done, renamed

jon-wei · 2019-09-11T01:28:29Z

+  {
+    Set<LdapName> groups;
+    LdapName userDn;
+    Map<String, Object> contexMap = new HashMap<>();


contexMap -> contextMap

Done, renamed

jon-wei · 2019-09-11T01:34:16Z

+            return null;
+          }
+          userDn = new LdapName(userResult.getNameInNamespace());
+          groups = getGroupsFromLdap(this.ldapConfig, userResult);


I think the group lookup should be moved to the LDAPRoleProvider, since the groups aren't needed to validate the user/password

Done. This change required a bit more work/effort than I had expected but I agree that moving it to the LDAPRoleProvider makes more sense. Now there's no group related properties or operations within LDAPCredentialsValidator
With this change I also had to move the groupFilters property to ldap authorizer
druid.auth.authorizer.MyBasicLDAPAuthorizer.roleProvider.groupFilters

jon-wei · 2019-09-11T01:37:08Z

+import java.util.Set;
+
+@JsonTypeName("db")
+public class DBRoleProvider implements RoleProvider


Suggest renaming to MetadataStoreRoleProvider

Done. renamed

jon-wei · 2019-09-11T02:06:57Z

@mohammadjkhan I'm done reviewing, can you fix conflicts and address comments? Thanks!

# Conflicts: # docs/development/extensions-core/druid-basic-security.md

lgtm-com · 2019-09-12T19:16:52Z

This pull request introduces 1 alert when merging 3db3c05 into 75978e5 - view on LGTM.com

new alerts:

1 for Information exposure through a stack trace

jon-wei · 2019-09-20T23:16:54Z

@mohammadjkhan Just checking in again, I think this is ready to merge after final round of comments are addressed. I'm really looking forward to Druid finally supporting LDAP!

asdf2014 · 2019-09-21T02:04:49Z

@mohammadjkhan Please help us fix this new LGTM alert to maintain the A+ quality.

mohammadjkhan · 2019-09-24T18:29:39Z

@jon-wei @asdf2014 Sounds great! Thank you! I'll update the PR with the final round of comments and the LGTM alert today and tomorrow and notify you guys. Thanks

nishantmonu51 · 2019-10-03T12:39:53Z

@mohammadjkhan : Any updates here ? were you able to look into final review comments ?

lgtm-com · 2019-10-07T16:01:16Z

This pull request introduces 1 alert when merging 0bca543 into 1d42551 - view on LGTM.com

new alerts:

1 for Information exposure through a stack trace

jon-wei · 2019-10-07T18:38:47Z

@mohammadjkhan CI is failing on spellcheck, can you add exclusions to https://github.com/apache/incubator-druid/blob/master/website/.spelling, and can you address the 1 new alert from LGTM?

lgtm-com · 2019-10-07T20:08:50Z

This pull request introduces 1 alert when merging d4a051d into 1d42551 - view on LGTM.com

new alerts:

1 for Information exposure through a stack trace

nishantmonu51

LGTM, 👍 after travis.

…aced DB with metadata store

mohammadjkhan · 2019-10-08T12:16:06Z

@jon-wei @asdf2014 @nishantmonu51 fixed spellcheck and LGTM alerts. All requested changes/updates are now complete. Thank you.

jon-wei · 2019-10-09T00:08:01Z

LGTM, thanks for the contribution!

mohammadjkhan · 2019-10-09T14:05:40Z

Thanks @jon-wei, @nishantmonu51, and @gianm; I really appreciate all your guys help, support, and direction with introducing this feature, and I hope the Druid community makes the most of it and gets benefited from it!

* Support LDAP authentication/authorization * fixed integration-tests * fixed Travis CI build errors related to druid-security module * fixed failing test * fixed failing test header * added comments, force build * fixes for strict compilation spotbugs checks * removed authenticator rolling credential update feature * removed escalator rolling credential update feature * fixed teamcity inspection deprecated API usage error * fixed checkstyle execution error, removed unused import * removed cached config as part of removing authenticator rolling credential update feature * removed config bundle entity as part of removing authenticator rolling credential update feature * refactored ldao configuration * added support for SSLContext configuration and TLSCertificateChecker * removed check to return authentication failure when user has no group assigned, will be checked and handled by the authorizer * Separate out authorizer checks between metadata-backed store user and LDAP user/groups * refactored BasicSecuritySSLSocketFactory usage to fix strict compilation spotbugs checks * fixes build issue * final review comments updates * final review comments updates * fixed LGTM and spellcheck alerts * Fixed Avatica auth failure error message check * Updated metadata credentials validator exception message string, replaced DB with metadata store

gianm assigned jon-wei Feb 19, 2019

mohammadjkhan closed this Feb 21, 2019

mohammadjkhan force-pushed the PR6416 branch from 9729492 to 417b9f2 Compare February 21, 2019 21:51

mohammadjkhan added 2 commits February 21, 2019 15:53

Support LDAP authentication/authorization

042766f

fixed integration-tests

72eb8e9

mohammadjkhan reopened this Feb 21, 2019

mohammadjkhan closed this Feb 21, 2019

mohammadjkhan reopened this Feb 21, 2019

mohammadjkhan added 5 commits February 22, 2019 11:53

fixed Travis CI build errors related to druid-security module

45f7f4f

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

1da3cab

# Conflicts: # docs/content/development/extensions-core/druid-basic-security.md

Merge branch 'master' into PR6416

e2f2e05

fixed failing test

a314ca4

fixed failing test header

1791c93

jon-wei reviewed Mar 22, 2019

View reviewed changes

Comment thread ...ic-security/src/main/java/org/apache/druid/security/basic/BasicSecuritySSLSocketFactory.java Outdated

nishantmonu51 reviewed Mar 28, 2019

View reviewed changes

jon-wei reviewed Mar 28, 2019

View reviewed changes

Comment thread .../java/org/apache/druid/security/basic/authentication/validator/LDAPCredentialsValidator.java Outdated

jon-wei reviewed Mar 29, 2019

View reviewed changes

Comment thread docs/content/development/extensions-core/druid-basic-security.md Outdated

jon-wei reviewed Sep 11, 2019

View reviewed changes

mohammadjkhan added 2 commits September 12, 2019 10:43

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

cb61783

# Conflicts: # docs/development/extensions-core/druid-basic-security.md

fixes build issue

3db3c05

mohammadjkhan added 2 commits October 7, 2019 09:49

final review comments updates

1c18bce

final review comments updates

0bca543

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

d4a051d

fixed LGTM and spellcheck alerts

98fa29d

nishantmonu51 approved these changes Oct 7, 2019

View reviewed changes

mohammadjkhan added 2 commits October 7, 2019 23:14

Fixed Avatica auth failure error message check

ba038a5

Updated metadata credentials validator exception message string, repl…

d5d0073

…aced DB with metadata store

jon-wei approved these changes Oct 9, 2019

View reviewed changes

jon-wei merged commit 18758f5 into apache:master Oct 9, 2019

jon-wei added this to the 0.17.0 milestone Dec 17, 2019

jon-wei added the Release Notes label Dec 18, 2019

jon-wei mentioned this pull request Dec 28, 2019

0.17.0 release notes #9066

Closed

jon-wei mentioned this pull request Feb 12, 2020

Fix basic auth polling to skip retries when cachedSerializedGroupMappingMap returns 404 #9354

Merged

8 tasks

Conversation

mohammadjkhan commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gianm commented Jan 31, 2019

Uh oh!

gianm commented Jan 31, 2019

Uh oh!

mohammadjkhan commented Feb 6, 2019

Uh oh!

nishantmonu51 commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohammadjkhan commented Feb 7, 2019

Uh oh!

mohammadjkhan commented Feb 21, 2019

Uh oh!

mohammadjkhan commented Mar 20, 2019

Uh oh!

jon-wei commented Mar 20, 2019

Uh oh!

Uh oh!

jon-wei commented Mar 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohammadjkhan Mar 28, 2019 • 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!

Uh oh!

Uh oh!

Uh oh!

nishantmonu51 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

jon-wei commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jon-wei commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohammadjkhan commented Apr 11, 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!

mohammadjkhan commented Jan 31, 2019 •

edited

Loading

nishantmonu51 commented Feb 6, 2019 •

edited

Loading

mohammadjkhan Mar 28, 2019 •

edited

Loading

jon-wei commented Mar 28, 2019 •

edited

Loading

jon-wei commented Apr 5, 2019 •

edited

Loading

mohammadjkhan Oct 7, 2019 •

edited

Loading

mohammadjkhan Oct 7, 2019 •

edited

Loading

mohammadjkhan Oct 7, 2019 •

edited

Loading

mohammadjkhan Oct 7, 2019 •

edited

Loading

mohammadjkhan Oct 7, 2019 •

edited

Loading

mohammadjkhan Oct 7, 2019 •

edited

Loading