Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
213473003:
This change introduces a way to tie source address token keys to specific QUIC server configs (Closed)
|
Created:
6 years, 9 months ago by ramant (doing other things) Modified:
6 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, avd, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis change introduces a way to tie source address token keys to specific QUIC
server configs, so that server can replace both the server config and
source address token key when communication with keystore succeeds.
Add retry logic and more graceful fallback to code that loads QUIC
insecure secrets from keystore. Not flag protected.
Merge internal change: 63497296
+ Changed ComputeSourceAddressTokenKey to DeriveSourceAddressTokenKey
+ Fixed comments.
+ Changed Config* to a const reference in BuildRejection,
NewSourceAddressToken and ValidateSourceAddressToken methods.
Merge internal change: 65382861
Use QuicEncrypter and QuicDecrypter with "AES128 + GCM-12" as the AEAD
algorithm to implement CryptoSecretBoxes' Box and Unbox methods. These
methods are used in unit tests only.
R=wtc@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266491
Patch Set 1 #Patch Set 2 : Use QuicEncrypter and QuicDecrypter to encrypt and decrypt SecretBoxer's Box/Unbox methods #
Total comments: 28
Patch Set 3 : Fix wtc's comments for patch set 2 #Patch Set 4 : Merge with TOT #Patch Set 5 : Merge internal change: 65382861 #
Total comments: 28
Patch Set 6 : Fix wtc's comments for patch set 5 #
Total comments: 6
Messages
Total messages: 26 (0 generated)
Hi Wan-Teh and Ryan,
This code is not ready for review yet. This patch works in internal source
code, but doesn't work in chromium because "if
(!config->source_address_token_boxer->Unbox(token, &storage, &plaintext)) {"
returns true when a token created with kPrimary is validated with kOverride. I
am investigating why chromium version of CryptoSecretBoxer::Unbox(" behaves
differently from server's version.
// Tokens generated by the primary config do not validate
// successfully against the override config, and vice versa.
EXPECT_FALSE(peer.ValidateSourceAddressToken(kOverride, token4, ip4, now));
Problem: crypto_secret_boxer in Chrome doesn't use key_ for anything: it just ignores the key! https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/cr... On Wed, Mar 26, 2014 at 6:14 PM, <rtenneti@chromium.org> wrote: > Hi Wan-Teh and Ryan, > This code is not ready for review yet. This patch works in internal > source > code, but doesn't work in chromium because "if > (!config->source_address_token_boxer->Unbox(token, &storage, &plaintext)) > {" > returns true when a token created with kPrimary is validated with > kOverride. I > am investigating why chromium version of CryptoSecretBoxer::Unbox(" behaves > differently from server's version. > > // Tokens generated by the primary config do not validate > // successfully against the override config, and vice versa. > EXPECT_FALSE(peer.ValidateSourceAddressToken(kOverride, token4, ip4, > now)); > > https://codereview.chromium.org/213473003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Wan-Teh, This is a very crude first cut at using QuicEncrypter and QuicDecrypter with "AES128 + GCM-12" as the AEAD algorithm to implement CryptoSecretBoxes' Box and Unbox methods. CryptoSecretBoxer's tests pass. AeadBaseEncrypter::Encrypt method accepts only 12 bytes nonce.size() (kNoncePrefixSize + sizeof(QuicPacketSequenceNumber)). Thus changed kBoxNonceSize to 12 bytes instead of 16 bytes. All unit tests have passed. PTAL. thanks raman
Review comments on patch set 2: I reviewed everything except quic_crypto_server_config_test.cc. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... File net/quic/crypto/crypto_secret_boxer.cc (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:10: #include "crypto/sha2.h" Please remove "crypto/secure_hash.h" and "crypto/sha2.h". https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:25: // TODO(rtenneti): Add support for kBoxNonceSize to be 16 bytes. Please explain why we want to switch to an algorithm that allows us to use a 16-byte nonce size. You can copy the explanation from Adam's email. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:29: size_t CryptoSecretBoxer::GetKeySize() { return kKeySize; } We should ideally get the key size from QuicEncrypter::GetKeySize(). But this may require changing this method to a non-static method. (It is not necessary to make this change.) https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:37: scoped_ptr<QuicEncrypter> encrypter(QuicEncrypter::Create(kAESG)); It seems that |encrypter| and |decrypter| should be data members and created in the constructor. Then, SetKey can just invoke encrypter_->SetKey() and decrypter_->SetKey(). The only downside I can see is that we may create encrypter_ without ever using it, if Box() is never called. (Similarly for decrypter_.) https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:39: DLOG(DFATAL) << "CryptoSecretBoxer's Encrypt failed."; This log message should say "SetKey" instead of "Encrypt". https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:53: if (!encrypter->Encrypt(string(data, kBoxNonceSize), StringPiece(), plaintext, 1. Change string(data, kBoxNonceSize) to StringPiece(data, kBoxNonceSize) 2. Make the same change to line 80. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... File net/quic/crypto/crypto_server_config_protobuf.h (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_server_config_protobuf.h:111: source_address_token_secret_override.as_string(); Nit: source_address_token_secret_override.CopyToString(&source_address_token_secret_override_) is slightly cheaper. You can make the same change to set_config(). https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_server_config_protobuf.h:112: LOG(ERROR) << "override: " << source_address_token_secret_override_; IMPORTANT: remove this error message? https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_server_config.cc (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.cc:51: string ComputeSourceAddressTokenKey(StringPiece source_address_token_secret) { Nit: "Derive" is the usual verb for this operation. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.cc:755: // to use. The comment "Otherwise we'll give it a copy of |primary_config_| to use.", or perhaps this whole paragraph, should be moved back to the call site at line 549 (or a little earlier, at line 547). The reference to |primary_config_| only makes sense there. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.cc:891: if (requested_config.get() != NULL && Why do we want to allow requested_config to be NULL? It may be better for the caller to ensure that requested_config is not NULL. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.cc:899: // No valid source address token. With the new code, this comment is no longer accurate. Now it should say something like "No server config with the requested ID, or no valid source address token". https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_server_config.h (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.h:381: const QuicCryptoServerConfig::Config* config, Why isn't this parameter a const reference? I found that we simply dereference |config| without checking, so this cannot be a NULL pointer.
Hi Wan-Teh, Thanks very much for your comments. Will merge this CL back into the internal source tree. Will make (or add a TODO comment) for couple of comments, I wasn't sure 100% off of original author's intentions. PTAL. thanks raman https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... File net/quic/crypto/crypto_secret_boxer.cc (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:10: #include "crypto/sha2.h" On 2014/04/07 18:38:24, wtc wrote: > > Please remove "crypto/secure_hash.h" and "crypto/sha2.h". Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:25: // TODO(rtenneti): Add support for kBoxNonceSize to be 16 bytes. On 2014/04/07 18:38:24, wtc wrote: > > Please explain why we want to switch to an algorithm that allows us to use a > 16-byte nonce size. You can copy the explanation from Adam's email. Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:29: size_t CryptoSecretBoxer::GetKeySize() { return kKeySize; } On 2014/04/07 18:38:24, wtc wrote: > > We should ideally get the key size from QuicEncrypter::GetKeySize(). But this > may require changing this method to a non-static method. (It is not necessary to > make this change.) Acknowledged. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:37: scoped_ptr<QuicEncrypter> encrypter(QuicEncrypter::Create(kAESG)); On 2014/04/07 18:38:24, wtc wrote: > > It seems that |encrypter| and |decrypter| should be data members and created in > the constructor. Then, SetKey can just invoke encrypter_->SetKey() and > decrypter_->SetKey(). The only downside I can see is that we may create > encrypter_ without ever using it, if Box() is never called. (Similarly for > decrypter_.) Will make this change in internal source code and will merge in chromium code. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:39: DLOG(DFATAL) << "CryptoSecretBoxer's Encrypt failed."; On 2014/04/07 18:38:24, wtc wrote: > > This log message should say "SetKey" instead of "Encrypt". Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:53: if (!encrypter->Encrypt(string(data, kBoxNonceSize), StringPiece(), plaintext, On 2014/04/07 18:38:24, wtc wrote: > > 1. Change > string(data, kBoxNonceSize) > to > StringPiece(data, kBoxNonceSize) > > 2. Make the same change to line 80. Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... File net/quic/crypto/crypto_server_config_protobuf.h (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_server_config_protobuf.h:111: source_address_token_secret_override.as_string(); On 2014/04/07 18:38:24, wtc wrote: > > Nit: > source_address_token_secret_override.CopyToString(&source_address_token_secret_override_) > is slightly cheaper. You can make the same change to set_config(). Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_server_config_protobuf.h:112: LOG(ERROR) << "override: " << source_address_token_secret_override_; On 2014/04/07 18:38:24, wtc wrote: > > IMPORTANT: remove this error message? Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_server_config.cc (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.cc:51: string ComputeSourceAddressTokenKey(StringPiece source_address_token_secret) { On 2014/04/07 18:38:24, wtc wrote: > > Nit: "Derive" is the usual verb for this operation. Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.cc:755: // to use. On 2014/04/07 18:38:24, wtc wrote: > > The comment "Otherwise we'll give it a copy of |primary_config_| to use.", or > perhaps this whole paragraph, should be moved back to the call site at line 549 > (or a little earlier, at line 547). The reference to |primary_config_| only > makes sense there. Please take a look at the new comments. Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.cc:891: if (requested_config.get() != NULL && On 2014/04/07 18:38:24, wtc wrote: > > Why do we want to allow requested_config to be NULL? It may be better for the > caller to ensure that requested_config is not NULL. Will make this change in internal source code. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.cc:899: // No valid source address token. On 2014/04/07 18:38:24, wtc wrote: > > With the new code, this comment is no longer accurate. Now it should say > something like "No server config with the requested ID, or no valid source > address token". Done. https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_server_config.h (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_server_config.h:381: const QuicCryptoServerConfig::Config* config, On 2014/04/07 18:38:24, wtc wrote: > > Why isn't this parameter a const reference? I found that we simply dereference > |config| without checking, so this cannot be a NULL pointer. Done.
https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... File net/quic/crypto/crypto_secret_boxer.cc (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:37: scoped_ptr<QuicEncrypter> encrypter(QuicEncrypter::Create(kAESG)); On 2014/04/21 22:39:29, ramant wrote: > On 2014/04/07 18:38:24, wtc wrote: > > > > It seems that |encrypter| and |decrypter| should be data members and created > in > > the constructor. Then, SetKey can just invoke encrypter_->SetKey() and > > decrypter_->SetKey(). The only downside I can see is that we may create > > encrypter_ without ever using it, if Box() is never called. (Similarly for > > decrypter_.) Done. > > Will make this change in internal source code and will merge in chromium code. "The only downside I can see is that we may create encrypter_ without ever using it, if Box() is never called.". BTW: It looks like Box is not always called (the following are couple of examples). https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/qu... https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/crypto/qu...
https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... File net/quic/crypto/crypto_secret_boxer.cc (right): https://codereview.chromium.org/213473003/diff/20001/net/quic/crypto/crypto_s... net/quic/crypto/crypto_secret_boxer.cc:37: scoped_ptr<QuicEncrypter> encrypter(QuicEncrypter::Create(kAESG)); On 2014/04/21 22:39:29, ramant wrote: > On 2014/04/07 18:38:24, wtc wrote: > > > > It seems that |encrypter| and |decrypter| should be data members and created > in > > the constructor. Then, SetKey can just invoke encrypter_->SetKey() and > > decrypter_->SetKey(). The only downside I can see is that we may create > > encrypter_ without ever using it, if Box() is never called. (Similarly for > > decrypter_.) > > Will make this change in internal source code and will merge in chromium code. Please ignore this comment. This change is n/a in the internal source.
Patch set 5 LGTM. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... File net/quic/crypto/crypto_secret_boxer.cc (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:33: size_t CryptoSecretBoxer::GetKeySize() { return kKeySize; } Please move the definition of this static method after the constructor and destructor. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:51: } Lines 48-51 should be deleted. Then, change SetKey to simply call encrypter_->SetKey(key) and decrypter_->SetKey(key), and remove the key_ data member. We may need to change the SetKey method to return bool so that it can report encrypter_->SetKey() or decrypter_->SetKey() failure. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:88: decrypter_->SetKey(key_); As noted above, remove this line. decrypter_->SetKey() should be done in the SetKey method. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... File net/quic/crypto/crypto_secret_boxer.h (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:13: #include "net/quic/crypto/crypto_protocol.h" We should not need to include this header. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:15: #include "net/quic/crypto/quic_encrypter.h" Nit: using a forward declaration of QuicDecrypter and QuicEncrypter may be sufficient. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:27: static size_t GetKeySize(); The Style Guide recommends that static methods should be declared after the constructor and destructor: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:31: virtual ~CryptoSecretBoxer(); Since this class has no virtual methods, I think the destructor doesn't need to be virtual. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:52: std::string key_; Remove the key_ member. I explain this in the .cc file. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_server_config.cc (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_server_config.cc:478: // to use. IMPORTANT: The original code doesn't have this comment in this function, so we should not add it. (This function doesn't have the "primary_config = primary_config_;" line.) https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_server_config.cc:750: scoped_refptr<QuicCryptoServerConfig::Config> Please see if we can change QuicCryptoServerConfig::Config to Config here. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_server_config.cc:1293: const QuicCryptoServerConfig::Config& config, Change QuicCryptoServerConfig::Config to Config on line 1280 and this line. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_server_config.h (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_server_config.h:393: const QuicCryptoServerConfig::Config& config, Change QuicCryptoServerConfig::Config to Config on line 384 and this line.
Thanks very much Wan-Teh for the comments. PTAL. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... File net/quic/crypto/crypto_secret_boxer.cc (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:33: size_t CryptoSecretBoxer::GetKeySize() { return kKeySize; } On 2014/04/22 19:12:10, wtc wrote: > > Please move the definition of this static method after the constructor and > destructor. Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:51: } On 2014/04/22 19:12:10, wtc wrote: > > Lines 48-51 should be deleted. Then, change SetKey to simply call > encrypter_->SetKey(key) and decrypter_->SetKey(key), and remove the key_ data > member. > > We may need to change the SetKey method to return bool so that it can report > encrypter_->SetKey() or decrypter_->SetKey() failure. Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:88: decrypter_->SetKey(key_); On 2014/04/22 19:12:10, wtc wrote: > > As noted above, remove this line. decrypter_->SetKey() should be done in the > SetKey method. Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... File net/quic/crypto/crypto_secret_boxer.h (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:13: #include "net/quic/crypto/crypto_protocol.h" On 2014/04/22 19:12:10, wtc wrote: > > We should not need to include this header. Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:15: #include "net/quic/crypto/quic_encrypter.h" On 2014/04/22 19:12:10, wtc wrote: > > Nit: using a forward declaration of QuicDecrypter and QuicEncrypter may be > sufficient. Was running into compilation errors ../../net/quic/crypto/crypto_secret_boxer.h:51:17: error: field has incomplete type 'net::QuicEncrypter' QuicEncrypter encrypter_; https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:27: static size_t GetKeySize(); On 2014/04/22 19:12:10, wtc wrote: > > The Style Guide recommends that static methods should be declared after the > constructor and destructor: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:31: virtual ~CryptoSecretBoxer(); On 2014/04/22 19:12:10, wtc wrote: > > Since this class has no virtual methods, I think the destructor doesn't need to > be virtual. Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:52: std::string key_; On 2014/04/22 19:12:10, wtc wrote: > > Remove the key_ member. I explain this in the .cc file. nit/small concern: this file is diverging from internal source tree. Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_server_config.cc (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_server_config.cc:478: // to use. On 2014/04/22 19:12:10, wtc wrote: > > IMPORTANT: The original code doesn't have this comment in this function, so we > should not add it. (This function doesn't have the "primary_config = > primary_config_;" line.) Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_server_config.cc:750: scoped_refptr<QuicCryptoServerConfig::Config> On 2014/04/22 19:12:10, wtc wrote: > > Please see if we can change QuicCryptoServerConfig::Config to Config here. Wasn't able to. Compile errors on linux. ../../net/quic/crypto/quic_crypto_server_config.cc:748:15: error: use of undeclared identifier 'Config' scoped_refptr<Config> ^ https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_server_config.cc:1293: const QuicCryptoServerConfig::Config& config, On 2014/04/22 19:12:10, wtc wrote: > > Change QuicCryptoServerConfig::Config to Config on line 1280 and this line. Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_server_config.h (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_server_config.h:393: const QuicCryptoServerConfig::Config& config, On 2014/04/22 19:12:10, wtc wrote: > > Change QuicCryptoServerConfig::Config to Config on line 384 and this line. Done.
The CQ bit was checked by rtenneti@chromium.org
The CQ bit was unchecked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/213473003/160001
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/213473003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/213473003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/213473003/160001
Message was sent while issue was closed.
Change committed as 266491
Message was sent while issue was closed.
Patch set 6 LGTM. I suggest some changes. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... File net/quic/crypto/crypto_secret_boxer.h (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:15: #include "net/quic/crypto/quic_encrypter.h" On 2014/04/25 23:40:58, ramant wrote: > > Was running into compilation errors > ../../net/quic/crypto/crypto_secret_boxer.h:51:17: error: field has incomplete > type 'net::QuicEncrypter' > QuicEncrypter encrypter_; Now the |encrypter_| member is a scoped_ptr: scoped_ptr<QuicEncrypter> encrypter_; a forward declaration of QuicEncrypter may be enough. In any case, you don't need to change this. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:52: std::string key_; On 2014/04/25 23:40:58, ramant wrote: > > nit/small concern: this file is diverging from internal source tree. That's true. I think we can keep the .h files in sync, but the .cc files will be different, and we will need to change encrypter_ and decrypter_ back to local variables in the Box and Unbox methods. https://codereview.chromium.org/213473003/diff/160001/net/quic/crypto/crypto_... File net/quic/crypto/crypto_secret_boxer.cc (right): https://codereview.chromium.org/213473003/diff/160001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:43: DCHECK_EQ(static_cast<size_t>(kKeySize), key.size()); Please remove the static_cast. kKeySize is already defined as a size_t. https://codereview.chromium.org/213473003/diff/160001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:44: string key_string = key.as_string(); It is not necessary to convert |key| to a string. encrypter_->SetKey() and decrypter_->SetKey() both take a StringPiece, so we can just pass |key| to them. https://codereview.chromium.org/213473003/diff/160001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:57: DCHECK_EQ(kKeySize, encrypter_->GetKeySize()); I think this DCHECK can be deleted. Alternatively, it should be moved to the SetKey method, where we call encrypter_->SetKey. But encrypter_->SetKey will check the key size, which is why I think this DCHECK can be removed.
Message was sent while issue was closed.
Hi Wan-Teh, Made the changes in the following CL. PTAL. https://codereview.chromium.org/257123002/ thanks raman https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... File net/quic/crypto/crypto_secret_boxer.h (right): https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:15: #include "net/quic/crypto/quic_encrypter.h" On 2014/04/28 18:44:58, wtc wrote: > > On 2014/04/25 23:40:58, ramant wrote: > > > > Was running into compilation errors > > ../../net/quic/crypto/crypto_secret_boxer.h:51:17: error: field has incomplete > > type 'net::QuicEncrypter' > > QuicEncrypter encrypter_; > > Now the |encrypter_| member is a scoped_ptr: > > scoped_ptr<QuicEncrypter> encrypter_; > > a forward declaration of QuicEncrypter may be enough. In any case, you don't > need to change this. Deleted the include files. Done. https://codereview.chromium.org/213473003/diff/120001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.h:52: std::string key_; On 2014/04/28 18:44:58, wtc wrote: > > On 2014/04/25 23:40:58, ramant wrote: > > > > nit/small concern: this file is diverging from internal source tree. > > That's true. I think we can keep the .h files in sync, but the .cc files will be > different, and we will need to change encrypter_ and decrypter_ back to local > variables in the Box and Unbox methods. Done. https://codereview.chromium.org/213473003/diff/160001/net/quic/crypto/crypto_... File net/quic/crypto/crypto_secret_boxer.cc (right): https://codereview.chromium.org/213473003/diff/160001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:43: DCHECK_EQ(static_cast<size_t>(kKeySize), key.size()); On 2014/04/28 18:44:58, wtc wrote: > > Please remove the static_cast. kKeySize is already defined as a size_t. Done. https://codereview.chromium.org/213473003/diff/160001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:44: string key_string = key.as_string(); On 2014/04/28 18:44:58, wtc wrote: > > It is not necessary to convert |key| to a string. encrypter_->SetKey() and > decrypter_->SetKey() both take a StringPiece, so we can just pass |key| to them. Done. https://codereview.chromium.org/213473003/diff/160001/net/quic/crypto/crypto_... net/quic/crypto/crypto_secret_boxer.cc:57: DCHECK_EQ(kKeySize, encrypter_->GetKeySize()); On 2014/04/28 18:44:58, wtc wrote: > > I think this DCHECK can be deleted. Alternatively, it should be moved to the > SetKey method, where we call encrypter_->SetKey. But encrypter_->SetKey will > check the key size, which is why I think this DCHECK can be removed. Done. Deleted the DECHEK_EQ. Wanted to make sure SetKey was called before calling Box. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
