Issue 346323002: net: Implement ChannelIDSourceChromium, which is based on Chromium's

Issue 346323002: net: Implement ChannelIDSourceChromium, which is based on Chromium's (Closed)

Created:
6 years, 6 months ago by wtc

Modified:
6 years, 5 months ago

Reviewers:
ramant (doing other things), Ryan Hamilton

CC:
chromium-reviews, cbentzel+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Project:
chromium

Visibility:
Public.

More Reviews

Description

net: Implement ChannelIDSourceChromium, which is based on Chromium's channel ID store. R=rch@chromium.org,rtenneti@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280730

Patch Set 4 : Address review comments #

Patch Set 5 : Rebase #

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+589 lines, -147 lines)			Patch
M	net/http/http_network_session.cc	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	net/net.gypi	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
A	net/quic/crypto/channel_id_chromium.h	View	1 2	1 chunk	+64 lines, -0 lines	0 comments	Download
A	net/quic/crypto/channel_id_chromium.cc	View		1 chunk	+249 lines, -0 lines	0 comments	Download
M	net/quic/crypto/proof_verifier_chromium.h	View	1 2 3 4	2 chunks	+1 line, -2 lines	0 comments	Download
M	net/quic/crypto/proof_verifier_chromium.cc	View	1 2 3 4	6 chunks	+8 lines, -9 lines	0 comments	Download
M	net/quic/quic_crypto_client_stream.h	View	1 2 3 4	7 chunks	+40 lines, -2 lines	0 comments	Download
M	net/quic/quic_crypto_client_stream.cc	View	1 2 3 4	8 chunks	+100 lines, -38 lines	0 comments	Download
M	net/quic/quic_crypto_server_stream_test.cc	View	1 2 3 4	1 chunk	+3 lines, -2 lines	0 comments	Download
M	net/quic/quic_stream_factory.h	View	1 2 3 4	5 chunks	+6 lines, -7 lines	0 comments	Download
M	net/quic/quic_stream_factory.cc	View	1 2 3 4	4 chunks	+4 lines, -1 line	0 comments	Download
M	net/quic/quic_stream_factory_test.cc	View	1 2 3 4	3 chunks	+6 lines, -0 lines	0 comments	Download
M	net/quic/test_tools/crypto_test_utils.h	View	1 2 3 4	1 chunk	+9 lines, -0 lines	0 comments	Download
M	net/quic/test_tools/crypto_test_utils.cc	View	1 2 3 4	1 chunk	+10 lines, -6 lines	0 comments	Download
M	net/quic/test_tools/crypto_test_utils_chromium.cc	View	1 2 3 4	3 chunks	+81 lines, -0 lines	0 comments	Download
M	net/quic/test_tools/crypto_test_utils_nss.cc	View	1 2 3 4	5 chunks	+5 lines, -80 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Ryan Hamilton

I still need to review the new state-machine in the crypto client stream. https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_client_stream.cc File ...

6 years, 6 months ago (2014-06-25 18:55:15 UTC) #2

ramant (doing other things)

https://codereview.chromium.org/346323002/diff/20001/net/quic/crypto/channel_id_chromium.h File net/quic/crypto/channel_id_chromium.h (right): https://codereview.chromium.org/346323002/diff/20001/net/quic/crypto/channel_id_chromium.h#newcode28 net/quic/crypto/channel_id_chromium.h:28: virtual std::string SerializeKey() const OVERRIDE; nit: consider adding #include ...

6 years, 6 months ago (2014-06-25 20:57:56 UTC) #3

Ryan Hamilton

I think I've reviewed the state machine changes now. Overall it looks good, but I ...

6 years, 6 months ago (2014-06-25 23:40:26 UTC) #4

wtc

Please review patch set 3. Thank you for your review comments. I addressed all of ...

6 years, 5 months ago (2014-06-28 16:03:30 UTC) #5

Please review patch set 3.

Thank you for your review comments. I addressed all of your comments, except for
renaming MockProofSource and MockProofVerifier. Please see my alternative
suggestions.

https://codereview.chromium.org/346323002/diff/20001/net/quic/crypto/channel_...
File net/quic/crypto/channel_id_chromium.h (right):

https://codereview.chromium.org/346323002/diff/20001/net/quic/crypto/channel_...
net/quic/crypto/channel_id_chromium.h:28: virtual std::string SerializeKey()
const OVERRIDE;

On 2014/06/25 20:57:56, ramant wrote:
> nit: consider adding #include <string>

Thanks for the suggestion. <string> is already included by the header of the
base class, "net/quic/crypto/channel_id.h". This is also why this file doesn't
include the header for StringPiece.

https://codereview.chromium.org/346323002/diff/20001/net/quic/crypto/channel_...
net/quic/crypto/channel_id_chromium.h:37:
ChannelIDSourceChromium(ServerBoundCertService* server_bound_cert_service);

On 2014/06/25 20:57:56, ramant wrote:
> nit:  explicit on line 37 (single argument)..

Done.

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:18: QuicCryptoClientStream* stream)

On 2014/06/25 18:55:15, Ryan Hamilton wrote:
> can you move this to the previous line?

Done.

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:331: }

On 2014/06/25 23:40:26, Ryan Hamilton wrote:
> This method is ... big. Can you factor this block into:
> bool do_channel_id = RequiresChannelId(cached)
> if (!do_channel_id) {
>   break;
> }
> ...

Done.

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:340: if (status == QUIC_PENDING) {

On 2014/06/25 23:40:26, Ryan Hamilton wrote:
> Consider using a switch here to be parallel with the code in
STATE_VERIFY_PROOF.

Done. I wrote it this way to avoid duplicating the "delete
channel_id_source_callback" in two cases of the switch statement. But I
understand consistency is good.

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:355: next_state_ = STATE_SEND_CHLO;

On 2014/06/25 23:40:26, Ryan Hamilton wrote:
> It seems like the channel id lookup might fail with an error code. In that
case,
> should we close the connection, or some such?

You are right -- we should handle synchronous and asynchronous channel id lookup
failures in the same way. I missed this point.

In my API design, an asynchronous channel id lookup failure is detected by a
null channel_id_key_. So if a channel id is not required, I am now transitioning
to the STATE_SEND_CHLO state directly.

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:90: friend class ProofVerifierCallbackImpl;

On 2014/06/25 18:55:15, Ryan Hamilton wrote:
> I *think* that you can remove these two friend statements since inner classes
> have access to the private members of the class that encloses them. Can you
try
> this out?

You are right. I verified these two inner classes do access the private members
of the enclosing class, and the code still compiles without the friend
statements. Thanks.

https://codereview.chromium.org/346323002/diff/20001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/346323002/diff/20001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:45: class MockProofSource :
public ProofSource {

On 2014/06/25 18:55:15, Ryan Hamilton wrote:
> Consider naming these classes "TestFoo" instead of "MockFoo" because when I
see
> "Mock" I think gmock and expect to be able to say EXPECT_CALL(...). Of course,
> these are temporary classes so this is not a terribly big deal.

There are many MockFoo classes in src/net that don't use gmock. However, I am
also unhappy with the MockProofSource and MockProofVerifier names. I didn't name
them TestProofSource and TestProofVerifier because those names are reserved for
the full implementation. See the TestProofVerifierChromium class on line 23.

Maybe I can name these HardcodedProofSource and HardcodedProofVerifier? Would
that be too wordy? Perhaps FixedProofSource and FixedProofVerifier?

https://codereview.chromium.org/346323002/diff/40001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/346323002/diff/40001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:141: // verify_context_ contains the
context object that we passed to an

Note to self: change "passed" to "pass" or "will pass".

Ryan Hamilton

lgtm https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_client_stream.cc File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_client_stream.cc#newcode340 net/quic/quic_crypto_client_stream.cc:340: if (status == QUIC_PENDING) { On 2014/06/28 16:03:29, ...

6 years, 5 months ago (2014-06-30 17:47:04 UTC) #6

lgtm

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/346323002/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:340: if (status == QUIC_PENDING) {
On 2014/06/28 16:03:29, wtc wrote:
> 
> On 2014/06/25 23:40:26, Ryan Hamilton wrote:
> > Consider using a switch here to be parallel with the code in
> STATE_VERIFY_PROOF.
> 
> Done. I wrote it this way to avoid duplicating the "delete
> channel_id_source_callback" in two cases of the switch statement. But I
> understand consistency is good.

Oh, fair enough! Feel free to switch both to the pattern you used in this CL if
you prefer...

https://codereview.chromium.org/346323002/diff/20001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/346323002/diff/20001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:45: class MockProofSource :
public ProofSource {
On 2014/06/28 16:03:30, wtc wrote:
> 
> On 2014/06/25 18:55:15, Ryan Hamilton wrote:
> > Consider naming these classes "TestFoo" instead of "MockFoo" because when I
> see
> > "Mock" I think gmock and expect to be able to say EXPECT_CALL(...). Of
course,
> > these are temporary classes so this is not a terribly big deal.
> 
> There are many MockFoo classes in src/net that don't use gmock. However, I am
> also unhappy with the MockProofSource and MockProofVerifier names. I didn't
name
> them TestProofSource and TestProofVerifier because those names are reserved
for
> the full implementation. See the TestProofVerifierChromium class on line 23.
> 
> Maybe I can name these HardcodedProofSource and HardcodedProofVerifier? Would
> that be too wordy? Perhaps FixedProofSource and FixedProofVerifier? 

Fixed sounds great to me!

https://codereview.chromium.org/346323002/diff/40001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/346323002/diff/40001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:445: }
Consider using early-returns.

if (!server_.is_https ||
    server == PRIVACY_MODE_ENABLED ||
    !crypto_config_->channel_id_source()) {
  return false;
}
..
if (!scfg || !scfg->GetTagList()) {
  return false;
}
...

Your call though.

wtc

Ryan: I made your suggested changes in patch set 4. I am going to rebase ...

6 years, 5 months ago (2014-06-30 19:35:15 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wtc@chromium.org/346323002/80001

6 years, 5 months ago (2014-07-01 00:46:35 UTC) #10

Message was sent while issue was closed.

Change committed as 280730

Expand Messages | Collapse Messages