Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(74)

Issue 10698177: Added crypto random-number generator (Closed)

Created:
8 years, 5 months ago by Mehrdad
Modified:
8 years, 4 months ago
CC:
chromium-reviews, albertb, Albert Bodenhamer, willchan no longer on Chromium
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Added crypto random-number generator Added a cryptographic random-number generator to crypto/. Modified sync to use this function instead. May also be used by Cloud Print in the future. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149689

Patch Set 1 #

Patch Set 2 : Fixed some lint issues. #

Patch Set 3 : Fixed another lint issue #

Patch Set 4 : Fixed another lint issue #

Patch Set 5 : crypto/RNG now calls base::Rand* #

Patch Set 6 : crypto/ now calls crypto/RandomNumberGenerator for RNG #

Patch Set 7 : Added comments to base::rand_util.h mentioning using crypto/ instead. #

Patch Set 8 : Removed extra code #

Total comments: 1

Patch Set 9 : Use WriteInto #

Total comments: 9

Patch Set 10 : Comment fixes #

Patch Set 11 : Comment changes #

Total comments: 2

Patch Set 12 : Comment fixes #

Total comments: 3

Patch Set 13 : More comment fixes #

Patch Set 14 : Added period. #

Total comments: 2

Patch Set 15 : Better comments (hopefully) #

Total comments: 1

Patch Set 16 : Done. #

Patch Set 17 : Fixed #include/base:: problem #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -20 lines) Patch
M base/rand_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +11 lines, -4 lines 0 comments Download
M base/rand_util_posix.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M base/rand_util_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M crypto/crypto.gyp View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M crypto/openpgp_symmetric_encryption.cc View 1 2 3 4 5 6 7 3 chunks +4 lines, -3 lines 0 comments Download
M crypto/p224_spake.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
A crypto/random.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +21 lines, -0 lines 0 comments Download
A crypto/random.cc View 1 2 3 4 5 6 7 8 9 1 chunk +19 lines, -0 lines 0 comments Download
A crypto/random_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +27 lines, -0 lines 0 comments Download
M sync/util/nigori.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +3 lines, -11 lines 0 comments Download

Messages

Total messages: 45 (0 generated)
Mehrdad
I added a cryptographic RNG, which sync can use (fixing a TODO). It might also ...
8 years, 5 months ago (2012-07-13 17:52:11 UTC) #1
Ryan Sleevi
NACK - we definitely do /not/ want to use CryptGenRandom /nor/ a PK11 slot. base/rand_util.h ...
8 years, 5 months ago (2012-07-13 17:55:04 UTC) #2
Ryan Sleevi
On 2012/07/13 17:55:04, Ryan Sleevi wrote: > NACK - we definitely do /not/ want to ...
8 years, 5 months ago (2012-07-13 17:59:29 UTC) #3
Mehrdad
On 2012/07/13 17:59:29, Ryan Sleevi wrote: > end up in RtlGenRandom, which is the underlying ...
8 years, 5 months ago (2012-07-13 18:03:53 UTC) #4
Mehrdad
I made crypto/random call base::Rand*.
8 years, 5 months ago (2012-07-13 18:48:00 UTC) #5
Ryan Sleevi
Is there a reason you created a class? I think I'd be fond of just ...
8 years, 5 months ago (2012-07-13 19:06:37 UTC) #6
Mehrdad
On 2012/07/13 19:06:37, Ryan Sleevi wrote: > Is there a reason you created a class? ...
8 years, 5 months ago (2012-07-13 19:13:56 UTC) #7
Mehrdad
On 2012/07/13 19:06:37, Ryan Sleevi wrote: > adds any value, and as you note, the ...
8 years, 5 months ago (2012-07-13 19:29:33 UTC) #8
Ryan Sleevi
On 2012/07/13 19:13:56, Mehrdad wrote: > On 2012/07/13 19:06:37, Ryan Sleevi wrote: > > Is ...
8 years, 5 months ago (2012-07-13 19:30:38 UTC) #9
Mehrdad
> > Should I remove it? > > Yeah. Okay sure. > Huh. That code ...
8 years, 5 months ago (2012-07-13 20:12:16 UTC) #10
Mehrdad
Okay I made some changes, hope that's good.
8 years, 5 months ago (2012-07-13 20:59:01 UTC) #11
akalin
http://codereview.chromium.org/10698177/diff/15001/sync/util/nigori.cc File sync/util/nigori.cc (right): http://codereview.chromium.org/10698177/diff/15001/sync/util/nigori.cc#newcode162 sync/util/nigori.cc:162: crypto::RandBytes(&*iv.begin(), kIvSize); why not have a crypto::RandBytesAsString also?
8 years, 5 months ago (2012-07-20 00:21:57 UTC) #12
Ryan Sleevi
On 2012/07/20 00:21:57, akalin wrote: > http://codereview.chromium.org/10698177/diff/15001/sync/util/nigori.cc > File sync/util/nigori.cc (right): > > http://codereview.chromium.org/10698177/diff/15001/sync/util/nigori.cc#newcode162 > ...
8 years, 5 months ago (2012-07-20 00:22:47 UTC) #13
akalin
On 2012/07/20 00:22:47, Ryan Sleevi wrote: > On 2012/07/20 00:21:57, akalin wrote: > > http://codereview.chromium.org/10698177/diff/15001/sync/util/nigori.cc ...
8 years, 5 months ago (2012-07-20 00:32:07 UTC) #14
Mehrdad
> Fair enough. In that case, shouldn't nigori.cc use WriteInto? Yup, fixed.
8 years, 5 months ago (2012-07-20 17:53:27 UTC) #15
akalin
sync lgtm
8 years, 5 months ago (2012-07-20 18:02:29 UTC) #16
akalin
On 2012/07/20 18:02:29, akalin wrote: > sync lgtm Any updates?
8 years, 4 months ago (2012-07-31 22:23:13 UTC) #17
Ryan Sleevi
Sorry, stamped LGTM, mod comment nits. http://codereview.chromium.org/10698177/diff/21001/base/rand_util.h File base/rand_util.h (right): http://codereview.chromium.org/10698177/diff/21001/base/rand_util.h#newcode37 base/rand_util.h:37: // for security-sensitive ...
8 years, 4 months ago (2012-07-31 23:03:48 UTC) #18
Mehrdad
Not sure about this one... thoughts? http://codereview.chromium.org/10698177/diff/21001/base/rand_util.h File base/rand_util.h (right): http://codereview.chromium.org/10698177/diff/21001/base/rand_util.h#newcode37 base/rand_util.h:37: // for security-sensitive ...
8 years, 4 months ago (2012-07-31 23:36:32 UTC) #19
Ryan Sleevi
http://codereview.chromium.org/10698177/diff/21001/base/rand_util.h File base/rand_util.h (right): http://codereview.chromium.org/10698177/diff/21001/base/rand_util.h#newcode37 base/rand_util.h:37: // for security-sensitive purposes instead of this function. On ...
8 years, 4 months ago (2012-08-01 17:08:55 UTC) #20
Mehrdad
http://codereview.chromium.org/10698177/diff/21001/base/rand_util.h File base/rand_util.h (right): http://codereview.chromium.org/10698177/diff/21001/base/rand_util.h#newcode37 base/rand_util.h:37: // for security-sensitive purposes instead of this function. On ...
8 years, 4 months ago (2012-08-01 17:19:03 UTC) #21
Mehrdad
Okay just made some comment changes. Does this look good?
8 years, 4 months ago (2012-08-01 17:47:57 UTC) #22
Ryan Sleevi
http://codereview.chromium.org/10698177/diff/31009/base/rand_util.h File base/rand_util.h (right): http://codereview.chromium.org/10698177/diff/31009/base/rand_util.h#newcode36 base/rand_util.h:36: // Use crypto/random for cryptographically random data instead of ...
8 years, 4 months ago (2012-08-01 17:52:28 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mniknami@chromium.org/10698177/33003
8 years, 4 months ago (2012-08-01 19:03:44 UTC) #24
commit-bot: I haz the power
Presubmit check for 10698177-33003 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 4 months ago (2012-08-01 19:04:00 UTC) #25
Mehrdad
Ah, just added a base/ OWNER. @Ryan: I made these changes.
8 years, 4 months ago (2012-08-01 19:09:23 UTC) #26
akalin
On 2012/08/01 19:09:23, Mehrdad wrote: > Ah, just added a base/ OWNER. > > @Ryan: ...
8 years, 4 months ago (2012-08-02 06:58:26 UTC) #27
akalin
LGTM after nits and you fix ryan's comments http://codereview.chromium.org/10698177/diff/33003/base/rand_util_posix.cc File base/rand_util_posix.cc (right): http://codereview.chromium.org/10698177/diff/33003/base/rand_util_posix.cc#newcode44 base/rand_util_posix.cc:44: // ...
8 years, 4 months ago (2012-08-02 07:01:11 UTC) #28
Mehrdad
On 2012/08/02 07:01:11, akalin wrote: > I don't see the changes. Did you forget to ...
8 years, 4 months ago (2012-08-02 16:41:57 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mniknami@chromium.org/10698177/31011
8 years, 4 months ago (2012-08-02 16:44:00 UTC) #30
commit-bot: I haz the power
Presubmit check for 10698177-31011 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 4 months ago (2012-08-02 16:44:26 UTC) #31
Mehrdad
On 2012/08/02 16:44:26, I haz the power (commit-bot) wrote: > Missing LGTM from an OWNER ...
8 years, 4 months ago (2012-08-02 16:45:35 UTC) #32
willchan no longer on Chromium
willchan is on vacation :) On Thu, Aug 2, 2012 at 9:45 AM, <mniknami@chromium.org> wrote: ...
8 years, 4 months ago (2012-08-02 16:46:00 UTC) #33
Mehrdad
On 2012/08/02 16:46:00, willchan wrote: > willchan is on vacation :) Oh lol xD ok, ...
8 years, 4 months ago (2012-08-02 16:47:13 UTC) #34
Mehrdad
(Needed a reviewer for base/.) -willchan +darin Thanks! :)
8 years, 4 months ago (2012-08-02 16:50:59 UTC) #35
darin (slow to review)
https://chromiumcodereview.appspot.com/10698177/diff/31011/base/rand_util.h File base/rand_util.h (right): https://chromiumcodereview.appspot.com/10698177/diff/31011/base/rand_util.h#newcode36 base/rand_util.h:36: // Use crypto::RandBytes() for cryptographically secure random data I'm ...
8 years, 4 months ago (2012-08-02 16:55:37 UTC) #36
Mehrdad
https://chromiumcodereview.appspot.com/10698177/diff/31011/base/rand_util.h File base/rand_util.h (right): https://chromiumcodereview.appspot.com/10698177/diff/31011/base/rand_util.h#newcode36 base/rand_util.h:36: // Use crypto::RandBytes() for cryptographically secure random data Oh ...
8 years, 4 months ago (2012-08-02 17:01:51 UTC) #37
Mehrdad
> // WARNING: > // Do not use for security-sensitive purposes. > // See crypto/ ...
8 years, 4 months ago (2012-08-02 17:02:35 UTC) #38
Mehrdad
Okay I changed them, hopefully this one is good.
8 years, 4 months ago (2012-08-02 17:15:25 UTC) #39
darin (slow to review)
LGTM! https://chromiumcodereview.appspot.com/10698177/diff/35004/base/rand_util.h File base/rand_util.h (right): https://chromiumcodereview.appspot.com/10698177/diff/35004/base/rand_util.h#newcode49 base/rand_util.h:49: // Note that this is a variation of ...
8 years, 4 months ago (2012-08-02 18:07:09 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mniknami@chromium.org/10698177/38003
8 years, 4 months ago (2012-08-02 18:16:20 UTC) #41
Mehrdad
Awesome, thanks!
8 years, 4 months ago (2012-08-02 18:16:21 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mniknami@chromium.org/10698177/33006
8 years, 4 months ago (2012-08-02 18:45:56 UTC) #43
commit-bot: I haz the power
Change committed as 149689
8 years, 4 months ago (2012-08-02 20:22:26 UTC) #44
Mehrdad
8 years, 4 months ago (2012-08-02 20:23:06 UTC) #45
On 2012/08/02 20:22:26, I haz the power (commit-bot) wrote:
> Change committed as 149689

w00t. :)

Powered by Google App Engine
This is Rietveld 408576698