Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919773005:
Fix SecurityStyleChangedObserverNonsecureConnection test (Closed)
DescriptionFix SecurityStyleChangedObserverNonsecureConnection test
This test originally tried to use a net::URLRequestMockHTTPJob subclass
(URLRequestNonsecureConnection) to mock a connection with obsolete TLS
settings, but due to some missing boilerplate,
URLRequestNonsecureConnections were never actually used, and the test
was rather nonsensical. URLRequestNonsecureConnection was removed as
dead code in https://codereview.chromium.org/1899003002/.
This CL adds URLRequestNonsecureConnection back in and adds the missing
boilerplate so that this class is actually used. The test now
meaningfully tests what it's supposed to test.
BUG=604629
Committed: https://crrev.com/22c33fdbe07b62bbdcd4c9c36f8e5fbfa3820bed
Cr-Commit-Position: refs/heads/master@{#390800}
Patch Set 1 #
Total comments: 12
Patch Set 2 : msw comments #Patch Set 3 : fix cert import on non-IO thread #
Total comments: 21
Patch Set 4 : mmenke comments #
Total comments: 12
Patch Set 5 : davidben comments #Patch Set 6 : rebase #Patch Set 7 : another const scoped_refptr& #Messages
Total messages: 21 (8 generated)
estark@chromium.org changed reviewers: + msw@chromium.org
msw: PTAL
I'm not the most knowledgeable reviewer in this area. If possible, I suggest pinging someone more familiar; another set of eyes can't hurt. https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3125: const uint16_t ciphersuite = 0xc02f; nit: Follow net/http/http_cache_unittest.cc, drop comments and rename: const uint16_t kTLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 = 0xc02f; https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3136: DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureConnection); nit: order at bottom of class definition. https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3137: net::URLRequest* request_; nit: is this needed? https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3174: filter->AddHostnameInterceptor( Rather than adding an interceptor for a specific hostname, can we simplify the test and related code by setting some sort of url request handler that always returns a HttpResponseInfo with old SSL info or similar? Or is there some easier way to set some HttpResponseInfo for test? (maybe something around URLRequestJobFactory::MaybeInterceptResponse). I'm just asking, since I'm not actually familiar with url request loading/mocking... https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3184: base::FilePath root_http; nit: merge this with |serve_file|. https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3211: EXPECT_EQ(content::SECURITY_STYLE_AUTHENTICATED, Why do we expect SECURITY_STYLE_AUTHENTICATED here? (shouldn't it be SECURITY_STYLE_AUTHENTICATION_BROKEN or something?)
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
estark@chromium.org changed reviewers: + mmenke@chromium.org
Thanks, msw. FWIW, this code is very awkwardly placed in chrome/browser/ui and we will be moving it to chrome/browser/ssl, where it fits logically and where it can be reviewed by people who are more familiar. I just wanted to get this test fixed ASAP while it's fresh in my mind. mmenke: could you please take a look at this test as a second pair of eyes? Thanks! https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3125: const uint16_t ciphersuite = 0xc02f; On 2016/04/27 02:04:51, msw wrote: > nit: Follow net/http/http_cache_unittest.cc, drop comments and rename: > const uint16_t kTLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 = 0xc02f; Done. https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3136: DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureConnection); On 2016/04/27 02:04:51, msw wrote: > nit: order at bottom of class definition. Done. https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3137: net::URLRequest* request_; On 2016/04/27 02:04:51, msw wrote: > nit: is this needed? Nope, sorry, left in from debugging. Removed. https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3174: filter->AddHostnameInterceptor( On 2016/04/27 02:04:51, msw wrote: > Rather than adding an interceptor for a specific hostname, can we simplify the > test and related code by setting some sort of url request handler that always > returns a HttpResponseInfo with old SSL info or similar? Or is there some easier > way to set some HttpResponseInfo for test? (maybe something around > URLRequestJobFactory::MaybeInterceptResponse). I'm just asking, since I'm not > actually familiar with url request loading/mocking... Hmm, I'm not aware of any simpler way to do this, but I'm adding mmenke@ as a second pair of eyes and I expect he'll know if there is a simpler way. https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3184: base::FilePath root_http; On 2016/04/27 02:04:51, msw wrote: > nit: merge this with |serve_file|. Done. https://codereview.chromium.org/1919773005/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:3211: EXPECT_EQ(content::SECURITY_STYLE_AUTHENTICATED, On 2016/04/27 02:04:51, msw wrote: > Why do we expect SECURITY_STYLE_AUTHENTICATED here? > (shouldn't it be SECURITY_STYLE_AUTHENTICATION_BROKEN or something?) We don't downgrade the lock icon for obsolete TLS settings (security style roughly corresponds to lock icon); for such connections, we remove the message in devtools saying that the connection uses strong TLS settings but otherwise the page gets a happy green lock. I'll add a comment explaining this.
lgtm, but review from mmenke is recommended.
Sorry to punt the CL yet again, but I think it really needs an SSL expert - I'm just not familiar with the codepaths for SSL errors of this type. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3106: class URLRequestNonsecureConnection : public net::URLRequestMockHTTPJob { While you're here, mind renaming this to "URLRequestNonsecureConnectionJob" or somesuch? It's not actually a connection - quite a few layers above classes that represent connections. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3124: const uint16_t kTLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 = 0xc02f; naming style here is wrong. As a constant, should be named something like kTlsEdchRsaWithAes128GcmSha256. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3135: const scoped_refptr<net::X509Certificate> cert_; include ref_counted.h and net/cert/x509_certificate.h https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3136: DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureConnection); nit: Blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3162: base::FilePath base_path_; need to include file_path.h https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3162: base::FilePath base_path_; const? https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3163: const scoped_refptr<base::SequencedWorkerPool> worker_pool_; Should include the SequencedWorkerPool header https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3165: DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureInterceptor); nit: Blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3171: void AddNonsecureUrlHandlers( nit: Handlers -> Handler https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3188: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); Is there some reason this can't just be done in the constructor? Would be simpler if we could just do it in SetUpOnMainThread and get rid of |cert_| all together, but I assume that runs into thread restrictions. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3223: observer.latest_security_style()); I think you really need to have an SSL expert review this. I can certainly verify that youre injecting the job correctly (which you are), but I don't have the background to verify anything else (Whether the job is correctly behaving like it's running into an error of this type, or this test is checking for the right behavior).
estark@chromium.org changed reviewers: + davidben@chromium.org
David, can you please take a look at this? (it's a test for part of the security panel plumbing) haven't addressed mmenke's nits yet
https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3106: class URLRequestNonsecureConnection : public net::URLRequestMockHTTPJob { On 2016/04/27 20:19:44, mmenke wrote: > While you're here, mind renaming this to "URLRequestNonsecureConnectionJob" or > somesuch? It's not actually a connection - quite a few layers above classes > that represent connections. Done. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3124: const uint16_t kTLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 = 0xc02f; On 2016/04/27 20:19:44, mmenke wrote: > naming style here is wrong. > > As a constant, should be named something like kTlsEdchRsaWithAes128GcmSha256. Done. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3135: const scoped_refptr<net::X509Certificate> cert_; On 2016/04/27 20:19:44, mmenke wrote: > include ref_counted.h and net/cert/x509_certificate.h Done. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3136: DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureConnection); On 2016/04/27 20:19:44, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3162: base::FilePath base_path_; On 2016/04/27 20:19:44, mmenke wrote: > need to include file_path.h it's already included https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3162: base::FilePath base_path_; On 2016/04/27 20:19:44, mmenke wrote: > const? Done. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3163: const scoped_refptr<base::SequencedWorkerPool> worker_pool_; On 2016/04/27 20:19:44, mmenke wrote: > Should include the SequencedWorkerPool header Done. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3165: DISALLOW_COPY_AND_ASSIGN(URLRequestNonsecureInterceptor); On 2016/04/27 20:19:44, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3171: void AddNonsecureUrlHandlers( On 2016/04/27 20:19:44, mmenke wrote: > nit: Handlers -> Handler Done. https://codereview.chromium.org/1919773005/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:3188: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); On 2016/04/27 20:19:44, mmenke wrote: > Is there some reason this can't just be done in the constructor? Would be > simpler if we could just do it in SetUpOnMainThread and get rid of |cert_| all > together, but I assume that runs into thread restrictions. Yeah, I did it this way because it complained about reading it on a non-IO thread.
Sorry about the delay. This past week has been constant fire-fighting. :-/ lgtm with some minor comments. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3115: const scoped_refptr<base::TaskRunner>& task_runner) As of recently, const scoped_refptr<T>& is dead!!! (I hated it so much. So ugly...) https://www.chromium.org/developers/smart-pointer-guidelines Instead, since you're taking a reference, you can write just plain scoped_refptr<T> worker_pool and then cert_(std::move(cert)) to save the refcount hop. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3125: << net::SSL_CONNECTION_VERSION_SHIFT); net::SSLConnectionStatusSetVersion? https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3128: &info->ssl_info.connection_status); This is kind of weird. That pair isn't actually possible. TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA maybe? https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3143: class URLRequestNonsecureInterceptor : public net::URLRequestInterceptor { I'm not especially familiar with all the mess around how to make these things so I assume mmenke's previously pass got it. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3147: const scoped_refptr<base::SequencedWorkerPool>& worker_pool, Ditto about const-ref of scoped_refptr nightmares. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3191: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); Nit: Might be worth an ASSERT_TRUE(cert_) since that function can fail.
LGTM as well. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3143: class URLRequestNonsecureInterceptor : public net::URLRequestInterceptor { On 2016/04/29 21:22:49, davidben (slow) wrote: > I'm not especially familiar with all the mess around how to make these things so > I assume mmenke's previously pass got it. Yea, I reviewed this mess. And certainly agree it's an ugly API. :(
Thanks all for the reviews. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3115: const scoped_refptr<base::TaskRunner>& task_runner) On 2016/04/29 21:22:49, davidben (slow) wrote: > As of recently, const scoped_refptr<T>& is dead!!! (I hated it so much. So > ugly...) > > https://www.chromium.org/developers/smart-pointer-guidelines > > Instead, since you're taking a reference, you can write just plain > scoped_refptr<T> worker_pool and then cert_(std::move(cert)) to save the > refcount hop. Done. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3125: << net::SSL_CONNECTION_VERSION_SHIFT); On 2016/04/29 21:22:49, davidben (slow) wrote: > net::SSLConnectionStatusSetVersion? Done. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3128: &info->ssl_info.connection_status); On 2016/04/29 21:22:49, davidben (slow) wrote: > This is kind of weird. That pair isn't actually possible. > TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA maybe? Done. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3147: const scoped_refptr<base::SequencedWorkerPool>& worker_pool, On 2016/04/29 21:22:49, davidben (slow) wrote: > Ditto about const-ref of scoped_refptr nightmares. Done. https://codereview.chromium.org/1919773005/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_browsertest.cc:3191: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); On 2016/04/29 21:22:49, davidben (slow) wrote: > Nit: Might be worth an ASSERT_TRUE(cert_) since that function can fail. Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, mmenke@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1919773005/#ps160001 (title: "another const scoped_refptr&")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919773005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919773005/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/22c33fdbe07b62bbdcd4c9c36f8e5fbfa3820bed Cr-Commit-Position: refs/heads/master@{#390800}
Message was sent while issue was closed.
Description was changed from ========== Fix SecurityStyleChangedObserverNonsecureConnection test This test originally tried to use a net::URLRequestMockHTTPJob subclass (URLRequestNonsecureConnection) to mock a connection with obsolete TLS settings, but due to some missing boilerplate, URLRequestNonsecureConnections were never actually used, and the test was rather nonsensical. URLRequestNonsecureConnection was removed as dead code in https://codereview.chromium.org/1899003002/. This CL adds URLRequestNonsecureConnection back in and adds the missing boilerplate so that this class is actually used. The test now meaningfully tests what it's supposed to test. BUG=604629 ========== to ========== Fix SecurityStyleChangedObserverNonsecureConnection test This test originally tried to use a net::URLRequestMockHTTPJob subclass (URLRequestNonsecureConnection) to mock a connection with obsolete TLS settings, but due to some missing boilerplate, URLRequestNonsecureConnections were never actually used, and the test was rather nonsensical. URLRequestNonsecureConnection was removed as dead code in https://codereview.chromium.org/1899003002/. This CL adds URLRequestNonsecureConnection back in and adds the missing boilerplate so that this class is actually used. The test now meaningfully tests what it's supposed to test. BUG=604629 Committed: https://crrev.com/22c33fdbe07b62bbdcd4c9c36f8e5fbfa3820bed Cr-Commit-Position: refs/heads/master@{#390800} ========== |
