New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kerberos Spnego Authentication Router Issue #5706
Conversation
Change-Id: I872f9282fb60bfa20524271535980a36a87b9621
related to #5322 |
@@ -59,4 +68,21 @@ public AuthenticationResult createEscalatedAuthenticationResult() | |||
{ | |||
return new AuthenticationResult(internalClientPrincipal, authorizerName, null); | |||
} | |||
|
|||
@Override | |||
public void decorateProxyRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should be part of the Authenticator instead of the Escalator.
Suppose a user has Kerberos authentication running alongside another authentication scheme, and they wish to use that other authentication scheme for Druid's internal communications. In that case, the Escalator would be of the other authentication type, and this Kerberos code would not be executed.
I think you could indicate the name of the Authenticator that validated a request (maybe put it in the AuthenticationResult), and in AsyncQueryForwardingServlet, get an injected AuthenticatorMapper and use the Authenticator name to lookup which Authenticator instance should be used to decorate the proxy request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jon-wei Thanks good point was not aware of that. Will use that map that has the chain and call decorate on each one of them, that should work i guess.
Change-Id: I7f94b9ff5ecf08e8abf7169b58bc410f33148448
Change-Id: I901543e52f0faf4666bfea6256a7c05593b1ae70
@jon-wei thanks please checkout this new version. |
@@ -313,6 +317,13 @@ protected void sendProxyRequest( | |||
// will log that on the remote node. | |||
clientRequest.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); | |||
|
|||
authenticatorMapper.getAuthenticatorChain() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would potentially "overdecorate" the request if decorateProxyRequest
is called for every Authenticator, suppose there are two authentication schemes that need request decoration but have mutually exclusive header fields, or some other conflict of that nature.
I would change AuthenticationResult to store the name of the Authenticator that created the AuthenticationResult, and read that name here to lookup the single Authenticator instance that authenticated the request, only that one should decorate the request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jon-wei am not sure what you mean by mutually exclusive headers? can you give an example for instance here am using cookies that can be redecorated by appending stuff not sure am getting this point?
I would change AuthenticationResult to store the name of the Authenticator that created the AuthenticationResult, and read that name here to lookup the single Authenticator instance that authenticated the request, only that one should decorate the request
From this i read there is only one AuthenticationResult
thus this implies that we only have one single authenticator even if the user has a chain of authenticators? is that correct ?
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure what you mean by mutually exclusive headers? can you give an example for instance here am using cookies that can be redecorated by appending stuff not sure am getting this point?
With just the Kerberos extension, it's not breaking anything, but across potential authentication implementations to me it's not unimaginable or unlikely that such an incompatibility could exist (maybe the proxy decoration for two different authentication schemes uses the same header field, but in an "overwrite" mode instead of "append").
From this i read there is only one AuthenticationResult thus this implies that we only have one single authenticator even if the user has a chain of authenticators? is that correct ?
Yes, the first authenticator that accepts the request as valid should be the one that's used for forwarded requests (the chain behavior is intended to be "stop at first successful authentication" (we already know who the user is now) and not "must succeed with all authentication schemes deployed in the chain").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That "stop at first successful authentication" behavior is enforced by AuthenticationWrappingFilter
which wraps all of the Authenticator filters
Change-Id: I052650de9cd02b4faefdbcdaf2332dd3b2966af5
Change-Id: I074d2933460165feeddb19352eac9bd0f96f42ca
@jon-wei thanks for all the feedback please check this out. |
Change-Id: Idb58e308f90db88224a06f3759114872165b24f5
@@ -37,6 +37,11 @@ | |||
*/ | |||
private final String authorizerName; | |||
|
|||
|
|||
/** | |||
* Name of authentiator whom created the results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authentiator -> authenticator
@@ -68,4 +75,9 @@ public String getAuthorizerName() | |||
{ | |||
return context; | |||
} | |||
|
|||
public String getAuthentiatedBy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getAuthentiatedBy -> getAuthenticatedBy
@@ -105,7 +105,7 @@ public AuthenticationResult authenticateJDBCContext(Map<String, Object> context) | |||
} | |||
|
|||
if (checkCredentials(user, password.toCharArray())) { | |||
return new AuthenticationResult(user, name, null); | |||
return new AuthenticationResult(user, name, name, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to:
return new AuthenticationResult(user, authorizerName, name, null);
It's not an error you introduced in this PR, but using "name" for the authorizerName parameter there is a bug
clientRequest, | ||
proxyResponse, | ||
proxyRequest | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you log an error here if the authenticator is not found?
Change-Id: I6801d49a05d5d8324406fc0280286954eb66db10
done |
Great, LGTM after CI |
@@ -57,6 +57,7 @@ public HttpClient createEscalatedClient(HttpClient baseClient) | |||
@Override | |||
public AuthenticationResult createEscalatedAuthenticationResult() | |||
{ | |||
return new AuthenticationResult(internalClientPrincipal, authorizerName, null); | |||
return new AuthenticationResult(internalClientPrincipal, authorizerName, "kerberos", null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user may not have authenticator configured by name "kerberos",
this should not be hardcoded
e.g for configs -
druid.auth.authenticatorChain=["MyKerberosAuthenticator"]
druid.auth.authenticator.MyKerberosAuthenticator.type=kerberos
this should be set to MyKerberosAuthenticator I think.
@@ -59,6 +59,6 @@ public HttpClient createEscalatedClient(HttpClient baseClient) | |||
@Override | |||
public AuthenticationResult createEscalatedAuthenticationResult() | |||
{ | |||
return new AuthenticationResult(internalClientUsername, authorizerName, null); | |||
return new AuthenticationResult(internalClientUsername, authorizerName, "basic", null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not hard code "basic" here ?
@@ -113,6 +122,7 @@ public KerberosAuthenticator( | |||
@JsonProperty("excludedPaths") List<String> excludedPaths, | |||
@JsonProperty("cookieSignatureSecret") String cookieSignatureSecret, | |||
@JsonProperty("authorizerName") String authorizerName, | |||
@JsonProperty("name") String name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new property - please add to docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is injected by the Jackson from the list of authenticators. user does not set this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good.
if (authenticationResult != null && authenticationResult.getAuthenticatedBy() != null) { | ||
Authenticator authenticator = authenticatorMapper.getAuthenticatorMap() | ||
.get(authenticationResult.getAuthenticatedBy()); | ||
if (authenticator != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am not wrong user needs to make sure that he defines a property like this for every authenticator -
druid.auth.authenticator..name=
If it possible to infer the name without making the user add this property ?
If not lets make sure we document this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is coming from the list of authenticators and injected at the Authenticator module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good.
Change-Id: I62c3ee763363781e52809ec912aafd50b8486b8e
@@ -59,6 +60,6 @@ public HttpClient createEscalatedClient(HttpClient baseClient) | |||
@Override | |||
public AuthenticationResult createEscalatedAuthenticationResult() | |||
{ | |||
return new AuthenticationResult(internalClientUsername, authorizerName, "basic", null); | |||
return new AuthenticationResult(internalClientUsername, authorizerName, BASIC, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Escalator" is really conceptually linked to one of the "Authenticator" implementations (the authentication used for internal communications has to be something that a configured Authenticator would accept), the interface was split for reasons described here (#5073)
I guess the way I would handle this now is to add something like a new "druid.escalator.authenticatorName" property that Escalator implementations can use to fill this AuthenticationResult property.
This createEscalatedAuthenticationResult() method is only used on the Broker for some internal SQL code paths (when the broker itself runs a segment metadata query and when it needs to use the SQL planner for a currently unfinished "views" feature, see DruidViewMacro), to allow it to reuse existing code paths without larger refactoring or introducing a special "skip authorization checks because I am the broker myself" code path. This value doesn't really matter in practice here, and maybe it'd be nice to get rid of the method altogether eventually, so I hadn't commented on it initially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jon-wei am not sure about adding a new property i don't think it the way to go it adds more confusion then benefits IMO. If i am getting this right there is one to one mapping between escalator type and authenticator name/type. Thus if the user selects/sets the property druid.escalator.type
it should be enough and the implementation of escalator with type X will set the appropriate name for the authenticator entity. How does this sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i am getting this right there is one to one mapping between escalator type and authenticator name/type.
Authenticator name/type are not necessarily correlated (I can name my kerberos authenticator "George" if I wanted).
There's also no restriction on having two Authenticators of the same type (maybe that makes sense for some hypothetical implementation where running two instances with a different "namespace" or similar concept makes sense).
So there is only a mapping between a specific escalator instance and an authenticator instance, which is why I suggest adding the property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum, i really feel like druid has way too much properties and i feel bad when i add another one to the zillion of properties...let me think more about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jon-wei since that escalated result is used only by druid and it is not suppose to be involved into the query path coming from a user will set that to null
i don't see why we need to decorate the proxied query using that result. What you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with having that null for this PR, can you add a comment explaining it (maybe link it to this comment thread)
Escalator. Change-Id: I4a675c372f59ebd8a8d19c61b85a1e4bf227a8ba
@jon-wei and @nishantmonu51 can you please take a QL on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, 👍
@@ -37,6 +37,15 @@ | |||
*/ | |||
private final String authorizerName; | |||
|
|||
|
|||
/** | |||
* Name of authenticator whom created the results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/whom/that/g
As discussed on the dev sync let's do this in 0.12.1 if it backports cleanly. |
* Adding decoration method to proxy servlet Change-Id: I872f9282fb60bfa20524271535980a36a87b9621 * moving the proxy request decoration to authenticators Change-Id: I7f94b9ff5ecf08e8abf7169b58bc410f33148448 * added docs Change-Id: I901543e52f0faf4666bfea6256a7c05593b1ae70 * use the authentication result to decorate request Change-Id: I052650de9cd02b4faefdbcdaf2332dd3b2966af5 * adding authenticated by name Change-Id: I074d2933460165feeddb19352eac9bd0f96f42ca * ensure that authenticator is not null Change-Id: Idb58e308f90db88224a06f3759114872165b24f5 * fix types and minor bug Change-Id: I6801d49a05d5d8324406fc0280286954eb66db10 * fix typo Change-Id: I390b12af74f44d760d0812a519125fbf0df4e97b * use actual type names Change-Id: I62c3ee763363781e52809ec912aafd50b8486b8e * set authenitcatedBy to null for AutheticationResults created by Escalator. Change-Id: I4a675c372f59ebd8a8d19c61b85a1e4bf227a8ba
* Adding decoration method to proxy servlet Change-Id: I872f9282fb60bfa20524271535980a36a87b9621 * moving the proxy request decoration to authenticators Change-Id: I7f94b9ff5ecf08e8abf7169b58bc410f33148448 * added docs Change-Id: I901543e52f0faf4666bfea6256a7c05593b1ae70 * use the authentication result to decorate request Change-Id: I052650de9cd02b4faefdbcdaf2332dd3b2966af5 * adding authenticated by name Change-Id: I074d2933460165feeddb19352eac9bd0f96f42ca * ensure that authenticator is not null Change-Id: Idb58e308f90db88224a06f3759114872165b24f5 * fix types and minor bug Change-Id: I6801d49a05d5d8324406fc0280286954eb66db10 * fix typo Change-Id: I390b12af74f44d760d0812a519125fbf0df4e97b * use actual type names Change-Id: I62c3ee763363781e52809ec912aafd50b8486b8e * set authenitcatedBy to null for AutheticationResults created by Escalator. Change-Id: I4a675c372f59ebd8a8d19c61b85a1e4bf227a8ba
* Adding decoration method to proxy servlet Change-Id: I872f9282fb60bfa20524271535980a36a87b9621 * moving the proxy request decoration to authenticators Change-Id: I7f94b9ff5ecf08e8abf7169b58bc410f33148448 * added docs Change-Id: I901543e52f0faf4666bfea6256a7c05593b1ae70 * use the authentication result to decorate request Change-Id: I052650de9cd02b4faefdbcdaf2332dd3b2966af5 * adding authenticated by name Change-Id: I074d2933460165feeddb19352eac9bd0f96f42ca * ensure that authenticator is not null Change-Id: Idb58e308f90db88224a06f3759114872165b24f5 * fix types and minor bug Change-Id: I6801d49a05d5d8324406fc0280286954eb66db10 * fix typo Change-Id: I390b12af74f44d760d0812a519125fbf0df4e97b * use actual type names Change-Id: I62c3ee763363781e52809ec912aafd50b8486b8e * set authenitcatedBy to null for AutheticationResults created by Escalator. Change-Id: I4a675c372f59ebd8a8d19c61b85a1e4bf227a8ba
Kerberos Spnego Auth Router Issue.
Current state of the Kerberos Auth module is broken if Router routes the query to a Broker on a different physical host.
Why?: During the Spnego Auth negotiation the ticket obtained to access the target service is tight to the hostname of the service in question.
This where for instance the Druid internal Http Client generates the challenge token
io.druid.security.kerberos.DruidKerberosUtil#kerberosChallenge
as you can see it is hostname based.Thus the Broker will not be able to decrypt such token and throw an
you can see the stack here
Fixing this:
Added a decoration method hook at the
io.druid.server.AsyncQueryForwardingServlet#sendProxyRequest
that gives the
EscalatorAuthenticator the chance to attache some Request Headers containing authentication tokens that can be used down the chain by Broker.For instance for the Kerberos Authentication module will be adding the cookie token generated by the early authentication chain.
This cookie is signed by a shared secret
druid.auth.authenticator.kerberos.cookieSignatureSecret
and tight the the original client Principal/Hostname thus no escalation of privilege at all.I don't think Basic Auth module needs such hook since the credentials are not tight to hostnames thus no change needed
More information about discussion can be found here
This change is