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/03 00:40:47 UTC

[kudu-CR] [security] add channel binding to krpc 

Hello Todd Lipcon, Alexey Serbin,

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

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

to review the following change.

Change subject: [security] add channel binding to krpc
......................................................................

[security] add channel binding to krpc

Channel binding prevents a MITM attack when using unauthenticated TLS
with Kerberos. The channel binding codepath is exercised by the existing
TLS + GSSAPI negotiation test, but I'm punting on testing that it
protects against a MITM for now.

Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/status.cc
13 files changed, 282 insertions(+), 25 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
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] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 2:

(26 comments)

http://gerrit.cloudera.org:8080/#/c/5884/2//COMMIT_MSG
Commit Message:

Line 7: [security] add channel binding to krpc
> can you add a little bit to rpc.md about it?
Done


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

Line 500:     // Send the channel bindings to the client.
> think this is misplaced
Done


Line 503:     RETURN_NOT_OK(tls_socket->GetRemoteCert(&cert));
> I think a _PREPEND here and below on 506 would be useful
Done


Line 508:     if (response.channel_bindings().empty()) {
> is .has_channel_bindings() more appropriate?
Done


PS2, Line 512:     string actual_channel_bindings;
             :     RETURN_NOT_OK(SaslDecode(response.channel_bindings(), &actual_channel_bindings));
> "actual" doesn't sound right to me. Maybe "channel_bindings_from_server"? o
Done


Line 516:       // TODO(dan): should we do a more drastic warning? This indicates an active MITM.
> yea I think a LOG(WARNING) is probably worth it (if we can include the remo
Done


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

Line 167:   Status HandleSaslSuccess(const NegotiatePB& response) WARN_UNUSED_RESULT;
> nit: doc
Done


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:

Line 126:   optional bytes channel_bindings = 6 [(REDACT) = true];
> hrm, does this need to be redacted? what would someone gain by seeing it? (
Yes, it should be perfectly safe to log unredacted.  The reason I added redaction is to keep it consistent with 'token' and 'tls_handhsake'.  All three are also safe to log, but have limited debugging usefulness (they are just binary).


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

Line 372:   sasl_security_properties_t sec_props = {};
> does this guarantee zero-initialization? or is a memset(0) a good idea here
Done


Line 374:   sec_props.max_ssf = numeric_limits<sasl_ssf_t>::max();
> error: no template named 'numeric_limits'; did you mean 'std::numeric_limit
Done


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

Line 550:     // TODO(dan): this should actually contain the hash of the cert per RFC 5929.
> isn't that what the GetServerEndPointChannelbindings is doing?
woops, yah that was left over from an earlier wip.


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 ext
Added a small test to makes sure no failures happen and the output is of the expected length.  I think the real test of this method is going to have to be the interop between java and C++ since right now the C++ client and server are using the same impl, so if it's wrong it won't be caught (we don't have anything to check against until java does an independent impl).


PS2, Line 62: GetServerEndPointChannelBindings
> nit: consider renaming into 'ComputeChannelBindings'
tls-server-end-point is the type of channel binding, as defined in RFC 5929.


Line 63:   // Find the signature type of the certificate. This corresponds to the digest
> curious if we're really getting much out of following the RFC here vs just 
The idea is to have 'hash-agility', so if someone has gone out of their way to use a cipher with an exceptionally strong hash, that will be inherited.


Line 65:   int signature_nid = X509_get_signature_nid(data_.get());
> ruh roh
Done


PS2, Line 73: is 
> typo
This typo is in the source text in the RFC.


Line 98:   ToBIO(bio.get(), DataFormat::DER, data_.get());
> doesn't this return Status?
Done


PS2, Line 102: EVP_MAX_MD_SIZE
> style nit: would sizeof(EVP_MAX_MD_SIZE) be more appropriate here?
I believe that would return 4 or 8, e.g. sizeof(int).


PS2, Line 104: append
> assign?
Done


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
I think CRYPTO_add just returns the incremented reference count, in this case.  It is consistent to add a CHECK that it's > 1, though so I'll do that.


PS2, Line 112: CHECK(X509_up_ref(data) != 1);
> From https://www.openssl.org/docs/manmaster/man3/X509_up_ref.html
Woops, good catch.  I'm running 1.0.2 on my system so it took the other branch.  I think X509_up_ref failing indicates a 0 or less refcount, which indicates a use after free situation, so I think the CHECK is appropriate.


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.h
File src/kudu/security/cert.h:

Line 45:   void AdoptAndIncrementRawData(X509* data);
> hrm, maybe AdoptAndAddRefRawData() or something? it's not really incrementi
Done


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?
Done


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:

Line 30: template<> struct SslTypeTraits<X509> {
> hrm, can we move this to a common header? eg openssl_util-inl.h or somethin
I'm going to put it in openssl_util.h, since it's not saving us any imports to put them in another file.


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/tls_socket.h
File src/kudu/security/tls_socket.h:

Line 20: #include <boost/optional/optional_fwd.hpp>
> hm, used?
Done


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/util/status.cc
File src/kudu/util/status.cc:

Line 25:   CHECK(code != kOk);
> I think this should be DCHECK
Done


-- 
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 <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 6:

Latest changes are just a rebase against todd's recently landed patches.

-- 
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: 6
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


[security] add channel binding to krpc

Channel binding prevents a MITM attack when using unauthenticated TLS
with Kerberos. The channel binding codepath is exercised by the existing
TLS + GSSAPI negotiation test, but I'm punting on testing that it
protects against a MITM for now.

Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
Reviewed-on: http://gerrit.cloudera.org:8080/5884
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/rpc_header.proto
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/status.cc
17 files changed, 369 insertions(+), 65 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
Gerrit-PatchSet: 7
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 6: Code-Review+2

-- 
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: 6
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [security] add channel binding to krpc

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

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

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

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

Change subject: [security] add channel binding to krpc
......................................................................

[security] add channel binding to krpc

Channel binding prevents a MITM attack when using unauthenticated TLS
with Kerberos. The channel binding codepath is exercised by the existing
TLS + GSSAPI negotiation test, but I'm punting on testing that it
protects against a MITM for now.

Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
---
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/status.cc
13 files changed, 282 insertions(+), 25 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
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] [security] add channel binding to krpc

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/5884

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

Change subject: [security] add channel binding to krpc
......................................................................

[security] add channel binding to krpc

Channel binding prevents a MITM attack when using unauthenticated TLS
with Kerberos. The channel binding codepath is exercised by the existing
TLS + GSSAPI negotiation test, but I'm punting on testing that it
protects against a MITM for now.

Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/status.cc
17 files changed, 379 insertions(+), 62 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
Gerrit-PatchSet: 4
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5884/3/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

PS3, Line 69: NotAuthorized(
> nit: would NotFound or NotSupported be a better fit?
Done


PS3, Line 104: CHECK
> style nit: consider using OPENSSL_CHECK_OK instead
Done


http://gerrit.cloudera.org:8080/#/c/5884/3/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

PS3, Line 27: #include "kudu/gutil/strings/substitute.h"
> nit: probably, this is no longer needed in this file.
Done


-- 
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: 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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 2:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/5884/2//COMMIT_MSG
Commit Message:

Line 7: [security] add channel binding to krpc
can you add a little bit to rpc.md about it?


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

Line 500:     // Send the channel bindings to the client.
think this is misplaced


Line 503:     RETURN_NOT_OK(tls_socket->GetRemoteCert(&cert));
I think a _PREPEND here and below on 506 would be useful


Line 508:     if (response.channel_bindings().empty()) {
is .has_channel_bindings() more appropriate?


PS2, Line 512:     string actual_channel_bindings;
             :     RETURN_NOT_OK(SaslDecode(response.channel_bindings(), &actual_channel_bindings));
"actual" doesn't sound right to me. Maybe "channel_bindings_from_server"? or "received_channel_bindings" or something?


Line 516:       // TODO(dan): should we do a more drastic warning? This indicates an active MITM.
yea I think a LOG(WARNING) is probably worth it (if we can include the remote peer addr/port that would be nice too)


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

Line 167:   Status HandleSaslSuccess(const NegotiatePB& response) WARN_UNUSED_RESULT;
nit: doc


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:

Line 126:   optional bytes channel_bindings = 6 [(REDACT) = true];
hrm, does this need to be redacted? what would someone gain by seeing it? (I think the fact that it's just integrity-protected means that it's OK for this to be over an untrusted channel or in a log)


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

Line 372:   sasl_security_properties_t sec_props = {};
does this guarantee zero-initialization? or is a memset(0) a good idea here? (not sure if there are any other fields)


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

Line 550:     // TODO(dan): this should actually contain the hash of the cert per RFC 5929.
isn't that what the GetServerEndPointChannelbindings is doing?


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

Line 63:   // Find the signature type of the certificate. This corresponds to the digest
curious if we're really getting much out of following the RFC here vs just always using SHA512. Seems a fair bit of complexity and unknowns


Line 65:   int signature_nid = X509_get_signature_nid(data_.get());
> error: use of undeclared identifier 'X509_get_signature_nid' [clang-diagnos
ruh roh


PS2, Line 73: is 
typo


Line 98:   ToBIO(bio.get(), DataFormat::DER, data_.get());
doesn't this return Status?


PS2, Line 104: append
assign?


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.h
File src/kudu/security/cert.h:

Line 45:   void AdoptAndIncrementRawData(X509* data);
hrm, maybe AdoptAndAddRefRawData() or something? it's not really incrementing the data.


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:

Line 30: template<> struct SslTypeTraits<X509> {
hrm, can we move this to a common header? eg openssl_util-inl.h or something if we don't want it in openssl_util.h? or a new ssl_types.h?


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/tls_socket.h
File src/kudu/security/tls_socket.h:

Line 20: #include <boost/optional/optional_fwd.hpp>
hm, used?


http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/util/status.cc
File src/kudu/util/status.cc:

Line 25:   CHECK(code != kOk);
I think this should be DCHECK


-- 
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 <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

PS2, Line 102: CRYPTO_LOCK_X50
> I believe that would return 4 or 8, e.g. sizeof(int).
ugh, I meant sizeof(buf)  :)

It seems I got distracted while writing this.


-- 
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: 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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 5: Code-Review+2

-- 
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: 5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 4: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5884/4/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:

PS4, Line 529:            Probably via a special SASL mechanism (`KUDU_CERTIFICATE`) which
             :            short-circuits any actual SASL messages.
no need to change this here yet, but I think this is what SASL EXTERNAL was meant for


-- 
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: 4
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] add channel binding to krpc

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/5884

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

Change subject: [security] add channel binding to krpc
......................................................................

[security] add channel binding to krpc

Channel binding prevents a MITM attack when using unauthenticated TLS
with Kerberos. The channel binding codepath is exercised by the existing
TLS + GSSAPI negotiation test, but I'm punting on testing that it
protects against a MITM for now.

Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/status.cc
17 files changed, 376 insertions(+), 62 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 4: Code-Review+1

(4 comments)

Looks good to me: just a couple of nits.  I don't feel confident in SASL, so just +1.

http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

PS2, Line 110: 
> I think CRYPTO_add just returns the incremented reference count, in this ca
Yep, it seems so: CRYPTO_add() returns the result ref count.


http://gerrit.cloudera.org:8080/#/c/5884/3/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

PS3, Line 69:  the multiple 
nit: would NotFound or NotSupported be a better fit?


PS3, Line 104: f OPE
style nit: consider using OPENSSL_CHECK_OK instead


http://gerrit.cloudera.org:8080/#/c/5884/3/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:

PS3, Line 27: #include "kudu/gutil/strings/substitute.h"
nit: probably, this is no longer needed in this file.


-- 
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: 4
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] add channel binding to krpc

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

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

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

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

Change subject: [security] add channel binding to krpc
......................................................................

[security] add channel binding to krpc

Channel binding prevents a MITM attack when using unauthenticated TLS
with Kerberos. The channel binding codepath is exercised by the existing
TLS + GSSAPI negotiation test, but I'm punting on testing that it
protects against a MITM for now.

Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/status.cc
18 files changed, 379 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/5884/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5884
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
Gerrit-PatchSet: 5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [security] add channel binding to krpc

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

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

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

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

Change subject: [security] add channel binding to krpc
......................................................................

[security] add channel binding to krpc

Channel binding prevents a MITM attack when using unauthenticated TLS
with Kerberos. The channel binding codepath is exercised by the existing
TLS + GSSAPI negotiation test, but I'm punting on testing that it
protects against a MITM for now.

Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
---
M docs/design-docs/rpc.md
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/client_negotiation.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/server_negotiation.h
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_handshake.cc
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/status.cc
17 files changed, 369 insertions(+), 65 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/5884/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5884
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id73fceebfcb47c881c30f6904cfd6fc6d80f50b8
Gerrit-PatchSet: 6
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [security] add channel binding to krpc

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [security] add channel binding to krpc

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

Change subject: [security] add channel binding to krpc
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5884/2/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

PS2, Line 102: 
> ugh, I meant sizeof(buf)  :)
Done


http://gerrit.cloudera.org:8080/#/c/5884/3/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:

Line 52:   int signature_nid = X509_get_signature_nid(data_.get());
> warning: redundant get() call on smart pointer [google-readability-redundan
Done


-- 
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: 4
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes