You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2019/05/15 05:56:51 UTC

[kudu-CR] WIP: support SPNEGO for web server 

Hello Lars Volker, Alexey Serbin,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: WIP: support SPNEGO for web server
......................................................................

WIP: support SPNEGO for web server

SPNEGO is a protocol for securing HTTP requests with Kerberos by passing
negotiation through HTTP headers. It's supported by most major browsers
and also by most of the Java-based Hadoop components. Notably, it's also
the typical way in which Apache Knox authenticates itself to Hadoop
components in the "trusted proxy" mode, allowing them to be secured
behind Knox's SSO and other policies.

This patch implements the SPNEGO protocol by driving GSSAPI. A simple
unit test implements a simplified "just open a socket" HTTP server and
authenticates to it using curl. There is also the beginnings of an
integration into the webserver itself, but the patch is marked as WIP
because that integration needs quite some work. Some very basic testing
against a kerberized mini-cluster seems to indicate it works, though.

Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/gssapi.cc
A src/kudu/security/gssapi.h
A src/kudu/security/spnego-test.cc
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/server/webserver.cc
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M thirdparty/build-definitions.sh
11 files changed, 485 insertions(+), 33 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/13341/1
-- 
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>

[kudu-CR] Support SPNEGO for web server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Tidy Bot, Lars Volker, Alexey Serbin, Kudu Jenkins, Hao Hao, 

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

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

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

Change subject: Support SPNEGO for web server
......................................................................

Support SPNEGO for web server

SPNEGO is a protocol for securing HTTP requests with Kerberos by passing
negotiation through HTTP headers. It's supported by most major browsers
and also by most of the Java-based Hadoop components. Notably, it's also
the typical way in which Apache Knox authenticates itself to Hadoop
components in the "trusted proxy" mode, allowing them to be secured
behind Knox's SSO and other policies.

This patch implements the SPNEGO protocol by driving GSSAPI, and
integrates it into the webserver when configured by a new
--webserver_require_spnego flag.

The new test verifies this end-to-end using curl's SPNEGO authentication
support.

Along the way I had to cross-port a small change to the Base64 functions
in gutil to avoid a UBSAN error. I found the fix in abseil-cpp's copy of
the same file.

Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
---
M src/kudu/gutil/strings/escaping.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/gssapi.cc
A src/kudu/security/gssapi.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/server/CMakeLists.txt
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/server/webserver_options.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/test_macros.h
M thirdparty/build-definitions.sh
16 files changed, 604 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/13341/2
-- 
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Support SPNEGO for web server

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

Change subject: Support SPNEGO for web server
......................................................................


Patch Set 4: Code-Review+1

(4 comments)

I didn't look deep at this patch, but overall looks good just a few nits.

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@20
PS3, Line 20: #include <cstdlib>
nit: #include <cstdlib>


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@144
PS3, Line 144: wlil
nit: will


http://gerrit.cloudera.org:8080/#/c/13341/4/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/13341/4/src/kudu/server/webserver.cc@287
PS4, Line 287:     if (!kt_file || !Env::Default()->FileExists(kt_file)) {
IIRC, there is some sequence of searching for the server-side keytab.  Could you add a comment explaining why to rely only on the highest-level override here?

Also, what is the relation between data in file pointed by --keytab_file gflag (i.e. client-side keytab) and this variable?  Could they be the same or they will always be different?


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/util/curl_util.h@95
PS3, Line 95: spnego_
nit: 'use_spnego_' might be a better name from readability perspective



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Jun 2019 05:11:59 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support SPNEGO for web server

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

Change subject: Support SPNEGO for web server
......................................................................


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc
File src/kudu/security/gssapi.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@144
PS3, Line 144:   *complete = s.ok();
> interesting. The manpage indicates it does need to be freed. Not sure why L
Added by yours truly:

  // KUDU-2653: Memory leak in libgssapi_krb5 [1]. Exists in certain patched
  // versions of krb5-1.12 (such as krb5 in Debian 8).
  //
  // Unfortunately there's no narrower match without resorting to
  // fast_unwind_on_malloc=0; the best alternative is to match on glob, but that
  // seems like overkill too.
  //
  // 1. http://krbdev.mit.edu/rt/Ticket/Display.html?id=7981
  "leak:libgssapi_krb5.so.2\n";



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Jun 2019 05:27:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support SPNEGO for web server

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

Change subject: Support SPNEGO for web server
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Jun 2019 20:56:31 +0000
Gerrit-HasComments: No

[kudu-CR] Support SPNEGO for web server

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

Change subject: Support SPNEGO for web server
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@144
PS3, Line 144: wlil
> nit: will
Done


http://gerrit.cloudera.org:8080/#/c/13341/4/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/13341/4/src/kudu/server/webserver.cc@287
PS4, Line 287:   if (opts_.require_spnego) {
> IIRC, there is some sequence of searching for the server-side keytab.  Coul
I believe at this point the --keytab_file is already propagated into $KRB5_KTNAME by security::InitKerberosForServer. I think we could customize the gss_accept_* calls to use some specific credential store/keytab if we wanted, but for SASL purposes I remember it was basically impossible to do without just setting this env var. So, here we're just relying on the same. I'll add a comment.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/util/curl_util.h
File src/kudu/util/curl_util.h:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/util/curl_util.h@95
PS3, Line 95: spnego_
> nit: 'use_spnego_' might be a better name from readability perspective
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Jun 2019 05:17:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support SPNEGO for web server

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

Change subject: Support SPNEGO for web server
......................................................................


Patch Set 3:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt@85
PS3, Line 85: set(SECURITY_LIBS
            :   gutil
            :   kudu_util
            :   token_proto
            : 
            :   krb5
            :   gssapi_krb5
            :   openssl_crypto
            :   openssl_ssl)
Nit: while you're here, could you resort this alphabetically?


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h
File src/kudu/security/gssapi.h:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@35
PS3, Line 35: header
Nit: did you mean 'token' here? Otherwise there's not enough context to understand what 'header' means.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@42
PS3, Line 42: '*complete' indicates whether any further rounds are required. On
            : // completion of negotiation,
I assume that 'complete' is only touched in the event of an OK status?


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc
File src/kudu/security/gssapi.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@54
PS3, Line 54:     gss_release_buffer(&minor, &buf);
Maybe also add a comment saying that we can't use the more natural ReleaseBufferOrWarn here because it'd call right back into ErrorToString.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@144
PS3, Line 144:     gss_buffer_desc name_buf {0, nullptr};
Does this need to be freed after the call to std::string::assign? If not, could you add a comment explaining?


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@20
PS3, Line 20: #include <stdlib.h>
Nit: cstdlib instead


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@166
PS3, Line 166:       return Status::OK();
Do you also want coverage for the case where the callback returns a bad Status and causes authentication to fail?


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@204
PS3, Line 204: 
Nit: extra blank line here.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@238
PS3, Line 238: well-formed token
Should probably prove that you can authenticate using this token first. Otherwise the various token failure tests below are less credible because the token was never going to work anyway.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@143
PS3, Line 143: ,
Nit: misplaced comma


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@153
PS3, Line 153:     neg_token = "";
This is neg_token's default value; you can invert the condition and combine it with the condition in the else if.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@459
PS3, Line 459:       s = Status::RuntimeError("SPNEGO indicated complete, but got empty principal");
Any particular reason the status message shouldn't also include the address as per the DFATAL just below?


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h
File src/kudu/server/webserver_options.h:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h@23
PS3, Line 23: #include <stdint.h>
Nit: since you're here already, could you convert this into cstdint?


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc
File src/kudu/server/webserver_options.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc@111
PS3, Line 111: 
Nit: extra blank line here.


http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh@653
PS3, Line 653:   # Configure for a very minimal install - basically only HTTP(S), since we only
             :   # use this for testing our own HTTP/HTTPS endpoints at this point in time.
May want to update this.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Jun 2019 03:53:28 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support SPNEGO for web server

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

Change subject: Support SPNEGO for web server
......................................................................


Patch Set 6: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Jun 2019 20:17:07 +0000
Gerrit-HasComments: No

[kudu-CR] Support SPNEGO for web server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Tidy Bot, Lars Volker, Alexey Serbin, Kudu Jenkins, Hao Hao, 

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

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

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

Change subject: Support SPNEGO for web server
......................................................................

Support SPNEGO for web server

SPNEGO is a protocol for securing HTTP requests with Kerberos by passing
negotiation through HTTP headers. It's supported by most major browsers
and also by most of the Java-based Hadoop components. Notably, it's also
the typical way in which Apache Knox authenticates itself to Hadoop
components in the "trusted proxy" mode, allowing them to be secured
behind Knox's SSO and other policies.

This patch implements the SPNEGO protocol by driving GSSAPI, and
integrates it into the webserver when configured by a new
--webserver_require_spnego flag.

The new test verifies this end-to-end using curl's SPNEGO authentication
support.

Along the way I had to cross-port a small change to the Base64 functions
in gutil to avoid a UBSAN error. I found the fix in abseil-cpp's copy of
the same file.

Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
---
M src/kudu/gutil/strings/escaping.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/gssapi.cc
A src/kudu/security/gssapi.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/server/CMakeLists.txt
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/server/webserver_options.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/test_macros.h
M thirdparty/build-definitions.sh
16 files changed, 619 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/13341/3
-- 
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Support SPNEGO for web server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13341 )

Change subject: Support SPNEGO for web server
......................................................................

Support SPNEGO for web server

SPNEGO is a protocol for securing HTTP requests with Kerberos by passing
negotiation through HTTP headers. It's supported by most major browsers
and also by most of the Java-based Hadoop components. Notably, it's also
the typical way in which Apache Knox authenticates itself to Hadoop
components in the "trusted proxy" mode, allowing them to be secured
behind Knox's SSO and other policies.

This patch implements the SPNEGO protocol by driving GSSAPI, and
integrates it into the webserver when configured by a new
--webserver_require_spnego flag.

The new test verifies this end-to-end using curl's SPNEGO authentication
support.

Along the way I had to cross-port a small change to the Base64 functions
in gutil to avoid a UBSAN error. I found the fix in abseil-cpp's copy of
the same file.

Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Reviewed-on: http://gerrit.cloudera.org:8080/13341
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/gutil/strings/escaping.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/gssapi.cc
A src/kudu/security/gssapi.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/server/CMakeLists.txt
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/server/webserver_options.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/test_macros.h
M thirdparty/build-definitions.sh
16 files changed, 623 insertions(+), 65 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, but someone else must approve
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Support SPNEGO for web server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Tidy Bot, Lars Volker, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, 

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

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

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

Change subject: Support SPNEGO for web server
......................................................................

Support SPNEGO for web server

SPNEGO is a protocol for securing HTTP requests with Kerberos by passing
negotiation through HTTP headers. It's supported by most major browsers
and also by most of the Java-based Hadoop components. Notably, it's also
the typical way in which Apache Knox authenticates itself to Hadoop
components in the "trusted proxy" mode, allowing them to be secured
behind Knox's SSO and other policies.

This patch implements the SPNEGO protocol by driving GSSAPI, and
integrates it into the webserver when configured by a new
--webserver_require_spnego flag.

The new test verifies this end-to-end using curl's SPNEGO authentication
support.

Along the way I had to cross-port a small change to the Base64 functions
in gutil to avoid a UBSAN error. I found the fix in abseil-cpp's copy of
the same file.

Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
---
M src/kudu/gutil/strings/escaping.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/gssapi.cc
A src/kudu/security/gssapi.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/server/CMakeLists.txt
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/server/webserver_options.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/test_macros.h
M thirdparty/build-definitions.sh
16 files changed, 622 insertions(+), 65 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/13341/4
-- 
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Support SPNEGO for web server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Tidy Bot, Lars Volker, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, 

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

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

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

Change subject: Support SPNEGO for web server
......................................................................

Support SPNEGO for web server

SPNEGO is a protocol for securing HTTP requests with Kerberos by passing
negotiation through HTTP headers. It's supported by most major browsers
and also by most of the Java-based Hadoop components. Notably, it's also
the typical way in which Apache Knox authenticates itself to Hadoop
components in the "trusted proxy" mode, allowing them to be secured
behind Knox's SSO and other policies.

This patch implements the SPNEGO protocol by driving GSSAPI, and
integrates it into the webserver when configured by a new
--webserver_require_spnego flag.

The new test verifies this end-to-end using curl's SPNEGO authentication
support.

Along the way I had to cross-port a small change to the Base64 functions
in gutil to avoid a UBSAN error. I found the fix in abseil-cpp's copy of
the same file.

Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
---
M src/kudu/gutil/strings/escaping.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/gssapi.cc
A src/kudu/security/gssapi.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/server/CMakeLists.txt
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/server/webserver_options.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/test_macros.h
M thirdparty/build-definitions.sh
16 files changed, 625 insertions(+), 65 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/13341/5
-- 
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Support SPNEGO for web server

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

Change subject: Support SPNEGO for web server
......................................................................


Patch Set 3:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/CMakeLists.txt@85
PS3, Line 85: set(SECURITY_LIBS
            :   gutil
            :   kudu_util
            :   token_proto
            : 
            :   krb5
            :   gssapi_krb5
            :   openssl_crypto
            :   openssl_ssl)
> Nit: while you're here, could you resort this alphabetically?
done, but left the kudu libs separate from the thirdparty ones.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h
File src/kudu/security/gssapi.h:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@35
PS3, Line 35: header
> Nit: did you mean 'token' here? Otherwise there's not enough context to und
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.h@42
PS3, Line 42: '*complete' indicates whether any further rounds are required. On
            : // completion of negotiation,
> I assume that 'complete' is only touched in the event of an OK status?
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc
File src/kudu/security/gssapi.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@54
PS3, Line 54:     gss_release_buffer(&minor, &buf);
> Maybe also add a comment saying that we can't use the more natural ReleaseB
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/security/gssapi.cc@144
PS3, Line 144:     gss_buffer_desc name_buf {0, nullptr};
> Does this need to be freed after the call to std::string::assign? If not, c
interesting. The manpage indicates it does need to be freed. Not sure why LSAN didn't catch it -- perhaps because we have some LSAN suppressions for other known leaks in gssapi. Good catch.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc
File src/kudu/server/webserver-test.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@20
PS3, Line 20: #include <stdlib.h>
> Nit: cstdlib instead
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@166
PS3, Line 166:       return Status::OK();
> Do you also want coverage for the case where the callback returns a bad Sta
the current use case for this thing is just for tests, so I'll make it a void callback instead of returning Status. I'll extend it to return Status later if we find an authorization use case for it.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@204
PS3, Line 204: 
> Nit: extra blank line here.
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver-test.cc@238
PS3, Line 238: well-formed token
> Should probably prove that you can authenticate using this token first. Oth
well, the token here is well-formed but invalid, since each run of the test is a different KDC, principal, etc. But it still turned up overflow bugs just during parsing. I'll add a note indicating this.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@143
PS3, Line 143: ,
> Nit: misplaced comma
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@153
PS3, Line 153:     neg_token = "";
> This is neg_token's default value; you can invert the condition and combine
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver.cc@459
PS3, Line 459:       s = Status::RuntimeError("SPNEGO indicated complete, but got empty principal");
> Any particular reason the status message shouldn't also include the address
the status here would just get propagated back to the requester as part of the HTTP response, and the requester already knows their own IP.


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h
File src/kudu/server/webserver_options.h:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.h@23
PS3, Line 23: #include <stdint.h>
> Nit: since you're here already, could you convert this into cstdint?
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc
File src/kudu/server/webserver_options.cc:

http://gerrit.cloudera.org:8080/#/c/13341/3/src/kudu/server/webserver_options.cc@111
PS3, Line 111: 
> Nit: extra blank line here.
Done


http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/13341/3/thirdparty/build-definitions.sh@653
PS3, Line 653:   # Configure for a very minimal install - basically only HTTP(S), since we only
             :   # use this for testing our own HTTP/HTTPS endpoints at this point in time.
> May want to update this.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Jun 2019 04:51:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support SPNEGO for web server

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

Change subject: Support SPNEGO for web server
......................................................................


Patch Set 5: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 07 Jun 2019 05:49:55 +0000
Gerrit-HasComments: No

[kudu-CR] Support SPNEGO for web server

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Tidy Bot, Lars Volker, Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, 

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

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

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

Change subject: Support SPNEGO for web server
......................................................................

Support SPNEGO for web server

SPNEGO is a protocol for securing HTTP requests with Kerberos by passing
negotiation through HTTP headers. It's supported by most major browsers
and also by most of the Java-based Hadoop components. Notably, it's also
the typical way in which Apache Knox authenticates itself to Hadoop
components in the "trusted proxy" mode, allowing them to be secured
behind Knox's SSO and other policies.

This patch implements the SPNEGO protocol by driving GSSAPI, and
integrates it into the webserver when configured by a new
--webserver_require_spnego flag.

The new test verifies this end-to-end using curl's SPNEGO authentication
support.

Along the way I had to cross-port a small change to the Base64 functions
in gutil to avoid a UBSAN error. I found the fix in abseil-cpp's copy of
the same file.

Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
---
M src/kudu/gutil/strings/escaping.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/gssapi.cc
A src/kudu/security/gssapi.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/server/CMakeLists.txt
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/server/webserver_options.cc
M src/kudu/server/webserver_options.h
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
M src/kudu/util/test_macros.h
M thirdparty/build-definitions.sh
16 files changed, 623 insertions(+), 65 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/13341/6
-- 
To view, visit http://gerrit.cloudera.org:8080/13341
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9449ac610aa7d11bbf320d9178a6d73684ff15f7
Gerrit-Change-Number: 13341
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>