Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1074193003:
Verify alternative server certificate validity for origin. (Closed)
|
Created:
5 years, 8 months ago by Bence Modified:
5 years, 7 months ago Reviewers:
Ryan Hamilton CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVerify alternative server certificate validity for origin.
Verify that the certificate presented by the alternative server is valid for the
origin when using HTTP/2, both when opening a new connection or when pooling to
an existing one.
Patch Set 1:
* Enable hopping to a different host for HTTP/2 (not for QUIC).
* Add unittests for both new connection and pooling cases. Note in trybot
output that *Valid tests pass and *Invalid tests fail.
Patch Set 2 and up:
* Verify certificate validity in HttpStreamFactoryImpl::Job.
BUG=474217
Committed: https://crrev.com/1b0e36850193d0bd16669260c8eac374e9e419fe
Cr-Commit-Position: refs/heads/master@{#327293}
Patch Set 1 #Patch Set 2 : Verify certificate validity. #
Total comments: 29
Patch Set 3 : Addressing comments. #
Total comments: 2
Patch Set 4 : Re: new comments in #5. #Patch Set 5 : Combine two switch cases. #
Total comments: 12
Patch Set 6 : SpdySessionPool member and fewer parentheses. #Patch Set 7 : Rebase. #Patch Set 8 : Re: comments #7. #
Total comments: 4
Patch Set 9 : Rebase. #Patch Set 10 : Remove helper method. #Patch Set 11 : Remove new certificate. #Patch Set 12 : Change tests to use hostname "www.example.org". #Patch Set 13 : Rebase on https://crrev.com/1090943002. #Patch Set 14 : Edit comments. #Patch Set 15 : Re: #9. #
Total comments: 2
Patch Set 16 : Re: #11. #Patch Set 17 : Nit. #
Messages
Total messages: 17 (3 generated)
bnc@chromium.org changed reviewers: + rch@chromium.org
Ryan: PTAL. I understand that we are trying to move away from OrderedSocketData, but I would appreciate if I could land this now and work on rewriting the test in a follow-up CL. I tried to use DeterministicSocketData instead but ran into difficulties, I'll need quite some handholding. Thanks.
I haven't looked at the tests yet. Don't worry about the socket data bits. if you were using them in a new class that would be a problem, but HttpNetworkTransactionTest is full of 'em https://codereview.chromium.org/1074193003/diff/20001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1074193003/diff/20001/net/base/net_error_list... net/base/net_error_list.h:455: NET_ERROR(ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN, -214) Great error name and comment! https://codereview.chromium.org/1074193003/diff/20001/net/base/net_error_list... net/base/net_error_list.h:463: NET_ERROR(CERT_END, -215) We'll have to look very carefully about code that makes use of IsCertificateError() to be sure your new error is treated accordingly. Or perhaps it'll be best to put it in a different place. In some sense, it's not a "Certificate Error" so much as it is an "Alternate Service Error (because of certs)". I recommend moving, so we don't have to reason about this. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:202: return kNoAlternativeService; We actually intend to make use of this feature first in QUIC. I'm not sure if that affects your sequencing of work, but just FYI. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:299: return true; We only speak SPDY over TLS, right? I think this check is redundant. If not, can you explain why not? https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:301: if (origin_url_.host() == spdy_session->host_port_pair().host()) { It seems that this is simply a performance optimization to avoid calling VerifyDomainAuthentication. Is that right? If so, I wonder if we can push this logic down to VerifyDomainAuthentication and avoid doing this here, hence being able to get rid of this method. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:473: result != ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN) { Yeah, definitely move this error out of the certificate error section. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:551: case ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN: This should only happen for a job that IsAlternate(), right? If so, can you DCHECK() this? https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:556: // (origin, alternative service) couple as invalid. Is this a TODO (instead of being implemented) because we don't have the right method on HttpServerProperties? https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:562: ERR_CONNECTION_REFUSED)); Why ERR_CONNECTION_REFUSED? Actually, this looks a lot like the default: case below. Is it different? https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:834: if (!IsAlternativeCertificateValidForOrigin(spdy_session)) { We don't need to do this call, unless this job is a SPDY Alt-Svc job to a different hostname, right? https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1186: return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN; Hm. It seems like there are a lot of different places that we need to perform this check. That feels a bit error prone to me. I'm not sure what the solution is, but it feels like there's room to clean this up.
Ryan, PTAL. Thanks for the first round of review. Unfortunately your comments do not show up for me when I open the side-by-side diff. I could write comments but cannot edit them. Anyway, this is the first step. https://codereview.chromium.org/1074193003/diff/20001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1074193003/diff/20001/net/base/net_error_list... net/base/net_error_list.h:463: NET_ERROR(CERT_END, -215) Done. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl.cc:202: return kNoAlternativeService; Okay. I started with HTTP/2 tests because there are half as many of them. I'll get to the QUIC case next. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:299: return true; Well, not quite, this member was called for non-ssl Jobs too. I replaced this with an if (!IsSpdyAlternate()) return true; statement, which is closer to our intentions, so you are right, this check is not necessary. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:301: if (origin_url_.host() == spdy_session->host_port_pair().host()) { Not exactly. I added this test here because otherwise some unittests (for example, NextProto/HttpNetworkTransactionTest.UseAlternateProtocolForNpnSpdy as of Patch Set 3, and NextProto/HttpNetworkTransactionTest.SpdyPreconnectErrorNotConnectedOnWrite as of Patch Set 2) fail, because they provide a dummy SSLSocketDataProvider with no actual certificate. That begin said, I would not mind modifying these tests, or moving this check from HttpStreamFactoryImpl::Job::IsAlternativeCertificateValidForOrigin to SpdySession::VerifyDomainAuthentication. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:473: result != ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN) { Done. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:551: case ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN: This wasn't actually true, but yes, now it can only happen for a Spdy alternate Job. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:556: // (origin, alternative service) couple as invalid. Correct. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:562: ERR_CONNECTION_REFUSED)); In fact, you are right, this is a lot like the default case. Note, however, that if I return |result| here, then the error codes surfacing to the unittest in the new connection and pooling case are different. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:834: if (!IsAlternativeCertificateValidForOrigin(spdy_session)) { That is true. I think it might be easier to check that in IsAlternativeCertificateValidForOrigin(). Maybe we need to find a better name for this method. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1186: return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN; I do agree with you. Please let me know if you have any suggestions.
https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:301: if (origin_url_.host() == spdy_session->host_port_pair().host()) { On 2015/04/10 19:55:16, Bence wrote: > Not exactly. I added this test here because otherwise some unittests (for > example, NextProto/HttpNetworkTransactionTest.UseAlternateProtocolForNpnSpdy as > of Patch Set 3, and > NextProto/HttpNetworkTransactionTest.SpdyPreconnectErrorNotConnectedOnWrite as > of Patch Set 2) fail, because they provide a dummy SSLSocketDataProvider with no > actual certificate. That begin said, I would not mind modifying these tests, or > moving this check from > HttpStreamFactoryImpl::Job::IsAlternativeCertificateValidForOrigin to > SpdySession::VerifyDomainAuthentication. Ah. I'm not a big fan of having "real" code that's only there for tests, so I think that tweaking those tests seems like the right idea. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:562: ERR_CONNECTION_REFUSED)); On 2015/04/10 19:55:16, Bence wrote: > In fact, you are right, this is a lot like the default case. Note, however, > that if I return |result| here, then the error codes surfacing to the unittest > in the new connection and pooling case are different. Is there a difference between the two cases? (default: vs ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN:) https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1186: return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN; On 2015/04/10 19:55:16, Bence wrote: > I do agree with you. Please let me know if you have any suggestions. So it looks like each time we get a SPDY session from the session pool, we need to verify that it's usable. So that means the calls to FindAvailableSession and CreateAvailableSessionFromSocket. One option would be to create a wrapper class (in an anon name space at the top of the .cc file?) which is constructed with the origin and the real spdy session pool. Then it exposes just the two methods you need, something like int FindAvailableSession(SpdySessionKey, NetLog, SpdySession*) Then we would use only this wrapped pool, not the real pool. What do you think? https://codereview.chromium.org/1074193003/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:303: DCHECK(spdy_session); I think it made more sense to do the DCHECK at the top, since the idea is that this method should only be called with a real session. Alternatively, you could make this explicit by taking a "const SpdySession&" instead.
Ryan, PTAL. Thanks. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:301: if (origin_url_.host() == spdy_session->host_port_pair().host()) { On 2015/04/11 02:40:41, Ryan Hamilton wrote: > On 2015/04/10 19:55:16, Bence wrote: > > Not exactly. I added this test here because otherwise some unittests (for > > example, NextProto/HttpNetworkTransactionTest.UseAlternateProtocolForNpnSpdy > as > > of Patch Set 3, and > > NextProto/HttpNetworkTransactionTest.SpdyPreconnectErrorNotConnectedOnWrite as > > of Patch Set 2) fail, because they provide a dummy SSLSocketDataProvider with > no > > actual certificate. That begin said, I would not mind modifying these tests, > or > > moving this check from > > HttpStreamFactoryImpl::Job::IsAlternativeCertificateValidForOrigin to > > SpdySession::VerifyDomainAuthentication. > > Ah. I'm not a big fan of having "real" code that's only there for tests, so I > think that tweaking those tests seems like the right idea. I totally agree with you. It turns out that there are much more tests doing this that what I thought. Most unittests do not bother to create an actual cert. (I think the only way to do it is to read a .pem file from disk, which would be too much of a performance hit for that many tests.) Also note that this hostname check only delays this problem, because there will be a lot more ALTSVC tests with different hosts. Maybe we need a bool member |check_cert_| in SSLInfo or SSLSocketDataProvider that would be set to false for unittests that do not care, how does that sound? https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:562: ERR_CONNECTION_REFUSED)); On 2015/04/11 02:40:41, Ryan Hamilton wrote: > On 2015/04/10 19:55:16, Bence wrote: > > In fact, you are right, this is a lot like the default case. Note, however, > > that if I return |result| here, then the error codes surfacing to the unittest > > in the new connection and pooling case are different. > > Is there a difference between the two cases? (default: vs > ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN:) Only the DCHECK (and the TODO), so I merged them for now. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1186: return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN; On 2015/04/11 02:40:41, Ryan Hamilton wrote: > On 2015/04/10 19:55:16, Bence wrote: > > I do agree with you. Please let me know if you have any suggestions. > > So it looks like each time we get a SPDY session from the session pool, we need > to verify that it's usable. So that means the calls to FindAvailableSession and > CreateAvailableSessionFromSocket. One option would be to create a wrapper class > (in an anon name space at the top of the .cc file?) which is constructed with > the origin and the real spdy session pool. Then it exposes just the two methods > you need, something like > > int FindAvailableSession(SpdySessionKey, NetLog, SpdySession*) > > Then we would use only this wrapped pool, not the real pool. What do you think? I've done it, but it doesn't quite feel right. There is too much dependency, like the ValidSpdySessionPool instance has three members that mirror Job members, so it can only be instantiated once there members are calculated, and it is a latent invariant that they should not diverge between Job and ValidSpdySessionPool. Maybe FindAvailableSession, CreateAvailableSessionFromSocket, and IsAlternativeCertificateValidForOrigin should just be private methods of Job, what do you think? https://codereview.chromium.org/1074193003/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:303: DCHECK(spdy_session); On 2015/04/11 02:40:41, Ryan Hamilton wrote: > I think it made more sense to do the DCHECK at the top, since the idea is that > this method should only be called with a real session. Alternatively, you could > make this explicit by taking a "const SpdySession&" instead. I moved the spdy_session == nullptr case to this method too, so now it's part of the if. Do I need a better name for the method?
Looking much better! I really like the pool wrapper's centralization of the logic around cert checking. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:301: if (origin_url_.host() == spdy_session->host_port_pair().host()) { On 2015/04/13 17:52:40, Bence wrote: > On 2015/04/11 02:40:41, Ryan Hamilton wrote: > > On 2015/04/10 19:55:16, Bence wrote: > > > Not exactly. I added this test here because otherwise some unittests (for > > > example, NextProto/HttpNetworkTransactionTest.UseAlternateProtocolForNpnSpdy > > as > > > of Patch Set 3, and > > > NextProto/HttpNetworkTransactionTest.SpdyPreconnectErrorNotConnectedOnWrite > as > > > of Patch Set 2) fail, because they provide a dummy SSLSocketDataProvider > with > > no > > > actual certificate. That begin said, I would not mind modifying these > tests, > > or > > > moving this check from > > > HttpStreamFactoryImpl::Job::IsAlternativeCertificateValidForOrigin to > > > SpdySession::VerifyDomainAuthentication. > > > > Ah. I'm not a big fan of having "real" code that's only there for tests, so I > > think that tweaking those tests seems like the right idea. > > I totally agree with you. It turns out that there are much more tests doing > this that what I thought. Most unittests do not bother to create an actual > cert. (I think the only way to do it is to read a .pem file from disk, which > would be too much of a performance hit for that many tests.) Also note that > this hostname check only delays this problem, because there will be a lot more > ALTSVC tests with different hosts. Maybe we need a bool member |check_cert_| in > SSLInfo or SSLSocketDataProvider that would be set to false for unittests that > do not care, how does that sound? It seems like it should be pretty easy to add a helper method to, say, spdy_test_util_common.h which loads a well defined cert so we could just say: SSLSocketDataProvider ssl(ASYNC, OK); ssl.cert = LoadDefaultTestCert(); session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl); So it'd basically be just a 1 line add to the tests which do "the wrong thing". Does that sound plausible? https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:279: return SpdySessionKey(server_, proxy_info_.proxy_server(), privacy_mode); I recommend doing this cleanup in a different CL since it's orthogonal. https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:538: DCHECK((result != ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN) || No need for the 2nd set of ()s. Feel free to put IsSpdyAlternate() first if that helps). Or do: if (!SpdyAlternate()) DCHECK_NE(ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN, result); https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1136: if (!is_valid) { Hm. I'm not in love with this API. Instead, I would make the ValidPool methods return int and make the session an out param. That way the logic of your new NAME_NOT_VALID error would go in the pool https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1486: HttpNetworkSession* session, Looks like you can just take the Pool instead of session_, so you don't have to do session_->spdy_session_pool() https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1522: return !is_spdy_alternate_ || !spdy_session || It seems like is_spdy_alternate_ is not adding any value here since it seems like it's basically the same thing (effectively) as checking the hostname?
Ryan: PTAL. I included the modified unittests here so that we can savor them in context, but once we converge, I'd like to land them in a separate CL and rebase this one on that. https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:301: if (origin_url_.host() == spdy_session->host_port_pair().host()) { On 2015/04/13 18:29:56, Ryan Hamilton wrote: > On 2015/04/13 17:52:40, Bence wrote: > > On 2015/04/11 02:40:41, Ryan Hamilton wrote: > > > On 2015/04/10 19:55:16, Bence wrote: > > > > Not exactly. I added this test here because otherwise some unittests (for > > > > example, > NextProto/HttpNetworkTransactionTest.UseAlternateProtocolForNpnSpdy > > > as > > > > of Patch Set 3, and > > > > > NextProto/HttpNetworkTransactionTest.SpdyPreconnectErrorNotConnectedOnWrite > > as > > > > of Patch Set 2) fail, because they provide a dummy SSLSocketDataProvider > > with > > > no > > > > actual certificate. That begin said, I would not mind modifying these > > tests, > > > or > > > > moving this check from > > > > HttpStreamFactoryImpl::Job::IsAlternativeCertificateValidForOrigin to > > > > SpdySession::VerifyDomainAuthentication. > > > > > > Ah. I'm not a big fan of having "real" code that's only there for tests, so > I > > > think that tweaking those tests seems like the right idea. > > > > I totally agree with you. It turns out that there are much more tests doing > > this that what I thought. Most unittests do not bother to create an actual > > cert. (I think the only way to do it is to read a .pem file from disk, which > > would be too much of a performance hit for that many tests.) Also note that > > this hostname check only delays this problem, because there will be a lot more > > ALTSVC tests with different hosts. Maybe we need a bool member |check_cert_| > in > > SSLInfo or SSLSocketDataProvider that would be set to false for unittests that > > do not care, how does that sound? > > It seems like it should be pretty easy to add a helper method to, say, > spdy_test_util_common.h which loads a well defined cert so we could just say: > > SSLSocketDataProvider ssl(ASYNC, OK); > ssl.cert = LoadDefaultTestCert(); > session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl); > > So it'd basically be just a 1 line add to the tests which do "the wrong thing". > Does that sound plausible? Done. https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:279: return SpdySessionKey(server_, proxy_info_.proxy_server(), privacy_mode); On 2015/04/13 18:29:56, Ryan Hamilton wrote: > I recommend doing this cleanup in a different CL since it's orthogonal. Done: https://crrev.com/1063873003. https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:538: DCHECK((result != ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN) || On 2015/04/13 18:29:56, Ryan Hamilton wrote: > No need for the 2nd set of ()s. Feel free to put IsSpdyAlternate() first if that > helps). Or do: > > if (!SpdyAlternate()) > DCHECK_NE(ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN, result); Done. https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1136: if (!is_valid) { On 2015/04/13 18:29:56, Ryan Hamilton wrote: > Hm. I'm not in love with this API. Instead, I would make the ValidPool methods > return int and make the session an out param. That way the logic of your new > NAME_NOT_VALID error would go in the pool Done. https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1486: HttpNetworkSession* session, On 2015/04/13 18:29:56, Ryan Hamilton wrote: > Looks like you can just take the Pool instead of session_, so you don't have to > do session_->spdy_session_pool() Done. https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1522: return !is_spdy_alternate_ || !spdy_session || On 2015/04/13 18:29:56, Ryan Hamilton wrote: > It seems like is_spdy_alternate_ is not adding any value here since it seems > like it's basically the same thing (effectively) as checking the hostname? Per your other comment, I removed hostname checking. Hostname checking was more restrictive, since almost every unittest uses the same hostname (as changing hostnames is not implemented yet, this very CL will enable it).
Looks good! Adding the certs in a different CL on which you rebase this would be a good idea. I'd recommend that you ask one of the SSL folks to review the "adding a new SSL Cert" part of the CL. It looks fine to me, but I'm not terribly familiar with those scripts. https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1522: return !is_spdy_alternate_ || !spdy_session || On 2015/04/15 21:07:46, Bence wrote: > On 2015/04/13 18:29:56, Ryan Hamilton wrote: > > It seems like is_spdy_alternate_ is not adding any value here since it seems > > like it's basically the same thing (effectively) as checking the hostname? > > Per your other comment, I removed hostname checking. Hostname checking was more > restrictive, since almost every unittest uses the same hostname (as changing > hostnames is not implemented yet, this very CL will enable it). Can you add a comment about why you don't need to call VerifyDomainAuthentication here when it's not a SPDY alt-svc job? (It's obvious if you're familiar with the code, but not otherwise). https://codereview.chromium.org/1074193003/diff/140001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1074193003/diff/140001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:9216: ssl.SetCertFromFile("spdy.pem"); Clever! https://codereview.chromium.org/1074193003/diff/140001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:819: return result; nit: no {}s in 1-line ifs.
Ryan, PTAL. Thanks. https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1522: return !is_spdy_alternate_ || !spdy_session || On 2015/04/15 21:24:34, Ryan Hamilton wrote: > On 2015/04/15 21:07:46, Bence wrote: > > On 2015/04/13 18:29:56, Ryan Hamilton wrote: > > > It seems like is_spdy_alternate_ is not adding any value here since it seems > > > like it's basically the same thing (effectively) as checking the hostname? > > > > Per your other comment, I removed hostname checking. Hostname checking was > more > > restrictive, since almost every unittest uses the same hostname (as changing > > hostnames is not implemented yet, this very CL will enable it). > > Can you add a comment about why you don't need to call > VerifyDomainAuthentication here when it's not a SPDY alt-svc job? (It's obvious > if you're familiar with the code, but not otherwise). Done. https://codereview.chromium.org/1074193003/diff/140001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1074193003/diff/140001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:9216: ssl.SetCertFromFile("spdy.pem"); On 2015/04/15 21:24:34, Ryan Hamilton wrote: > Clever! Thanks. Too bad that it was vetoed in https://crrev.com/1090943002. https://codereview.chromium.org/1074193003/diff/140001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1074193003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:819: return result; On 2015/04/15 21:24:34, Ryan Hamilton wrote: > nit: no {}s in 1-line ifs. Done.
LGTM, modulo simplifying/cleaning up a comment. https://codereview.chromium.org/1074193003/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1074193003/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:157: // be used. nit: This comment is confusing, I think because it's trying to document two different methods. They both return the same value and treat |spdy_session| the same way, but they behave differently otherwise. I suggest that you document them independently.
Thank you. https://codereview.chromium.org/1074193003/diff/280001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1074193003/diff/280001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.h:157: // be used. On 2015/04/27 15:35:55, Ryan Hamilton wrote: > nit: This comment is confusing, I think because it's trying to document two > different methods. They both return the same value and treat |spdy_session| the > same way, but they behave differently otherwise. I suggest that you document > them independently. Done.
The CQ bit was checked by bnc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/1074193003/#ps320001 (title: "Nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1074193003/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/1b0e36850193d0bd16669260c8eac374e9e419fe Cr-Commit-Position: refs/heads/master@{#327293} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
