Split internal client escalation from Authenticator interface by jon-wei · Pull Request #5073 · apache/druid

jon-wei · 2017-11-10T19:48:23Z

While developing an extension for basic HTTP authentication, I encountered the following problem:

The Authenticator implementation needed to communicate with the coordinator during initialization, grabbing data from an HTTP endpoint, using a DruidLeaderClient. This led to circular dependency issues as the DruidLeaderClient needs an escalated HTTP client created by an Authenticator to successfully authenticate with the coordinator.

This PR splits the "send requests with privileged internal system user credentials" functionality out of the Authenticator interface into a new Escalator interface, to help avoid circular dependency issues when Authenticator extension implementations need to communicate with Druid cluster nodes.

gianm · 2017-11-13T21:21:27Z

@jon-wei there's a conflict here now.

jon-wei · 2017-11-13T21:27:13Z

I would like to get this into 0.11.0 as well, this isn't a "bug" but I think authenticator extension implementers are likely to run into similar circular dependency issues if they have intra-cluster communication requirements during authenticator initialization, and it would nice to have that addressed in the initial interface release

jon-wei · 2017-11-13T21:27:35Z

@gianm fixed conflict

gianm

👍 LGTM.

gianm · 2017-11-13T21:59:13Z

-      final AuthConfig authConfig,
      final AuthorizerMapper authorizerMapper,
-      final AuthenticatorMapper authenticatorMapper
+      final Escalator escalator


It's cool that what's needed here got more fine grained.

himanshug · 2017-11-13T23:07:09Z

+
+    for (String authenticator : authenticators) {
+      String typeProperty = StringUtils.format("druid.auth.authenticator.%s.type", authenticator);
+      if (escalatorType.equals(properties.getProperty(typeProperty))) {


why is it required that escalatorType must match one of the Authenticator types ? Also, does this mean Escalator impls and Authenticator impls must have same json type names ?

Actually, I think this is a good question -- do we really need to require that escalators match up with an authenticator?

I think there's no reason they should need to match exactly. It's plausible that an escalator doesn't line up totally with an authenticator. Imagine that the authenticator is a standard RBAC authenticator, and the escalator is some custom extension that pulls the internal user password from somewhere more secure than a config file.

Whatever authentication scheme an Escalator uses, it has to be supported by some Authenticator that's configured, so I impose that type matching restriction to conceptually link the implementations.

It could function without that restriction, but I don't see much reason to have them differ.

This does mean they must have the same JSON type names, I have a section in the doc that calls out this requirement:

The Escalator chosen for this property must have the same type as an Authenticator in `druid.auth.authenticationChain. Authenticator extension implementors must also provide a corresponding Escalator implementation with the same type name if they intend to use a particular authentication scheme for internal Druid communications.

I see, so technically its not necessary but you're imposing the restriction so that user thinks about Escalator and Authenticator impl together.
I think it would help extension writers if we put some comment on both Authenticator and Escalator interfaces that implementers must ensure that both Escalator and Authenticator implementations must have exactly same json type name. Its easy to miss from that line in the doc alone. thanks.

@gianm how did your comment do the time travel ? (it shows up as 2nd comment wherease it should be 4th comment)

@himanshug @gianm I removed the type matching enforcement

himanshug · 2017-11-13T23:08:25Z

- * getEscalatedClient() method.
- */
-public class AuthenticatorHttpClientWrapper
+public class AllowAllEscalator implements Escalator


this is more of "NoopEscalator" , but I think it is named that way to match name with corresponding Authenticator impl used on the target server.

renamed this to "NoopEscalator"

gianm

some additional comments.

gianm · 2017-11-13T23:09:55Z

+  {
+    String escalatorType = properties.getProperty("druid.escalator.type");
+    if (escalatorType == null) {
+      escalatorType = "allowAll";


This should be a constant (static final somewhere) and ideally all instances of the string "allowAll" would reference that.

Made "allowAll" a constant under AuthConfig

gianm · 2017-11-13T23:13:16Z

+
+    for (String authenticator : authenticators) {
+      String typeProperty = StringUtils.format("druid.auth.authenticator.%s.type", authenticator);
+      if (escalatorType.equals(properties.getProperty(typeProperty))) {


Actually, I think this is a good question -- do we really need to require that escalators match up with an authenticator?

I think there's no reason they should need to match exactly. It's plausible that an escalator doesn't line up totally with an authenticator. Imagine that the authenticator is a standard RBAC authenticator, and the escalator is some custom extension that pulls the internal user password from somewhere more secure than a config file.

himanshug · 2017-11-13T23:20:59Z

👍 once #5073 (comment) is resolved .

himanshug · 2017-11-14T01:23:46Z

 |--------|-----------|--------|--------|--------|
 |`druid.auth.authenticationChain`|JSON List of Strings|List of Authenticator type names|["allowAll"]|no|
-|`druid.escalator.type`|String|Type of the Escalator that should be used for internal Druid communications. This Escalator must have the same type as an Authenticator in `druid.auth.authenticationChain`.|"allowAll"|no|
+|`druid.escalator.type`|String|Type of the Escalator that should be used for internal Druid communications. This Escalator must have the same type as an Authenticator in `druid.auth.authenticationChain`.|"noop"|no|


"This Escalator..." line is not correct anymore.

whoops, thanks

…#5073) * Split internal client escalation from Authenticator interface * PR comments

…#5084) * Split internal client escalation from Authenticator interface * PR comments

jon-wei added Improvement Security Area - Extension labels Nov 10, 2017

jon-wei force-pushed the escalator branch from db464fa to a649931 Compare November 10, 2017 22:30

Split internal client escalation from Authenticator interface

c0855db

jon-wei force-pushed the escalator branch from a649931 to c0855db Compare November 13, 2017 21:25

jon-wei added this to the 0.11.0 milestone Nov 13, 2017

gianm approved these changes Nov 13, 2017

View reviewed changes

himanshug reviewed Nov 13, 2017

View reviewed changes

gianm reviewed Nov 13, 2017

View reviewed changes

PR comments

9de94c2

himanshug reviewed Nov 14, 2017

View reviewed changes

Merge with master, PR comment

9e07715

jon-wei merged commit 9ac150c into apache:master Nov 14, 2017

jon-wei added a commit to jon-wei/druid that referenced this pull request Nov 14, 2017

Split internal client escalation from Authenticator interface (apache…

5756256

…#5073) * Split internal client escalation from Authenticator interface * PR comments

jon-wei mentioned this pull request Nov 14, 2017

Split internal client escalation from Authenticator interface (#5073) #5084

Merged

jon-wei added a commit that referenced this pull request Nov 14, 2017

Split internal client escalation from Authenticator interface (#5073) (…

a40a0ba

…#5084) * Split internal client escalation from Authenticator interface * PR comments

jon-wei mentioned this pull request May 3, 2018

Kerberos Spnego Authentication Router Issue #5706

Merged

Conversation

jon-wei commented Nov 10, 2017

Uh oh!

gianm commented Nov 13, 2017

Uh oh!

jon-wei commented Nov 13, 2017

Uh oh!

jon-wei commented Nov 13, 2017

Uh oh!

gianm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug Nov 13, 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!

himanshug Nov 13, 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!

gianm 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!

himanshug commented Nov 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

himanshug Nov 13, 2017 •

edited

Loading

himanshug Nov 13, 2017 •

edited

Loading