Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15793005:
Per discussion, implement the Omaha Client Update Protocol (CUP) in src/crypto. (Closed)
|
Created:
7 years, 6 months ago by Ryan Myers (chromium) Modified:
7 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionPer discussion, implement the Omaha Client Update Protocol (CUP) in src/crypto.
Since this will not be used on Android or iOS, only the NSS implementation is
complete; OpenSSL is stubbed out.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204755
Patch Set 1 : #
Total comments: 46
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 1
Patch Set 8 : #
Total comments: 44
Patch Set 9 : #Patch Set 10 : #
Total comments: 10
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Messages
Total messages: 23 (0 generated)
Previous advice/thread was greatly appreciated. On to round two! I've moved the entire CUP protocol into a class in src/crypto, and reimplemented the RSA portion of it using NSS. PTAL.
(I'm just about to head out and I'll be away until Monday.)
I have only done an initial pass getting familiar with the high level design. My biggest concern is the URL-twiddling in crypto, which avoids GURL entirely. That gives me some pause. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc File crypto/cup.cc (right): https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode16 crypto/cup.cc:16: // See cup_nss.cc or cup_openssl.cc for implementations of those methods. Generally unnecessary; this is understood when dealing with Chromium code, particularly in crypto/ https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode28 crypto/cup.cc:28: if (v_ > 0) { style nit: This is not strictly required by the style guide, other than "follow local style", but both "crypto/" and "net/" omit braces for one-line conditionals. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode32 crypto/cup.cc:32: DCHECK(key_version > 0 && public_key); DCHECK_GT(key_version, 0); DCHECK(public_key); https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode74 crypto/cup.cc:74: cup_url_query.data()); I'm generally not a fan of constructing and manipulating URLs like this. This is why we have googleurl/, to avoid manipulating URLs as strings - particularly when dealing with something like "url_query" (which crypto/ can't/shouldn't depend on) It feels like the URL construction might be better separated, if at all possible. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode87 crypto/cup.cc:87: DCHECK(internal_hashes.size() == 3 * HashDigestSize()); DCHECK_EQ https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode122 crypto/cup.cc:122: comment nit: Rewrite the comment without using "our" or "we". Avoid using pronouns in comments. // If the request was received by the server, the server will use its // private key to decrypt v|w, yielding the contents of r. The server // can then recreate sk, compute hw, and SymSign(3|hw) to ensure that // the cp sent matches. It will then use sk to sign its response. Probably need quotes (or more likely, |hw_|, etc) around variables (or more aptly, their expanded names - see previous comment) style nit: delete blank line https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode128 crypto/cup.cc:128: return sp_in == expected_sp; SECURITY: This raises red flags as an unsafe comparison (especially when the field is called "expected_hmac"). See crypto/secure_util.h for securely (eg: in a timing-independent manner) comparing two buffers. I've not done cryptanalysis on the protocol to know what impact would be had from such leaking, but it's enough to say "easier to be safe"... eg: This is why we have HMAC::Verify() to begin with :) https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode132 crypto/cup.cc:132: DCHECK(public_key_length >= HashDigestSize()); DCHECK_GE https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode233 crypto/cup.cc:233: DCHECK(args[i]->size() == HashDigestSize()); DCHECK_EQ https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode252 crypto/cup.cc:252: DCHECK(rsa_key_size >= HashDigestSize()); DCHECK_GE https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode266 crypto/cup.cc:266: DCHECK(result.size() == rsa_key_size); DCHECK_GE https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h File crypto/cup.h (right): https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode3 crypto/cup.h:3: // found in the LICENSE file. As per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_N... , make your file names specific This file (and the related ones) should be called "client_update_protocol" https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode21 crypto/cup.h:21: // CUP, or Client Update Protocol, is used by Google Update (Omaha) servers to Client Update Protocol, or CUP, ... [to match file naming] https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode29 crypto/cup.h:29: Delete newline https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode37 crypto/cup.h:37: // header/footer. (i.e. a Base64 encoding of a DER encoding of an ASN.1 key.) Design: Can you explain why you're using PEM, rather than DER? Especially if this is coded in, it makes no sense to use PEM. Given that PEM itself is ambiguous and subject to interpretation, a strong preference is to just take the binary data. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode41 crypto/cup.h:41: bool Init(int key_version, const char* public_key); Design: Rather than the Init() pattern, it seems more desirable to use the Factory pattern (eg: Create*), to avoid the partially-initialized-object pattern. Chrome uses both, to be fair. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode78 crypto/cup.h:78: std::string vw_; These field names should all be updated to more meaningful values. If you're trying to mirror the specification, use comments to indicate that. It's more important to be able to understand this class *without* having to read the spec http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode82 crypto/cup.h:82: #endif No OpenSSL handling? https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode101 crypto/cup.h:101: static std::string UrlSafeB64Encode(const std::vector<uint8>& data); Design: All of these static-but-private member functions should be in an unnamed namespace in the implementation file, unless you've explicitly added them for unit tests (and I would suggest that they're not quite unit tests at that point, because they're no longer testing the *public* interface). https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode125 crypto/cup.h:125: friend class CupUnitTest; per general Chromium style, move this friend to line 72. Similarly, DISALLOW_COPY_AND_ASSIGN(ClientUpdateProtocol) ( see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... ) https://codereview.chromium.org/15793005/diff/27001/crypto/cup_nss.cc File crypto/cup_nss.cc (right): https://codereview.chromium.org/15793005/diff/27001/crypto/cup_nss.cc#newcode32 crypto/cup_nss.cc:32: style: delete blank line. https://codereview.chromium.org/15793005/diff/27001/crypto/cup_nss.cc#newcode35 crypto/cup_nss.cc:35: ATOB_AsciiToData(public_key, &der_encoded_spki_len)); eg: using DER you don't have to go through these hijinks. Especially because we have base::Base64Encode/Decode. https://codereview.chromium.org/15793005/diff/27001/crypto/cup_nss.cc#newcode64 crypto/cup_nss.cc:64: // arbitrarily and expect it to work and/or remain secure! Comment nit: Avoid "we" in comments.
Thanks! https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc File crypto/cup.cc (right): https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode16 crypto/cup.cc:16: // See cup_nss.cc or cup_openssl.cc for implementations of those methods. On 2013/05/30 02:28:08, Ryan Sleevi wrote: > Generally unnecessary; this is understood when dealing with Chromium code, > particularly in crypto/ Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode28 crypto/cup.cc:28: if (v_ > 0) { On 2013/05/30 02:28:08, Ryan Sleevi wrote: > style nit: This is not strictly required by the style guide, other than "follow > local style", but both "crypto/" and "net/" omit braces for one-line > conditionals. Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode32 crypto/cup.cc:32: DCHECK(key_version > 0 && public_key); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > DCHECK_GT(key_version, 0); > DCHECK(public_key); Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode74 crypto/cup.cc:74: cup_url_query.data()); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > I'm generally not a fan of constructing and manipulating URLs like this. This is > why we have googleurl/, to avoid manipulating URLs as strings - particularly > when dealing with something like "url_query" (which crypto/ can't/shouldn't > depend on) > > It feels like the URL construction might be better separated, if at all > possible. Agreed. I've pushed this out to the caller; they can do it easily enough with GURL. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode87 crypto/cup.cc:87: DCHECK(internal_hashes.size() == 3 * HashDigestSize()); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > DCHECK_EQ Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode122 crypto/cup.cc:122: On 2013/05/30 02:28:08, Ryan Sleevi wrote: > comment nit: Rewrite the comment without using "our" or "we". Avoid using > pronouns in comments. > > // If the request was received by the server, the server will use its > // private key to decrypt v|w, yielding the contents of r. The server > // can then recreate sk, compute hw, and SymSign(3|hw) to ensure that > // the cp sent matches. It will then use sk to sign its response. > > Probably need quotes (or more likely, |hw_|, etc) around variables (or more > aptly, their expanded names - see previous comment) > > style nit: delete blank line Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode128 crypto/cup.cc:128: return sp_in == expected_sp; On 2013/05/30 02:28:08, Ryan Sleevi wrote: > SECURITY: This raises red flags as an unsafe comparison (especially when the > field is called "expected_hmac"). > > See crypto/secure_util.h for securely (eg: in a timing-independent manner) > comparing two buffers. > > I've not done cryptanalysis on the protocol to know what impact would be had > from such leaking, but it's enough to say "easier to be safe"... > > eg: This is why we have HMAC::Verify() to begin with :) Replaced with HMAC::Verify(). https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode132 crypto/cup.cc:132: DCHECK(public_key_length >= HashDigestSize()); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > DCHECK_GE Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode233 crypto/cup.cc:233: DCHECK(args[i]->size() == HashDigestSize()); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > DCHECK_EQ Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode252 crypto/cup.cc:252: DCHECK(rsa_key_size >= HashDigestSize()); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > DCHECK_GE Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.cc#newcode266 crypto/cup.cc:266: DCHECK(result.size() == rsa_key_size); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > DCHECK_GE Done (used DCHECK_EQ). https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h File crypto/cup.h (right): https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode3 crypto/cup.h:3: // found in the LICENSE file. On 2013/05/30 02:28:08, Ryan Sleevi wrote: > As per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_N... > , make your file names specific > > This file (and the related ones) should be called "client_update_protocol" ACK. I'll make this change in the final patchset, however, so that diffs in these files remain trackable during the review process. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode21 crypto/cup.h:21: // CUP, or Client Update Protocol, is used by Google Update (Omaha) servers to On 2013/05/30 02:28:08, Ryan Sleevi wrote: > Client Update Protocol, or CUP, ... [to match file naming] Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode29 crypto/cup.h:29: On 2013/05/30 02:28:08, Ryan Sleevi wrote: > Delete newline Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode37 crypto/cup.h:37: // header/footer. (i.e. a Base64 encoding of a DER encoding of an ASN.1 key.) On 2013/05/30 02:28:08, Ryan Sleevi wrote: > Design: Can you explain why you're using PEM, rather than DER? Especially if > this is coded in, it makes no sense to use PEM. > > Given that PEM itself is ambiguous and subject to interpretation, a strong > preference is to just take the binary data. Switched to DER. Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode41 crypto/cup.h:41: bool Init(int key_version, const char* public_key); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > Design: Rather than the Init() pattern, it seems more desirable to use the > Factory pattern (eg: Create*), to avoid the partially-initialized-object > pattern. Chrome uses both, to be fair. Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode78 crypto/cup.h:78: std::string vw_; On 2013/05/30 02:28:08, Ryan Sleevi wrote: > These field names should all be updated to more meaningful values. If you're > trying to mirror the specification, use comments to indicate that. It's more > important to be able to understand this class *without* having to read the spec > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... Done. (Renamed them, with added comments to describe use and reference CUP spec names.) https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode82 crypto/cup.h:82: #endif On 2013/05/30 02:28:08, Ryan Sleevi wrote: > No OpenSSL handling? I hadn't coded one yet, because this isn't used on any of the platforms that use OpenSSL yet. I can add one if it's desired for completeness' sake, although I generally try to not write code that's not going to be used. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode101 crypto/cup.h:101: static std::string UrlSafeB64Encode(const std::vector<uint8>& data); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > Design: All of these static-but-private member functions should be in an unnamed > namespace in the implementation file, unless you've explicitly added them for > unit tests (and I would suggest that they're not quite unit tests at that point, > because they're no longer testing the *public* interface). I explicitly added them here for unit tests -- I've found it easier to diagnose a break that way. I've moved it to an anonymous namespace. Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup.h#newcode125 crypto/cup.h:125: friend class CupUnitTest; On 2013/05/30 02:28:08, Ryan Sleevi wrote: > per general Chromium style, move this friend to line 72. > > Similarly, DISALLOW_COPY_AND_ASSIGN(ClientUpdateProtocol) ( see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... > ) Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup_nss.cc File crypto/cup_nss.cc (right): https://codereview.chromium.org/15793005/diff/27001/crypto/cup_nss.cc#newcode32 crypto/cup_nss.cc:32: On 2013/05/30 02:28:08, Ryan Sleevi wrote: > style: delete blank line. Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup_nss.cc#newcode35 crypto/cup_nss.cc:35: ATOB_AsciiToData(public_key, &der_encoded_spki_len)); On 2013/05/30 02:28:08, Ryan Sleevi wrote: > eg: using DER you don't have to go through these hijinks. > > Especially because we have base::Base64Encode/Decode. Done. https://codereview.chromium.org/15793005/diff/27001/crypto/cup_nss.cc#newcode64 crypto/cup_nss.cc:64: // arbitrarily and expect it to work and/or remain secure! On 2013/05/30 02:28:08, Ryan Sleevi wrote: > Comment nit: Avoid "we" in comments. Done.
Review comments on patch set 2: I reviewed cup_nss.cc carefully and cup.h (just a cursory review). After seeing how you defined the class, I think this class should reside in a directory other than src/crypto. If there is only one consumer of this class, we should put this class in that consumer's directory. https://codereview.chromium.org/15793005/diff/48001/crypto/cup.cc File crypto/cup.cc (right): https://codereview.chromium.org/15793005/diff/48001/crypto/cup.cc#newcode132 crypto/cup.cc:132: continue; I would replace these three 'continue' statements by 'break'. A 'continue' after the switch statement is not necessary in this case because there is nothing after the switch statement in the loop's body. https://codereview.chromium.org/15793005/diff/48001/crypto/cup.h File crypto/cup.h (right): https://codereview.chromium.org/15793005/diff/48001/crypto/cup.h#newcode59 crypto/cup.h:59: std::string* client_proof_out); Nit: it is uncommon to encode _in and _out in parameter names in the Chromium source tree. The convention of using const references for inputs and non-const pointers for outputs is usually enough. https://codereview.chromium.org/15793005/diff/48001/crypto/cup.h#newcode66 crypto/cup.h:66: const base::StringPiece& server_proof_in); |cookie_in| and |server_proof_in| need to be documented. Nit: with your naming convention, the first argument should be named response_body_in. But I recommend dropping the _in suffix. https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc File crypto/cup_nss.cc (right): https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode28 crypto/cup_nss.cc:28: // The binary blob |public_key| is expected to be a DER-encoded ASN.1-encoded ASN.1-encoded => ASN.1 https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode33 crypto/cup_nss.cc:33: spki_item.data = &pkey[0]; You can set spki_item.data to (unsigned char*)public_key.data() directly. (In proper C++ this requires a const_cast followed by a reinterpret_cast.) It is not necessary to copy public_key to pkey. SECKEY_DecodeDERSubjectPublicKeyInfo uses spki_item as const data, so the const_cast is safe. (This is a limitation of the SECItem struct, whose |data| field is a non-const pointer.) https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode50 crypto/cup_nss.cc:50: // WARNING: This call bypasses the usual PKCS padding and does direct RSA PKCS => PKCS #1 (There are other PKCS standards. One of them, PKCS #7, happens to also deal with padding.) https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode51 crypto/cup_nss.cc:51: // exponentiation. This not secure without taking measures to ensure that This not secure => This is not secure https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode55 crypto/cup_nss.cc:55: SECKEYPublicKey* pkv = public_key_.get(); The Style Guide recommends against variable names like |pkv| that are uncommon abbreviations. You can name it |public_key| or |key|. https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode64 crypto/cup_nss.cc:64: // takes the data in a non-const pointer. It *should* be safe to use a const_cast here as well because NSS does not need to modify the data when encrypting it. You can remove "for some reason" from the comment. I believe this is partly because NSS wasn't very careful about using 'const' input arguments before, and partly because the underlying PKCS #11 interface that NSS uses to talk to crypto modules does not have a const pointer type.
Thanks. Changes made. Regarding location, there's currently a single consumer for this code, but there may be other consumers later (those changes are being discussed for a potential M31 feature). If you guys don't think this is suitable for src/crypto, I'm fine moving it. https://codereview.chromium.org/15793005/diff/48001/crypto/cup.cc File crypto/cup.cc (right): https://codereview.chromium.org/15793005/diff/48001/crypto/cup.cc#newcode132 crypto/cup.cc:132: continue; On 2013/05/30 21:38:56, wtc wrote: > > I would replace these three 'continue' statements by 'break'. A 'continue' after > the switch statement is not necessary in this case because there is nothing > after the switch statement in the loop's body. Done. https://codereview.chromium.org/15793005/diff/48001/crypto/cup.h File crypto/cup.h (right): https://codereview.chromium.org/15793005/diff/48001/crypto/cup.h#newcode59 crypto/cup.h:59: std::string* client_proof_out); On 2013/05/30 21:38:56, wtc wrote: > > Nit: it is uncommon to encode _in and _out in parameter names in the Chromium > source tree. The convention of using const references for inputs and non-const > pointers for outputs is usually enough. Done. https://codereview.chromium.org/15793005/diff/48001/crypto/cup.h#newcode66 crypto/cup.h:66: const base::StringPiece& server_proof_in); On 2013/05/30 21:38:56, wtc wrote: > > |cookie_in| and |server_proof_in| need to be documented. > > Nit: with your naming convention, the first argument should be named > response_body_in. But I recommend dropping the _in suffix. Done. https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc File crypto/cup_nss.cc (right): https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode28 crypto/cup_nss.cc:28: // The binary blob |public_key| is expected to be a DER-encoded ASN.1-encoded On 2013/05/30 21:38:56, wtc wrote: > > ASN.1-encoded => ASN.1 Done. https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode33 crypto/cup_nss.cc:33: spki_item.data = &pkey[0]; On 2013/05/30 21:38:56, wtc wrote: > > You can set spki_item.data to (unsigned char*)public_key.data() directly. > (In proper C++ this requires a const_cast followed by a reinterpret_cast.) > It is not necessary to copy public_key to pkey. > > SECKEY_DecodeDERSubjectPublicKeyInfo uses spki_item as const data, so the > const_cast is safe. (This is a limitation of the SECItem struct, whose |data| > field is a non-const pointer.) Done. https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode50 crypto/cup_nss.cc:50: // WARNING: This call bypasses the usual PKCS padding and does direct RSA On 2013/05/30 21:38:56, wtc wrote: > > PKCS => PKCS #1 > > (There are other PKCS standards. One of them, PKCS #7, happens to also deal > with padding.) Done. https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode51 crypto/cup_nss.cc:51: // exponentiation. This not secure without taking measures to ensure that On 2013/05/30 21:38:56, wtc wrote: > > This not secure => This is not secure Done. https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode55 crypto/cup_nss.cc:55: SECKEYPublicKey* pkv = public_key_.get(); On 2013/05/30 21:38:56, wtc wrote: > > The Style Guide recommends against variable names like |pkv| that are > uncommon abbreviations. You can name it |public_key| or |key|. Done. https://codereview.chromium.org/15793005/diff/48001/crypto/cup_nss.cc#newcode64 crypto/cup_nss.cc:64: // takes the data in a non-const pointer. On 2013/05/30 21:38:56, wtc wrote: > > It *should* be safe to use a const_cast here as well because NSS does not > need to modify the data when encrypting it. > > You can remove "for some reason" from the comment. I believe this is partly > because NSS wasn't very careful about using 'const' input arguments before, > and partly because the underlying PKCS #11 interface that NSS uses to talk > to crypto modules does not have a const pointer type. Done.
actually, seems like src/google_apis is a *perfect* place to put this :) It's allowed to depend on crypto/ and net/ - all that you need.
Sounds good. I've moved it to google_apis, and added Joi as reviewer for that dir. Implementation has had one minor change due to unittests now living in browser_unittests -- NSS/OpenSSL impls are now implementations of virtual classes. The design hasn't changed at all, but PTAL just in case. Thanks!
https://codereview.chromium.org/15793005/diff/86001/google_apis/cup/client_up... File google_apis/cup/client_update_protocol.h (right): https://codereview.chromium.org/15793005/diff/86001/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:78: const std::vector<uint8>& key_source) = 0; You don't need to do this. We just use compile-time flags to switch which needs to be compiled. Just declare the methods and then implement in the appropriate _nss/_openssl file See src/crypto/rsa_private_key* or src/net/cert/x509_certificate*
On 2013/06/04 18:58:02, Ryan Sleevi wrote: > https://codereview.chromium.org/15793005/diff/86001/google_apis/cup/client_up... > File google_apis/cup/client_update_protocol.h (right): > > https://codereview.chromium.org/15793005/diff/86001/google_apis/cup/client_up... > google_apis/cup/client_update_protocol.h:78: const std::vector<uint8>& > key_source) = 0; > You don't need to do this. > > We just use compile-time flags to switch which needs to be compiled. Just > declare the methods and then implement in the appropriate _nss/_openssl file > > See src/crypto/rsa_private_key* or src/net/cert/x509_certificate* The issue I ran into is that the unit tests for google_apis aren't built on their own -- they're merged into the browser unit tests. Something is slightly funky about its build environment; USE_NSS is declared, but the NSS libraries aren't on the include path, so the previous design would fail to compile when cup.h couldn't find <keyhi.h> for a SECKEYPublicKey decl. If you can help me diagnose that, I can go back to the old compiled-in design.
On 2013/06/04 19:06:30, Ryan Myers (chromium) wrote: > On 2013/06/04 18:58:02, Ryan Sleevi wrote: > > > https://codereview.chromium.org/15793005/diff/86001/google_apis/cup/client_up... > > File google_apis/cup/client_update_protocol.h (right): > > > > > https://codereview.chromium.org/15793005/diff/86001/google_apis/cup/client_up... > > google_apis/cup/client_update_protocol.h:78: const std::vector<uint8>& > > key_source) = 0; > > You don't need to do this. > > > > We just use compile-time flags to switch which needs to be compiled. Just > > declare the methods and then implement in the appropriate _nss/_openssl file > > > > See src/crypto/rsa_private_key* or src/net/cert/x509_certificate* > > The issue I ran into is that the unit tests for google_apis aren't built on > their own -- they're merged into the browser unit tests. Something is slightly > funky about its build environment; USE_NSS is declared, but the NSS libraries > aren't on the include path, so the previous design would fail to compile when > cup.h couldn't find <keyhi.h> for a SECKEYPublicKey decl. > > If you can help me diagnose that, I can go back to the old compiled-in design. If you're introducing a dependency on NSS/OpenSSL in your public header, make sure to add 'export_dependent_settings': [ '../crypto/crypto.gyp:crypto', ] to google_apis.gyp (recommendation: under 'dependencies') This will ensure whatever flags needed to build the src/crypto types (and NSS & friends) are properly exposed. However, it's to avoid this that, as you can see in both crypto/ and net/, we avoid directly including the NSS headers in the .h files, and instead forward declare just the types necessary.
On 2013/06/04 19:25:47, Ryan Sleevi wrote: > On 2013/06/04 19:06:30, Ryan Myers (chromium) wrote: > > On 2013/06/04 18:58:02, Ryan Sleevi wrote: > > > > > > https://codereview.chromium.org/15793005/diff/86001/google_apis/cup/client_up... > > > File google_apis/cup/client_update_protocol.h (right): > > > > > > > > > https://codereview.chromium.org/15793005/diff/86001/google_apis/cup/client_up... > > > google_apis/cup/client_update_protocol.h:78: const std::vector<uint8>& > > > key_source) = 0; > > > You don't need to do this. > > > > > > We just use compile-time flags to switch which needs to be compiled. Just > > > declare the methods and then implement in the appropriate _nss/_openssl file > > > > > > See src/crypto/rsa_private_key* or src/net/cert/x509_certificate* > > > > The issue I ran into is that the unit tests for google_apis aren't built on > > their own -- they're merged into the browser unit tests. Something is > slightly > > funky about its build environment; USE_NSS is declared, but the NSS libraries > > aren't on the include path, so the previous design would fail to compile when > > cup.h couldn't find <keyhi.h> for a SECKEYPublicKey decl. > > > > If you can help me diagnose that, I can go back to the old compiled-in design. > > If you're introducing a dependency on NSS/OpenSSL in your public header, make > sure to add > > 'export_dependent_settings': [ > '../crypto/crypto.gyp:crypto', > ] > > to google_apis.gyp (recommendation: under 'dependencies') > > This will ensure whatever flags needed to build the src/crypto types (and NSS & > friends) are properly exposed. > > However, it's to avoid this that, as you can see in both crypto/ and net/, we > avoid directly including the NSS headers in the .h files, and instead forward > declare just the types necessary. Okay, trying that. New patchset returns to the previous design, forward declares SECKEYPublicKey.
The addition of this directory to //google_apis and the change to the .gyp files LGTM. I didn't review the implementation files. Cheers, Jói
On 2013/06/04 22:27:51, Jói wrote: > The addition of this directory to //google_apis and the change to the .gyp files > LGTM. I didn't review the implementation files. > > Cheers, > Jói Ty :) Crypto guys, any further changes?
First pass: Style & Design changes (not the crypto review) https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... File google_apis/cup/client_update_protocol.cc (right): https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:85: return hmac.Verify(ByteVectorToSP(hashes), ByteVectorToSP(server_proof)); If either |hashes| or |server_proof| are empty, this will crash (on line 19) It seems at least |server_proof| is attacker controlled, so we should be more robust in validating this (eg: like line 40) https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:118: } nit: drop the braces - you start off not using them (eg: all above). Consistent with net/ and crypto/ code - although if Joi prefers {} for components, then update above to use {} as well. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:138: std::vector<uint8> UrlSafeB64Decode(const base::StringPiece& input) { In the Chromium compiler chain, you're guaranteed to get any return-value-optimization here. Are you OK with the copying of the data? Would this be better as an out-param? https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:153: unsafe.append("="); nit: switch the while loop with just unsafe.append(unsafe.length() % 4, '='); https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:193: return result.release(); with the updated sig, this would be result.Pass() rather than .release() https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:206: return false; // Init() hasn't been called, and/or BuildSharedKey failed. Doesn't this make more sense to DCHECK(), rather than explicitly handle, as it indicates a misuse of the API? We tend to prefer DCHECK() for API constraints, and explicitly handling when dealing with external inputs. This is very much an API usage constraint. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:244: return false; // There hasn't been a call to SignRequest() yet. Same comments here re: DCHECK https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:269: drop the newline. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:272: return false; Per http://www.chromium.org/developers/coding-style , it's discouraged to both DCHECK() AND handle it. If public_key is user input, then don't DCHECK - but if public_key is hardcoded input, seems more appropriate to only DCHECK. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... File google_apis/cup/client_update_protocol.h (right): https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:15: // Forward declare types for NSS/OpenSSL. comment nit: You're not using OpenSSL ATM, so lets not say. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:35: static ClientUpdateProtocol* Create(int key_version, Chromium style is to prefer returning scoped_ptr<ClientUpdateProtocol> to force an ownership conversion. We have a C++11 move emulation layer, detailed at http://www.chromium.org/developers/smart-pointer-guidelines , that makes this efficient. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:49: // On success, returns true, and |cp_out| receives a Base64-encoded client s/cp_out/client_proof/ https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:55: // initialize a separate CUP instance for each one. This last bit seems better moved to the class level, around lines 27. That is, that each ClientUpdateProtocol instance represents a single update check. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:61: // with SignRequest(). |request_body| contains the body of the response in comment/OCD nit: Either use only a single space between each param, or newline separate. Preference is single-space. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:72: friend class CupUnitTest; naming nit: The vast majority of our tests omit the "Unit" part of test eg: CupTest https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:74: // Constructor is private -- use the Create method above. comment nit: Seems a superfluous comment. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:93: bool BuildSharedKey(size_t public_key_length, const uint8* opt_key_source); style/design: I would encourage two different calls here BuildSharedKey() BuildSharedKeyForTesting() We have special pre-submit scripts to ensure that *ForTesting() methods are only used in unit tests (... more or less), which is a handy design separation. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:126: DISALLOW_COPY_AND_ASSIGN(ClientUpdateProtocol); nit: DISALLOW_IMPLICIT_CONSTRUCTORS https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... File google_apis/cup/client_update_protocol_nss.cc (right): https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol_nss.cc:33: spki_item.data = (unsigned char*) public_key.data(); style: Use C++ casts - reinterpret_cast<unsigned char*>(const_cast<char*>(public_key.data())); https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol_nss.cc:59: const std::vector<uint8>& key_source) { Is it necessary to line wrap here? Seems like it fits on one line. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... File google_apis/cup/client_update_protocol_unittest.cc (right): https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol_unittest.cc:110: style: drop the extra newline. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol_unittest.cc:117: EXPECT_TRUE(CUP().SignRequest(url, kRequest, &cp)); Combined with the DCHECK comments for CUP, this should be ASSERT_TRUE() [since there is no way the test can succeed going on, and because the test will DCHECK if this failed]
Thanks! https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... File google_apis/cup/client_update_protocol.cc (right): https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:85: return hmac.Verify(ByteVectorToSP(hashes), ByteVectorToSP(server_proof)); On 2013/06/06 19:59:10, Ryan Sleevi wrote: > If either |hashes| or |server_proof| are empty, this will crash (on line 19) > > It seems at least |server_proof| is attacker controlled, so we should be more > robust in validating this (eg: like line 40) Done. (Added safety to ByteVectorToSP, and also added DCHECKs and early returns for empty inputs to SymSign and SymSignVerify.) https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:118: } On 2013/06/06 19:59:10, Ryan Sleevi wrote: > nit: drop the braces - you start off not using them (eg: all above). Consistent > with net/ and crypto/ code - although if Joi prefers {} for components, then > update above to use {} as well. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:138: std::vector<uint8> UrlSafeB64Decode(const base::StringPiece& input) { On 2013/06/06 19:59:10, Ryan Sleevi wrote: > In the Chromium compiler chain, you're guaranteed to get any > return-value-optimization here. Are you OK with the copying of the data? Would > this be better as an out-param? > I'm okay with the copying in this case, given that this is used to decode an HMAC (known small length). It makes the caller's code more intuitive, which is important, given that the caller is a crypto op. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:153: unsafe.append("="); On 2013/06/06 19:59:10, Ryan Sleevi wrote: > nit: switch the while loop with just > > unsafe.append(unsafe.length() % 4, '='); NACK -- that has different behavior entirely. While loop on "a" produces "a===" Your code on "a" produces "a=". Changed to a correct non-looping form, although I think that the while loop was perfectly readable. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:193: return result.release(); On 2013/06/06 19:59:10, Ryan Sleevi wrote: > with the updated sig, this would be result.Pass() rather than .release() Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:206: return false; // Init() hasn't been called, and/or BuildSharedKey failed. On 2013/06/06 19:59:10, Ryan Sleevi wrote: > Doesn't this make more sense to DCHECK(), rather than explicitly handle, as it > indicates a misuse of the API? > > We tend to prefer DCHECK() for API constraints, and explicitly handling when > dealing with external inputs. This is very much an API usage constraint. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:244: return false; // There hasn't been a call to SignRequest() yet. On 2013/06/06 19:59:10, Ryan Sleevi wrote: > Same comments here re: DCHECK Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:269: On 2013/06/06 19:59:10, Ryan Sleevi wrote: > drop the newline. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.cc:272: return false; On 2013/06/06 19:59:10, Ryan Sleevi wrote: > Per http://www.chromium.org/developers/coding-style , it's discouraged to both > DCHECK() AND handle it. > > If public_key is user input, then don't DCHECK - but if public_key is hardcoded > input, seems more appropriate to only DCHECK. Done, both here and elsewhere. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... File google_apis/cup/client_update_protocol.h (right): https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:15: // Forward declare types for NSS/OpenSSL. On 2013/06/06 19:59:10, Ryan Sleevi wrote: > comment nit: You're not using OpenSSL ATM, so lets not say. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:35: static ClientUpdateProtocol* Create(int key_version, On 2013/06/06 19:59:10, Ryan Sleevi wrote: > Chromium style is to prefer returning scoped_ptr<ClientUpdateProtocol> to force > an ownership conversion. > > We have a C++11 move emulation layer, detailed at > http://www.chromium.org/developers/smart-pointer-guidelines , that makes this > efficient. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:49: // On success, returns true, and |cp_out| receives a Base64-encoded client On 2013/06/06 19:59:10, Ryan Sleevi wrote: > s/cp_out/client_proof/ Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:55: // initialize a separate CUP instance for each one. On 2013/06/06 19:59:10, Ryan Sleevi wrote: > This last bit seems better moved to the class level, around lines 27. > > That is, that each ClientUpdateProtocol instance represents a single update > check. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:55: // initialize a separate CUP instance for each one. On 2013/06/06 19:59:10, Ryan Sleevi wrote: > This last bit seems better moved to the class level, around lines 27. > > That is, that each ClientUpdateProtocol instance represents a single update > check. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:72: friend class CupUnitTest; On 2013/06/06 19:59:10, Ryan Sleevi wrote: > naming nit: The vast majority of our tests omit the "Unit" part of test > > eg: CupTest Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:74: // Constructor is private -- use the Create method above. On 2013/06/06 19:59:10, Ryan Sleevi wrote: > comment nit: Seems a superfluous comment. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:93: bool BuildSharedKey(size_t public_key_length, const uint8* opt_key_source); On 2013/06/06 19:59:10, Ryan Sleevi wrote: > style/design: I would encourage two different calls here > > BuildSharedKey() > BuildSharedKeyForTesting() > > We have special pre-submit scripts to ensure that *ForTesting() methods are only > used in unit tests (... more or less), which is a handy design separation. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol.h:126: DISALLOW_COPY_AND_ASSIGN(ClientUpdateProtocol); On 2013/06/06 19:59:10, Ryan Sleevi wrote: > nit: DISALLOW_IMPLICIT_CONSTRUCTORS Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... File google_apis/cup/client_update_protocol_nss.cc (right): https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol_nss.cc:33: spki_item.data = (unsigned char*) public_key.data(); On 2013/06/06 19:59:10, Ryan Sleevi wrote: > style: Use C++ casts - reinterpret_cast<unsigned > char*>(const_cast<char*>(public_key.data())); Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol_nss.cc:59: const std::vector<uint8>& key_source) { On 2013/06/06 19:59:10, Ryan Sleevi wrote: > Is it necessary to line wrap here? Seems like it fits on one line. 84 chars. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... File google_apis/cup/client_update_protocol_unittest.cc (right): https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol_unittest.cc:110: On 2013/06/06 19:59:10, Ryan Sleevi wrote: > style: drop the extra newline. Done. https://codereview.chromium.org/15793005/diff/95004/google_apis/cup/client_up... google_apis/cup/client_update_protocol_unittest.cc:117: EXPECT_TRUE(CUP().SignRequest(url, kRequest, &cp)); On 2013/06/06 19:59:10, Ryan Sleevi wrote: > Combined with the DCHECK comments for CUP, this should be ASSERT_TRUE() [since > there is no way the test can succeed going on, and because the test will DCHECK > if this failed] Done.
I did not review the unit tests beyond style - I'm assuming they're intentionally chosen known-answer-tests that interop. From the security/crypto side, everything looks fine. A few more style nits, but I think at this point, this LGTM. https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... File google_apis/cup/client_update_protocol.cc (right): https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:86: DCHECK(!key.empty() && !hashes.empty() && !server_proof.empty()); Make this three DCHECKs - it's clearer when the failure happens what it was DCHECK(!key.empty()); DCHECK(!hashes.empty()); DCHECK(!server_proof.empty()); https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:119: // Omaha has its own implementation in base/security/b64.c; for Chromium, nit: Don't mention this path in public code. https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:164: } nit: inconsistent braces https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:183: if (!result.get()) if (!result) should compile fine, and is more idiomatic Chromium code. https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:275: crypto::RandBytes(&entropy[0], entropy.size()); I guess I didn't realize how little you'd be able to share between these implementations. Seems like you can factor the shared bit of these two into a common method - the only difference seems to be whether or not |key_source| is fixed or truly random.
Thanks! And yes, the unit tests just load a key and perform checks against known answers. https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... File google_apis/cup/client_update_protocol.cc (right): https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:86: DCHECK(!key.empty() && !hashes.empty() && !server_proof.empty()); On 2013/06/06 22:45:16, Ryan Sleevi wrote: > Make this three DCHECKs - it's clearer when the failure happens what it was > > DCHECK(!key.empty()); > DCHECK(!hashes.empty()); > DCHECK(!server_proof.empty()); Done (here and elsewhere). https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:119: // Omaha has its own implementation in base/security/b64.c; for Chromium, On 2013/06/06 22:45:16, Ryan Sleevi wrote: > nit: Don't mention this path in public code. Done. https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:164: } On 2013/06/06 22:45:16, Ryan Sleevi wrote: > nit: inconsistent braces Done. https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:183: if (!result.get()) On 2013/06/06 22:45:16, Ryan Sleevi wrote: > if (!result) should compile fine, and is more idiomatic Chromium code. Done. https://codereview.chromium.org/15793005/diff/130002/google_apis/cup/client_u... google_apis/cup/client_update_protocol.cc:275: crypto::RandBytes(&entropy[0], entropy.size()); On 2013/06/06 22:45:16, Ryan Sleevi wrote: > I guess I didn't realize how little you'd be able to share between these > implementations. > > Seems like you can factor the shared bit of these two into a common method - the > only difference seems to be whether or not |key_source| is fixed or truly > random. Done. PTAL.
LGTM. Since this isn't crypto/ or net/, I'm not going to be too pedantic about it, but "pronouns in comments considered harmful" ( https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/-wTxR... ) There's lots of "We" in the comments that can be trivially factored out.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ryanmyers@chromium.org/15793005/164001
Retried try job too often on win_rel for step(s) chrome_frame_net_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ryanmyers@chromium.org/15793005/164001
Message was sent while issue was closed.
Change committed as 204755 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
