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/01/21 04:47:58 UTC

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation 

Hello Adar Dembo, Todd Lipcon, Alexey Serbin,

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

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

to review the following change.

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................

TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
16 files changed, 211 insertions(+), 476 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation

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

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................


TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Reviewed-on: http://gerrit.cloudera.org:8080/5762
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
17 files changed, 368 insertions(+), 586 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] TLS-negotiation [8/n]: TLS negotiation

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................

TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
17 files changed, 368 insertions(+), 586 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/5762/9
-- 
To view, visit http://gerrit.cloudera.org:8080/5762
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] TLS-negotiation [8/n]: TLS negotiation

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................


Patch Set 12:

For some reason gerrit wanted a rebase, so I did that. Will submit when it passes

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-HasComments: No

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................


Patch Set 11: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-HasComments: No

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................


Patch Set 3:

(12 comments)

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

Line 95: }
> error: no template named 'unique_ptr'; did you mean 'std::unique_ptr'? [cla
Done


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

Line 93:   // Allow TLS to be used on the connection. 'tls_context' is borrowed, and must
> ClientNegotiation
Done


Line 101: 
> error: no template named 'unique_ptr'; did you mean 'std::unique_ptr'? [cla
Done


http://gerrit.cloudera.org:8080/#/c/5762/1/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:

Line 247:   std::unique_ptr<security::TlsContext> tls_context_;
> error: no template named 'unique_ptr'; did you mean 'std::unique_ptr'? [cla
Done


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

Line 63:                                         struct propctx *propctx) {
> error: no template named 'unique_ptr'; did you mean 'std::unique_ptr'? [cla
Done


Line 323:       client_features_.insert(feature_flag);
> error: unexpected type name 'string': expected expression [clang-diagnostic
Done


Line 324:     }
> error: use of undeclared identifier 'server_mechs' [clang-diagnostic-error]
Done


Line 333:     RETURN_NOT_OK(SendError(ErrorStatusPB::FATAL_UNAUTHORIZED, s));
> error: use of undeclared identifier 'server_mechs' [clang-diagnostic-error]
Done


Line 337:   RETURN_NOT_OK(SendNegotiate(server_mechs));
> error: expected ')' [clang-diagnostic-error]
Done


Line 341: Status ServerNegotiation::SendNegotiate(const set<string>& server_mechs) {
> error: use of undeclared identifier 'server_mechs' [clang-diagnostic-error]
Done


http://gerrit.cloudera.org:8080/#/c/5762/1/src/kudu/rpc/server_negotiation.h
File src/kudu/rpc/server_negotiation.h:

Line 105: 
> error: no template named 'unique_ptr'; did you mean 'std::unique_ptr'? [cla
Done


Line 153: 
> warning: function 'kudu::rpc::ServerNegotiation::HandleTlsHandshake' has a 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-HasComments: Yes

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................

TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
17 files changed, 368 insertions(+), 586 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/5762/12
-- 
To view, visit http://gerrit.cloudera.org:8080/5762
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] TLS-negotiation [8/n]: TLS negotiation

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................


Patch Set 6:

(9 comments)

Not ready to review just yet

http://gerrit.cloudera.org:8080/#/c/5762/5//COMMIT_MSG
Commit Message:

Line 9: This commit adds TLS negotiation to the KRPC protocol, and removes the
> can you document this in rpc.md?
Done


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

Line 338:   NegotiatePB msg;
> I suppose it could but I don't see it has a potential perf issue (compared 
I went ahead and implemented the TODO since it was just a few method call changes.


PS5, Line 345: ation::HandleTl
> would prefer NetworkError or something here (same below in a few places)
Changed to NotAuthorized to match up with other invalid message error cases.


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

PS5, Line 93: anisms.
> borrowed makes it sound like it's temporarily taken exclusive ownership of,
Done


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

Line 28: set<RpcFeatureFlag> kSupportedServerRpcFeatureFlags = { APPLICATION_FEATURE_FLAGS };
> Add a comment why TLS isn't listed on the server too?
Done


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

PS5, Line 305:   tls_context_.reset(new security::TlsContext());
             :   RETURN_NOT_OK(tls_context_->Init());
> I think it's worth a comment saying something like "enable TLS unconditiona
Done


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

Line 135:   if (tls_context_ && ContainsKey(client_features_, TLS)) {
> also worth a TODO here about allowing the server to specify TLS-required?
Done


PS5, Line 365: est.step() != N
> same as elsewhere
Done


http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/server_negotiation.h
File src/kudu/rpc/server_negotiation.h:

Line 95:   // Must be called before Negotiate(). Required for some mechanisms.
> same comment as the equivalent in client_negotation
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-HasComments: Yes

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................

TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
16 files changed, 226 insertions(+), 484 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] TLS-negotiation [8/n]: TLS negotiation

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................

TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
17 files changed, 368 insertions(+), 581 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/5762/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5762
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] TLS-negotiation [8/n]: TLS negotiation

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................

TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
16 files changed, 216 insertions(+), 481 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] TLS-negotiation [8/n]: TLS negotiation

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................


Patch Set 11: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-HasComments: No

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................


Patch Set 1:

(2 comments)

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

Line 93:   // outlive this SaslClient.
ClientNegotiation


http://gerrit.cloudera.org:8080/#/c/5762/1/src/kudu/rpc/server_negotiation.h
File src/kudu/rpc/server_negotiation.h:

Line 97:   // outlive this SaslServer.
ServerNegotiation


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-HasComments: Yes

[kudu-CR] TLS-negotiation [8/n]: TLS negotiation

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................

TLS-negotiation [8/n]: TLS negotiation

This commit adds TLS negotiation to the KRPC protocol, and removes the
existing non-negotiated SSL support.

Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/constants.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/CMakeLists.txt
D src/kudu/security/ssl_factory.cc
D src/kudu/security/ssl_factory.h
D src/kudu/security/ssl_socket.cc
D src/kudu/security/ssl_socket.h
16 files changed, 218 insertions(+), 481 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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] TLS-negotiation [8/n]: TLS negotiation

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: TLS-negotiation [8/n]: TLS negotiation
......................................................................


Patch Set 5:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/5762/5//COMMIT_MSG
Commit Message:

Line 9: This commit adds TLS negotiation to the KRPC protocol, and removes the
can you document this in rpc.md?


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

Line 338:   // TODO(dan): should this be done without copying the response token?
I suppose it could but I don't see it has a potential perf issue (compared to the cost of waiting for the round trip, what's the extra increment of a refcount?)


PS5, Line 345: InvalidArgument
would prefer NetworkError or something here (same below in a few places)


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

PS5, Line 93: borrowed
borrowed makes it sound like it's temporarily taken exclusive ownership of, whereas really it just references it.


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

Line 28: set<RpcFeatureFlag> kSupportedServerRpcFeatureFlags = { APPLICATION_FEATURE_FLAGS };
Add a comment why TLS isn't listed on the server too?


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

PS5, Line 305:   tls_context_.reset(new security::TlsContext());
             :   RETURN_NOT_OK(tls_context_->Init());
I think it's worth a comment saying something like "enable TLS unconditionally for use by clients"


http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:

Line 232:   bool server_tls_enabled_;
now that we support negotiation, I'm guessing we'll soon need the 'RequireTLS()' type call on the client side as well to prevent downgrades (eg in Impala's use case they want to blanket on/off it, and we'll need it too)


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

Line 135:   if (tls_context_ && ContainsKey(client_features_, TLS)) {
also worth a TODO here about allowing the server to specify TLS-required?


PS5, Line 365: InvalidArgument
same as elsewhere


http://gerrit.cloudera.org:8080/#/c/5762/5/src/kudu/rpc/server_negotiation.h
File src/kudu/rpc/server_negotiation.h:

Line 95:   // Allow TLS to be used on the connection. 'tls_context' is borrowed, and must
same comment as the equivalent in client_negotation


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f7d36ed8ecf6067a3043344557f45ebd3cdcf9d
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@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-HasComments: Yes