1325909 - Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Amy Chung [:Amy]]

Add a new bucket for comparing exact matches and "near matches" on BLOCKED_DOWNGRADE_SECURE.
And modify bucket 2 "blocked evicting secure cookie" to "downgraded secure cookie from secure website".

Comment 1

[Amy Chung [:Amy]]

Attachment: Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Hi Daniel,
I have added a new bucket and modified the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.
1. Added DOWNGRADE_SECURE_FROM_SECURE_BY_EXACT to record the exacting conditions.
2. If found a existing cookie (match exact conditions), and matched the situation of https downgraded the secure cookie.
   Added a telemetry to record the above situation.
3. Modified the description of telemetry to "https downgraded the similar secure cookie"

Please help me to review my patch, thanks!

Comment 2

[Daniel Veditz [:dveditz]]

Comment on attachment 8822152 [details] [diff] [review]
Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Review of attachment 8822152 [details] [diff] [review]:
-----------------------------------------------------------------

r- primarily because you're changing the bucket numbers for the untouched measures.

I'm not sure what the goal of this change is. It's nice to know which secure "downgrades" are overrides and which are just "near misses", but to a large degree we're just satisfying curiosity because we allow the cookie to be set either way. It might give hints to what we really want to know: how often are insecure sites blocked because of an exact match (definitely a bad thing) and how many are blocked because of a "near-match" (maybe an attack)? Is it worth changing telemetry at all if we don't capture both bits of data?

If we want that then we need to move things around a bit more:

1. move the call to "foundCookie = FindCookie(...) from line 3504 to just above the "if (mLeaveSecureAlone" on line 3476.

2. don't reset foundCookie from FindSecureCookie(), instead delete line 3487 and change the following "if" to

   if (!aCookie->IsSecure() && (foundCookie || FindSecureCookie(aKey, aCookie))) {

3. Then in each branch of the "if (!isSecure) ... else ..." you can set the "exact" telemetry bucket if foundCookie is true, and set the "inexact" bucket if it's false. OR, if you want to preserve the existing buckets -- ALWAYS set one of the two existing buckets (secure or not) and then "if (foundCookie)" also set the "_EXACT" version of the bucket. That way is only two new _EXACT buckets rather than four new buckets and retiring two old buckets (or changing the meaning of the old buckets).

4. delete the chunk you added at 3513 because you will have already counted that in step 3 above.
 
double-counting exact matches will alter the percentages reported by telemetry, although they are already pretty meaningless since they're lumped in with the expiration counts. I slightly prefer counting IN-exact matches separately from exact matches rather than counting "all" and then the exact sub-set but I could be talked into going either way.

::: netwerk/cookie/nsCookieService.cpp
@@ +126,5 @@
> +#define DOWNGRADE_SECURE_FROM_SECURE_BY_EXACT 3
> +#define EVICTED_NEWER_INSECURE                4
> +#define EVICTED_OLDEST_COOKIE                 5
> +#define EVICTED_PREFERRED_COOKIE              6
> +#define EVICTING_SECURE_BLOCKED               7

No, please do not ever change bucket numbers for existing buckets. This would mean we cannot compare across versions for the things that are measured the same. Breaks the "histogram evolution" views. Stick new ones on the end and leave the existing ones the same.

If you change the meaning of the existing buckets (looks like you didn't) it might even be worth retiring the old numbers. If you retire any replace those lines with a comment saying not to re-use those numbers. Whether to retire buckets depends on how big a change.

I haven't read the rest of the code yet, but it's at least as interesting to know for the ones we _block_ whether it's exact or not so we might want to make a bucket for that one, too. 

If you really want to keep the buckets together (can be useful) and signal that the old and new values can't be compared then you could stop using the old buckets and use new ones on the end. In this case, though, you haven't changed the meaning of DOWNGRADE_SECURE_FROM_SECURE so I don't think you need to do that. If you changed it to DOWNGRADE_SECURE_FROM_SECURE_INEXACT and only counted part of what it's currently counting then you might want to do that. What your patch does is perfectly fine: it's easy enough to subtract the new measurement from the original, and that keeps consistency with the old bucket.

@@ +3491,2 @@
>      if (foundCookie && !aCookie->IsSecure()) {
> +      if (!downgradeSecureCookieBySecureSite) {

The code would be clearer to leave this bit unchanged (apart from the bool initialization on line 3477) and then set

    downgradeSecureCookieBySecureSite = true;

inside the "else" clause below where we do the secure-site telemetry collection.

Comment 3

[Amy Chung [:Amy]]

Attachment: Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Hi Daniel,
Thanks for your suggestions, I have modified my patch as below:
1. Restored the buckets number on telemetry COOKIE_LEAVE_SECURE_ALONE.
2. Added two buckets:
   #define BLOCKED_DOWNGRADE_SECURE_BY_EXACT     7
   #define DOWNGRADE_SECURE_FROM_SECURE_BY_EXACT 8
3. Moved "foundCookie = FindCookie(...)" to above the "if (mLeaveSecureAlone)".
4. Modified if condition to if (!aCookie->IsSecure() && (foundCookie || FindSecureCookie(aKey, aCookie))).
5. Added condition "if (foundCookie)" into "if (!isSecure) {} else {}".

Please help me to review my patch, thanks!

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 8825272 [details] [diff] [review]
Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Review of attachment 8825272 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great except for some incomplete advice I gave (see below) so we'll need another revision.

::: netwerk/cookie/nsCookieService.cpp
@@ +121,5 @@
>  
>  // For telemetry COOKIE_LEAVE_SECURE_ALONE
> +#define BLOCKED_SECURE_SET_FROM_HTTP          0
> +#define BLOCKED_DOWNGRADE_SECURE              1
> +#define DOWNGRADE_SECURE_FROM_SECURE          2

Since 1 and 2 are no longer totals it would be clearer to add _INEXACT to the names (much the way you adjusted their description in Histograms.json)

I'm more interested in the _EXACT totals, actually, and would prefer 1 and 2 being the _EXACT versions and 7 and 8 being the _INEXACT versions. But that's less important than the clarity of adding _INEXACT.

@@ +3488,5 @@
>      // of the "request-uri" does not denote a "secure" protocol,
>      // then ignore the new cookie.
>      // (draft-ietf-httpbis-cookie-alone section 3.2)
> +    if (!aCookie->IsSecure()
> +         && (foundCookie || FindSecureCookie(aKey, aCookie))) {

Oh no, I gave incomplete, bad advice! Just because we found a cookie doesn't mean it's a _secure_ cookie that should block things. I'm so sorry for that. It will need to be something like

if (!aCookie->IsSecure()
    && ((foundCookie && exactIter.Cookie()->IsSecure())
        || FindSecureCookie(aKey, aCookie)) ) {

I don't know if extra indenting for the || violates our style guides, but it seems clearer to me that way. Or you could create a boolean adding a couple lines to make the If clause simpler:

bool foundSecureExact = (foundCookie) ? exactIter.Cookie()->IsSecure()
                                      : false;

(or more lines:
  bool foundSecureExact = false;
  if (foundCookie) {
    foundSecureExact = exactIter.Cookie()->IsSecure();
  }
)

Either way, then you use "foundSecureExact" instead of "foundCookie" in the if () as you wrote it. They all mean the same thing so it's just a matter of style and perceived clarity (in order of my preference).

Comment 5

[Amy Chung [:Amy]]

Attachment: Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Hi Daniel,
I have modified the patch from your suggestions,
1. Modified BLOCKED_DOWNGRADE_SECURE to BLOCKED_DOWNGRADE_SECURE_INEXCAT
2. Modified DOWNGRADE_SECURE_FROM_SECURE to DOWNGRADE_SECURE_FROM_SECURE_INEXACT.
3. Added BLOCKED_DOWNGRADE_SECURE_EXACT
4. DOWNGRADE_SECURE_FROM_SECURE_EXACT
5. Created the foundSecureExact to record foundCookie & the exciting cookie set to secure.
6. Modified the description on toolkit/components/telemetry/Histograms.json.

Please help me to review my patch, thanks!

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8826147 [details] [diff] [review]
Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Review of attachment 8826147 [details] [diff] [review]:
-----------------------------------------------------------------

Need to carry through on the implications of foundSecureExact, r- but close.

::: netwerk/cookie/PCookieService.ipdl
@@ +99,5 @@
>                                              nsCString cookieString,
>                                              nsCString serverTime,
>                                              bool fromHttp,
>                                              NeckoOriginAttributes attrs);
> +  

Please remove changes to this file from this patch. And in any case we don't want to be adding whitespace to the end of lines.

::: netwerk/cookie/nsCookieService.cpp
@@ +3477,5 @@
>    bool foundCookie = false;
> +  foundCookie = FindCookie(aKey, aCookie->Host(),
> +                           aCookie->Name(), aCookie->Path(), exactIter);
> +  bool foundSecureExact = (foundCookie) ? exactIter.Cookie()->IsSecure()
> +                                        : false;

This could be even simpler now that I'm looking at it and not just spouting off the top of my head:

bool foundSecureExact = foundCookie && exactIter.Cookie()->IsSecure();

@@ +3491,5 @@
>      // then ignore the new cookie.
>      // (draft-ietf-httpbis-cookie-alone section 3.2)
> +    if (!aCookie->IsSecure()
> +         && (foundSecureExact
> +             || FindSecureCookie(aKey, aCookie))) {

Code style: Looks like this line would still be short enough if you put "FindSecureCookie" on the same line as "foundSecureExact". It's clearer that way so the only reason to split the line is if it's "too long". People argue about what is "too long" but in this case it's < 80 chars so short enough by any definition. (I split them in my suggestion because the review comment box is only 70-ish characters on my screen and I worried bugzilla would wrap them in a way that made no sense.)

@@ +3496,4 @@
>        if (!isSecure) {
>          COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
> +          "cookie can't save because older cookie is secure cookie but newer cookie is non-secure cookie");
> +        if (foundCookie) {

We need to use "foundSecureExact" here (and a few lines later) to measure the correct thing, for the same reason we had to use it above. Otherwise we're counting as "exact" some cases where the exact match is IN-secure but we're blocking because of a FindSecureCookie() inexact match.

@@ +3506,5 @@
>          return;
>        } else {
> +        if (foundCookie) {
> +          // Record a secure site is allowed to downgrade a secure cookie by
> +          // exacting situation.

The old comment was fine here, put above "if (foundSecureExact)". We don't need to comment each of the telemetry accumulations separately for this case. (If you think the code needs this much clarification then why doesn't the !isSecure case need comments?) IMO the only potentially surprising thing here is that we're measuring a case where we take no action. It's reasonable to comment to that effect so people don't later worry we forgot to do something. Saying it twice doesn't make it any clearer, and the "if (foundSecureExact)" plus the telemetry names explain why we have two well enough.

::: toolkit/components/telemetry/Histograms.json
@@ +8185,5 @@
>      "expires_in_version": "57",
>      "kind": "enumerated",
>      "n_values": 10,
>      "releaseChannelCollection": "opt-out",
> +    "description": "Measuring the effects of draft-ietf-httpbis-cookie-alone blocking. 0=blocked http setting secure cookie; 1=blocked http downgrading the inexac secure cookie; 2=https downgraded the inexact secure cookie; 3=evicting newer insecure cookie; 4=evicting the oldest insecure cookie; 5=evicting the preferred cookie; 6=evicting the secure blocked; 7=blocked http downgrading the exact secure cookie; 8=https downgraded the exact secure cookie"

typo "inexac" -> "inexact"

Wow, this is getting long, can we shorten it a little?

"Strict Secure Cookies: 0=blocked secure cookie from http; 1=blocked shadowing secure cookie from http; 2=shadowing secure cookie from https; 3=evicted newer insecure cookie; 4=evicted oldest insecure cookie; 5=evicted secure cookie; 6=evicting secure cookie blocked; 7=blocked downgrading secure cookie from http; 8=downgraded secure cookie from https"

I used "shadow" for inexact matches and reserved "downgrade" for exact matches. Only half a line longer than the current text despite adding two new buckets. Although that saved 15% it's still long (by necessity) so it doesn't really matter if you prefer sticking with your version.

Comment 7

[Amy Chung [:Amy]]

Attachment: Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Hi Daniel,
I have modified the code from your suggestions as below:
1. Simplified bool foundSecureExact.
2. Moved the condition "FindSecureCookie(aKey, aCookie)" to above line.
3. Modified the if condition "if (foundCookie)" to if (foundSecureExact)".
4. Moved the comment above "if (foundSecureExact)" and modified the comment.
5. Modified the description.

Please help me to review my patch, thanks!

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 8827883 [details] [diff] [review]
Add a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE.

Review of attachment 8827883 [details] [diff] [review]:
-----------------------------------------------------------------

r=dveditz but I'd like you to make a couple of comment changes. If you don't have check-in permission yourself you'll need to upload the modified version here so the "checkin-needed" folks can use the right thing. If someone else is checking in you'll need a new version anyway to fix up the check-in comment.

::: netwerk/cookie/nsCookieService.cpp
@@ +3495,5 @@
>          COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, aCookieHeader,
> +          "cookie can't save because older cookie is secure cookie but newer cookie is non-secure cookie");
> +        // On Telemetry::COOKIE_LEAVE_SECURE_ALONE, we would like to record
> +        // the blocked non-secure cookie which is exacting or similar to the
> +        // existing secure cookie.

You don't need this comment -- the LOGFAILURE() is good enough documentation that we're not saving and the comment completely overshadows the important difference in this block which is the return statement. There's no surprise we want to measure this, and the telemetry statements are self-documenting.

@@ +3507,5 @@
>          return;
>        } else {
> +        // On Telemetry::COOKIE_LEAVE_SECURE_ALONE, we would like to record
> +        // the downgraded secure cookie which is exacting or similar to the
> +        // non-secure cookie from a secure site.

This comment merely repeats *what* the telemetry statements are clearly doing. The original comment did a better job explaining *why*. Sorry if it was not clear that I wanted to keep that original one and not just move the most recent version.

   // A secure site is allowed to downgrade a secure cookie
   // but we want to measure anyway

Comment 9

[Amy Chung [:Amy]]

Attachment: Added a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE, r=dveditz

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba1f2560ff68d6ebb0b355b028915a655e588ec0&selectedJob=71153379

Comment 10

[Ryan VanderMeulen [:RyanVM]]

New Telemetry probes need to go through a data review before they can land.
https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval

Comment 11

[Amy Chung [:Amy]]

Comment on attachment 8829752 [details] [diff] [review]
Added a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE, r=dveditz

Hi Francois,
Would you help me to review the Histograms.json of my patch?

Thanks!

Comment 12

[François Marier [:francois]]

Comment on attachment 8829752 [details] [diff] [review]
Added a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE, r=dveditz

Review of attachment 8829752 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Just a missing bug number to add. datareview+

::: toolkit/components/telemetry/Histograms.json
@@ +8181,4 @@
>    },
>    "COOKIE_LEAVE_SECURE_ALONE": {
>      "alert_emails": ["[e-mail]"],
>      "bug_numbers": [976073],

Can you please add bug 1325909 to this array (this field can contain more than one bug number) so that we can easily track when the new buckets were added?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad41ef2d3f8
Added a new bucket and modify the bucket name on telemetry COOKIE_LEAVE_SECURE_ALONE, r=dveditz

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fad41ef2d3f8

Comment 15

[Shian-Yow Wu [:swu]]

Amy, did you forget to upload new patch to address reviewer's comment 12?

Comment 16

[Amy Chung [:Amy]]

Hi Shian-Yow,
Sorry, I forgot to update the reviewer to my patch.
Should I upload patch again?

Comment 17

[François Marier [:francois]]

(In reply to Amy Chung [:Amy] from comment #16)
> Should I upload patch again?

You could simply do a follow-up patch (with the same bug number) that only adds a second bug number.

I'll be happy to review that.

Comment 18

[Amy Chung [:Amy]]

Attachment: Added a 1325909 to bug_numbers on telemetry.

Hi Francois,
Really thanks for your help, would you review my patch again?

Comment 19

[Amy Chung [:Amy]]

Attachment: Added a 1325909 to bug_numbers on telemetry.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a8ef757cddc9b26c3b82a754cd4178c74c3e620&selectedJob=76039033

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/324fb2d465fc
Added a 1325909 to bug_numbers on telemetry, r=francois

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/324fb2d465fc