[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] do actual token signing/verification
..


Patch Set 12: Code-Review+2

Propagating +2 from Todd's review to this version (comment updates).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [security] do actual token signing/verification
..


[security] do actual token signing/verification

Replaced temporary stub sign/verify calls with calls of actual
cryptographic functions.

Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Reviewed-on: http://gerrit.cloudera.org:8080/5812
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/security/token_signing_key.cc
M src/kudu/security/token_signing_key.h
M src/kudu/security/token_verifier.cc
3 files changed, 42 insertions(+), 20 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Hello Todd Lipcon,

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

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

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

Change subject: [security] do actual token signing/verification
..

[security] do actual token signing/verification

Replaced temporary stub sign/verify calls with calls of actual
cryptographic functions.

Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
---
M src/kudu/security/token_signing_key.cc
M src/kudu/security/token_signing_key.h
M src/kudu/security/token_verifier.cc
3 files changed, 42 insertions(+), 20 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] do actual token signing/verification
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5812/11/src/kudu/security/token_signing_key.h
File src/kudu/security/token_signing_key.h:

Line 35: class TokenSigningPublicKey {
> would be good to keep a comment saying 'This class is thread-safe after Ini
Added the comment for VerifySignature().


http://gerrit.cloudera.org:8080/#/c/5812/11/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS11, Line 56: copy construction
> I guess this is a bit out of date -- probably should say just say 'construc
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] openssl util: avoid non-POD vector in static storage

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin,

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

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

to review the following change.

Change subject: openssl_util: avoid non-POD vector in static storage
..

openssl_util: avoid non-POD vector in static storage

This addresses an ASAN crash I've seen a couple times now in test runs
during static destruction. We previously used a vector for the
OpenSSL locks, but the order of static destruction isn't very easy to
predict, and the destructor of things like SSL sockets may end up
needing to acquire these locks.

This patch switches to a C-style dynamic array.

An example ASAN trace follows.

=
==28629==ERROR: AddressSanitizer: heap-use-after-free on address 0x6130b6c0 
at pc 0x7f25c7d03339 bp 0x7f25bbdc3af0 sp 0x7f25bbdc3ae8
READ of size 8 at 0x6130b6c0 thread T4 (rpc reactor-286)
#0 0x7f25c7d03338 in kudu::security::(anonymous namespace)::LockingCB(int, 
int, char const*, int) 
/home/jenkins-slave/workspace/kudu-1/src/kudu/security/openssl_util.cc:54:14
#1 0x7f25c4c41836 in CRYPTO_add_lock 
(/lib/x86_64-linux-gnu/libcrypto.so.1.0.0+0x5f836)
#2 0x7f25c49bcedb in SSL_free 
(/lib/x86_64-linux-gnu/libssl.so.1.0.0+0x39edb)
#3 0x7f25c7d10f20 in std::_Function_handler::_M_invoke(std::_Any_data const&, ssl_st*) 
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:2071:2
#4 0x7f25c8764ed4 in std::function::operator()(ssl_st*) 
const 
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:2471:14
#5 0x7f25c7d0daf7 in std::unique_ptr::reset(ssl_st*) 
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/unique_ptr.h:262:4
#6 0x7f25c7d14f78 in kudu::security::TlsSocket::Close() 
/home/jenkins-slave/workspace/kudu-1/src/kudu/security/tls_socket.cc:115:8
#7 0x7f25c8773072 in kudu::rpc::Connection::Shutdown(kudu::Status const&) 
/home/jenkins-slave/workspace/kudu-1/src/kudu/rpc/connection.cc:174:5
#8 0x7f25c87bfb25 in kudu::rpc::ReactorThread::ShutdownInternal() 
/home/jenkins-slave/workspace/kudu-1/src/kudu/rpc/reactor.cc:134:11
#9 0x7f25c87c0f3f in kudu::rpc::ReactorThread::AsyncHandler(ev::async&, 
int) /home/jenkins-slave/workspace/kudu-1/src/kudu/rpc/reactor.cc:198:5
...

0x6130b6c0 is located 128 bytes inside of 328-byte region 
[0x6130b640,0x6130b788)
freed by thread T0 here:
#0 0x5c1f30 in operator delete(void*) 
/home/jenkins-slave/workspace/kudu-1/thirdparty/src/llvm-3.9.1.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:110
#1 0x7f25c7d05235 in std::_Vector_base >::~_Vector_base() 
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_vector.h:160:9
#2 0x7f25c2b30539 in __cxa_finalize 
/build/eglibc-oGUzwX/eglibc-2.19/stdlib/cxa_finalize.c:56

previously allocated by thread T0 here:
#0 0x5c1870 in operator new(unsigned long) 
/home/jenkins-slave/workspace/kudu-1/thirdparty/src/llvm-3.9.1.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:78
#1 0x7f25c7d03cfc in kudu::Mutex** std::vector 
>::_M_allocate_and_copy >(unsigned long, 
std::move_iterator, std::move_iterator) 
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../
#2 0x7f25c7d038e6 in std::vector 
>::reserve(unsigned long) 
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/vector.tcc:73:20
#3 0x7f25c7d02420 in kudu::security::(anonymous 
namespace)::DoInitializeOpenSSL() 
/home/jenkins-slave/workspace/kudu-1/src/kudu/security/openssl_util.cc:78:16
...

Change-Id: Id6fdd1162eb39114c67f3c46073345829530434f
---
M src/kudu/security/openssl_util.cc
1 file changed, 7 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id6fdd1162eb39114c67f3c46073345829530434f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] [security] Add negotiation test with TLS + GSSAPI

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] Add negotiation test with TLS + GSSAPI
..


Patch Set 1:

Just in case you missed it on slack, here's what I said:

> @dan my patch as it is doesn't fix your verification problem -- haven't 
> hooked together the bit that says "if I"m using GSSAPI I don't need to verify 
> the server cert"
> although my tip patch (the WIP integration one) does set VERIFY_NONE for both 
> client and server negotiation
> so in that sense it "fixes" it, but it also just disables security

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I37a3010c7b13e9cb64f1f9397b39efcc4bf3ab41
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] do actual token signing/verification

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] do actual token signing/verification
..


Patch Set 11: Code-Review+2

(3 comments)

Just a couple nits, feel free to commit after fixing.

http://gerrit.cloudera.org:8080/#/c/5812/11/src/kudu/security/token_signing_key.h
File src/kudu/security/token_signing_key.h:

Line 34: // This represents a standalone public key useful for token 
verification.
oops, thanks for correcting this comment that I forgot to update last time.


Line 35: class TokenSigningPublicKey {
would be good to keep a comment saying 'This class is thread-safe after 
Init()'. Or, since there's only one other interesting method, you could add a 
'This is thread-safe' on VerifySignature().


http://gerrit.cloudera.org:8080/#/c/5812/11/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS11, Line 56: copy construction
I guess this is a bit out of date -- probably should say just say 
'construction' now?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] do actual token signing/verification

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] do actual token signing/verification
..


Patch Set 11:

I think I blew away the workspace that was causing that error (it bit me on a 
couple builds too)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1856: always truncate containers when they get full

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1856: always truncate containers when they get full
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5852/1//COMMIT_MSG
Commit Message:

PS1, Line 18: truncate. Instead, all full containers are truncated 
unconditionally. The
: assumption here is that a no-op truncate should be free.
I think it's worth validating this assumption (eg by timing startup on one of 
these hosts that has 10k+ full containers, after a drop_cache=3). It would suck 
to find out only upon release that this is causing a journal fsync per truncate 
call or something.


http://gerrit.cloudera.org:8080/#/c/5852/1/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

Line 1080: ASSERT_OK(env_->GetFileSizeOnDisk(fname, 
_after_preallocate));
have you tried this new test on ext4 on el6? I'm not sure whether fallocate 
KEEP_SIZE works on old ext4, would be good to make sure we don't have a 
surprise test failure in that env


http://gerrit.cloudera.org:8080/#/c/5852/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 137:   CHECK_EQ(0, mode);
I thought it was mode, 0


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic959c59489a08f92efa2df5c85b22e56740f1346
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] do actual token signing/verification
..


Patch Set 11: Verified+1

Unrelated breakage:

19:15:06 CMake Error at CMakeLists.txt:917 (find_package):
19:15:06   Could not find a package configuration file provided by "LLVM" with 
any of
19:15:06   the following names:
19:15:06 
19:15:06 LLVMConfig.cmake
19:15:06 llvm-config.cmake
19:15:06 
19:15:06   Add the installation prefix of "LLVM" to CMAKE_PREFIX_PATH or set
19:15:06   "LLVM_DIR" to a directory containing one of the above files.  If 
"LLVM"
19:15:06   provides a separate development package or SDK, be sure it has been
19:15:06   installed.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1856: always truncate containers when they get full

[Adar Dembo (Code Review)]

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: KUDU-1856: always truncate containers when they get full
..

KUDU-1856: always truncate containers when they get full

On a production cluster, I found that XFS's speculative preallocation
feature often winds up permanently growing LBM containers without increasing
their file sizes. It's not clear why this happens; I don't think we're
running afoul of any of the conditions spelled out in the XFS FAQ [1].
Nevertheless, on this cluster the preallocated space accounted for almost a
third of the total data consumption, so we need to address it.

This patch changes the existing LBM container truncation behavior such that
at startup, the container's file size no longer determines whether to
truncate. Instead, all full containers are truncated unconditionally. The
assumption here is that a no-op truncate should be free.

We don't always run on XFS, and triggering its speculative preallocation is
hard. But, we can approximate it by passing FALLOC_FL_KEEP_SIZE to
fallocate(), which grows the file without changing its size.

1. http://xfs.org/index.php/XFS_FAQ#Q:_Is_speculative_preallocation_permanent.3F

Change-Id: Ic959c59489a08f92efa2df5c85b22e56740f1346
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
6 files changed, 59 insertions(+), 29 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic959c59489a08f92efa2df5c85b22e56740f1346
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: tls: hook up internal PKI system to TlsContext

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: WIP: tls: hook up internal PKI system to TlsContext
..

WIP: tls: hook up internal PKI system to TlsContext

This hooks up the existing ServerCertManager to TlsContext, so that once
a server has a cert signed by the master's CA, that cert will be
configured in the Messenger's TlsContext.

A number of changes were necessary to get this to work:

* The master needs to sign its own server cert with its CA cert. As
  before, this is still done in the "wrong place" -- we eventually need
  to do it somewhere in the CatalogManager after we've loaded the
  persisted CA cert. But, for now, we're using the ephemeral one and
  self-signing at startup.

* When the master signs a tablet server's cert, it needs to send back
  the CA cert as well as the signed TS cert. The TS then needs to add
  the CA cert as a trusted cert in its TlsContext.

* Noticed that we hadn't marked the cert fields as redacted in the proto
  and did so along the way.

* Changed negotiation so that, rather than depending on the global
  configuration flags to know whether TLS was enabled, now consults the
  TlsContext to know if the context has a configured cert and CA.

* The new method to set the cert and key now verifies that the cert and
  key match each other, and that the appropriate CA certs are already
  installed such that a full trusted cert chain is established.

This is still a WIP, but I verified that I can run a tablet server and
master, and connect to them from a client, and that all of the traffic
is encrypted (based on tshark output).

Before committing we should also look at whether this causes a big
performance regression since it encrypts all traffic, and have a way to
disable TLS if so.

Change-Id: I6a98ef522e51751562d47907aa90a32c2dac3918
---
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/server_cert_manager.cc
M src/kudu/security/server_cert_manager.h
M src/kudu/security/test/test_certs.h
M src/kudu/server/server_base.cc
M src/kudu/tserver/heartbeater.cc
16 files changed, 124 insertions(+), 35 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6a98ef522e51751562d47907aa90a32c2dac3918
Gerrit-PatchSet: 5
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] security: simplify CertSigner interface

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: security: simplify CertSigner interface
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5846/2/src/kudu/security/ca/cert_management-test.cc
File src/kudu/security/ca/cert_management-test.cc:

Line 40: using std::thread;
> warning: using decl 'thread' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/5846/2/src/kudu/security/ca/cert_management.h
File src/kudu/security/ca/cert_management.h:

Line 158:   CertSigner(const Cert* ca_cert, const PrivateKey* ca_key);
> warning: function 'kudu::security::ca::CertSigner::CertSigner' has a defini
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I50ee09a8bb6fba4ab6111288769b660ce61f048b
Gerrit-PatchSet: 2
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 
Gerrit-HasComments: Yes

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [security] do actual token signing/verification
..

[security] do actual token signing/verification

Replaced temporary stub sign/verify calls with calls of actual
cryptographic functions.

Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
---
M src/kudu/security/token_signing_key.cc
M src/kudu/security/token_signing_key.h
M src/kudu/security/token_verifier.cc
3 files changed, 40 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/5812/11
-- 
To view, visit http://gerrit.cloudera.org:8080/5812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] avoid crashing when importing invalid TSKs

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [security] avoid crashing when importing invalid TSKs
..


[security] avoid crashing when importing invalid TSKs

Kudu typically tries to avoid crashing when validating data from
external processes. In this case the data is from another Kudu server,
but it's still better not to crash.

Change-Id: I51dece30df0ec612417917f6855d57432fe2f7ec
Reviewed-on: http://gerrit.cloudera.org:8080/5843
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
M src/kudu/security/token-test.cc
M src/kudu/security/token_verifier.cc
M src/kudu/security/token_verifier.h
3 files changed, 20 insertions(+), 9 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I51dece30df0ec612417917f6855d57432fe2f7ec
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [security] do actual token signing/verification
..

[security] do actual token signing/verification

Replaced temporary stub sign/verify calls with calls of actual
cryptographic functions.

Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
---
M src/kudu/security/token_signing_key.cc
M src/kudu/security/token_signing_key.h
M src/kudu/security/token_verifier.cc
3 files changed, 40 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/5812/10
-- 
To view, visit http://gerrit.cloudera.org:8080/5812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [security] sign/verify data using RSA key pair
..


[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Reviewed-on: http://gerrit.cloudera.org:8080/5805
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 234 insertions(+), 7 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Extract a static function to generate a self-signed CA

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin,

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

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

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

Change subject: Extract a static function to generate a self-signed CA
..

Extract a static function to generate a self-signed CA

We had some similar code in a test case and in MasterCertAuthority. This
extracts a function to create a self-signed CA, as well as a test
utility function which creates both a self-signed CA and private key.

Currently the test utility is only used from one test case, but I have
plans to use it elsewhere.

Change-Id: Ib40add99ed96e4753a3810709c66d07c96ab70bd
---
M src/kudu/master/master_cert_authority.cc
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
A src/kudu/security/security-test-util.cc
M src/kudu/security/security-test-util.h
7 files changed, 112 insertions(+), 54 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib40add99ed96e4753a3810709c66d07c96ab70bd
Gerrit-PatchSet: 2
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] security: simplify CertSigner interface

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin,

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

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

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

Change subject: security: simplify CertSigner interface
..

security: simplify CertSigner interface

This makes CertSigner a "one-shot" type of instance, which no longer
holds long-lived references to CA certs or keys. This also removes the
'InitFromFiles()' path in CertSigner, since that's now doable directly
using the wrapper objects, and the code ended up only used by tests.

With these changes, a bunch of other things got simplified:
- no need for shared_ptr in a few places where it had leaked
- no need for multi-threading tests in cert_management-test

Change-Id: I50ee09a8bb6fba4ab6111288769b660ce61f048b
---
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/security-test-util.cc
M src/kudu/security/security-test-util.h
9 files changed, 117 insertions(+), 317 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I50ee09a8bb6fba4ab6111288769b660ce61f048b
Gerrit-PatchSet: 2
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

[kudu-CR] [security] Add negotiation test with TLS + GSSAPI

[Dan Burkert (Code Review)]

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: [security] Add negotiation test with TLS + GSSAPI
..

[security] Add negotiation test with TLS + GSSAPI

This is a WIP since the test is not yet passing, due to the client
failing verification of the server's self-signed cert.

Change-Id: I37a3010c7b13e9cb64f1f9397b39efcc4bf3ab41
---
M src/kudu/rpc/negotiation-test.cc
1 file changed, 71 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I37a3010c7b13e9cb64f1f9397b39efcc4bf3ab41
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] avoid crashing when importing invalid TSKs

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] avoid crashing when importing invalid TSKs
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5843/1/src/kudu/security/token-test.cc
File src/kudu/security/token-test.cc:

> I'm not a huge fan of tests which just assert on error messages, but if you
nope, if you think it's good to go without that trivial coverage, I'm fine with 
that as is.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I51dece30df0ec612417917f6855d57432fe2f7ec
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Extract a static function to generate a self-signed CA

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Extract a static function to generate a self-signed CA
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5844/1/src/kudu/security/security-test-util.h
File src/kudu/security/security-test-util.h:

Line 103: Status GenerateSelfSignedCAForTests(std::shared_ptr* 
ca_key,
> warning: function 'kudu::security::GenerateSelfSignedCAForTests' has a defi
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib40add99ed96e4753a3810709c66d07c96ab70bd
Gerrit-PatchSet: 1
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 
Gerrit-HasComments: Yes

[kudu-CR] [security] introduced crypto-test

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [security] introduced crypto-test
..


[security] introduced crypto-test

The new crypto-test module is for test scenarios involving common
non-CA crypto entities in the Kudu security library.  The CA-related
scenarios are kept in the cert_management-test.

Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Reviewed-on: http://gerrit.cloudera.org:8080/5798
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management-test.cc
A src/kudu/security/crypto-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 192 insertions(+), 70 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] method to extract public part of an RSA key

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: [security] method to extract public part of an RSA key
..


[security] method to extract public part of an RSA key

Added a method to get the public part of an RSA keypair.
Split the Key class into PublicKey and PrivateKey.  Reorganized the
layout of files in the src/security sub-directory: separated the common
crypto stuff from openssl_util.h into crypto.h.

Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Reviewed-on: http://gerrit.cloudera.org:8080/5783
Reviewed-by: Dan Burkert 
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
---
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/security/CMakeLists.txt
R src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
A src/kudu/security/cert.cc
A src/kudu/security/cert.h
A src/kudu/security/crypto.cc
A src/kudu/security/crypto.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
A src/kudu/security/openssl_util_bio.h
M src/kudu/security/server_cert_manager.cc
M src/kudu/security/server_cert_manager.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/tls_handshake.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signing_key.cc
M src/kudu/security/token_signing_key.h
23 files changed, 703 insertions(+), 350 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Extract a static function to generate a self-signed CA

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin,

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

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

to review the following change.

Change subject: Extract a static function to generate a self-signed CA
..

Extract a static function to generate a self-signed CA

We had some similar code in a test case and in MasterCertAuthority. This
extracts a function to create a self-signed CA, as well as a test
utility function which creates both a self-signed CA and private key.

Currently the test utility is only used from one test case, but I have
plans to use it elsewhere.

Change-Id: Ib40add99ed96e4753a3810709c66d07c96ab70bd
---
M src/kudu/master/master_cert_authority.cc
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
A src/kudu/security/security-test-util.cc
M src/kudu/security/security-test-util.h
7 files changed, 112 insertions(+), 54 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib40add99ed96e4753a3810709c66d07c96ab70bd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] Allow configuring TlsContext with key wrappers

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin,

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

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

to review the following change.

Change subject: Allow configuring TlsContext with key wrappers
..

Allow configuring TlsContext with key wrappers

Change-Id: Idb82427aea5f1bd29bad7529f2d54638b90811e2
---
M src/kudu/rpc/messenger.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
6 files changed, 126 insertions(+), 38 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idb82427aea5f1bd29bad7529f2d54638b90811e2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] WIP: tls: hook up internal PKI system to TlsContext

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: WIP: tls: hook up internal PKI system to TlsContext
..

WIP: tls: hook up internal PKI system to TlsContext

This hooks up the existing ServerCertManager to TlsContext, so that once
a server has a cert signed by the master's CA, that cert will be
configured in the Messenger's TlsContext.

A number of changes were necessary to get this to work:

* The master needs to sign its own server cert with its CA cert. As
  before, this is still done in the "wrong place" -- we eventually need
  to do it somewhere in the CatalogManager after we've loaded the
  persisted CA cert. But, for now, we're using the ephemeral one and
  self-signing at startup.

* When the master signs a tablet server's cert, it needs to send back
  the CA cert as well as the signed TS cert. The TS then needs to add
  the CA cert as a trusted cert in its TlsContext.

* Noticed that we hadn't marked the cert fields as redacted in the proto
  and did so along the way.

* Changed negotiation so that, rather than depending on the global
  configuration flags to know whether TLS was enabled, now consults the
  TlsContext to know if the context has a configured cert and CA.

* The new method to set the cert and key now verifies that the cert and
  key match each other, and that the appropriate CA certs are already
  installed such that a full trusted cert chain is established.

This is still a WIP, but I verified that I can run a tablet server and
master, and connect to them from a client, and that all of the traffic
is encrypted (based on tshark output).

Before committing we should also look at whether this causes a big
performance regression since it encrypts all traffic, and have a way to
disable TLS if so.

Change-Id: I6a98ef522e51751562d47907aa90a32c2dac3918
---
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/server_negotiation.cc
M src/kudu/security/server_cert_manager.cc
M src/kudu/security/server_cert_manager.h
M src/kudu/security/test/test_certs.h
M src/kudu/server/server_base.cc
M src/kudu/tserver/heartbeater.cc
16 files changed, 123 insertions(+), 35 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6a98ef522e51751562d47907aa90a32c2dac3918
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] security: simplify CertSigner interface

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin,

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

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

to review the following change.

Change subject: security: simplify CertSigner interface
..

security: simplify CertSigner interface

This makes CertSigner a "one-shot" type of instance, which no longer
holds long-lived references to CA certs or keys. This also removes the
'InitFromFiles()' path in CertSigner, since that's now doable directly
using the wrapper objects, and the code ended up only used by tests.

With these changes, a bunch of other things got simplified:
- no need for shared_ptr in a few places where it had leaked
- no need for multi-threading tests in cert_management-test

Change-Id: I50ee09a8bb6fba4ab6111288769b660ce61f048b
---
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/security-test-util.cc
M src/kudu/security/security-test-util.h
9 files changed, 120 insertions(+), 308 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I50ee09a8bb6fba4ab6111288769b660ce61f048b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] method to extract public part of an RSA key

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] method to extract public part of an RSA key
..


Patch Set 14: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] do actual token signing/verification
..


Patch Set 2:

(1 comment)

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

Line 35:   CHECK(pb_.has_rsa_key_der());
> >  there are multiple CHECK() statements in TokenVerifier::ImportPublicKeys
Great!  Thank you for addressing that.  I'll update the Init() part in the next 
revision of this patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] avoid crashing when importing invalid TSKs

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] avoid crashing when importing invalid TSKs
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5843/1/src/kudu/security/token-test.cc
File src/kudu/security/token-test.cc:

> Consider adding a unit test for this update.
I'm not a huge fan of tests which just assert on error messages, but if you 
have something specific to test for I can add it.


http://gerrit.cloudera.org:8080/#/c/5843/1/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS1, Line 62: RuntimeError
> nit: does RuntimeError() seem to be the best fit compared with Corruption()
Yah, I wasn't exactly sure what to put here.  I think above all if this fails 
it indicates a version mismatch between the server versions (in the 
hypothetical case that a future version stops setting these fields).  AFAIK 
none of these really hint towards that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I51dece30df0ec612417917f6855d57432fe2f7ec
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] avoid crashing when importing invalid TSKs

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] avoid crashing when importing invalid TSKs
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5843/1/src/kudu/security/token-test.cc
File src/kudu/security/token-test.cc:

Consider adding a unit test for this update.


http://gerrit.cloudera.org:8080/#/c/5843/1/src/kudu/security/token_verifier.cc
File src/kudu/security/token_verifier.cc:

PS1, Line 62: RuntimeError
nit: does RuntimeError() seem to be the best fit compared with Corruption() and 
InvalidArgument() ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I51dece30df0ec612417917f6855d57432fe2f7ec
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] sign/verify data using RSA key pair

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 14: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] do actual token signing/verification

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] do actual token signing/verification
..


Patch Set 2:

(1 comment)

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

Line 35:   CHECK(pb_.has_rsa_key_der());
> It does not make much sense to me: there are multiple CHECK() statements in
>  there are multiple CHECK() statements in TokenVerifier::ImportPublicKeys()

That's definitely an issue, thanks for pointing it out.  I'll submit a patch 
for change ImportPublicKeys to return a Status.

In this case, though, theres no way to change the constructor to return a 
Status, so I think this needs a Status-returning Init function.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] avoid crashing when importing invalid TSKs

[Dan Burkert (Code Review)]

Hello Todd Lipcon, Alexey Serbin,

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

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

to review the following change.

Change subject: [security] avoid crashing when importing invalid TSKs
..

[security] avoid crashing when importing invalid TSKs

Kudu typically tries to avoid crashing when validating data from
external processes. In this case the data is from another Kudu server,
but it's still better not to crash.

Change-Id: I51dece30df0ec612417917f6855d57432fe2f7ec
---
M src/kudu/security/token-test.cc
M src/kudu/security/token_verifier.cc
M src/kudu/security/token_verifier.h
3 files changed, 20 insertions(+), 9 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I51dece30df0ec612417917f6855d57432fe2f7ec
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 234 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/14
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 13:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5805/13//COMMIT_MSG
Commit Message:

PS13, Line 11: SHA1
> Looks like maybe SHA1 was removed in later iterations?
right. thanks -- will update


http://gerrit.cloudera.org:8080/#/c/5805/13/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

Line 93: default:
> Wasn't the consensus to remove the default case?
ugh, this one has leaked.  will fix


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] sign/verify data using RSA key pair

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 13:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5805/13//COMMIT_MSG
Commit Message:

PS13, Line 11: SHA1
Looks like maybe SHA1 was removed in later iterations?


http://gerrit.cloudera.org:8080/#/c/5805/13/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

Line 93: default:
Wasn't the consensus to remove the default case?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] introduced crypto-test

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] introduced crypto-test
..


Patch Set 13: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] method to extract public part of an RSA key

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] method to extract public part of an RSA key
..


Patch Set 14: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] method to extract public part of an RSA key

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] method to extract public part of an RSA key
..


Patch Set 13:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5783/11/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:

Line 26: typedef struct rsa_st RSA;
> Is this still necessary?
Good catch -- will remove.


http://gerrit.cloudera.org:8080/#/c/5783/13/src/kudu/security/crypto.h
File src/kudu/security/crypto.h:

PS13, Line 48: being at different
 : // inheritance branch from the PublicKey class
> I think this would read better as "It's important to have PrivateKey and Pu
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] method to extract public part of an RSA key

[Alexey Serbin (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: [security] method to extract public part of an RSA key
..

[security] method to extract public part of an RSA key

Added a method to get the public part of an RSA keypair.
Split the Key class into PublicKey and PrivateKey.  Reorganized the
layout of files in the src/security sub-directory: separated the common
crypto stuff from openssl_util.h into crypto.h.

Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
---
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/security/CMakeLists.txt
R src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
A src/kudu/security/cert.cc
A src/kudu/security/cert.h
A src/kudu/security/crypto.cc
A src/kudu/security/crypto.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
A src/kudu/security/openssl_util_bio.h
M src/kudu/security/server_cert_manager.cc
M src/kudu/security/server_cert_manager.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/tls_handshake.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signing_key.cc
M src/kudu/security/token_signing_key.h
23 files changed, 703 insertions(+), 350 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/5783/14
-- 
To view, visit http://gerrit.cloudera.org:8080/5783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Upgrade Snappy to 1.1.4

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: Upgrade Snappy to 1.1.4
..


Upgrade Snappy to 1.1.4

In my random perusal of github, I noticed that we are a few point
releases out of date on Snappy. The release notes indicate that
compression should be ~5% faster and decompression 20% faster in 1.1.4.
v1.1.1 also claims to have increased decompression by 13-20% in some
particular benchmark cases.

Seems worth upgrading given the one-character diff.

Change-Id: Ieab52cc0c968f17b6a44328d49a42872284968bb
Reviewed-on: http://gerrit.cloudera.org:8080/5833
Tested-by: Todd Lipcon 
Reviewed-by: Mike Percy 
---
M thirdparty/vars.sh
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Todd Lipcon: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ieab52cc0c968f17b6a44328d49a42872284968bb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tls: hook up internal PKI system to TlsContext

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: tls: hook up internal PKI system to TlsContext
..


Patch Set 2:

(2 comments)

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

PS2, Line 128: ERR_clear_error();
I see this in different places as well, but why is this necessary?  Could you 
add a comment about this at least some where?

Ideally it would be nice to get rid of that, otherwise it looks like some 
errors are obliterated which could hide a problem.


http://gerrit.cloudera.org:8080/#/c/5808/2/src/kudu/security/tls_handshake-test.cc
File src/kudu/security/tls_handshake-test.cc:

PS2, Line 130: TEST_F(TestTlsHandshake, Test_ClientNone_ServerSelfSigned) {
 :   ASSERT_OK(server_tls_.LoadCertificateAndKey(cert_path_, 
key_path_));
 :   ASSERT_OK(server_tls_.LoadCertificateAuthority(cert_path_));
 : 
 :   // If the client wants to verify the server, it should fail 
because
 :   // the server cert is self-signed.
 :   Status s = 
RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST,
 :   TlsVerificationMode::VERIFY_NONE);
 :   ASSERT_STR_MATCHES(s.ToString(), "client error:.*certificate 
verify failed");
 : 
 :   // If the client doesn't care, it should succeed against the 
self-signed
 :   // server.
 :   ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE,
 :  TlsVerificationMode::VERIFY_NONE));
 : 
 :   // If the client loads the cert as trusted, then it should 
succeed
 :   // with verification enabled.
 :   ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_));
 :   
ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST,
 :  TlsVerificationMode::VERIFY_NONE));
 : 
 :   // It should still succeed with verification off, as well.
 :   ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE,
 :  TlsVerificationMode::VERIFY_NONE));
 : 
 :   // If the server requires authentication of the client, the 
handshake should fail.
 :   s = RunHandshake(TlsVerificationMode::VERIFY_NONE,
 :
TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST);
 :   ASSERT_TRUE(s.IsRuntimeError());
 :   ASSERT_STR_MATCHES(s.ToString(), "server error:.*peer did not 
return a certificate");
 : }
 : 
 : // TODO(PKI): this test has both the client and server using the 
same cert,
 : // which isn't very realistic. We should have this generate 
self-signed certs
 : // on the fly.
 : TEST_F(TestTlsHandshake, Test_ClientSelfSigned_ServerSelfSigned) 
{
 :   ASSERT_OK(client_tls_.LoadCertificateAndKey(cert_path_, 
key_path_));
 :   ASSERT_OK(client_tls_.LoadCertificateAuthority(cert_path_));
 :   ASSERT_OK(server_tls_.LoadCertificateAndKey(cert_path_, 
key_path_));
 :   ASSERT_OK(server_tls_.LoadCertificateAuthority(cert_path_));
 : 
 :   // This scenario should succeed in all cases.
 :   
ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST,
 :  
TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST));
 : 
 :   
ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST,
 :  TlsVerificationMode::VERIFY_NONE));
 : 
 :   ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE,
 :  
TlsVerificationMode::VERIFY_REMOTE_CERT_AND_HOST));
 : 
 :   ASSERT_OK(RunHandshake(TlsVerificationMode::VERIFY_NONE,
 :  TlsVerificationMode::VERIFY_NONE));
 : }
 : 
 : // TODO(PKI): add test coverage for mismatched common-name in 
the cert.
It seems this has been moved into different changelist, so I anticipate this 
patch to change.  I think it's better to continue when a new version is 
published for review.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6a98ef522e51751562d47907aa90a32c2dac3918
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] method to extract public part of an RSA key

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] method to extract public part of an RSA key
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5783/11/src/kudu/master/master_cert_authority.h
File src/kudu/master/master_cert_authority.h:

Line 26: typedef struct rsa_st RSA;
Is this still necessary?


http://gerrit.cloudera.org:8080/#/c/5783/13/src/kudu/security/crypto.h
File src/kudu/security/crypto.h:

PS13, Line 48: 
 : 
I think this would read better as "It's important to have PrivateKey and 
PublicKey be different types to avoid accidental leakage of private keys."


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] method to extract public part of an RSA key

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] method to extract public part of an RSA key
..


Patch Set 13: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpc: allow using encrypted private key for SSL

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: rpc: allow using encrypted private key for SSL
..


Patch Set 1:

> For Kudu's usage, we're a little less interested in this
 > specifically and more interested in the ability to programatically
 > pass in our new 'Key' and 'Cert' wrappers into the RPC system. That
 > is being done in this WIP patch:
 > https://gerrit.cloudera.org/#/c/5808/
 > 
 > After the above is complete, I think it's a matter of hooking up
 > some code which can load a password-protected key into an internal
 > 'Key' object.
 > 
 > I'm guessing you're asking about this from the Impala perspective -
 > I guess the question I'd have in response is whether you are
 > planning on pulling a new snapshot of krpc in the near term, or
 > continuing to work off the most recent snapshot you have? Because a
 > few of these security-related APIs are very much in flux right now.

We will be pulling in a newer version then the one we're working with right 
now, but we will probably not get the latest snapshot so as to avoid having 
some of the security features incomplete. We will probably cherry-pick what's 
necessary after that for the near-term.

For eg: We will probably not be requiring the WIP patch mentioned 
(https://gerrit.cloudera.org/#/c/5808/), but will probably cherry-pick this 
patch when it goes in. But again, this is short term, and in the near future, 
we probably will go back to KRPC as it will be at that point.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedf60251d9212c0c135db00902c00119c9d8849c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Fix flakiness of TestConnectionCache

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: Fix flakiness of TestConnectionCache
..


Fix flakiness of TestConnectionCache

This test would fail if one of the masters was a bit slow in starting. I
reproduced the failure reliably by adding a 3-second sleep in
ServerBase::Start.

This patch just makes a first dummy call that by design ensures that the
masters are available before we start pinging them.

Change-Id: Ib5061c002132901d9d1aab50400a7bdac4bc87d3
Reviewed-on: http://gerrit.cloudera.org:8080/5803
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectionCache.java
1 file changed, 6 insertions(+), 1 deletion(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib5061c002132901d9d1aab50400a7bdac4bc87d3
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix flakiness of TestConnectionCache

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: Fix flakiness of TestConnectionCache
..


Patch Set 1:

This is still causing build failures.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5061c002132901d9d1aab50400a7bdac4bc87d3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1854 Fixed broken links to Cloudera's Kudu documentation and archive.cloudera

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: KUDU-1854 Fixed broken links to Cloudera's Kudu documentation 
and archive.cloudera
..


KUDU-1854 Fixed broken links to Cloudera's Kudu documentation
and archive.cloudera

With the 1.2 release, Cloudera's Kudu documentation was moved
out of the /betas/ folder. This broke any existing links we had to
the Cloudera docs. I made a pass through the docs and fixed the links.

The Kudu packages on archive.cloudera were also moved out of the /betas
directory. Those links were updated as well.

Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
Reviewed-on: http://gerrit.cloudera.org:8080/5837
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans 
---
M docs/administration.adoc
M docs/configuration.adoc
M docs/installation.adoc
3 files changed, 15 insertions(+), 15 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1854 Fixed broken links to Cloudera's Kudu documentation and archive.cloudera

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1854 Fixed broken links to Cloudera's Kudu documentation 
and archive.cloudera
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [security] introduced crypto-test

[Alexey Serbin (Code Review)]

Hello Dan Burkert,

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

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

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

Change subject: [security] introduced crypto-test
..

[security] introduced crypto-test

The new crypto-test module is for test scenarios involving common
non-CA crypto entities in the Kudu security library.  The CA-related
scenarios are kept in the cert_management-test.

Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management-test.cc
A src/kudu/security/crypto-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 192 insertions(+), 70 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5798/12
-- 
To view, visit http://gerrit.cloudera.org:8080/5798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] introduced crypto-test

[Alexey Serbin (Code Review)]

Hello Dan Burkert,

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

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

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

Change subject: [security] introduced crypto-test
..

[security] introduced crypto-test

The new crypto-test module is for test scenarios involving common
non-CA crypto entities in the Kudu security library.  The CA-related
scenarios are kept in the cert_management-test.

Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management-test.cc
A src/kudu/security/crypto-test.cc
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 192 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5798/11
-- 
To view, visit http://gerrit.cloudera.org:8080/5798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] method to extract public part of an RSA key

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new patch set (#12).

Change subject: [security] method to extract public part of an RSA key
..

[security] method to extract public part of an RSA key

Added a method to get the public part of an RSA keypair.
Split the Key class into PublicKey and PrivateKey.  Reorganized the
layout of files in the src/security sub-directory: separated the common
crypto stuff from openssl_util.h into crypto.h.

Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
---
M src/kudu/master/master_cert_authority.cc
M src/kudu/master/master_cert_authority.h
M src/kudu/security/CMakeLists.txt
R src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
A src/kudu/security/cert.cc
A src/kudu/security/cert.h
A src/kudu/security/crypto.cc
A src/kudu/security/crypto.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
A src/kudu/security/openssl_util_bio.h
M src/kudu/security/server_cert_manager.cc
M src/kudu/security/server_cert_manager.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/tls_handshake.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signing_key.cc
M src/kudu/security/token_signing_key.h
23 files changed, 707 insertions(+), 350 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/5783/12
-- 
To view, visit http://gerrit.cloudera.org:8080/5783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ac61a08c8165152d0046468c0fa655131ff8fef
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] introduced crypto-test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] introduced crypto-test
..


Patch Set 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5798/10/src/kudu/security/crypto-test.cc
File src/kudu/security/crypto-test.cc:

Line 153:   static const uint16_t kKeyBits[] = { 256, 512, 1024, 2048, 3072, 
4096 };
> why would this test behave any differetly depending on the number of bits?
If there were a bug somewhere like using short buffer, etc.  But it's not 
realistic, of course.  I will leave 2048 and remove everything else.


Line 178:   static const uint16_t kKeyBits[] = { 256, 512, 1024, 2048, 3072, 
4096 };
> same
Done


Line 189:   // Extract public part of the key
> could we just combine this and the above test case? I guess it violates the
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibace36b6336b650879cf3f9139ccf562f82765a7
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [security] do actual token signing/verification
..

[security] do actual token signing/verification

Replaced temporary stub sign/verify calls with calls of actual
cryptographic functions.

Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
---
M src/kudu/security/token_signing_key.cc
M src/kudu/security/token_signing_key.h
2 files changed, 25 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/5812/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] sign/verify data using RSA key pair
..


Patch Set 10:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto-test.cc
File src/kudu/security/crypto-test.cc:

PS10, Line 231: // data. The idea is to verify the modified data yields 
different signatures.
  : T
> I feel this test is a little excessive -- we should already be able to rely
removed


Line 265: TEST_F(CryptoTest, VerifySignatureRef) {
> maybe just combine this with the top test case that generates the signature
Done


PS10, Line 299:   // Corrupt the reference data a bit.
  :   const int num_changes = rand() % iter_num + 1;
  :   for (int j = 0; j < num_changes; ++j) {
  : const size_t idx = rand() % data.size();
  : // Using the fact that the reference data contains only 
ASCII symbols.
  : data[idx] |= 0x80;
  :   }
> what about just using a constant string data = "not-the-original-data"? Or 
Done


http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

Line 127: Status DoMakeSignature(DigestType digest,
> what's the purpose of these helper methods? they're only called from a sing
I separated them for better readability.  ok, if it does not look good, I'll 
inline them.


Line 141:   auto buf = ssl_make_unique(OPENSSL_malloc(sig_len));
> why use OPENSSL_malloc here instead of just a normal C++ allocation? (even 
It's a good question.

I was thinking about replacing it with memory on the stack, but decided to keep 
OPENSSL_malloc (i.e. CRYPTO_malloc) because I thought there were some 
restrictions on how the memory for EVP_DigestSignXXX() should be allocated.  
However, after replacing it with stack-allocated memory everything was OK.

Signature length depends on the message digest, key type, and number of bits in 
the key.  The wiki page says it's 64 bytes for SHA2 digests, but openssl 
produces 256 bytes for SHA512 and RSA2048 bit key (64 bytes is for RSA512 and 
SHA224).  For SHA512 and RSA4096 -- 512 bytes, SHA512 and RSA8192 -- 1024 bytes.

I think we can safely put 4K for now -- the EVP_DigestSignFinal() should report 
an error if the buffer is too short.


PS10, Line 166: #if OPENSSL_VERSION_NUMBER < 0x10002000L
  :   unsigned char* sig_data = reinterpret_cast(
  :   const_cast(signature.data()));
  : #else
  :   const unsigned char* sig_data = reinterpret_cast(
  :   signature.data());
  : #endif
> we have this kind of messiness in a bunch of places. let's consider a separ
yes, I would better do that in a separate patch.


Line 174:   if (rc < 0) {
> being extra paranoid here: EVP_DigestVerifyFinal explicitly says that it re
Done


http://gerrit.cloudera.org:8080/#/c/5805/10/src/kudu/security/crypto.h
File src/kudu/security/crypto.h:

Line 36:   SHA1,
> Do we need SHA1 at all? It's known to be relatively weak. eg the NIST guide
SHA1 is faster than SHA2 digests -- we could use it for tests.However, if 
we are about to use it only for tests, may be we can drop it at all.


Line 84:   virtual Status MakeSignature(DigestType digest,
> can you add a note whether the signature is already base64-ed or whether th
Done


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

PS2, Line 436: 
 : 
> btw http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-f
sounds good


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] sign/verify data using RSA key pair

[Alexey Serbin (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [security] sign/verify data using RSA key pair
..

[security] sign/verify data using RSA key pair

Added functionality to make a signature of a data chunk and
verify signature of a data chunk using RSA keys.  As for now,
supporting SHA1, SHA256 and SHA512 message digests with test coverage
for SHA512.

Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
---
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
5 files changed, 236 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/5805/11
-- 
To view, visit http://gerrit.cloudera.org:8080/5805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3fa04bb7d09aa416363998e2f8d7ccbdea625e4f
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] do actual token signing/verification

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] do actual token signing/verification
..


Patch Set 6:

(6 comments)

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

Line 35: TokenSigningPublicKey::TokenSigningPublicKey(const 
TokenSigningPublicKeyPB& pb)
> oh, I missed your response to Dan. I think it's fishy here because this is 
It does not make much sense to me: there are multiple CHECK() statements in 
TokenVerifier::ImportPublicKeys().  If we are so concerned about 
externally-provided data, I suggest to address that separately and fix that 
part as well.


http://gerrit.cloudera.org:8080/#/c/5812/6/src/kudu/security/token_signing_key.cc
File src/kudu/security/token_signing_key.cc:

Line 37:   CHECK(pb_.has_rsa_key_der());
> +1 to what Dan said about doing an Init() method (or a factory function)
There are CHECK() statements in TokenVerifie::ImportPublicKeys().  I don't 
think it's worth dancing around Init() here since those CHECKS() are in place 
anyway -- it will crash first there even if I added that Init().

Let's fix that in a separate changelist.


Line 56:   CHECK_OK(public_key.ToString(_key_der_, DataFormat::DER));
> can you make public_key_der_ a 'const' member so it's clear it just gets se
I'm not sure this makes sense given the current signatures of these methods.  
If doing so, it would be necessary to do const_cast, which does not look good 
to me.


Line 66:   token->set_signature(signature);
> I think you can save an allocation by doing: token->mutable_signature()->as
Done


http://gerrit.cloudera.org:8080/#/c/5812/6/src/kudu/security/token_signing_key.h
File src/kudu/security/token_signing_key.h:

Line 53:   RsaPublicKey key_;
> worth doccing that this is just a cached "parsed" version of the PB
Done


Line 78:   std::string public_key_der_;
> worth adding a doc that this is just a "cache"
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf035c64032320a450731ae921e92615bf2efd98
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tls: move setting of verification modes into TlsHandshake

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] Make delete table-test less flaky

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Make delete_table-test less flaky
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5830/2/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

Line 1013:   AssertEventually([&]() {
> Surprised this is needed given the WaitForNumTabletsOnTS() above. Can you e
That function only waits for a certain number of tablets to be registered. 
There may be a period of time where the tablet is still in the process of being 
created in TSTabletManager::OpenTablet() and during that time the tablet is 
still in the transition_in_progress_ set.

In most tests, we don't have to worry about it much because the client will 
retry writes, etc.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idaa6bc8fb760351e9ecd8c39ef93c59559ce62f8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Make delete table-test less flaky

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Make delete_table-test less flaky
..


Make delete_table-test less flaky

Add AssertEventually to this DeleteTablet() call so it doesn't race
against the completion of the tablet creation process.

Change-Id: Idaa6bc8fb760351e9ecd8c39ef93c59559ce62f8
Reviewed-on: http://gerrit.cloudera.org:8080/5830
Tested-by: Mike Percy 
Reviewed-by: Adar Dembo 
---
M src/kudu/integration-tests/delete_table-test.cc
1 file changed, 4 insertions(+), 2 deletions(-)

Approvals:
  Mike Percy: Verified
  Adar Dembo: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Idaa6bc8fb760351e9ecd8c39ef93c59559ce62f8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] Upgrade Snappy to 1.1.4

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Upgrade Snappy to 1.1.4
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52cc0c968f17b6a44328d49a42872284968bb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5439/6/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 941:   KuduPredicate* MakePredicate(const Slice& col_name, const 
PredicateMakerFunc& f);
As per the C++ ABI guidelines, I think it's safe to introduce this. But, 
couldn't it be moved to KuduTable::Data instead, which would help client.h stay 
public API only? I took a quick look at how MakePredicate is used and I think 
it could be moved, though I'm not 100% sure.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

[Will Berkeley (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
..

KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.

These are both purely new features, so the changes are backwards-compatible.

Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
21 files changed, 888 insertions(+), 103 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-1854 Fixed broken links to Cloudera's Kudu documentation and archive.cloudera

[Ambreen Kazi (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1854 Fixed broken links to Cloudera's Kudu documentation 
and archive.cloudera
..

KUDU-1854 Fixed broken links to Cloudera's Kudu documentation
and archive.cloudera

With the 1.2 release, Cloudera's Kudu documentation was moved
out of the /betas/ folder. This broke any existing links we had to
the Cloudera docs. I made a pass through the docs and fixed the links.

The Kudu packages on archive.cloudera were also moved out of the /betas
directory. Those links were updated as well.

Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
---
M docs/administration.adoc
M docs/configuration.adoc
M docs/installation.adoc
3 files changed, 15 insertions(+), 15 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1854 Fixed broken links to Cloudera's Kudu documentation and archive.cloudera

[Ambreen Kazi (Code Review)]

Ambreen Kazi has abandoned this change.

Change subject: KUDU-1854 Fixed broken links to Cloudera's Kudu documentation 
and archive.cloudera
..


Abandoned

Did not notice a new changeID. The changes will be added to an existing gerrit 
- Change 5837.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I5e4abb1f410209d8e8e98493502d5417c61544db
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1854 Fixed broken links to Cloudera's Kudu documentation and archive.cloudera

[Ambreen Kazi (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1854 Fixed broken links to Cloudera's Kudu documentation 
and archive.cloudera
..

KUDU-1854 Fixed broken links to Cloudera's Kudu documentation
and archive.cloudera

With the 1.2 release, Cloudera's Kudu documentation was moved
out of the /betas/ folder. This broke any existing links we had to
the Cloudera docs. I made a pass through the docs and fixed the links.

The Kudu packages on archive.cloudera were also moved out of the /betas
directory. Those links were updated as well.

Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
---
M docs/administration.adoc
M docs/configuration.adoc
M docs/installation.adoc
3 files changed, 3 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Control mutex stack walking in DEBUG mode with a gflag

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: Control mutex stack walking in DEBUG mode with a gflag
..


Control mutex stack walking in DEBUG mode with a gflag

This patch disables the Mutex owner stack trace collection on DEBUG
builds by default, only enabling it when a certain gflag is set.

In DEBUG mode, our Mutex implementation collects a stack trace of the
owning thread each time the Mutex is acquired. It does this by calling
google::GetStackTrace() from glog, which in the context of the Kudu
build environment calls into libunwind to collect the stack trace.

At the time of writing, google::GetStackTrace() only allows access by
one thread at a time. If more than one thread attempts to invoke this
function simultaneously, there is a CAS that determines exclusivity. The
"loser" of this contest gets a short-circuit return along with an empty
stack trace, indicating a failure to collect the stack trace.

NB: I have filed a glog issue about that behavior upstream. For more
information, see https://github.com/google/glog/issues/160

This situation becomes a problem when there are one or more Mutexes
constantly being acquired. When that happens, there is always a thread
collecting a stack trace, and so the probability of being able to
successfully collect a stack trace at any given moment is greatly
reduced.

One important caller of google::GetStackTrace() is the glog failure
function and SIGABRT signal handler that is called when a CHECK() fails
or a LOG(FATAL) call is invoked. I have observed that this crash handler
will often print an empty stack trace in DEBUG mode. Investigating this
issue led me to discover that we had a thread (our AsyncLogger thread)
constantly acquiring a Mutex and racing on the above-mentioned CAS check
inside google::GetStackTrace(). Depriving our DEBUG builds of stack
traces on LOG(FATAL) or CHECK failures, especially on Jenkins runs, is
counterproductive. One simple solution to this problem is to disable
this behavior by default.

Change-Id: Ie4593cf7173867ce2f6151e03df0be94f97d95d2
Reviewed-on: http://gerrit.cloudera.org:8080/5741
Tested-by: Mike Percy 
Reviewed-by: Adar Dembo 
---
M src/kudu/util/mutex.cc
M src/kudu/util/mutex.h
2 files changed, 37 insertions(+), 8 deletions(-)

Approvals:
  Mike Percy: Verified
  Adar Dembo: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie4593cf7173867ce2f6151e03df0be94f97d95d2
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] consensus peers-test: Call FsManager::Open() in SetUp()

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: consensus_peers-test: Call FsManager::Open() in SetUp()
..


Patch Set 1:

I just don't think this test previously called uuid() or other checked methods.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I077fc2c95257c48a24e1e8fe87516293881ce5c5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] wal: Include standard prefix on glog lines

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: wal: Include standard prefix on glog lines
..


wal: Include standard prefix on glog lines

Change-Id: I724e85001beb68adbb806212ea3cb63292d0de56
Reviewed-on: http://gerrit.cloudera.org:8080/5827
Tested-by: Mike Percy 
Reviewed-by: Mike Percy 
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
2 files changed, 47 insertions(+), 34 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I724e85001beb68adbb806212ea3cb63292d0de56
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] wal: Include standard prefix on glog lines

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: wal: Include standard prefix on glog lines
..


Patch Set 4: Code-Review+2

Carrying forward Adar's previous +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I724e85001beb68adbb806212ea3cb63292d0de56
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] consensus peers-test: Call FsManager::Open() in SetUp()

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: consensus_peers-test: Call FsManager::Open() in SetUp()
..


consensus_peers-test: Call FsManager::Open() in SetUp()

This test previously neglected to call FsManager::Open() after
FsManager::CreateInitialFileSystemLayout(), resulting in an invalid
state for the FsManager in this test.

Change-Id: I077fc2c95257c48a24e1e8fe87516293881ce5c5
Reviewed-on: http://gerrit.cloudera.org:8080/5831
Tested-by: Mike Percy 
Reviewed-by: Adar Dembo 
---
M src/kudu/consensus/consensus_peers-test.cc
1 file changed, 4 insertions(+), 3 deletions(-)

Approvals:
  Mike Percy: Verified
  Adar Dembo: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I077fc2c95257c48a24e1e8fe87516293881ce5c5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
..

KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.

These are both purely new features, so the changes are backwards-compatible.

Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
21 files changed, 888 insertions(+), 103 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] Update Impala docs for Impala 2.8 release

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Update Impala docs for Impala 2.8 release
..

Update Impala docs for Impala 2.8 release

* No longer needs to document the special 'IMPALA_KUDU' build.
* Fixed syntax for new style
* Added back CTAS, which has syntax (we'd mistakenly thought it was
  removed)
* Fixed a few typos/issues elsewhere (eg use of 'int32' instead of 'int'
  type)
* Removed the docs for composite range partitioning, which seems to
  no longer be supported in Impala.

Change-Id: Ia43d18e8d92c52e5868e1d48b91351bca41b53f8
---
M docs/kudu_impala_integration.adoc
M docs/quickstart.adoc
2 files changed, 156 insertions(+), 403 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia43d18e8d92c52e5868e1d48b91351bca41b53f8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1854 Fixed broken links to Cloudera's Kudu documentation

[Ambreen Kazi (Code Review)]

Ambreen Kazi has posted comments on this change.

Change subject: KUDU-1854 Fixed broken links to Cloudera's Kudu documentation
..


Patch Set 1:

> Looks good. What about the various installation links to
 > archive.cloudera.com in installation.adoc? Do you have more
 > up-to-date ones to replace there? It seems they're still pointing
 > to /beta/ and they don't have 1.2 packages/parcels.

Ah, let me fix those.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1854 Fixed broken links to Cloudera's Kudu documentation

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1854 Fixed broken links to Cloudera's Kudu documentation
..


Patch Set 1:

Looks good. What about the various installation links to archive.cloudera.com 
in installation.adoc? Do you have more up-to-date ones to replace there? It 
seems they're still pointing to /beta/ and they don't have 1.2 packages/parcels.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1854 Fixed broken links to Cloudera's Kudu documentation

[Ambreen Kazi (Code Review)]

Ambreen Kazi has uploaded a new change for review.

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

Change subject: KUDU-1854 Fixed broken links to Cloudera's Kudu documentation
..

KUDU-1854 Fixed broken links to Cloudera's Kudu documentation

Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
---
M docs/administration.adoc
M docs/configuration.adoc
M docs/installation.adoc
3 files changed, 3 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic158ddd6a576e7a3680d0f69c326768b7b3e5049
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 

[kudu-CR] build-thirdparty.sh: remove Bash 4.0 specific syntax

[Dan Burkert (Code Review)]

Dan Burkert has abandoned this change.

Change subject: build-thirdparty.sh: remove Bash 4.0 specific syntax
..


Abandoned

already fixed.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I28db6bea207f699609291c33fc3b62b2c1e3483e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [thirdparty] fix build-thirdparty.sh for bash 3.x

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: [thirdparty] fix build-thirdparty.sh for bash 3.x
..


[thirdparty] fix build-thirdparty.sh for bash 3.x

The fall-through directive ';&' works only for bash >= 4.0.
This change makes it work for bash 3.x as well.

Change-Id: I624e5d13b8a981491a65bae590be1f1e432710a9
Reviewed-on: http://gerrit.cloudera.org:8080/5832
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
Reviewed-by: Mike Percy 
---
M thirdparty/build-thirdparty.sh
1 file changed, 9 insertions(+), 4 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I624e5d13b8a981491a65bae590be1f1e432710a9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] build-thirdparty.sh: remove Bash 4.0 specific syntax

[Dan Burkert (Code Review)]

Hello Mike Percy, Adar Dembo,

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

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

to review the following change.

Change subject: build-thirdparty.sh: remove Bash 4.0 specific syntax
..

build-thirdparty.sh: remove Bash 4.0 specific syntax

OS X 10.10 ships with the prehistoric Bash 3.2.

Change-Id: I28db6bea207f699609291c33fc3b62b2c1e3483e
---
M thirdparty/build-thirdparty.sh
1 file changed, 5 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I28db6bea207f699609291c33fc3b62b2c1e3483e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] Upgrade Snappy to 1.1.4

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Upgrade Snappy to 1.1.4
..


Patch Set 1:

Test failures look unrelated

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52cc0c968f17b6a44328d49a42872284968bb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Upgrade Snappy to 1.1.4

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Upgrade Snappy to 1.1.4
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52cc0c968f17b6a44328d49a42872284968bb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5439/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:

Line 751: if (!col_schema.is_nullable()) {
Since you are already doing this check in ColumnPredicate::IsNull, could you 
skip it here?  I think that would simplify this, since you shouldn't need the 
NonePredicateData class.


http://gerrit.cloudera.org:8080/#/c/5439/4/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:

Line 197: if (predicate_pair.second.predicate_type() == 
PredicateType::None) {
Good catch - we were already implicitly short circuiting if there was a None 
predicate since the partition pruner would call CanShortCircuit() on the scan 
spec, and contain no partitions (e.g. no trips through the while loop below).  
However, I'm guessing adding this is necessary to avoid a CHECK failure when 
converting a None predicate to protobuf?  In that case I think it would be 
better to do the check by calling

if (configuration_.CanShortCircuit()) {
return Status::OK();
}

immediately after the scan spec is optimized on line 171.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty] fix build-thirdparty.sh for bash 3.x

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: [thirdparty] fix build-thirdparty.sh for bash 3.x
..


Patch Set 1: Code-Review+2

Oops, sorry for breaking Mac!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I624e5d13b8a981491a65bae590be1f1e432710a9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Control mutex stack walking in DEBUG mode with a gflag

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Control mutex stack walking in DEBUG mode with a gflag
..


Patch Set 12: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4593cf7173867ce2f6151e03df0be94f97d95d2
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [thirdparty] fix build-thirdparty.sh for bash 3.x

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [thirdparty] fix build-thirdparty.sh for bash 3.x
..


Patch Set 1: Code-Review+2

Looks fine to me, though maybe Mike wants to take a look since that's his code.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I624e5d13b8a981491a65bae590be1f1e432710a9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] consensus peers-test: Call FsManager::Open() in SetUp()

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: consensus_peers-test: Call FsManager::Open() in SetUp()
..


Patch Set 1: Code-Review+2

So how did the test pass at all?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I077fc2c95257c48a24e1e8fe87516293881ce5c5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Make delete table-test less flaky

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Make delete_table-test less flaky
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5830/2/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:

Line 1013:   AssertEventually([&]() {
Surprised this is needed given the WaitForNumTabletsOnTS() above. Can you 
explain why?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idaa6bc8fb760351e9ecd8c39ef93c59559ce62f8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates
..

KUDU-1595 and KUDU-1642: IS NOT NULL and IS NULL predicates

This patch exposes client APIs for the IS NOT NULL predicate and
adds an IS NULL predicate type.

These are both purely new features, so the changes are backwards-compatible.

Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanPredicate.java
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_predicate-internal.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/common.proto
M src/kudu/common/key_util.cc
M src/kudu/common/scan_spec.cc
M src/kudu/common/wire_protocol.cc
21 files changed, 901 insertions(+), 103 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ide23ab6d3cad524553aa6ea5c44570fdadde5ace
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley