[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
---
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 23 Feb 2018 21:04:24 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 12 Feb 2018 21:29:10 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:52:14 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:52:39 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 06 Feb 2018 17:25:12 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Sailesh Mukil (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 06 Feb 2018 04:34:17 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 06 Feb 2018 00:58:53 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 06 Feb 2018 00:59:41 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Sailesh Mukil (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 01 Feb 2018 22:08:25 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 01 Feb 2018 01:51:17 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 31 Jan 2018 21:11:04 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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:
 :   
 : hadoop.rpc.protection
 : $5
 :   
> 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. 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Sailesh Mukil (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:11:21 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Todd Lipcon (Code Review)]

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:
 :   
 : hadoop.rpc.protection
 : $5
 :   
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 24 Jan 2018 22:20:38 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 24 Jan 2018 19:58:56 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Sailesh Mukil (Code Review)]

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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 02 Jan 2018 19:43:02 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
> 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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 08 Dec 2017 19:16:54 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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