You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2019/07/02 00:02:43 UTC

[Impala-ASF-CR] Support SPNEGO for Impala webserver

Todd Lipcon has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13774


Change subject: Support SPNEGO for Impala webserver
......................................................................

Support SPNEGO for Impala webserver

This ports over changes from kudu commit
1f291b77ef0868ac888a850678adc2d7cce65529 which implemented SPNEGO for
the Kudu webserver.

Unfortunately, thorough testing of this is difficult given that curl
isn't currently in the toolchain. I was able to manually test this by
adding a 'sleep(1000)' call into the newly added test case, then setting
up $KRB5_CONFIG in my shell to point to the temporary KDC's environment,
and using 'curl -u : --negotiate http://...' to authenticate.

Strangely, using the version of curl on el7 didn't seem to work properly
(perhaps an el7 curl bug) but using curl on my Ubuntu 18 laptop I was
able to authenticate with SPNEGO.

Change-Id: Ife2b04310e1571d231bf8ee1bcfd3b7afc2edd8f
---
M be/src/exec/kudu-util.h
M be/src/gutil/strings/escaping.cc
M be/src/util/CMakeLists.txt
A be/src/util/kudu-status-util.h
M be/src/util/webserver-test.cc
M be/src/util/webserver.cc
M be/src/util/webserver.h
7 files changed, 263 insertions(+), 42 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ife2b04310e1571d231bf8ee1bcfd3b7afc2edd8f
Gerrit-Change-Number: 13774
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>

[Impala-ASF-CR] Support SPNEGO for Impala webserver

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Hello Thomas Tauber-Marshall, Impala Public Jenkins, 

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

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

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

Change subject: Support SPNEGO for Impala webserver
......................................................................

Support SPNEGO for Impala webserver

This ports over changes from kudu commit
1f291b77ef0868ac888a850678adc2d7cce65529 which implemented SPNEGO for
the Kudu webserver.

Unfortunately, thorough testing of this is difficult given that curl
isn't currently in the toolchain. I was able to manually test this by
adding a 'sleep(1000)' call into the newly added test case, then setting
up $KRB5_CONFIG in my shell to point to the temporary KDC's environment,
and using 'curl -u : --negotiate http://...' to authenticate.

Strangely, using the version of curl on el7 didn't seem to work properly
(perhaps an el7 curl bug) but using curl on my Ubuntu 18 laptop I was
able to authenticate with SPNEGO.

Change-Id: Ife2b04310e1571d231bf8ee1bcfd3b7afc2edd8f
---
M be/src/exec/kudu-util.h
M be/src/gutil/strings/escaping.cc
M be/src/util/CMakeLists.txt
A be/src/util/kudu-status-util.h
M be/src/util/webserver-test.cc
M be/src/util/webserver.cc
M be/src/util/webserver.h
7 files changed, 267 insertions(+), 42 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife2b04310e1571d231bf8ee1bcfd3b7afc2edd8f
Gerrit-Change-Number: 13774
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
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] Support SPNEGO for Impala webserver

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13774 )

Change subject: Support SPNEGO for Impala webserver
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/13774/1/be/src/util/kudu-status-util.h
File be/src/util/kudu-status-util.h:

http://gerrit.cloudera.org:8080/#/c/13774/1/be/src/util/kudu-status-util.h@26
PS1, Line 26: \
> Might as well fix the formatting while you're here
Done


http://gerrit.cloudera.org:8080/#/c/13774/1/be/src/util/webserver.cc
File be/src/util/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/13774/1/be/src/util/webserver.cc@203
PS1, Line 203: // is invalid, a bad Status will be returned (and the other out-parameters left untouched).
> line too long (91 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/13774/1/be/src/util/webserver.cc@214
PS1, Line 214:   RETURN_NOT_OK(kudu::gssapi::SpnegoStep(neg_token, &resp_token_b64, &is_complete, authn_user));
> line too long (96 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/13774/1/be/src/util/webserver.cc@370
PS1, Line 370: We assume that security::InitKerberosForServer()
I don't think the --principal is actually necessary -- the way SPNEGO works is that the client provides a principal, and the server will look in the configured keytab for any matching principal. This is what allows users to have multiple DNS names, and clients can connect to them via any name (useful for L4 load balancing or CNAMEs to work, for example).

If we don't have KRB5_KTNAME set at this point, it'll be null, and we'll get the expected bad Status below. Does that seem right? We actually do have some test case for this on the equivalent Kudu code, but it wasn't really easy to port it over to Impala's tests.

> What about the opposite case - where --principal is set but --webserver_require_spnego=false? Right now I think we just silently allow this, but users may find it surprising that setting up kerberos doesn't automatically secure the webserver. Maybe worth logging a warning in that case?

The problem with this is that, while SPNEGO is a standard, it's a huge pain in the ass for users to set up, and in fact I think even on secure clusters, most users don't enable SPNEGO on their web UIs. If we started WARNING about being insecure if you don't enable SPNEGO I think people will get pretty grouchy.

That said, this is still useful because we'll use this to allow access via Knox Trusted Proxy support, which uses SPNEGO to authenticate the Knox service to Impala (and Knox itself gets authentication via pluggable modules, including SSO integration)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife2b04310e1571d231bf8ee1bcfd3b7afc2edd8f
Gerrit-Change-Number: 13774
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
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: Wed, 10 Jul 2019 04:45:14 +0000
Gerrit-HasComments: Yes