You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/03/10 20:48:14 UTC
[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random
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 <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1843. Client UUIDs should be cryptographically random
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No