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/02/08 22:15:39 UTC
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Hello Todd Lipcon, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5946
to review the following change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
[rpc] Assume existence of TlsContext during negotiation
This changes the client and server RPC negotiatiators to always assume
the existence of a TlsContext for the messenger, and check whether there
are valid certs in the context in order to decide whether to use TLS
encryption. This will simplify authentication negotiation down the line.
Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
6 files changed, 61 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/5946/1
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5946/1/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS1, Line 71: const security::TlsContext* tls_context
> We do allow the socket and tls context to be null in one special case: when
Would it be better to split it into two classes instead: { ServerNegotiationTls and ServerNegotiation } or { ServerNegotiation and ServerNegotiationGSSAPI }?
Doing so would make it clearer when tls_context should or should not be present.
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5946/1/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS1, Line 71: const security::TlsContext* tls_context
> Would it be better to split it into two classes instead: { ServerNegotiatio
I think PreflightCheckGSSAPI should not be creating ServerNegotiation instances with null socket and tls_context, but I think that refactor should be its own patch.
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5946/1/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS1, Line 137: tls_context_
I'm not sure I follow this: if nullptr as passed as a second parameter for ServerNegotiation constructor, then tls_context_ i nullptr, right? Then I would expect to see a check for tls_context_ prior to calling has_cert() on that.
PS1, Line 364: has_cert
ditto
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
[rpc] Assume existence of TlsContext during negotiation
This changes the client and server RPC negotiatiators to always assume
the existence of a TlsContext for the messenger, and check whether there
are valid certs in the context in order to decide whether to use TLS
encryption. This will simplify authentication negotiation down the line.
Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Reviewed-on: http://gerrit.cloudera.org:8080/5946
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
8 files changed, 74 insertions(+), 66 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5946/1/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS1, Line 71: const security::TlsContext* tls_context
Also, if assuming it's never nullptr, then may be it's worth passing it and storing it as a constant reference?
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [rpc] Assume existence of TlsContext during 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/5946
to look at the new patch set (#2).
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
[rpc] Assume existence of TlsContext during negotiation
This changes the client and server RPC negotiatiators to always assume
the existence of a TlsContext for the messenger, and check whether there
are valid certs in the context in order to decide whether to use TLS
encryption. This will simplify authentication negotiation down the line.
Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
8 files changed, 74 insertions(+), 66 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/5946/2
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5946/1/src/kudu/rpc/server_negotiation.cc
File src/kudu/rpc/server_negotiation.cc:
PS1, Line 71: const security::TlsContext* tls_context
> Also, if assuming it's never nullptr, then may be it's worth passing it and
We do allow the socket and tls context to be null in one special case: when the ServerNegotiation is being used to call PreflightCheckGSSAPI. I've added comments and checks to make sure we don't get a segfault.
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [rpc] Assume existence of TlsContext during negotiation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [rpc] Assume existence of TlsContext during negotiation
......................................................................
Patch Set 2:
> (1 comment)
ok, doing that in a separate patch sounds like a good option.
--
To view, visit http://gerrit.cloudera.org:8080/5946
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I806ca360bcd168b1ab075a4c7143436a7ccbba06
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No