Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(197)

Issue 2959593002: Update SCT serialization format in Expect-CT reports (Closed)

Created:
3 years, 6 months ago by estark
Modified:
3 years, 5 months ago
Reviewers:
mattm
CC:
chromium-reviews, rsleevi+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, Eran Messeri, net-reviews_chromium.org, martijn+crwatch_martijnc.be
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Update SCT serialization and other format details in Expect-CT reports This adds a net::ct::EncodeSignedCertificateTimestamp function alongside the existing CT serialization functions, and uses it to properly encode SCTs in Expect-CT reports. The relevant spec change is https://github.com/httpwg/http-extensions/commit/20c5cfd5ef5b630e142b3251ecafc004ad8f2092, though it hasn't made it into a published draft yet. Before, we were including a JSON object containing a subset of information from the SCT based on the source of the SCT, but that was deemed unnecessary and now the spec just says to include a standard serialization of the SCT. The other report format changes made to bring the implementation in line with the spec are: - Shortening the 'origin' string values - Wrapping the report in a JSON dictionary with a single 'expect-ct-report' key BUG=679012 Review-Url: https://codereview.chromium.org/2959593002 Cr-Commit-Position: refs/heads/master@{#482905} Committed: https://chromium.googlesource.com/chromium/src/+/19bd384f656790e8e409086b5b97a53b3a6f5921

Patch Set 1 #

Patch Set 2 : remove unnecessary include #

Total comments: 10

Patch Set 3 : mattm comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+128 lines, -175 lines) Patch
M chrome/browser/ssl/chrome_expect_ct_reporter.cc View 1 2 3 chunks +46 lines, -77 lines 0 comments Download
M chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc View 1 2 4 chunks +50 lines, -98 lines 0 comments Download
M net/cert/ct_serialization.h View 1 chunk +5 lines, -0 lines 0 comments Download
M net/cert/ct_serialization.cc View 1 chunk +15 lines, -0 lines 0 comments Download
M net/cert/ct_serialization_unittest.cc View 1 chunk +12 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 22 (15 generated)
estark
mattm, PTAL?
3 years, 6 months ago (2017-06-26 05:29:23 UTC) #10
mattm
https://codereview.chromium.org/2959593002/diff/20001/chrome/browser/ssl/chrome_expect_ct_reporter.cc File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/2959593002/diff/20001/chrome/browser/ssl/chrome_expect_ct_reporter.cc#newcode58 chrome/browser/ssl/chrome_expect_ct_reporter.cc:58: default: Use case "net:ct::SignedCertificateTimestamp::SCT_ORIGIN_MAX:" instead of default:? (So that ...
3 years, 5 months ago (2017-06-28 01:25:23 UTC) #11
estark
https://codereview.chromium.org/2959593002/diff/20001/chrome/browser/ssl/chrome_expect_ct_reporter.cc File chrome/browser/ssl/chrome_expect_ct_reporter.cc (right): https://codereview.chromium.org/2959593002/diff/20001/chrome/browser/ssl/chrome_expect_ct_reporter.cc#newcode58 chrome/browser/ssl/chrome_expect_ct_reporter.cc:58: default: On 2017/06/28 01:25:22, mattm wrote: > Use case ...
3 years, 5 months ago (2017-06-28 04:44:03 UTC) #13
mattm
lgtm
3 years, 5 months ago (2017-06-28 04:53:04 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2959593002/40001
3 years, 5 months ago (2017-06-28 04:54:22 UTC) #18
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/19bd384f656790e8e409086b5b97a53b3a6f5921
3 years, 5 months ago (2017-06-28 06:36:48 UTC) #21
engedy
3 years, 5 months ago (2017-06-28 09:22:20 UTC) #22
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2960163002/ by engedy@chromium.org.

The reason for reverting is: Heap-buffer-overflow on Chromium OS ASAN builders
during ChromeExpectCTReporterTest.SendReport.

Please see comments on revert CL for more detail..

Powered by Google App Engine
This is Rietveld 408576698