Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1270043004:
Add URLRequest unit tests for HPKP violation reports (Closed)
DescriptionAdd URLRequest unit tests for HPKP violation reports
This CL adds URLRequest unit tests for sending HPKP violation
reports. The tests check that HPKP reports are sent when a URLRequest
violates a previously set pin, and that reports are sent when a
URLRequest violates the pins in a PKP-Report-Only header on that
request.
This is based on top of crrev.com/1266723003
BUG=445793
Committed: https://crrev.com/06e0dac77736300f1c675a491ca70711894fe3d4
Cr-Commit-Position: refs/heads/master@{#342450}
Patch Set 1 #
Total comments: 10
Patch Set 2 : rebase #Patch Set 3 : davidben comments #Patch Set 4 : move kHPKPReportUri inside OS_IOS block and closer to where it's used #Patch Set 5 : add BaseTestServer::GetCertificate() #
Total comments: 4
Patch Set 6 : GetLocalCertificatesDir() #
Messages
Total messages: 33 (10 generated)
estark@chromium.org changed reviewers: + davidben@chromium.org
David, could you please take a look? Added some additional unit tests for HPKP reports.
https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5386: #if defined(OS_ANDROID) Maybe for a separate CL, but I'm pretty sure you can just enable these now. Stuff has been moved around. I think Android will do HPKP, but not preloaded pins. https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5452: ASSERT_TRUE(hash2.FromString("sha1/o5OZxATDsgmwgcIfIWIneMJ0jkw=")); These values aren't significant, right? Maybe do something like nab this function: https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_secu... (Or perhaps just 111[..]111=?) I was going to ask at first if we could come up with a better story for keeping the pins and certificates in sync (compute them from the certificate or something?), but it seems this doesn't matter because the CertVerifier is also mocked. https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5488: context.CreateRequest(https_test_server.GetURL("files/hpkp-headers.html"), Probably best use simple.html or something, since that file has its own HPKP headers. (Which get ignored because they don't match the mocked public_key_hashes, but still.) https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5509: TEST_F(URLRequestTestHTTP, MAYBE_ProcessPKPReportOnly) { Perhaps add a test that also asserts there is no report if it matches? This would make sure we update those header files when the certs rotate. (A little sad we're adding another one, but yours isn't the first of those files.) https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5537: ASSERT_TRUE(hash.FromString("sha1/4BjDjn8v2lWeUFQnqSs0BgbIcrU=")); Ditto about making fake values obvious.
Thanks, David. https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5386: #if defined(OS_ANDROID) On 2015/08/05 18:23:51, David Benjamin wrote: > Maybe for a separate CL, but I'm pretty sure you can just enable these now. > Stuff has been moved around. I think Android will do HPKP, but not preloaded > pins. Ack: filed a bug, crbug.com/517311 https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5452: ASSERT_TRUE(hash2.FromString("sha1/o5OZxATDsgmwgcIfIWIneMJ0jkw=")); On 2015/08/05 18:23:51, David Benjamin wrote: > These values aren't significant, right? Maybe do something like nab this > function: > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_secu... > > (Or perhaps just 111[..]111=?) > > I was going to ask at first if we could come up with a better story for keeping > the pins and certificates in sync (compute them from the certificate or > something?), but it seems this doesn't matter because the CertVerifier is also > mocked. Done. https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5488: context.CreateRequest(https_test_server.GetURL("files/hpkp-headers.html"), On 2015/08/05 18:23:51, David Benjamin wrote: > Probably best use simple.html or something, since that file has its own HPKP > headers. (Which get ignored because they don't match the mocked > public_key_hashes, but still.) Done. https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5509: TEST_F(URLRequestTestHTTP, MAYBE_ProcessPKPReportOnly) { On 2015/08/05 18:23:51, David Benjamin wrote: > Perhaps add a test that also asserts there is no report if it matches? This > would make sure we update those header files when the certs rotate. > > (A little sad we're adding another one, but yours isn't the first of those > files.) Done. https://codereview.chromium.org/1270043004/diff/1/net/url_request/url_request... net/url_request/url_request_unittest.cc:5537: ASSERT_TRUE(hash.FromString("sha1/4BjDjn8v2lWeUFQnqSs0BgbIcrU=")); On 2015/08/05 18:23:51, David Benjamin wrote: > Ditto about making fake values obvious. Done.
lgtm
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270043004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270043004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1270043004/#ps60001 (title: "move kHPKPReportUri inside OS_IOS block and closer to where it's used")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270043004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270043004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
So it looks like this code is problematic on Windows: base::FilePath certificate_file = ssl_options.GetCertificateFile(); scoped_refptr<X509Certificate> cert = ImportCertFromFile(GetTestCertsDirectory(), certificate_file.BaseName().value()); Because ImportCertFromFile() expects the second arg to be a std::string, and on Windows it's a std::wstring because that's what base::FilePath::StringType is. Here are a couple options I see: 1. Modify ImportCertFromFile() to expect a base::FilePath::StringType instead of a std::string. 2. Give up and hard-code "localhost_cert.pem" instead of trying to ask the SpawnedTestServer what cert file it's using. Any thoughts?
On 2015/08/06 19:12:00, estark wrote: > So it looks like this code is problematic on Windows: > > base::FilePath certificate_file = ssl_options.GetCertificateFile(); > scoped_refptr<X509Certificate> cert = > ImportCertFromFile(GetTestCertsDirectory(), > certificate_file.BaseName().value()); > > Because ImportCertFromFile() expects the second arg to be a std::string, and on > Windows it's a std::wstring because that's what base::FilePath::StringType is. > Here are a couple options I see: > > 1. Modify ImportCertFromFile() to expect a base::FilePath::StringType instead of > a std::string. > 2. Give up and hard-code "localhost_cert.pem" instead of trying to ask the > SpawnedTestServer what cert file it's using. > > Any thoughts? Hrm. That's rather a nuisance. Changing ImportCertFromFile means everyone needs to use FILE_PATH_LITERAL for things, while ImportCertFromFile is currently only used with static strings. I guess there's also: 3. Change GetCertificateFile to return a std::string, since it's only ever returning one of some hard-coded data files. It's currently only used by the test server launcher. 4. Add an API to SSLOptions to give back the certificate. 5. Don't use ImportCertFromFile and implement something that consumes a base::FilePath yourself. (ImportCertFromFile is pretty tiny.) (I don't have strong opinions here.)
On 2015/08/06 21:39:35, David Benjamin wrote: > On 2015/08/06 19:12:00, estark wrote: > > So it looks like this code is problematic on Windows: > > > > base::FilePath certificate_file = ssl_options.GetCertificateFile(); > > scoped_refptr<X509Certificate> cert = > > ImportCertFromFile(GetTestCertsDirectory(), > > certificate_file.BaseName().value()); > > > > Because ImportCertFromFile() expects the second arg to be a std::string, and > on > > Windows it's a std::wstring because that's what base::FilePath::StringType is. > > Here are a couple options I see: > > > > 1. Modify ImportCertFromFile() to expect a base::FilePath::StringType instead > of > > a std::string. > > 2. Give up and hard-code "localhost_cert.pem" instead of trying to ask the > > SpawnedTestServer what cert file it's using. > > > > Any thoughts? > > Hrm. That's rather a nuisance. Changing ImportCertFromFile means everyone needs > to use FILE_PATH_LITERAL for things, while ImportCertFromFile is currently only > used with static strings. I guess there's also: Hmm? I don't think that's necessary. The second argument of ImportCertFromFile() is currently a std::string; I'm suggesting changing it to a base::FilePath::StringType (which is a typedef for std::string everywhere except on Windows, where it's a typedef for std::wstring). > > 3. Change GetCertificateFile to return a std::string, since it's only ever > returning one of some hard-coded data files. It's currently only used by the > test server launcher. > > 4. Add an API to SSLOptions to give back the certificate. > > 5. Don't use ImportCertFromFile and implement something that consumes a > base::FilePath yourself. (ImportCertFromFile is pretty tiny.) > > (I don't have strong opinions here.)
On 2015/08/06 21:43:37, estark wrote: > On 2015/08/06 21:39:35, David Benjamin wrote: > > On 2015/08/06 19:12:00, estark wrote: > > > So it looks like this code is problematic on Windows: > > > > > > base::FilePath certificate_file = ssl_options.GetCertificateFile(); > > > scoped_refptr<X509Certificate> cert = > > > ImportCertFromFile(GetTestCertsDirectory(), > > > certificate_file.BaseName().value()); > > > > > > Because ImportCertFromFile() expects the second arg to be a std::string, and > > on > > > Windows it's a std::wstring because that's what base::FilePath::StringType > is. > > > Here are a couple options I see: > > > > > > 1. Modify ImportCertFromFile() to expect a base::FilePath::StringType > instead > > of > > > a std::string. > > > 2. Give up and hard-code "localhost_cert.pem" instead of trying to ask the > > > SpawnedTestServer what cert file it's using. > > > > > > Any thoughts? > > > > Hrm. That's rather a nuisance. Changing ImportCertFromFile means everyone > needs > > to use FILE_PATH_LITERAL for things, while ImportCertFromFile is currently > only > > used with static strings. I guess there's also: > > Hmm? I don't think that's necessary. The second argument of ImportCertFromFile() > is currently a std::string; I'm suggesting changing it to a > base::FilePath::StringType (which is a typedef for std::string everywhere except > on Windows, where it's a typedef for std::wstring). Right, std::wstring takes L"blahblahblah" instead of "blahblahblah". FILE_PATH_LITERAL smooths those differences out.
On 2015/08/06 21:49:46, David Benjamin wrote: > On 2015/08/06 21:43:37, estark wrote: > > On 2015/08/06 21:39:35, David Benjamin wrote: > > > On 2015/08/06 19:12:00, estark wrote: > > > > So it looks like this code is problematic on Windows: > > > > > > > > base::FilePath certificate_file = ssl_options.GetCertificateFile(); > > > > scoped_refptr<X509Certificate> cert = > > > > ImportCertFromFile(GetTestCertsDirectory(), > > > > certificate_file.BaseName().value()); > > > > > > > > Because ImportCertFromFile() expects the second arg to be a std::string, > and > > > on > > > > Windows it's a std::wstring because that's what base::FilePath::StringType > > is. > > > > Here are a couple options I see: > > > > > > > > 1. Modify ImportCertFromFile() to expect a base::FilePath::StringType > > instead > > > of > > > > a std::string. > > > > 2. Give up and hard-code "localhost_cert.pem" instead of trying to ask the > > > > SpawnedTestServer what cert file it's using. > > > > > > > > Any thoughts? > > > > > > Hrm. That's rather a nuisance. Changing ImportCertFromFile means everyone > > needs > > > to use FILE_PATH_LITERAL for things, while ImportCertFromFile is currently > > only > > > used with static strings. I guess there's also: > > > > Hmm? I don't think that's necessary. The second argument of > ImportCertFromFile() > > is currently a std::string; I'm suggesting changing it to a > > base::FilePath::StringType (which is a typedef for std::string everywhere > except > > on Windows, where it's a typedef for std::wstring). > > Right, std::wstring takes L"blahblahblah" instead of "blahblahblah". > FILE_PATH_LITERAL smooths those differences out. Ah, I see. I'll pick one of options 3-5, then, thanks. :)
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
I like Option 4, fwiw :)
On 2015/08/06 23:40:06, Ryan Sleevi wrote: > I like Option 4, fwiw :) Option 4 it is! Done. Though I think GetCertificate() actually belongs on BaseTestServer, not SSLOptions, so that it can be read out of BaseTestServer's |certificates_dir_|. Does that seem right?
https://codereview.chromium.org/1270043004/diff/80001/net/test/spawned_test_s... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/1270043004/diff/80001/net/test/spawned_test_s... net/test/spawned_test_server/base_test_server.cc:340: base::FilePath certificate_path(certificates_dir_); Huh. Apparently certificates_dir_ is kind of wonky. The function above has to correct it somewhat. And the remote test server passes in paths relative to the source directory: https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... Whereas the local one is reasonable: https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... DIR_SOURCE_ROOT, when queried locally, is actually really funny on Android, probably to get the isolate-based data file pushing thing to work: https://code.google.com/p/chromium/codesearch#chromium/src/base/base_paths_an... You might need to add a GetLocalCertificateDir() or something for what to do if you want to read the certificate from where you are. I think right now certificate_dir_ is suitable for wherever the test server ends up running from, which isn't always local. https://codereview.chromium.org/1270043004/diff/80001/net/test/spawned_test_s... net/test/spawned_test_server/base_test_server.cc:352: cert_data.data(), cert_data.size(), X509Certificate::FORMAT_AUTO); Since the test server itself only accepts PEM, I think you can assume PEM and use FORMAT_PEM_CERT_SEQUENCE. Though ImportCertFromFile uses FORMAT_AUTO and probably reads the same set of files, so this is fine too. *shrug* Mostly a "explicit format is better than sniffing" thing.
https://codereview.chromium.org/1270043004/diff/80001/net/test/spawned_test_s... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/1270043004/diff/80001/net/test/spawned_test_s... net/test/spawned_test_server/base_test_server.cc:340: base::FilePath certificate_path(certificates_dir_); On 2015/08/07 14:50:28, David Benjamin wrote: > Huh. Apparently certificates_dir_ is kind of wonky. The function above has to > correct it somewhat. And the remote test server passes in paths relative to the > source directory: > https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... > > Whereas the local one is reasonable: > https://code.google.com/p/chromium/codesearch#chromium/src/net/test/spawned_t... > > DIR_SOURCE_ROOT, when queried locally, is actually really funny on Android, > probably to get the isolate-based data file pushing thing to work: > https://code.google.com/p/chromium/codesearch#chromium/src/base/base_paths_an... > > You might need to add a GetLocalCertificateDir() or something for what to do if > you want to read the certificate from where you are. I think right now > certificate_dir_ is suitable for wherever the test server ends up running from, > which isn't always local. Ah, so you mean that GetLocalCertificatesDir() should do what LoadTestRootCert() does to fix up certificates_dir_? Done. https://codereview.chromium.org/1270043004/diff/80001/net/test/spawned_test_s... net/test/spawned_test_server/base_test_server.cc:352: cert_data.data(), cert_data.size(), X509Certificate::FORMAT_AUTO); On 2015/08/07 14:50:28, David Benjamin wrote: > Since the test server itself only accepts PEM, I think you can assume PEM and > use FORMAT_PEM_CERT_SEQUENCE. Though ImportCertFromFile uses FORMAT_AUTO and > probably reads the same set of files, so this is fine too. *shrug* Mostly a > "explicit format is better than sniffing" thing. Done.
lgtm
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270043004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270043004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270043004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270043004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/06e0dac77736300f1c675a491ca70711894fe3d4 Cr-Commit-Position: refs/heads/master@{#342450} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
