[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random

2017-05-04 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

Change subject: KUDU-1843. Client UUIDs should be cryptographically random
..


Patch Set 2: Code-Review-1

I still think this is a bad idea.  At the very least, this patch needs to go 
through every place a client ID is currently logged and redact it.  If the 
client ID is exposed in client API, it needs to be documented that is should be 
considered a secret.  We need to be systematically more careful with client IDs 
if we treat them as secrets.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random

2017-05-04 Thread David Ribeiro Alves (Code Review) 
David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1843. Client UUIDs should be cryptographically random
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random

2017-04-05 Thread Todd Lipcon (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1843. Client UUIDs should be cryptographically random
..

KUDU-1843. Client UUIDs should be cryptographically random

This switches from using boost::uuid to generate client IDs to instead
use OpenSSL's random functionality.

This is a more secure source of randomness which can prevent IDs from
being hijacked.

The random bytes are converted to hex to ensure that they are still
printable as before.

The Java client needed no update since it already uses
cryptographically-strong UUIDs.

Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M src/kudu/client/client.cc
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
5 files changed, 43 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random

2017-04-05 Thread Todd Lipcon (Code Review) 
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1843. Client UUIDs should be cryptographically random
..


Patch Set 1:

(3 comments)

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

PS1, Line 257: for (int i = 0; i < 5; i++) {
> nit: would running this for 100 iters or something increase meaningfully ch
nah, I don't think so. With 128-bit strings as we're using here, we'd need 
10^16 iterations to reach a one-in-a-million chance of collision :) 
(https://en.wikipedia.org/wiki/Birthday_problem)


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

PS1, Line 258: std::
> Nit: don't need
Done


Line 264:   OPENSSL_RET_NOT_OK(RAND_bytes(buf.data(), bytes), "failed to 
generate random bytes");
> I'm looking at https://wiki.openssl.org/index.php/Random_Numbers#Initializa
https://security.stackexchange.com/questions/3936/is-a-rand-from-dev-urandom-secure-for-a-login-key
 says that urandom is suitably random for this type of application. That's also 
the advice I've read in other recent sources


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random

2017-03-10 Thread David Ribeiro Alves (Code Review) 
David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1843. Client UUIDs should be cryptographically random
..


Patch Set 1:

(1 comment)

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

PS1, Line 257: for (int i = 0; i < 5; i++) {
nit: would running this for 100 iters or something increase meaningfully 
chances of collision?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random

2017-03-10 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: KUDU-1843. Client UUIDs should be cryptographically random
..


Patch Set 1:

(2 comments)

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

PS1, Line 258: std::
Nit: don't need


Line 264:   OPENSSL_RET_NOT_OK(RAND_bytes(buf.data(), bytes), "failed to 
generate random bytes");
I'm looking at 
https://wiki.openssl.org/index.php/Random_Numbers#Initialization. It looks like 
entropy is read from /dev/urandom rather than /dev/random; if you want to use 
the latter you have to call RAND_load_file() on it. Is that what we want? Is it 
secure to initialize openssl's PRNG using /dev/urandom, which is itself a PRNG?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random

2017-03-10 Thread Todd Lipcon (Code Review) 
Hello Dan Burkert, David Ribeiro Alves,

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

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

to review the following change.

Change subject: KUDU-1843. Client UUIDs should be cryptographically random
..

KUDU-1843. Client UUIDs should be cryptographically random

This switches from using boost::uuid to generate client IDs to instead
use OpenSSL's random functionality.

This is a more secure source of randomness which can prevent IDs from
being hijacked.

The random bytes are converted to hex to ensure that they are still
printable as before.

The Java client needed no update since it already uses
cryptographically-strong UUIDs.

Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M src/kudu/client/client.cc
M src/kudu/security/crypto-test.cc
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
5 files changed, 43 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0ea6868773cf046944f70c32c647184e4b48c772
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves