dengzhhu653 commented on code in PR #4774:
URL: https://github.com/apache/hive/pull/4774#discussion_r1349617707
##########
service/src/java/org/apache/hive/service/cli/thrift/ThriftHttpServlet.java:
##########
@@ -283,7 +283,7 @@ protected void doPost(HttpServletRequest request,
HttpServletResponse response)
}
// Send a 401 to the client
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
- if(isAuthTypeEnabled(request, HiveAuthConstants.AuthTypes.KERBEROS)) {
+ if(authType.isEnabled(HiveAuthConstants.AuthTypes.KERBEROS)) {
Review Comment:
Agreed that we should add the Negotiate challenge to the WWW-Authenticate
header in case of Kerberos auth and the reactive authentication, things change
a bit after the HS2 can support Kerberos mixing with other auth methods, if the
invalid ldap Hive JDBC client(with wrong password) connects to the HS2
directly, we should not send `Negotiate` to the client.
> However, I didn't want to include support for the Basic challenge in this
PR because I don't see the consequences
Makes sense to me.
Back to this case(Knox), cloud we change the condition to
```
if (e instanceof HttpEmptyAuthenticationException &&
authType.isEnabled(HiveAuthConstants.AuthTypes.KERBEROS)) {
// add header
}
```
The `SpnegoAuthInterceptor` will go for Kerberos authentication only when
Knox enables the Kerberos, so if the HS2 is Kerberos based, the auth will be
success finally.
For other cases without the gateway, the `e` should not be an instance of
`HttpEmptyAuthenticationException`, so we don't add the extra header to the
response.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
