[kudu-CR] client: trust master's cert, adopt authn token
Dan Burkert has submitted this change and it was merged. Change subject: client: trust master's cert, adopt authn token .. client: trust master's cert, adopt authn token This changes the client to actually act on the security-related data coming back from the ConnectToMaster() RPC. The client now adds the master's CA cert to its trusted list, and adopts the authentication token in a class member. The token isn't used yet -- that's waiting on some concurrent work by Dan to use tokens during RPC negotiation. But, a unit test verifies that the token is getting set. Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Reviewed-on: http://gerrit.cloudera.org:8080/5899 Tested-by: Kudu Jenkins Reviewed-by: Dan Burkert--- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/master/master.proto M src/kudu/rpc/messenger.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h 8 files changed, 79 insertions(+), 4 deletions(-) Approvals: Dan Burkert: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] client: trust master's cert, adopt authn token
Dan Burkert has posted comments on this change. Change subject: client: trust master's cert, adopt authn token .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] client: trust master's cert, adopt authn token
Dan Burkert has posted comments on this change. Change subject: client: trust master's cert, adopt authn token .. Patch Set 7: Code-Review+2 restarted verify, I think it was fallout from the workspace issues. -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] client: trust master's cert, adopt authn token
Alexey Serbin has posted comments on this change. Change subject: client: trust master's cert, adopt authn token .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] client: trust master's cert, adopt authn token
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5899 to look at the new patch set (#7). Change subject: client: trust master's cert, adopt authn token .. client: trust master's cert, adopt authn token This changes the client to actually act on the security-related data coming back from the ConnectToMaster() RPC. The client now adds the master's CA cert to its trusted list, and adopts the authentication token in a class member. The token isn't used yet -- that's waiting on some concurrent work by Dan to use tokens during RPC negotiation. But, a unit test verifies that the token is getting set. Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/master/master.proto M src/kudu/rpc/messenger.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h 8 files changed, 79 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5899/7 -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] client: trust master's cert, adopt authn token
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5899 to look at the new patch set (#6). Change subject: client: trust master's cert, adopt authn token .. client: trust master's cert, adopt authn token This changes the client to actually act on the security-related data coming back from the ConnectToMaster() RPC. The client now adds the master's CA cert to its trusted list, and adopts the authentication token in a class member. The token isn't used yet -- that's waiting on some concurrent work by Dan to use tokens during RPC negotiation. But, a unit test verifies that the token is getting set. Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/master/master.proto M src/kudu/rpc/messenger.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h 8 files changed, 79 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5899/6 -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] client: trust master's cert, adopt authn token
Todd Lipcon has posted comments on this change. Change subject: client: trust master's cert, adopt authn token .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/5899/5/src/kudu/client/master_rpc.cc File src/kudu/client/master_rpc.cc: Line 271: user_cb_(status, addrs_[leader_idx_], std::move(responses_[leader_idx_])); > warning: passing result of std::move() as a const reference argument; no mo hm, I think this is not working because the PB type itself isn't movable. this isn't a hot path so I'm gonna go back to the const-ref arg -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] client: trust master's cert, adopt authn token
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5899 to look at the new patch set (#5). Change subject: client: trust master's cert, adopt authn token .. client: trust master's cert, adopt authn token This changes the client to actually act on the security-related data coming back from the ConnectToMaster() RPC. The client now adds the master's CA cert to its trusted list, and adopts the authentication token in a class member. The token isn't used yet -- that's waiting on some concurrent work by Dan to use tokens during RPC negotiation. But, a unit test verifies that the token is getting set. Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/client/master_rpc.cc M src/kudu/client/master_rpc.h M src/kudu/master/master.proto M src/kudu/rpc/messenger.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h 10 files changed, 83 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5899/5 -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] client: trust master's cert, adopt authn token
Todd Lipcon has posted comments on this change. Change subject: client: trust master's cert, adopt authn token .. Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/5899/4/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: PS4, Line 614: for (const string& cert_der : connect_response.ca_cert_der()) > So, it's possible to pass multiple CA certificates back and they are not su will do Line 623: s = messenger_->mutable_tls_context()->AddTrustedCertificate(cert); > I would have expected this to require a std::move, since I don't think Cert AddTrustedCertificate takes a const ref (it accesses the underlying X509 object, doesn't take ownership) Line 641: authn_token_ = connect_response.authn_token(); > Consider a swap or move here. done. had to change around a few parameters for it to work http://gerrit.cloudera.org:8080/#/c/5899/4/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 4658: ASSERT_TRUE(client_->data_->authn_token_ != boost::none); > I don't think you should need the '!= boost::none', it should implicitly ca Doesn't appear to, probably because it would require multiple implicit-casts ../../src/kudu/client/client-test.cc:4658:3: error: no matching conversion for functional-style cast from 'boost::optional' to '::testing::AssertionResult' ASSERT_TRUE(client_->data_->authn_token_); -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] client: trust master's cert, adopt authn token
Alexey Serbin has posted comments on this change. Change subject: client: trust master's cert, adopt authn token .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/5899/4/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: PS4, Line 614: for (const string& cert_der : connect_response.ca_cert_der()) So, it's possible to pass multiple CA certificates back and they are not supposed to form a chain, right? So, just multiple root CA certs. If so, consider adding a comment in the corresponding field of the protobuf message about that. -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] client: trust master's cert, adopt authn token
Dan Burkert has posted comments on this change. Change subject: client: trust master's cert, adopt authn token .. Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/5899/4/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: Line 623: s = messenger_->mutable_tls_context()->AddTrustedCertificate(cert); I would have expected this to require a std::move, since I don't think Cert has a copy ctor, do you know what's happening here? Line 641: authn_token_ = connect_response.authn_token(); Consider a swap or move here. http://gerrit.cloudera.org:8080/#/c/5899/4/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 4658: ASSERT_TRUE(client_->data_->authn_token_ != boost::none); I don't think you should need the '!= boost::none', it should implicitly cast to a bool like unique_ptr. -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] client: trust master's cert, adopt authn token
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5899 to look at the new patch set (#4). Change subject: client: trust master's cert, adopt authn token .. client: trust master's cert, adopt authn token This changes the client to actually act on the security-related data coming back from the ConnectToMaster() RPC. The client now adds the master's CA cert to its trusted list, and adopts the authentication token in a class member. The token isn't used yet -- that's waiting on some concurrent work by Dan to use tokens during RPC negotiation. But, a unit test verifies that the token is getting set. Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/rpc/messenger.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h 7 files changed, 75 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5899/4 -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] client: trust master's cert, adopt authn token
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5899 to look at the new patch set (#3). Change subject: client: trust master's cert, adopt authn token .. client: trust master's cert, adopt authn token This changes the client to actually act on the security-related data coming back from the ConnectToMaster() RPC. The client now adds the master's CA cert to its trusted list, and adopts the authentication token in a class member. The token isn't used yet -- that's waiting on some concurrent work by Dan to use tokens during RPC negotiation. But, a unit test verifies that the token is getting set. Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/master/master_service.cc M src/kudu/rpc/messenger.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h 8 files changed, 103 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5899/3 -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] client: trust master's cert, adopt authn token
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5899 to look at the new patch set (#2). Change subject: client: trust master's cert, adopt authn token .. client: trust master's cert, adopt authn token This changes the client to actually act on the security-related data coming back from the ConnectToMaster() RPC. The client now adds the master's CA cert to its trusted list, and adopts the authentication token in a class member. The token isn't used yet -- that's waiting on some concurrent work by Dan to use tokens during RPC negotiation. But, a unit test verifies that the token is getting set. Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/rpc/messenger.h M src/kudu/security/tls_context.cc M src/kudu/security/tls_context.h 7 files changed, 57 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5899/2 -- To view, visit http://gerrit.cloudera.org:8080/5899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa3eec178110b77d81608cfdce989e0fe494ee05 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins