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/11/30 01:55:55 UTC
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Dan Burkert has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8692
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 820 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/1
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 4:
(2 comments)
I didn't review sasl_client_transport or the krpc changes.
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/hms_client.cc@140
PS4, Line 140: // TODO(dan): check necessary?
Perhaps move this logic (and thus the construction of protocol and client_) into Start(), so that you can return a bad Status?
Alternatively, you could make HmsClient use two-phase initialization and add an Init() method that can return a bad Status.
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/mini_hms.h@58
PS4, Line 58: // Configures the mini HMS to use Kerberos.
Why build this as a setter vs. adding it to the constructor? There's no use case for calling it multiple times, right? Or for calling it after Start?
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 04 Jan 2018 00:44:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#9).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 995 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/9
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 9
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 10: Code-Review+1
(2 comments)
This LGTM
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123
PS5, Line 123: ead
> It's probably not possible currently, but exception handling is non-local,
Ok that makes sense. I'm fine with keeping it this way too.
The reason I brought it up was because if some unexpected exception was thrown, we'd ideally want to fix it rather than transparently drop it. But I'm fine with it either way.
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@205
PS7, Line 205:
> Yes, the 'ResetWriteBuf' method restores the prefix, and that's called at t
Oops, missed that. Thanks for the explanation.
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 06 Feb 2018 04:34:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#10).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 994 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/10
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123
PS5, Line 123: ans
> I didn't realize we could hit non Thrift exceptions in Negotiate(), since S
It's probably not possible currently, but exception handling is non-local, and this is more robust to future changes. Is there a reason to prefer scoping down the catch?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274
PS5, Line 274:
> nvm, I misunderstood the fact that quth-int and auth-conf need to be negoti
I ended up changing the API so that the minimum protection level is passed in explicitly; hopefully this makes it a bit more clear (and it reduces some code).
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@141
PS7, Line 141: read_buf_.resize(kFrameHeaderSize);
: transport_->readAll(read_buf_.data(), kFrameHeaderSize);
> Why not just read into a uint32_t here and avoid the resize(kFrameHeaderSiz
Done
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@205
PS7, Line 205: plaintext.remove_prefix(kFrameHeaderSize);
> Is this correct?
Yes, the 'ResetWriteBuf' method restores the prefix, and that's called at the very end of flush, so it should be present on subsequent trips through the loop.
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 06 Feb 2018 00:58:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 7:
Todd had some offline feedback to give the receive buffer size a flag, which necessitated a few refactors, so the latest diff is kinda big.
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 06 Feb 2018 00:59:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#5).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 847 insertions(+), 73 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/5
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#8).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 987 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/8
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#7).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 953 insertions(+), 77 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/7
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Reviewed-on: http://gerrit.cloudera.org:8080/8692
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 994 insertions(+), 83 deletions(-)
Approvals:
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 12
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8692/3/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/3/src/kudu/hms/sasl_client_transport.cc@23
PS3, Line 23: #include <string>
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/8692/3/src/kudu/hms/sasl_client_transport.cc@30
PS3, Line 30: #include "kudu/gutil/strings/substitute.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 19:16:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/hms/sasl_client_transport.cc@111
PS10, Line 111: // is complete the negotiated value will be used.
> does HMS know how to interpret split frames? I don't see any reason we woul
Yes, on the reading side in both this impl and Java, messages split across frames can be read. Reason being is in both cases you can always have a 'short' read, and this is what a split frame typically manifests as. Also has to do with the fact that the protocol layer interprets the messages, and the framing is done at the lower level transport layer.
http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/rpc/sasl_common.cc@461
PS10, Line 461: switch (val) {
> nit: indentation
Done
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 12 Feb 2018 21:29:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#4).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 853 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/4
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#11).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 994 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/11
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 11
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123
PS5, Line 123: ans
> That doesn't catch all exceptions.
I didn't realize we could hit non Thrift exceptions in Negotiate(), since SaslException is also a type of TException.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274
PS5, Line 274:
> I'm not following, in this line needs_wrap_ is being retrieved from the SAS
nvm, I misunderstood the fact that quth-int and auth-conf need to be negotiated. I thought we were forcing them on.
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@141
PS7, Line 141: read_buf_.resize(kFrameHeaderSize);
: transport_->readAll(read_buf_.data(), kFrameHeaderSize);
Why not just read into a uint32_t here and avoid the resize(kFrameHeaderSize) ?
Then in L154, you can do just the single resize and just put both the header and the payload at once into read_buf_.
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@205
PS7, Line 205: plaintext.remove_prefix(kFrameHeaderSize);
Is this correct?
If in L191, we figure that the write_buf_.size() > kSaslMaxBufLen, we first flush() a buffer of kSaslMaxBufLen, and we remove the prefix 'kFrameHeaderSize'. When the loop runs the second time, we're still removing the header prefix again.
But the header will not be present in the buffer in second iteration of the loop.
Unless I'm missing something.
http://gerrit.cloudera.org:8080/#/c/8692/7/src/kudu/hms/sasl_client_transport.cc@217
PS7, Line 217: size_t payload_len = write_buf_.size() - kFrameHeaderSize;
Same here, in the second iteration of the loop, we'll be losing 'kFrameHeaderSize' bytes of information.
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 01 Feb 2018 22:08:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123
PS5, Line 123: ...
> Ok that makes sense. I'm fine with keeping it this way too.
Note that this is unconditionally re-throwing the exception, it just making sure to close the transport if that happens. The syntax is a little weird, since it doesn't assign the exception to a local variable.
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 06 Feb 2018 17:25:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 5:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@68
PS5, Line 68: CHECK(!krb5_conf.empty());
worth CHECK(!hms_process_) here too to ensure that it's set up before starting
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@240
PS5, Line 240:
: <property>
: <name>hadoop.rpc.protection</name>
: <value>$5</value>
: </property>
odd this is in hive-site and not core-site... but guess it works
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@53
PS5, Line 53: Width
"width" strikes me as odd relative to "Size"
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@157
PS5, Line 157: ResetReadBuf();
are we guaranteed that read_slice_.data() != read_buf_.data()? ie it's always a new internal buffer? can we CHECK it?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@177
PS5, Line 177: // If we have some data in the read buffer, then return it.
: if (!read_slice_.empty()) {
: return read_fast();
: }
:
: // Otherwise, read a new frame from the wire and return it.
: ReadFrame();
: return read_fast();
this code is a bit odd to me. why not just do:
if (read_slice_.empty()) {
ReadFrame();
}
// contents of read_fast go here
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@224
PS5, Line 224: ResetWriteBuf();
the shrink_to_fit() in here seems a bit odd -- if you're doing a multi-frame write() call you'll end up flushing and reallocating many times, right?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h
File src/kudu/rpc/sasl_common.h:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@116
PS5, Line 116: // The ciphertext data must not be greater than the maximum buffer size.
not sure how to interpret this comment, since ciphertext is an out-param
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@122
PS5, Line 122: Slice* ciphertext) WARN_UNUSED_RESULT;
think this param needs a bit more doc - it's an in-out param, right? is it guaranteed that it still point to the same spot on return or could it write to some other buffer?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@126
PS5, Line 126: // The encoded data must not be greater than the maximum buffer size.
same
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@48
PS5, Line 48: extern const size_t kSaslMaxOutBufLen = 64 * 1024;
if this is our max frame length for thrift connections to the HMS, is this going to blow up when we try to use the "pubsub" thing which apparently often returns huge responses? Or will a single call response be appropriately fragmented into smaller frames?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@356
PS5, Line 356: No max output buffer size
wrong message here
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@367
PS5, Line 367: Status SaslEncode(sasl_conn_t* conn, Slice plaintext, Slice* ciphertext) {
we seem to have lost the looping behavior in the case that the input is larger than the buffer. Can we at least return a bad Status or CHECK if you don't want to keep that implementation?
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 22:20:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#3).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 859 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/3
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 5:
(24 comments)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h@82
PS5, Line 82: "127.0.0.1"
> Are we sure that this won't clash with some other test services that may be
Yes, it uses an ephemeral port. If it's shutdown and started again it will use the same port, but ephemeral ports tend not to get used again immediately.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@68
PS5, Line 68: CHECK(!krb5_conf.empty());
> worth CHECK(!hms_process_) here too to ensure that it's set up before start
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@240
PS5, Line 240:
: <property>
: <name>hadoop.rpc.protection</name>
: <value>$5</value>
: </property>
> odd this is in hive-site and not core-site... but guess it works
Despite the property name this is applying to the HMS thrift server interface, not an HRPC interface.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@53
PS5, Line 53: Width
> "width" strikes me as odd relative to "Size"
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@56
PS5, Line 56: kFrameHeaderWidth
> nit: Same as Todd's comment above.
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@107
PS5, Line 107: sasl_helper_.EnableGSSAPI();
> Does this mean this won't work with SASL plain?
Correct, the HMS doesn't appear to use SASL plain.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@120
PS5, Line 120: transport_->open();
> DCHECK(!isOpen());
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123
PS5, Line 123: ...
> catch(TException& e) ?
That doesn't catch all exceptions.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@157
PS5, Line 157: ResetReadBuf();
> are we guaranteed that read_slice_.data() != read_buf_.data()? ie it's alwa
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@177
PS5, Line 177: // If we have some data in the read buffer, then return it.
: if (!read_slice_.empty()) {
: return read_fast();
: }
:
: // Otherwise, read a new frame from the wire and return it.
: ReadFrame();
: return read_fast();
> this code is a bit odd to me. why not just do:
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@224
PS5, Line 224: ResetWriteBuf();
> the shrink_to_fit() in here seems a bit odd -- if you're doing a multi-fram
Yes, this is the only place I could find that was appropriate for releasing the write buffer. Otherwise the connection will hold on to the buffer indefinitely, and never shrink it.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@254
PS5, Line 254: !s.IsIncomplete() && !s.ok())
> sasl_client_step() could return SASL_OK or SASL_CONTINUE, both of which are
WrapSaslCall will convert SASL_CONTINUE to a Status::Incomplete value, and SASL_OK to a Status::OK, both of which are handled here.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274
PS5, Line 274: needs_wrap_ = rpc::NeedsWrap(sasl_conn_.get());
> Based on how you're initializing the transport, this looks like it should b
I'm not following, in this line needs_wrap_ is being retrieved from the SASL connection, which negotiates it with the remote server. Ultimately, whether auth-conf, auth-int or no wrap is needed is determined by server configuration.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@284
PS5, Line 284: payload.size()
> nit: Not that it's likely, but Slice objects can have lengths greater than
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@292
PS5, Line 292: // Read the fixed-length message header.
> DCHECK_EQ(payload.size(), 0);
I've changed it so that payload gets overwritten, so the caller doesn't need to clear.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@343
PS5, Line 343: s = rpc::AllowProtection(sasl_conn_.get());
: if (!s.ok()) {
: throw SaslException(s);
: }
> Integrity and confidentiality is set as a strict requirement here. Is that
This is not setting a strict requirement, it's allowing for integrity and confidentiality. We leave it up to the server to set the requirement (min_ssf).
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@363
PS5, Line 363: "hive"
> #define this with a comment stating that we'll only be talking to hive for
I've added a TODO. I don't think there's any point in defining a constant since we'll ultimately want to pass it in anyway.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@378
PS5, Line 378: resize(0);
> nit: clear()
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h
File src/kudu/rpc/sasl_common.h:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@116
PS5, Line 116: // The ciphertext data must not be greater than the maximum buffer size.
> not sure how to interpret this comment, since ciphertext is an out-param
This is a typo.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@122
PS5, Line 122: Slice* ciphertext) WARN_UNUSED_RESULT;
> think this param needs a bit more doc - it's an in-out param, right? is it
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.h@126
PS5, Line 126: // The encoded data must not be greater than the maximum buffer size.
> same
This is a little subtle - it's saying that the plaintext must not be longer than the maximum negotiated buffer size. This is entirely controlled by whomever encoded the data to begin with, but getting it wrong will cause the decode to fail.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@48
PS5, Line 48: extern const size_t kSaslMaxOutBufLen = 64 * 1024;
> if this is our max frame length for thrift connections to the HMS, is this
Yes, unfortunately right now we'll fail to receive large objects sent by the HMS when auth-int or auth-conf is enabled. Ultimately this is due to a bug in the Thrift Java implementation, THRIFT-4483. At this point I don't know what to do about this, since this has been broken for years and I doubt it will change. I don't know of a workaround on our side, since it's caused by Thrift not being RFC 4422 compliant, while Cyrus SASL is.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@356
PS5, Line 356: No max output buffer size
> wrong message here
Done
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@367
PS5, Line 367: Status SaslEncode(sasl_conn_t* conn, Slice plaintext, Slice* ciphertext) {
> we seem to have lost the looping behavior in the case that the input is lar
Yeah, turns out the looping never worked, anyway. I've updated the Status returned in this case to hint that it may be due to a too-long message.
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 31 Jan 2018 20:08:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 5:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h@82
PS5, Line 82: "127.0.0.1"
Are we sure that this won't clash with some other test services that may be started on the same address ?
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h
File src/kudu/hms/sasl_client_transport.h:
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h@130
PS4, Line 130: // Reads a frame from the underlying transport, storing the payload into
: // read_slice_. If the connection is using SASL auth-conf or auth-int
: // protection the data is automatically decoded.
: void ReadFrame();
> The biggest reason is that the framing isn't used during the SASL handshake
Ok makes sense. Thanks for the explanation.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@56
PS5, Line 56: kFrameHeaderWidth
nit: Same as Todd's comment above.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@107
PS5, Line 107: sasl_helper_.EnableGSSAPI();
Does this mean this won't work with SASL plain?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@120
PS5, Line 120: transport_->open();
DCHECK(!isOpen());
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123
PS5, Line 123: ...
catch(TException& e) ?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@254
PS5, Line 254: !s.IsIncomplete() && !s.ok())
sasl_client_step() could return SASL_OK or SASL_CONTINUE, both of which are valid statuses. This doesn't seem to take that into consideration.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274
PS5, Line 274: needs_wrap_ = rpc::NeedsWrap(sasl_conn_.get());
Based on how you're initializing the transport, this looks like it should be more like a DCHECK ? i.e. needs_wrap_ should always be true.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@284
PS5, Line 284: payload.size()
nit: Not that it's likely, but Slice objects can have lengths greater than can fit in 32 bits. Something we want to add a DCHECK for ?
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@292
PS5, Line 292: // Read the fixed-length message header.
DCHECK_EQ(payload.size(), 0);
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@343
PS5, Line 343: s = rpc::AllowProtection(sasl_conn_.get());
: if (!s.ok()) {
: throw SaslException(s);
: }
Integrity and confidentiality is set as a strict requirement here. Is that what you want? Since sasl_encode() and sasl_decode() are known to be pretty slow.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@363
PS5, Line 363: "hive"
#define this with a comment stating that we'll only be talking to hive for now.
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@378
PS5, Line 378: resize(0);
nit: clear()
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:11:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/hms_client.cc@140
PS4, Line 140: // TODO(dan): check necessary?
> Perhaps move this logic (and thus the construction of protocol and client_)
This TODO was actually about entirely removing SaslInit from this code, which I've done.
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/mini_hms.h@58
PS4, Line 58: // Configures the mini HMS to use Kerberos.
> Why build this as a setter vs. adding it to the constructor? There's no use
Because it doesn't always get called.
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h
File src/kudu/hms/sasl_client_transport.h:
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h@130
PS4, Line 130: // Reads a frame from the underlying transport, storing the payload into
: // read_slice_. If the connection is using SASL auth-conf or auth-int
: // protection the data is automatically decoded.
: void ReadFrame();
> One high level question before I delve into the details; why do you choose
The biggest reason is that the framing isn't used during the SASL handshake, so we'd have to do internal buffering during that anyway. I believe Impala's impl skips TFramedTransport for the same reason. By having more control over the internal buffer we can also do some neat tricks like eliminating a copy during sasl unwrap/decode (see sasl_client_transport.cc:153).
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 24 Jan 2018 19:58:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#6).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 959 insertions(+), 75 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/6
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 11
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 23 Feb 2018 21:04:24 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8692/6/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/8692/6/src/kudu/hms/hms_client-test.cc@247
PS6, Line 247: fails
> does it fail gracefully or does it CHECK? would be good to ensure that we d
oops, accidentally published this comment on earlier rev. ignore.
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:52:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@48
PS5, Line 48: extern const size_t kSaslMaxOutBufLen = 64 * 1024;
> hrm, so I guess we need to either set this max buf len to be quite large (1
I figured out a workaround, which is pretty much to do exactly what Thrift does: completely ignore the max buffer length. I've set it to 100MiB to match the max for thrift and the HMS, hopefully that's big enough.
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 01 Feb 2018 01:51:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h
File src/kudu/hms/sasl_client_transport.h:
http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/sasl_client_transport.h@130
PS4, Line 130: // Reads a frame from the underlying transport, storing the payload into
: // read_slice_. If the connection is using SASL auth-conf or auth-int
: // protection the data is automatically decoded.
: void ReadFrame();
One high level question before I delve into the details; why do you choose to implement your own framing vs. wrap around a TFramedTransport?
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 02 Jan 2018 19:43:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8692/6/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/8692/6/src/kudu/hms/hms_client-test.cc@247
PS6, Line 247:
does it fail gracefully or does it CHECK? would be good to ensure that we don't crash kudu if we run against a misbehaving hms
http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:
http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/hms/sasl_client_transport.cc@111
PS10, Line 111: // is complete the negotiated value will be used.
does HMS know how to interpret split frames? I don't see any reason we would send a thrift message >64kb for our use cases but I wonder if the error would be completely impossible to understand if we did
http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/rpc/sasl_common.cc@461
PS10, Line 461: switch (val) {
nit: indentation
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:52:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8692 )
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@48
PS5, Line 48: extern const size_t kSaslMaxOutBufLen = 64 * 1024;
> Yes, unfortunately right now we'll fail to receive large objects sent by th
hrm, so I guess we need to either set this max buf len to be quite large (1gb or something) or we need to fix the Java bug and make sure that HMS upstream uses a bug-fixed thrift?
I am a somewhat-inactive Thrift committer so the first half of that shouldn't be too hard to get done, happy to review and commit a fix
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 31 Jan 2018 21:11:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8692
to look at the new patch set (#2).
Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
......................................................................
KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:
- Impala has a client and server SASL transport, necessitating a common
superclass (SaslTransport). Since we only need a client transport, I
collapsed all of the logic into a single class, which I think makes the
code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.
There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.
Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 822 insertions(+), 63 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/8692/2
--
To view, visit http://gerrit.cloudera.org:8080/8692
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot