|
|
DescriptionProcess Expect-CT HTTP header
This CL processes the Expect-CT header when received on HTTP
responses. TransportSecurityState parses the header and, if valid and received
on a compliant connection, stores the Expect-CT state for the current host. (If
valid but received on a non-compliant connection, the header information is not
stored but a report is sent to alert the site owner of the misconfiguration.) A
follow-up CL will check the dynamic Expect-CT state on connection setup.
BUG=679012
Review-Url: https://codereview.chromium.org/2774763005
Cr-Commit-Position: refs/heads/master@{#467555}
Committed: https://chromium.googlesource.com/chromium/src/+/ee03de14cb653c1e21ea4df5499a9fc183d9a921
Patch Set 1 #Patch Set 2 : Update comment #Patch Set 3 : rebase #Patch Set 4 : fix unit test report syntax #Patch Set 5 : rebase #
Total comments: 3
Patch Set 6 : fix header syntax in tests #
Total comments: 6
Patch Set 7 : mattm comments #Patch Set 8 : fix histograms deprecated date #
Depends on Patchset: Messages
Total messages: 38 (30 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
estark@chromium.org changed reviewers: + mattm@chromium.org
mattm: PTAL, thanks! (again, no time pressure) https://codereview.chromium.org/2774763005/diff/80001/net/http/transport_secu... File net/http/transport_security_state.cc (left): https://codereview.chromium.org/2774763005/diff/80001/net/http/transport_secu... net/http/transport_security_state.cc:1384: enum ExpectCTHeaderResult { This histogram was not particularly useful and makes less sense with the dynamic opt-in implemented, so I'm deprecating it. https://codereview.chromium.org/2774763005/diff/80001/net/http/transport_secu... File net/http/transport_security_state.cc (right): https://codereview.chromium.org/2774763005/diff/80001/net/http/transport_secu... net/http/transport_security_state.cc:1433: expect_ct_reporter_->OnExpectCTFailed(host_port_pair, report_uri, This part is a bit weird. In a follow-up CL, SSLClientSocketImpl will check the dynamic Expect-CT state when setting up connections (like HPKP does [1]). That check will send a report if the connection is not CT-compliant. When a site is in report-only mode, after setting up the connection and sending a report, we'll then go on to receive the HTTP header here, where we want to send a report *only* if it wasn't sent already in SSLClientSocketImpl (i.e., only if this header is setting Expect-CT state for the first time). Without the check on line 1432, we'd send two reports: one from checking Expect-CT in SSLClientSocketImpl, and one from processing the header here. An alternative to what I've done here is that we could send the report unconditionally here (i.e. remove the check in line 1432), and de-duplicate reports inside the reporter so that it only gets sent once. [1] https://cs.chromium.org/chromium/src/net/socket/ssl_client_socket_impl.cc?l=1254 https://codereview.chromium.org/2774763005/diff/80001/net/http/transport_secu... File net/http/transport_security_state.h (right): https://codereview.chromium.org/2774763005/diff/80001/net/http/transport_secu... net/http/transport_security_state.h:520: // connection is not CT-compliant, then a report will be sent. For more context, we currently have an experimental version of the Expect-CT header. If a site sends `Expect-CT: preload` and they are on the preload list, then they get a report when the connection is not CT-compliant. It's a little awkward to maintain this experimental version alongside the new dynamic header version. For now I'm trying to maintain the same behavior as before for sites that send `Expect-CT: preload`, though at some point it might make sense to make some changes so that it plays more nicely with the new dynamic opt-in version.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2774763005/diff/100001/net/http/transport_sec... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2774763005/diff/100001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:2681: base::Time now; not initialized https://codereview.chromium.org/2774763005/diff/100001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:2682: TransportSecurityState state; Should there be a SetExpectCTReporter here? Or add a separate test that it doesn't send a report when: ssl.is_issued_by_known_root = true; ssl.ct_compliance_details_available = true; ssl.ct_cert_policy_compliance = CERT_POLICY_COMPLIES_VIA_SCTS; https://codereview.chromium.org/2774763005/diff/100001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:2723: ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS; use CERT_POLICY_NOT_ENOUGH_SCTS to ensure it's really because of ct_compliance_details_available = false?
The CQ bit was checked by estark@chromium.org to run a CQ dry run
https://codereview.chromium.org/2774763005/diff/100001/net/http/transport_sec... File net/http/transport_security_state_unittest.cc (right): https://codereview.chromium.org/2774763005/diff/100001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:2681: base::Time now; On 2017/04/25 22:08:42, mattm wrote: > not initialized Done. https://codereview.chromium.org/2774763005/diff/100001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:2682: TransportSecurityState state; On 2017/04/25 22:08:42, mattm wrote: > Should there be a SetExpectCTReporter here? Or add a separate test that it > doesn't send a report when: > ssl.is_issued_by_known_root = true; > ssl.ct_compliance_details_available = true; > ssl.ct_cert_policy_compliance = CERT_POLICY_COMPLIES_VIA_SCTS; Done. https://codereview.chromium.org/2774763005/diff/100001/net/http/transport_sec... net/http/transport_security_state_unittest.cc:2723: ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS; On 2017/04/25 22:08:42, mattm wrote: > use CERT_POLICY_NOT_ENOUGH_SCTS to ensure it's really because of > ct_compliance_details_available = false? Done (same above)
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
estark@chromium.org changed reviewers: + isherman@chromium.org
isherman, can you please review histograms.xml?
histograms.xml lgtm, thanks
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org Link to the patchset: https://codereview.chromium.org/2774763005/#ps140001 (title: "fix histograms deprecated date")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493252719142000, "parent_rev": "d846315f83096c9b86e9206d901e56fa1190710a", "commit_rev": "ee03de14cb653c1e21ea4df5499a9fc183d9a921"}
Message was sent while issue was closed.
Description was changed from ========== Process Expect-CT HTTP header This CL processes the Expect-CT header when received on HTTP responses. TransportSecurityState parses the header and, if valid and received on a compliant connection, stores the Expect-CT state for the current host. (If valid but received on a non-compliant connection, the header information is not stored but a report is sent to alert the site owner of the misconfiguration.) A follow-up CL will check the dynamic Expect-CT state on connection setup. BUG=679012 ========== to ========== Process Expect-CT HTTP header This CL processes the Expect-CT header when received on HTTP responses. TransportSecurityState parses the header and, if valid and received on a compliant connection, stores the Expect-CT state for the current host. (If valid but received on a non-compliant connection, the header information is not stored but a report is sent to alert the site owner of the misconfiguration.) A follow-up CL will check the dynamic Expect-CT state on connection setup. BUG=679012 Review-Url: https://codereview.chromium.org/2774763005 Cr-Commit-Position: refs/heads/master@{#467555} Committed: https://chromium.googlesource.com/chromium/src/+/ee03de14cb653c1e21ea4df5499a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/ee03de14cb653c1e21ea4df5499a... |