|
|
Created:
6 years, 4 months ago by ramant (doing other things) Modified:
6 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Robbie Shade, avd, wtc, agl Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionPersist the server config that is received via kSCUP.
R=rjshade@chromium.org, rch@chromium.org, asvitkine@chromium.org, rvargas@chromium.org
Committed: https://crrev.com/c584fb92ddd78cb2c2a36b90f0d7be30227d3cff
Cr-Commit-Position: refs/heads/master@{#295582}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Verify proof from kSCUP message #
Total comments: 4
Patch Set 3 : Added histogram and minor cleanup per comments #
Total comments: 2
Patch Set 4 : Keep disk cache entry open for multiple writes #
Total comments: 2
Messages
Total messages: 30 (11 generated)
Persisted server config if proof is valid after chromium receives STK and server config update. If proof is not valid (when we receive STK update), we could set proof to be valid for insecure QUIC. Talking with rjshade if server sends kSCUP (server config updates) for insecure QUIC or not. if (!cached->proof_valid()) { if (!server_id_.is_https()) { // We don't check the certificates for insecure QUIC connections. SetCachedProofValid(cached);
https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_... net/quic/quic_crypto_client_stream.cc:162: if (cached->proof_valid()) { If we have access to the proof verifier here, we should verify the proof (for an https server) that is not yet verified.
https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_... net/quic/quic_crypto_client_stream.cc:162: if (cached->proof_valid()) { On 2014/07/31 00:43:06, wtc wrote: > > If we have access to the proof verifier here, we should verify the proof (for an > https server) that is not yet verified. Base on internal CL: 71770356, it looks like we are not getting certs and proof when we receive kSCUP message. Would like to talk to you about async proof verification (may be we could separate out the state machine for proof verification). Will hold off checking this change until we merge 71770356. rjshade@: what do you think? Or is there any advantage to add a TODO for proof verification and check-in this change for proof's that are already valid.
The intention is to send certs and proofs in the SCUP. cl/71770356 is attempting to do this. On Fri, Aug 1, 2014 at 3:52 PM, <rtenneti@chromium.org> wrote: > > https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_... > File net/quic/quic_crypto_client_stream.cc (right): > > https://codereview.chromium.org/427313002/diff/1/net/quic/quic_crypto_client_... > net/quic/quic_crypto_client_stream.cc:162: if (cached->proof_valid()) { > On 2014/07/31 00:43:06, wtc wrote: > >> If we have access to the proof verifier here, we should verify the > > proof (for an >> >> https server) that is not yet verified. > > > Base on internal CL: 71770356, it looks like we are not getting certs > and proof when we receive kSCUP message. > > Would like to talk to you about async proof verification (may be we > could separate out the state machine for proof verification). > > Will hold off checking this change until we merge 71770356. rjshade@: > what do you think? Or is there any advantage to add a TODO for proof > verification and check-in this change for proof's that are already > valid. > > https://codereview.chromium.org/427313002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
I think this looks pretty good. One issue we discussed offline is to make sure that we don't confused the cert used during the handshake with the one pushed in SCUP and hence become confused about what cert a page should be associated with. One suggestion about refactoring the loop. https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli... net/quic/quic_crypto_client_stream.cc:503: next_state_ = STATE_VERIFY_PROOF_COMPLETE; Since we have to conditionally set next_state_ in these methods, I wonder if it wouldn't be easier to just have a single DoLoop (or DoHandshakeLoop) method which is capable of handling any of the state transitions, instead of a HandshakeLoop and a ProofLoop? https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli... net/quic/quic_crypto_client_stream.cc:544: return QUIC_PROOF_INVALID; possibly add a histogram here when the handshake has been confirmed to see how often (never?) this happens.
Patchset #3 (id:80001) has been deleted
Hi Ryan, Made the changes, we have discussed. PTAL. thanks raman https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli... net/quic/quic_crypto_client_stream.cc:503: next_state_ = STATE_VERIFY_PROOF_COMPLETE; On 2014/09/15 22:37:56, Ryan Hamilton wrote: > Since we have to conditionally set next_state_ in these methods, I wonder if it > wouldn't be easier to just have a single DoLoop (or DoHandshakeLoop) method > which is capable of handling any of the state transitions, instead of a > HandshakeLoop and a ProofLoop? Done. https://codereview.chromium.org/427313002/diff/60001/net/quic/quic_crypto_cli... net/quic/quic_crypto_client_stream.cc:544: return QUIC_PROOF_INVALID; On 2014/09/15 22:37:56, Ryan Hamilton wrote: > possibly add a histogram here when the handshake has been confirmed to see how > often (never?) this happens. Done.
this change LGTM. I'd prefer to see the states simplified, but I'd be happy to see that in a follow up. (It would also be great to refactor the state machine so that each STATE has a DoState method and to use the conventions that we typically use in our state machines. But such a cleanup would only make sense in a followup) https://codereview.chromium.org/427313002/diff/100001/net/quic/quic_crypto_cl... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/427313002/diff/100001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:467: break; Since the implementation of these two states are basically the same thing, I think we should merge the two states. DoProofVerify will always transition to STATE_VERIFY_PROOF_COMPLETE DoProofVerifyComplete will conditionally transition to the next state based on handshake_confirmed(). What do you think?
rtenneti@chromium.org changed reviewers: + asvitkine@chromium.org
rtenneti@chromium.org changed reviewers: - wtc@chromium.org
+agl@: would appreciate your comments. +asvitkine@: for histograms.xml changes. https://codereview.chromium.org/427313002/diff/100001/net/quic/quic_crypto_cl... File net/quic/quic_crypto_client_stream.cc (right): https://codereview.chromium.org/427313002/diff/100001/net/quic/quic_crypto_cl... net/quic/quic_crypto_client_stream.cc:467: break; On 2014/09/17 15:52:41, Ryan Hamilton wrote: > Since the implementation of these two states are basically the same thing, I > think we should merge the two states. > > DoProofVerify will always transition to STATE_VERIFY_PROOF_COMPLETE > > DoProofVerifyComplete will conditionally transition to the next state based on > handshake_confirmed(). > > What do you think? Done.
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
rtenneti@chromium.org changed reviewers: + rvargas@chromium.org
Hi Ryan and Ricardo, Made a small change to keep disk cache entry open for multiple writes to happen. Previously we were closing the entry after write is complete. Ricardo and I have seen, when we tried to write again, we get IO_PENDING or FAILED error and we weren't writing again. This change keeps the cache entry to be opened and we close it when the QuicServerInfo is deleted. The following is an example of multiple writes to disk cache entry: t= 90479 [st= 0] +DISK_CACHE_ENTRY_IMPL [dt=31980] --> created = true --> key = "quicserverinfo:http://www.google.com:80" t= 90523 [st= 44] +ENTRY_WRITE_DATA [dt=0] --> buf_len = 216 --> index = 0 --> offset = 0 --> truncate = true t= 90523 [st= 44] -ENTRY_WRITE_DATA --> bytes_copied = 216 t= 93436 [st= 2957] +ENTRY_WRITE_DATA [dt=1] --> buf_len = 248 --> index = 0 --> offset = 0 --> truncate = true t= 93437 [st= 2958] -ENTRY_WRITE_DATA --> bytes_copied = 248 t=122458 [st=31979] ENTRY_CLOSE t=122459 [st=31980] -DISK_CACHE_ENTRY_IMPL PTAL.
lgtm
agl@chromium.org changed reviewers: + agl@chromium.org
lgtm
lgtm, thanks for doing this and looking forward to seeing the results https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info.cc (right): https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info.cc:227: entry_ = data_shim_->entry; Chromium style guide doesn't require {} around single line bodies?
Still LGTM https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas... File net/http/disk_cache_based_quic_server_info.cc (right): https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas... net/http/disk_cache_based_quic_server_info.cc:227: entry_ = data_shim_->entry; On 2014/09/18 19:42:51, rjshade wrote: > Chromium style guide doesn't require {} around single line bodies? Nope; Chrome's unofficial net/ style guide prohibits them :>
The CQ bit was checked by rtenneti@chromium.org
On 2014/09/18 19:52:52, Ryan Hamilton wrote: > Still LGTM > > https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas... > File net/http/disk_cache_based_quic_server_info.cc (right): > > https://codereview.chromium.org/427313002/diff/180001/net/http/disk_cache_bas... > net/http/disk_cache_based_quic_server_info.cc:227: entry_ = data_shim_->entry; > On 2014/09/18 19:42:51, rjshade wrote: > > Chromium style guide doesn't require {} around single line bodies? > > Nope; Chrome's unofficial net/ style guide prohibits them :> Many many thanks Adam, Alexei, Robbie and Ryan for code review, raman
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/427313002/180001
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as 6e75c2235199be33b0e598360473721cb5e09460
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c584fb92ddd78cb2c2a36b90f0d7be30227d3cff Cr-Commit-Position: refs/heads/master@{#295582}
Message was sent while issue was closed.
lgtm |