You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/12/04 22:44:47 UTC

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional 

Hello Alexey Serbin,

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

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

to review the following change.


Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................

KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

The original issue was filed for certain Java edge cases, however
Sailesh and David who are working with KRPC in Impala pointed out that
the C++ side never does SASL PLAIN fallback when the server has Kerberos
enabled. This commit fixes both clients to correctly fall back to SASL
PLAIN when the server is Kerberized and authentication is optional.

Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/security/tls_handshake.cc
9 files changed, 222 insertions(+), 64 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Kudu Jenkins, Sailesh Mukil, 

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

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

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................

KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

The original issue was filed for certain Java edge cases, however
Sailesh and David who are working with KRPC in Impala pointed out that
the C++ side never does SASL PLAIN fallback when the server has Kerberos
enabled. This commit fixes both clients to correctly fall back to SASL
PLAIN when the server is Kerberized and authentication is optional.

Detecting whether the client has Kerberos credentials requires
using the GSSAPI directly instead of the SASL api. As a result, we now
find GSSAPI while building. Since GSSAPI is a direct dependency of Cyrus
SASL, this shouldn't cause any issues or require updating installation
docs.

Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
---
M CMakeLists.txt
A cmake_modules/FindGSSAPI.cmake
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M src/kudu/integration-tests/security-faults-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
18 files changed, 333 insertions(+), 98 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Kudu Jenkins, Sailesh Mukil, 

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

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

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................

KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

The original issue was filed for certain Java edge cases, however
Sailesh and David who are working with KRPC in Impala pointed out that
the C++ side never does SASL PLAIN fallback when the server has Kerberos
enabled. This commit fixes both clients to correctly fall back to SASL
PLAIN when the server is Kerberized and authentication is optional.

Detecting whether the client has Kerberos credentials requires
using the GSSAPI directly instead of the SASL api. As a result, we now
find GSSAPI while building. Since GSSAPI is a direct dependency of Cyrus
SASL, this shouldn't cause any issues or require updating installation
docs.

Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
---
M CMakeLists.txt
A cmake_modules/FindGSSAPI.cmake
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
16 files changed, 282 insertions(+), 75 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................


Patch Set 6:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/integration-tests/security-itest.cc@a185
PS5, Line 185: 
> nit: maybe, it's worth keeping this line of the description for this test?
woops, good call.


http://gerrit.cloudera.org:8080/#/c/8755/4/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8755/4/src/kudu/rpc/client_negotiation.cc@809
PS4, Line 809:                                    gss_error_description(minor, GSS_C_MECH_CODE));
> warning: do not use 'else' after 'return' [readability-else-after-return]
Done


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@432
PS5, Line 432: ials are 
> nit: Should this be "enabled", since GSSAPI is available but just not used?
I've tightened up this message to read 'Kerberos credentials are not available', which I think is the clearest way of expressing the error.  I also fixed up the other places where this error is created in order to make it more consistent across clients.


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@789
PS5, Line 789: OM_uint32 messa
> nit: maybe, move this under the 'do {} while()' scope below?
Done


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@790
PS5, Line 790: 
> ditto
Done


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@829
PS5, Line 829:                          nullptr);
> Are we leaking 'cred' if we return here?
I don't think so, since we aren't getting an msan memory leak about this, and we do have a test that triggers this early return.  That being said, I've reorganized the control flow here just in case there's some corner case not covered by our unit tests.  It's a bit less linear, but does now always call release.


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@834
PS5, Line 834: as to be
> nit: remaining_life ?
I want to keep it as 'lifetime', because that's what the param is named in the gss_inquire_cred declaration. I have added a comment sentence explaining it's meaning, though.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 19:51:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................


Patch Set 5:

(3 comments)

Sorry for the late review, got caught up in some other work.

LGTM for the most part, just a few comments.

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@432
PS5, Line 432: available
nit: Should this be "enabled", since GSSAPI is available but just not used?


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@829
PS5, Line 829: RETURN_NOT_OK(check_gss_error(major, minor));
Are we leaking 'cred' if we return here?


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@834
PS5, Line 834: lifetime
nit: remaining_life ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 11:06:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Kudu Jenkins, Sailesh Mukil, 

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

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

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................

KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

The original issue was filed for certain Java edge cases, however
Sailesh and David who are working with KRPC in Impala pointed out that
the C++ side never does SASL PLAIN fallback when the server has Kerberos
enabled. This commit fixes both clients to correctly fall back to SASL
PLAIN when the server is Kerberized and authentication is optional.

Detecting whether the client has Kerberos credentials requires
using the GSSAPI directly instead of the SASL api. As a result, we now
find GSSAPI while building. Since GSSAPI is a direct dependency of Cyrus
SASL, this shouldn't cause any issues or require updating installation
docs.

Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
---
M CMakeLists.txt
A cmake_modules/FindGSSAPI.cmake
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M src/kudu/integration-tests/security-faults-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
17 files changed, 319 insertions(+), 93 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8755/1/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8755/1/src/kudu/rpc/client_negotiation.cc@59
PS1, Line 59: using strings::Substitute;
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 04 Dec 2017 23:34:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 20:33:20 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................

KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

The original issue was filed for certain Java edge cases, however
Sailesh and David who are working with KRPC in Impala pointed out that
the C++ side never does SASL PLAIN fallback when the server has Kerberos
enabled. This commit fixes both clients to correctly fall back to SASL
PLAIN when the server is Kerberized and authentication is optional.

Detecting whether the client has Kerberos credentials requires
using the GSSAPI directly instead of the SASL api. As a result, we now
find GSSAPI while building. Since GSSAPI is a direct dependency of Cyrus
SASL, this shouldn't cause any issues or require updating installation
docs.

Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Reviewed-on: http://gerrit.cloudera.org:8080/8755
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M CMakeLists.txt
A cmake_modules/FindGSSAPI.cmake
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java
M src/kudu/integration-tests/security-faults-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
18 files changed, 333 insertions(+), 98 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................


Patch Set 5: Code-Review+2

(4 comments)

Looks good, just a few nits, feel free to ignore.

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/integration-tests/security-faults-itest.cc
File src/kudu/integration-tests/security-faults-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/integration-tests/security-faults-itest.cc@a248
PS5, Line 248: 
             : 
             : 
             : 
             : 
That's good it's gone now.


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/integration-tests/security-itest.cc
File src/kudu/integration-tests/security-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/integration-tests/security-itest.cc@a185
PS5, Line 185: 
nit: maybe, it's worth keeping this line of the description for this test?


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@789
PS5, Line 789: OM_uint32 minor
nit: maybe, move this under the 'do {} while()' scope below?


http://gerrit.cloudera.org:8080/#/c/8755/5/src/kudu/rpc/client_negotiation.cc@790
PS5, Line 790: gss_buffer_desc buf;
ditto



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 07 Dec 2017 04:28:26 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................


Patch Set 2:

(3 comments)

lgtm, just a few nits.

http://gerrit.cloudera.org:8080/#/c/8755/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java:

http://gerrit.cloudera.org:8080/#/c/8755/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java@1
PS2, Line 1: package org.apache.kudu.client;
licensing nit: add ASF copyright header?


http://gerrit.cloudera.org:8080/#/c/8755/2/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8755/2/src/kudu/rpc/client_negotiation.cc@418
PS2, Line 418: set<SaslMechanism::Type> common_mechs
Is it needed?


http://gerrit.cloudera.org:8080/#/c/8755/2/src/kudu/security/tls_handshake.cc
File src/kudu/security/tls_handshake.cc:

http://gerrit.cloudera.org:8080/#/c/8755/2/src/kudu/security/tls_handshake.cc@259
PS2, Line 259: (cipher == nullptr)
style nit: mind updating this to be '(!cipher)' to be in sync with the code below?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 05 Dec 2017 19:56:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8755/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java:

http://gerrit.cloudera.org:8080/#/c/8755/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java@1
PS2, Line 1: package org.apache.kudu.client;
> licensing nit: add ASF copyright header?
Done


http://gerrit.cloudera.org:8080/#/c/8755/2/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:

http://gerrit.cloudera.org:8080/#/c/8755/2/src/kudu/rpc/client_negotiation.cc@418
PS2, Line 418: set<SaslMechanism::Type> common_mechs
> Is it needed?
Done


http://gerrit.cloudera.org:8080/#/c/8755/2/src/kudu/security/tls_handshake.cc
File src/kudu/security/tls_handshake.cc:

http://gerrit.cloudera.org:8080/#/c/8755/2/src/kudu/security/tls_handshake.cc@259
PS2, Line 259: (cipher == nullptr)
> style nit: mind updating this to be '(!cipher)' to be in sync with the code
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 06 Dec 2017 01:23:50 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Kudu Jenkins, Sailesh Mukil, 

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

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

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................

KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

The original issue was filed for certain Java edge cases, however
Sailesh and David who are working with KRPC in Impala pointed out that
the C++ side never does SASL PLAIN fallback when the server has Kerberos
enabled. This commit fixes both clients to correctly fall back to SASL
PLAIN when the server is Kerberized and authentication is optional.

Detecting whether the client has Kerberos credentials requires
using the GSSAPI directly instead of the SASL api. As a result, we now
find GSSAPI while building. Since GSSAPI is a direct dependency of Cyrus
SASL, this shouldn't cause any issues or require updating installation
docs.

Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
---
M CMakeLists.txt
A cmake_modules/FindGSSAPI.cmake
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M src/kudu/integration-tests/security-faults-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
17 files changed, 311 insertions(+), 93 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Alexey Serbin, Kudu Jenkins, Sailesh Mukil, 

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

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

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

Change subject: KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional
......................................................................

KUDU-2121: fix SASL PLAIN fallback with rpc-authentication=optional

The original issue was filed for certain Java edge cases, however
Sailesh and David who are working with KRPC in Impala pointed out that
the C++ side never does SASL PLAIN fallback when the server has Kerberos
enabled. This commit fixes both clients to correctly fall back to SASL
PLAIN when the server is Kerberized and authentication is optional.

Detecting whether the client has Kerberos credentials requires
using the GSSAPI directly instead of the SASL api. As a result, we now
find GSSAPI while building. Since GSSAPI is a direct dependency of Cyrus
SASL, this shouldn't cause any issues or require updating installation
docs.

Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
---
M CMakeLists.txt
A cmake_modules/FindGSSAPI.cmake
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestNegotiation.java
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/security/tls_handshake.cc
13 files changed, 265 insertions(+), 68 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f42f4b7a8ac767ccae439feb1dcd49080827276
Gerrit-Change-Number: 8755
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot