Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1939623002:
Strict Secure Cookies re-implemented to be priority aware (Closed)
DescriptionStrict Secure Cookies re-implemented to be priority aware
This implements Strict Secure Cookies to be priority-aware. Modifies the
cookie eviction and garbage collection algorithms to evict low priority
cookies first, then low priority secure cookies, followed by all other
non-secure cookies, and finally all secure cookies.
Our original Strict Secure Cookies implementation ignored priorities
completely, which led to some surprising interactions with applications.
This solution maintains a basic level of control over eviction order,
while maintaining that, overall, secure cookies should be lowest on the
eviction ladder (or highest: whichever you think means 'evicted later').
BUG=591686, 546820
R=mkwst@chromium.org
Committed: https://crrev.com/c00ac71cdb3caff6710e3c35c0b23493c3cde0d3
Cr-Commit-Position: refs/heads/master@{#391926}
Patch Set 1 #Patch Set 2 : Rebase on ToT #
Total comments: 20
Patch Set 3 : Nits from mkwst #Patch Set 4 : Rebase on ToT #Patch Set 5 : Fix Windows build #
Messages
Total messages: 16 (6 generated)
Mike, can you look at this?
Thank you, Joel. LGTM % nits and comments. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (left): https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1877: GarbageCollectNonSecure(non_expired_cookie_its, &secure_cookie_its); Nit: I think this was the only callsite to `GarbageCollectNonSecure()`, right? Can we remove the method entirely? https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:328: } This is clever. It wasn't at all how I'd considered approaching the problem; it's better. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1913: size_t quota; Nit: Maybe "additionalQuota"? I missed the `+=` bit at first, and was confused about why the secure rounds had `0u` as their quota. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1919: {COOKIE_PRIORITY_LOW, 0U, false}, Tiny nit: lowercase 'u', here and below. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1931: for (size_t i = 0; i < arraysize(purge_rounds) && purge_goal > 0; i++) { Nit: C++11 `for`? We'd used a regular `for` loop in the previous iteration because we needed `i` to cast into a `CookiePriority`. Now that the priority is part of the struct, can we be all fancy and modern? :) The `purge_goal > 0` bit should be covered by the DCHECK_LE on 1949, and the DCHECK on 1955, right? https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:2040: } Nit: This took me longer than I care to admit to wrap my head around. I think this might be clearer if you broke it out into a helper function named something like `CountProtectedSecureCookiesForPriority`, and did something like: ``` if (protect_secure_cookies) to_protect -= std::min(to_protect, CountProtectedSecureCookiesForPriority(cookies, priority)); ``` https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:408: // Take last character as security status, then discard it. Nit: You're not actually discarding either of these until line 418. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:414: // The rest of the string (possibly empty) specifies repetition. Nit: I think the "possibly empty" bit here is confusing. Is it meant to support `MN LS`? If so, it fails to do so, as `token.empty()` will be false, but `base::StringToInt` will return false. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:466: EXPECT_EQ(expected_count[i], num_for_priority); Nit: You could change this to `ASSERT_EQ` and skip the `if` on line 469. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:608: // should probably be clarified or made explict. The goal of the quota system as I understand it is to ensure that services that require low or medium priority cookies can't be completely starved by services on the same registrable domain that set high priority cookies. Erik and Samuel spelled this out to some extent in https://tools.ietf.org/html/draft-west-cookie-priority-00#section-3. It seems reasonable to me... WDYT?
On 2016/05/04 at 08:08:20, Mike West wrote: > Thank you, Joel. LGTM % nits and comments. Would you mind poking our friends in GAIA and Docs to let them know that we'll be trying this again? :)
https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (left): https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1877: GarbageCollectNonSecure(non_expired_cookie_its, &secure_cookie_its); On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > Nit: I think this was the only callsite to `GarbageCollectNonSecure()`, right? > Can we remove the method entirely? Done. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:328: } On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > This is clever. It wasn't at all how I'd considered approaching the problem; > it's better. Yay! Thanks. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1913: size_t quota; On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > Nit: Maybe "additionalQuota"? I missed the `+=` bit at first, and was confused > about why the secure rounds had `0u` as their quota. Done. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1919: {COOKIE_PRIORITY_LOW, 0U, false}, On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > Tiny nit: lowercase 'u', here and below. Done. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1931: for (size_t i = 0; i < arraysize(purge_rounds) && purge_goal > 0; i++) { On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > Nit: C++11 `for`? We'd used a regular `for` loop in the previous iteration > because we needed `i` to cast into a `CookiePriority`. Now that the priority is > part of the struct, can we be all fancy and modern? :) > > The `purge_goal > 0` bit should be covered by the DCHECK_LE on 1949, and the > DCHECK on 1955, right? Done. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:2040: } On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > Nit: This took me longer than I care to admit to wrap my head around. I think > this might be clearer if you broke it out into a helper function named something > like `CountProtectedSecureCookiesForPriority`, and did something like: > > ``` > if (protect_secure_cookies) > to_protect -= std::min(to_protect, > CountProtectedSecureCookiesForPriority(cookies, priority)); > ``` Done. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster_unittest.cc (right): https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:408: // Take last character as security status, then discard it. On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > Nit: You're not actually discarding either of these until line 418. Yeah! Whoever wrote all of these tests and comments in the first place must not have known what they were doing! (These were your tests and comments that I just took from your earlier reverted CL; I agree with this comment, though, so I'm making the change :-) https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:414: // The rest of the string (possibly empty) specifies repetition. On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > Nit: I think the "possibly empty" bit here is confusing. Is it meant to support > `MN LS`? If so, it fails to do so, as `token.empty()` will be false, but > `base::StringToInt` will return false. I think what it means is that simple 'MS' is a valid string so that you don't have to write 'MS1' to represent 1 medium secure cookie. This seems pretty understandable to me, but let me know if you'd like a further clarification. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:466: EXPECT_EQ(expected_count[i], num_for_priority); On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > Nit: You could change this to `ASSERT_EQ` and skip the `if` on line 469. The nice thing about keeping this as an EXPECT is that, if one priority fails the EXPECT, you'll get all the other priorities failing at the same time, so you can see what all of their values are. If it's an ASSERT, I believe it will crash at the first one, so you won't see what the values are, which is a bit harder for debugging. Again, let me know if you disagree. https://codereview.chromium.org/1939623002/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster_unittest.cc:608: // should probably be clarified or made explict. On 2016/05/04 08:08:19, Mike West (OOO until the 6th) wrote: > The goal of the quota system as I understand it is to ensure that services that > require low or medium priority cookies can't be completely starved by services > on the same registrable domain that set high priority cookies. Erik and Samuel > spelled this out to some extent in > https://tools.ietf.org/html/draft-west-cookie-priority-00#section-3. It seems > reasonable to me... WDYT? It does seem reasonable, however I'm pretty sure that is not how our priority implementation has ever worked. I'm modifying this comment to clarify, but here's an example (in our unit test terms): //Round 1 => 20L; round 2 => 0; round 3 => 11H TestPriorityCookieCase(cm.get(), "50LN 131HN", 30U, 0U, 120U, 150U, 0U); Based on your comments and the spec, I would expect this to pass because, as the comment suggests, it should remove 20L in the first round, down to the 30 low-priority quota, nothing in the second round b/c there are no medium priority cookies, and in the final round, I would expect 11 high-priority cookies to be removed, down to the 70 quota for high priority cookies while maintaining the 30 low-priority to match the low-priority quota. However, in practice, this test fails on both the current/old and new implementations. It results in 19LN and 139HN. Because the quotas accumulate from round to round, and what priority they apply to is lost, when it gets to the last round, all the purge sees is a quota of 150, and it starts purging from least recently accessed, which are the low-priority cookies. To summarize, our current implementation and this new implementation do not match what I think the spec says. However, I think we should address that in a separate CL (hence the comment), since it's how it has worked for a while.
On 2016/05/04 08:09:02, Mike West (OOO until the 6th) wrote: > On 2016/05/04 at 08:08:20, Mike West wrote: > > Thank you, Joel. LGTM % nits and comments. > > Would you mind poking our friends in GAIA and Docs to let them know that we'll > be trying this again? :) Yes, I'll email them shortly.
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1939623002/#ps60001 (title: "Rebase on ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1939623002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1939623002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1939623002/#ps80001 (title: "Fix Windows build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1939623002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1939623002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Strict Secure Cookies re-implemented to be priority aware This implements Strict Secure Cookies to be priority-aware. Modifies the cookie eviction and garbage collection algorithms to evict low priority cookies first, then low priority secure cookies, followed by all other non-secure cookies, and finally all secure cookies. Our original Strict Secure Cookies implementation ignored priorities completely, which led to some surprising interactions with applications. This solution maintains a basic level of control over eviction order, while maintaining that, overall, secure cookies should be lowest on the eviction ladder (or highest: whichever you think means 'evicted later'). BUG=591686,546820 R=mkwst@chromium.org ========== to ========== Strict Secure Cookies re-implemented to be priority aware This implements Strict Secure Cookies to be priority-aware. Modifies the cookie eviction and garbage collection algorithms to evict low priority cookies first, then low priority secure cookies, followed by all other non-secure cookies, and finally all secure cookies. Our original Strict Secure Cookies implementation ignored priorities completely, which led to some surprising interactions with applications. This solution maintains a basic level of control over eviction order, while maintaining that, overall, secure cookies should be lowest on the eviction ladder (or highest: whichever you think means 'evicted later'). BUG=591686,546820 R=mkwst@chromium.org Committed: https://crrev.com/c00ac71cdb3caff6710e3c35c0b23493c3cde0d3 Cr-Commit-Position: refs/heads/master@{#391926} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c00ac71cdb3caff6710e3c35c0b23493c3cde0d3 Cr-Commit-Position: refs/heads/master@{#391926} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
