Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2751803002:
Serialize and deserialize dynamic Expect-CT state (Closed)
DescriptionSerialize and deserialize dynamic Expect-CT state
This CL serializes/deserializes dynamic Expect-CT state in
TransportSecurityPersister. This is a step in the implementation of the draft
Expect-CT HTTP header
(https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct-00).
BUG=679012
Review-Url: https://codereview.chromium.org/2751803002
Cr-Commit-Position: refs/heads/master@{#465436}
Committed: https://chromium.googlesource.com/chromium/src/+/a368232f23a0dc98b909bdc424e11f794a8364e6
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix dictionary keys comment #
Total comments: 13
Patch Set 4 : mattm comments #Patch Set 5 : Expect-CT subdictionary, more tests #Patch Set 6 : add test for LoadEntries clearing #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 34 (24 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: linux_chromium_chromeos_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 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
Next in the Expect-CT series https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... File net/http/transport_security_persister.h (left): https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister.h:128: void PopulateEntryWithDefaults(base::DictionaryValue* host); This wasn't needed in the public interface, I moved it to just be a helper function in the .cc https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... File net/http/transport_security_persister.h (right): https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister.h:103: // "expect_ct_report_uri": string Only these last 4 are new in this CL; this comment had gotten out-of-date so I updated it to include the missing fields while I was here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... File net/http/transport_security_persister.cc (right): https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister.cc:107: // TODO(davidben): This can be removed when the STS and PKP states are stored I don't know the history here, but is there anything that can be done here instead of just continuing the current course? Is the end goal to have them in different files or just a nicer organization within a single file? If it's the first, then we could just put expect-CT data in a new file right from the start. If it's the latter then it may be a bit more complex.. maybe store all the expect-ct keys in a sub-dictionary under each hostname instead? https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister.cc:123: // Expect-CT default values. Should kExpectCTReportUri be in here? https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister.cc:255: if (!has_observed && !has_expiry && !has_enforce) Is the PopulateEntryWithDefaults change necessary if you already check whether the entries are present? https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... File net/http/transport_security_persister_unittest.cc (right): https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister_unittest.cc:251: EXPECT_TRUE(persister_->LoadEntries(serialized, &dirty)); So this is loading the serialized data back into the same state_ object? Would the test still pass if persister_ did nothing at all? https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister_unittest.cc:260: // Update the state for the domian and check that it is domain
davidben: what was the vision for on-disk storage of dynamic STS and PKP state? Trying to figure out if I should store Expect-CT state in a separate file, or in its own subdictionary in each host key, or... something else? https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... File net/http/transport_security_persister.cc (right): https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister.cc:123: // Expect-CT default values. On 2017/04/15 04:45:01, mattm wrote: > Should kExpectCTReportUri be in here? Removed this section per your comment below. https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister.cc:255: if (!has_observed && !has_expiry && !has_enforce) On 2017/04/15 04:45:01, mattm wrote: > Is the PopulateEntryWithDefaults change necessary if you already check whether > the entries are present? Hmm no it's not, removed. https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... File net/http/transport_security_persister_unittest.cc (right): https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister_unittest.cc:251: EXPECT_TRUE(persister_->LoadEntries(serialized, &dirty)); On 2017/04/15 04:45:02, mattm wrote: > So this is loading the serialized data back into the same state_ object? Would > the test still pass if persister_ did nothing at all? LoadEntries() clears existing dynamic data (https://cs.chromium.org/chromium/src/net/http/transport_security_persister.h?...) I noted this in a comment to make it clearer. I could instead reset |state_| and |persister_| before calling LoadEntries() to make it more explicit; any preference? https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister_unittest.cc:260: // Update the state for the domian and check that it is On 2017/04/15 04:45:01, mattm wrote: > domain Done.
On 2017/04/15 20:18:03, estark wrote: > davidben: what was the vision for on-disk storage of dynamic STS and PKP state? > Trying to figure out if I should store Expect-CT state in a separate file, or in > its own subdictionary in each host key, or... something else? I... don't have very strong opinions. Logically it should act like each of those is its own separate dictionary. I was thinking we should maybe do that at some point. You could also argue that this scheme is more compressed maybe? On grounds that the hosts are each only emitted once. Then again, we've got this PopulateEntryWithDefaults silliness in there which makes it less compressed. I don't remember why that's there. I guess so we don't try to parse the legacy kIncludeSubdomains and bits? I dunno, there's something to be said for compression not being all that coherent of a goal when we're storing as JSON and we should keep the format simple. Though I've forgotten enough of the details so you're probably a better judge of what's simplest than me.
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...
Thanks, David. In the absence of strong opinions, I think an Expect-CT subdictionary is simplest, and keeps the Expect-CT data separate from PKP/STS while also not repeating the hostname. mattm, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/18 17:10:59, estark wrote: > Thanks, David. In the absence of strong opinions, I think an Expect-CT > subdictionary is simplest, and keeps the Expect-CT data separate from PKP/STS > while also not repeating the hostname. mattm, PTAL? Unless someone can think of a good reason to need to load/save each list independently, this seems good to me. https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... File net/http/transport_security_persister_unittest.cc (right): https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister_unittest.cc:251: EXPECT_TRUE(persister_->LoadEntries(serialized, &dirty)); On 2017/04/15 20:18:03, estark wrote: > On 2017/04/15 04:45:02, mattm wrote: > > So this is loading the serialized data back into the same state_ object? Would > > the test still pass if persister_ did nothing at all? > > LoadEntries() clears existing dynamic data > (https://cs.chromium.org/chromium/src/net/http/transport_security_persister.h?...) > > I noted this in a comment to make it clearer. I could instead reset |state_| and > |persister_| before calling LoadEntries() to make it more explicit; any > preference? I think this would be fine if there was another test that verified that LoadEntries actually did clear entries (eg, create entries of each type, load an empty json dictionary, check that the entries no longer exist.)
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...
Patchset #6 (id:100001) has been deleted
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...
https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... File net/http/transport_security_persister_unittest.cc (right): https://codereview.chromium.org/2751803002/diff/40001/net/http/transport_secu... net/http/transport_security_persister_unittest.cc:251: EXPECT_TRUE(persister_->LoadEntries(serialized, &dirty)); On 2017/04/18 20:25:35, mattm wrote: > On 2017/04/15 20:18:03, estark wrote: > > On 2017/04/15 04:45:02, mattm wrote: > > > So this is loading the serialized data back into the same state_ object? > Would > > > the test still pass if persister_ did nothing at all? > > > > LoadEntries() clears existing dynamic data > > > (https://cs.chromium.org/chromium/src/net/http/transport_security_persister.h?...) > > > > I noted this in a comment to make it clearer. I could instead reset |state_| > and > > |persister_| before calling LoadEntries() to make it more explicit; any > > preference? > > I think this would be fine if there was another test that verified that > LoadEntries actually did clear entries (eg, create entries of each type, load an > empty json dictionary, check that the entries no longer exist.) Done.
lgtm
The CQ bit was unchecked by estark@chromium.org
The CQ bit was checked by estark@chromium.org
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": 120001, "attempt_start_ts": 1492556412381290,
"parent_rev": "ecc7661918e467da103b9e8f1c05b83265b1e707", "commit_rev":
"a368232f23a0dc98b909bdc424e11f794a8364e6"}
Message was sent while issue was closed.
Description was changed from ========== Serialize and deserialize dynamic Expect-CT state This CL serializes/deserializes dynamic Expect-CT state in TransportSecurityPersister. This is a step in the implementation of the draft Expect-CT HTTP header (https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct-00). BUG=679012 ========== to ========== Serialize and deserialize dynamic Expect-CT state This CL serializes/deserializes dynamic Expect-CT state in TransportSecurityPersister. This is a step in the implementation of the draft Expect-CT HTTP header (https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct-00). BUG=679012 Review-Url: https://codereview.chromium.org/2751803002 Cr-Commit-Position: refs/heads/master@{#465436} Committed: https://chromium.googlesource.com/chromium/src/+/a368232f23a0dc98b909bdc424e1... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/a368232f23a0dc98b909bdc424e1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
