You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2019/07/25 17:01:37 UTC

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13918


Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................

IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

IMPALA-8538 added support for http connections to the hs2 server along
with LDAP auth support. This patch adds support for authorizing with
Kerberos via SPNEGO.

Testing:
- Added a unit test that runs a minikdc and an Impala server against
  it. Currently disabled until curl is available in the toolchain.
- Tested manually on a real cluster with a full Kerberos setup and
  beeline.

Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/kerberos-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
11 files changed, 366 insertions(+), 91 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13918/1
-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4670/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 27 Jul 2019 00:22:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13918

to look at the new patch set (#7).

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................

IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

IMPALA-8538 added support for http connections to the hs2 server along
with LDAP auth support. This patch adds support for authorizing with
Kerberos via SPNEGO.

Testing:
- Added a unit test that runs a minikdc and an Impala server against
  it. Currently disabled until curl is available in the toolchain.
- Tested manually on a real cluster with a full Kerberos setup and
  beeline.

Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/kerberos-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
11 files changed, 395 insertions(+), 100 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13918/7
-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4037/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 22:20:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4042/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 23:35:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13918/3/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/3/be/src/transport/THttpServer.cpp@199
PS3, Line 199: ) total_negotiate_auth_failure_->Increment(1);
             :     }
> Right, my handling of !is_complete here is wrong and I'll fix it.
Actually, I guess what I just said didn't quite make sense. 

The real difference this block makes is that it allows us to, say, return a 'WWW-Authenticate: Basic' if we got a 'WWW-Authenticate: Negotiate' that wasn't valid (at least once I fix the is_complete handling). I think based on our discussion offline, though, that's not really desirable anyways.

So a simpler design is: if a valid auth method was specified, only call that callback. Otherwise, call all supported callbacks.



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 21:52:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 1:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/3999/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jul 2019 17:42:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 7:

> k, seems like this is good. Just to triple check, I think you
 > should run one manual test where you do add some code locally like:
 > 
 > if (!token.empty()) { token[5]++; }
 > 
 > and make sure that it doesn't allow connecting :)

done


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 27 Jul 2019 00:22:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................

IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

IMPALA-8538 added support for http connections to the hs2 server along
with LDAP auth support. This patch adds support for authorizing with
Kerberos via SPNEGO.

Testing:
- Added a unit test that runs a minikdc and an Impala server against
  it. Currently disabled until curl is available in the toolchain.
- Tested manually on a real cluster with a full Kerberos setup and
  beeline.

Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Reviewed-on: http://gerrit.cloudera.org:8080/13918
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/kerberos-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
11 files changed, 395 insertions(+), 100 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 9
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/rpc/authentication.cc@565
PS1, Line 565: connection_context->return_headers;
nit: std::move() around this


http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp@256
PS1, Line 256:     h << "WWW-Authenticate: Negotiate" << CRLF;
shouldn't we be using a Negotiate header which includes the token returned by gssapi here? I guess this will work for certain configurations where the server's initial challenge is empty, but I think there might be cases where this isn't the case.



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jul 2019 17:47:33 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4025/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 18:33:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 8: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 27 Jul 2019 00:22:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4011/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 01:11:00 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4040/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 22:07:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13918

to look at the new patch set (#2).

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................

IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

IMPALA-8538 added support for http connections to the hs2 server along
with LDAP auth support. This patch adds support for authorizing with
Kerberos via SPNEGO.

Testing:
- Added a unit test that runs a minikdc and an Impala server against
  it. Currently disabled until curl is available in the toolchain.
- Tested manually on a real cluster with a full Kerberos setup and
  beeline.

Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/kerberos-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
11 files changed, 402 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13918/2
-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13918

to look at the new patch set (#5).

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................

IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

IMPALA-8538 added support for http connections to the hs2 server along
with LDAP auth support. This patch adds support for authorizing with
Kerberos via SPNEGO.

Testing:
- Added a unit test that runs a minikdc and an Impala server against
  it. Currently disabled until curl is available in the toolchain.
- Tested manually on a real cluster with a full Kerberos setup and
  beeline.

Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/kerberos-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
11 files changed, 394 insertions(+), 100 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13918/5
-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13918

to look at the new patch set (#3).

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................

IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

IMPALA-8538 added support for http connections to the hs2 server along
with LDAP auth support. This patch adds support for authorizing with
Kerberos via SPNEGO.

Testing:
- Added a unit test that runs a minikdc and an Impala server against
  it. Currently disabled until curl is available in the toolchain.
- Tested manually on a real cluster with a full Kerberos setup and
  beeline.

Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/kerberos-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
11 files changed, 401 insertions(+), 99 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13918/3
-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13918

to look at the new patch set (#6).

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................

IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

IMPALA-8538 added support for http connections to the hs2 server along
with LDAP auth support. This patch adds support for authorizing with
Kerberos via SPNEGO.

Testing:
- Added a unit test that runs a minikdc and an Impala server against
  it. Currently disabled until curl is available in the toolchain.
- Tested manually on a real cluster with a full Kerberos setup and
  beeline.

Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/kerberos-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
11 files changed, 395 insertions(+), 100 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13918/6
-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/rpc/authentication.cc@565
PS1, Line 565: ext->username = username;
> nit: std::move() around this
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 17:54:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 8: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 27 Jul 2019 06:53:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/13918/6/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/6/be/src/transport/THttpServer.cpp@184
PS6, Line 184:  if (has_ldap_ && (!got_negotiate_auth || !has_kerberos_)) {
> shouldn't this be conditioned on got_basic_auth? otherwise a non-authentica
We don't want to condition on got_basic_auth here because we want to call the callback and get return headers if we didn't get a 'Authorization' at all or got one with an invalid method.

You're right that this messes up the metrics, though. I'll add a check before updating the metrics, though maybe it would be clearer to go back to the way I had it before where we condition on got_basic_auth here and then call the callback with the empty string in a second step below if auth wasn't successful and we hadn't already called it.


http://gerrit.cloudera.org:8080/#/c/13918/6/be/src/transport/THttpServer.cpp@196
PS6, Line 196: is_complete && metrics_enabled_) total_negotiate_auth_success_->Increment(1);
             :     } else {
> combine these two?
Done


http://gerrit.cloudera.org:8080/#/c/13918/6/be/src/transport/THttpServer.cpp@200
PS6, Line 200: 
> shouldn't we only increment this if there was a token present? the normal f
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 22:56:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/4041/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 22:50:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp@256
PS1, Line 256:   sprintf(buff,
> This is for the error case, i.e. if a client attempts to connect without pr
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 00:30:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/13918/6/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/6/be/src/transport/THttpServer.cpp@184
PS6, Line 184:  if (has_ldap_ && (!got_negotiate_auth || !has_kerberos_)) {
shouldn't this be conditioned on got_basic_auth? otherwise a non-authenticated request is going to count as a basic_auth_failure


http://gerrit.cloudera.org:8080/#/c/13918/6/be/src/transport/THttpServer.cpp@196
PS6, Line 196: is_complete) {
             :         if (metrics_enabled_)
combine these two?


http://gerrit.cloudera.org:8080/#/c/13918/6/be/src/transport/THttpServer.cpp@200
PS6, Line 200: total_negotiate_auth_failure_
shouldn't we only increment this if there was a token present? the normal flow is that someone will connect, send no auth header, and get a 401. We don't want to count this 401 as a failed authentication



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 22:41:42 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp@256
PS1, Line 256:     h << "WWW-Authenticate: Negotiate" << CRLF;
> shouldn't we be using a Negotiate header which includes the token returned 
This is for the error case, i.e. if a client attempts to connect without providing a 'Authorization: Negotiate' header at all.

I guess you're saying that in that case I should be passing an empty token to kudu::gssapi::SpnegoStep and returning whatever the response is?



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jul 2019 18:00:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp@187
PS1, Line 187:     bool is_complete;
> Similar question to the earlier comment -- isn't it possible for SpnegoStep
You're right, fixed it to return a 401 unless is_complete is true.


http://gerrit.cloudera.org:8080/#/c/13918/3/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/3/be/src/transport/THttpServer.cpp@199
PS3, Line 199: call the auth function with an empty string to allow them to
             :   // generate return headers.
> shouldn't they have generated the return header from the bad token? ie i ht
Right, my handling of !is_complete here is wrong and I'll fix it.

My idea here is that if the client picked a valid auth method, we'll have called the corresponding callback above and generated return headers if needed, either because auth failed or is incomplete.

This block is intended to handle the cases where the client didn't send a Authorization header at all, or send one with an invalid method. We'll send back a 'WWW-Authenticate Basic' and/or 'WWW-Authenticate Negotiate <token produced by passing the empty string to spnego>' depending on what's supported.

This is admittedly sort of hacky, but it means we don't have to worry about for example basic_auth_fn generating a 'WWW-Authenticate: Basic' if we got a valid 'Authorization: Negotiate'.



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 21:40:17 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13918

to look at the new patch set (#4).

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................

IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

IMPALA-8538 added support for http connections to the hs2 server along
with LDAP auth support. This patch adds support for authorizing with
Kerberos via SPNEGO.

Testing:
- Added a unit test that runs a minikdc and an Impala server against
  it. Currently disabled until curl is available in the toolchain.
- Tested manually on a real cluster with a full Kerberos setup and
  beeline.

Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
---
M be/src/rpc/CMakeLists.txt
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/auth-provider.h
M be/src/rpc/authentication.cc
A be/src/rpc/kerberos-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M common/thrift/metrics.json
11 files changed, 401 insertions(+), 99 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/13918/4
-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/1/be/src/transport/THttpServer.cpp@187
PS1, Line 187:     bool is_complete;
Similar question to the earlier comment -- isn't it possible for SpnegoStep to return "OK" but not "complete"? In that case, it would be responding with a token, and you'd want to be sending the token back with the Unauthorized response?


http://gerrit.cloudera.org:8080/#/c/13918/3/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:

http://gerrit.cloudera.org:8080/#/c/13918/3/be/src/transport/THttpServer.cpp@199
PS3, Line 199: call the auth function with an empty string to allow them to
             :   // generate return headers.
shouldn't they have generated the return header from the bad token? ie i htink the SPNEGO spec includes the possibility of a multi-step authentication, where the client sends a token, the server responds with a different token (but not authorized). Is that not the case?



-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 20:39:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13918 )

Change subject: IMPALA-8783: Add Kerberos SPNEGO support to the http hs2 server
......................................................................


Patch Set 7: Code-Review+2

k, seems like this is good. Just to triple check, I think you should run one manual test where you do add some code locally like:

if (!token.empty()) { token[5]++; }

and make sure that it doesn't allow connecting :)


-- 
To view, visit http://gerrit.cloudera.org:8080/13918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15d9a842ab37ebc34b9fde5917137ff2961d870a
Gerrit-Change-Number: 13918
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Jul 2019 23:18:51 +0000
Gerrit-HasComments: No