Alexey Serbin has posted comments on this change. Change subject: [security] add channel binding to krpc ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.cc File src/kudu/security/cert.cc: PS2, Line 62: Status Cert::GetServerEndPointChannelBindings(string* channel_bindings) I think it's worth adding a test into cert_management-tests.cc to check extracting this info from generated certs. PS2, Line 62: GetServerEndPointChannelBindings nit: consider renaming into 'ComputeChannelBindings' Why ServerEndPoint is crucial in this context? PS2, Line 102: EVP_MAX_MD_SIZE style nit: would sizeof(EVP_MAX_MD_SIZE) be more appropriate here? PS2, Line 110: CRYPTO_add(&data->references, 1, CRYPTO_LOCK_X509); I think CRYPTO_add also returns error code. Does it make sense to check it? PS2, Line 112: CHECK(X509_up_ref(data) != 1); >From https://www.openssl.org/docs/manmaster/man3/X509_up_ref.html X509_up_ref() returns 1 for success and 0 for failure. If so, then I don't understand what this check is for. Should this operation always fail? http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.h File src/kudu/security/cert.h: PS2, Line 42: GetServerEndPointChannelBindings(std::string* channel_bindings) Is it possible to compute that using openssl's digest tools? If yes, could you add the reference command line in the comment? http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/openssl_util.cc File src/kudu/security/openssl_util.cc: PS2, Line 30: #include <openssl/x509.h> Why is it needed in here? -- To view, visit http://gerrit.cloudera.org:8080/5884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
