You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/01/31 21:49:22 UTC
[kudu-CR] tls: move setting of verification modes into TlsHandshake
Hello Dan Burkert, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5839
to review the following change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
tls: move setting of verification modes into TlsHandshake
Depending on the particular type of connection, we need to configure the
TLS verification differently. For example, a connection that uses a
token for client authentication needs to verify the server cert for
server authentication, whereas the initial connection uses Kerberos for
mutual authentication and doesn't need TLS authentication at all.
As such, the global configuration of SSL verification mode in TlsContext
is no longer appropriate. This moves the configuration to be
per-TlsHandshake.
Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
---
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_handshake.h
4 files changed, 229 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5839/1
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] tls: move setting of verification modes into TlsHandshake
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Alexey Serbin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5839
to look at the new patch set (#2).
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
tls: move setting of verification modes into TlsHandshake
Depending on the particular type of connection, we need to configure the
TLS verification differently. For example, a connection that uses a
token for client authentication needs to verify the server cert for
server authentication, whereas the initial connection uses Kerberos for
mutual authentication and doesn't need TLS authentication at all.
As such, the global configuration of SSL verification mode in TlsContext
is no longer appropriate. This moves the configuration to be
per-TlsHandshake.
Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
---
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_handshake.h
4 files changed, 227 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5839/2
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tls: move setting of verification modes into TlsHandshake
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5839/1/src/kudu/security/tls_handshake-test.cc
File src/kudu/security/tls_handshake-test.cc:
PS1, Line 134: Test_ClientNone_ServerSelfSigned
It might make sense to add a test for expired certificates as well. Probably, it's better to do that in a separate changelist. If so, consider adding a TODO for expired certs.
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tls: move setting of verification modes into TlsHandshake
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
Patch Set 1: Verified+1
Flaky test being addressed elsewhere
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] tls: move setting of verification modes into TlsHandshake
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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] tls: move setting of verification modes into TlsHandshake
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5839/1/src/kudu/security/tls_handshake-test.cc
File src/kudu/security/tls_handshake-test.cc:
PS1, Line 142: ASSERT_STR_MATCHES(s.
does it make sense to check for the status code as well?
PS1, Line 155: // It should still succeed with verification off, as well.
: ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE,
: TlsVerificationMode::VERIFY_NONE));
Why is it necessary to verify this as well here? As I see, this case seems to be covered by the case above. Why would adding cert authority would change the result of verification for VERIFY_NONE mode?
http://gerrit.cloudera.org:8080/#/c/5839/1/src/kudu/security/tls_handshake.h
File src/kudu/security/tls_handshake.h:
PS1, Line 65: use
nit: remove
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tls: move setting of verification modes into TlsHandshake
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] tls: move setting of verification modes into TlsHandshake
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5839
to look at the new patch set (#3).
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
tls: move setting of verification modes into TlsHandshake
Depending on the particular type of connection, we need to configure the
TLS verification differently. For example, a connection that uses a
token for client authentication needs to verify the server cert for
server authentication, whereas the initial connection uses Kerberos for
mutual authentication and doesn't need TLS authentication at all.
As such, the global configuration of SSL verification mode in TlsContext
is no longer appropriate. This moves the configuration to be
per-TlsHandshake.
Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
---
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_handshake.h
4 files changed, 225 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/5839/3
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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] tls: move setting of verification modes into TlsHandshake
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5839/1/src/kudu/security/tls_handshake-test.cc
File src/kudu/security/tls_handshake-test.cc:
PS1, Line 142: ASSERT_STR_MATCHES(s.
> does it make sense to check for the status code as well?
sure will do
PS1, Line 155: // It should still succeed with verification off, as well.
: ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE,
: TlsVerificationMode::VERIFY_NONE));
> Why is it necessary to verify this as well here? As I see, this case seems
fair enough
http://gerrit.cloudera.org:8080/#/c/5839/1/src/kudu/security/tls_handshake.h
File src/kudu/security/tls_handshake.h:
PS1, Line 65: use
> nit: remove
I think this is correct as is. Parse as: must be initialized (before use) (using TlsContext::InitiateHandshake)
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tls: move setting of verification modes into TlsHandshake
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
tls: move setting of verification modes into TlsHandshake
Depending on the particular type of connection, we need to configure the
TLS verification differently. For example, a connection that uses a
token for client authentication needs to verify the server cert for
server authentication, whereas the initial connection uses Kerberos for
mutual authentication and doesn't need TLS authentication at all.
As such, the global configuration of SSL verification mode in TlsContext
is no longer appropriate. This moves the configuration to be
per-TlsHandshake.
Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Reviewed-on: http://gerrit.cloudera.org:8080/5839
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_handshake.h
4 files changed, 225 insertions(+), 21 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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] tls: move setting of verification modes into TlsHandshake
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
Patch Set 2:
I've got this rebased on top of Alexey's change that moves everything to new files, etc. Waiting on his to be +2ed to push my rebase, though.
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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] tls: move setting of verification modes into TlsHandshake
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: tls: move setting of verification modes into TlsHandshake
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5839/1/src/kudu/security/tls_handshake.h
File src/kudu/security/tls_handshake.h:
PS1, Line 65: use
> I think this is correct as is. Parse as: must be initialized (before use) (
ok, thanks -- now it makes more sense to me.
--
To view, visit http://gerrit.cloudera.org:8080/5839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37cbc20b1a70a7469a2a2f43702599b1b55ff294
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@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