1413512 - Get telemetry on how often we see HTTP(S) -> FTP redirects

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, enhancement, P2)

Description

[Jason Duell]

FTP is a large attack surface. As part of making it harder to exploit, we're considering deprecating HTTP -> FTP redirects. (I.e we'd still support FTP in the browser, but the user interaction would be needed to enter into using it).  But first we need to see how common these redirects are.

We're already planning to block FTP subresources (see bug 1404744), so we only need telemetry for top-level (document) loads (i.e. channel.IsDocument() == true).

I'm thinking we want a new telemetry probe that we fire each time we do a redirect for a document (top-level) channel, that notes the "type" of the redirect transition: http->ftp, http->https, https->ftp, http->data etc. 

Note that we can redirect only from http and https, but there are lots of schemes we can redirect to--I'm not actually sure which ones are prohibited.  So part of this bug is figuring out which types of redirect are possible so we know which buckets to have for telemetry.  (Top-level navigation to ws doesn't make sense, and we don't allow redirects to ws unless a pref is set so probably we don't need a category for it).  But I'd guess it would be useful to know about redirects to resource: data: etc. 

If it starts to be too much work to collect the full data here, even just a binary probe ("redirected to FTP" vs "redirected to something other than FTP") would be good enough to the specific question we want to know now.

Comment 1

[Amy Chung [:Amy]]

Attachment: Added a telemtry for counting the redirection from http(s) to ftp.

1) What questions will you answer with this data?
Count the redirection from http/https to ftp.

2) Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?
We're already planning to block FTP subresources (see bug 1404744), so we only need telemetry to count how often how often we see HTTP(S) -> FTP redirects.

3) What alternative methods did you consider to answer these questions? Why were they not sufficient?
We don’t have any telemetry to record the redirection.

4) Can current instrumentation answer these questions?
no

5) List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories on the found on the Mozilla wiki.
Only one probes HTTP_REDIRECT_TO_FTP can measure how often we see HTTP(S) -> FTP redirects.
In’t Category 2 “Interaction data”.
The counting number base on user loads which websites.

6) How long will this data be collected? Choose one of the following:
We can stop to record the data on HTTP_REDIRECT_TO_FTP when we finish bug 1404744.

7) What populations will you measure?
central mainly, good to know in nightly, beta

8) Please provide a general description of how you will analyze this data.
Count how many redirctation which is from http to ftp.

9) Where do you intend to share the results of your analysis?
Only in bugzilla in Networking: FTP components

Comment 2

[François Marier [:francois]]

Comment on attachment 8931024 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

I'm not clear on when the probe is sent. According to comment 0, this is meant to fire every time we do a redirection from HTTP/HTTPS.

Based on that, I would expect a boolean probe (true=redirect to ftp, false=redirect to something else).

::: toolkit/components/telemetry/Histograms.json
@@ +2136,5 @@
>      "description": "HTTP Proxy Type (none, http, socks)"
>    },
> +  "HTTP_REDIRECT_TO_FTP": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail]"],

Can you please add someone's email address here? Having a team email is fine, but we also want to attach each probe to a specific person too.

@@ +2142,5 @@
> +    "expires_in_version": "64",
> +    "kind": "linear",
> +    "high": 150,
> +    "n_buckets": 50,
> +    "description": "Count how many redirctation which is from http to ftp."

typo/grammar: "Count the number of redirections from HTTP to FTP."

Comment 3

[Amy Chung [:Amy]]

Attachment: Added a telemtry for counting the redirection from http(s) to ftp.

Hi SC,
I have added a telemetry for counting the number of redirections from HTTP to FTP.
1. e10s
   i.   Added telemetry on HttpChannelParentListener::AsyncOnChannelRedirect().
   ii.  If the scheme of new channel is ftp, send true to Telemetry::HTTP_REDIRECT_TO_FTP.
   iii. Otherwise, send false to Telemetry::HTTP_REDIRECT_TO_FTP.

2. non-e10s
   i.   Added telemetry on nsHttpHandler::AsyncOnChannelRedirect().
   ii.  If the scheme of new channel is ftp, send true to Telemetry::HTTP_REDIRECT_TO_FTP.
   iii. Otherwise, send false to Telemetry::HTTP_REDIRECT_TO_FTP.

Would you help me to review my patch?
Thanks!

Comment 4

[Amy Chung [:Amy]]

Comment on attachment 8931586 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

Hi Francois,
I have modified the telemetry from your suggestions.
1. Modified kind to boolean.
2. Modified the description.
3. Added my e-mail on alert_emails.

Would you help me to review my patch?

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8931586 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

I'll suggest to use keyed scalar for this telemetry since we want to know the full statistics of what scheme we redirect to. The "key" should be the scheme redirected to, with |Telemetry::ScalarAdd| for counting the frequency. Bug 1265472 is a good example of using keyed scalar.

```
nsAutoCString scheme;
newUri->GetScheme(scheme);
Telemetry::ScalarAdd(HTTP_REDIRECT_TO_SCHEME, NS_ConvertUTF8toUTF16(scheme), 1);
```

::: netwerk/protocol/http/HttpChannelParentListener.cpp
@@ +182,5 @@
>  
>    rv = registrar->RegisterChannel(newChannel, &mRedirectChannelId);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  bool isDocOnNewChan = false,

Is the code in nsHttpHandler::AsyncOnChannelRedirect not enough for e10s http channel?

Comment 6

[François Marier [:francois]]

Comment on attachment 8931586 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

::: toolkit/components/telemetry/Histograms.json
@@ +2140,5 @@
> +    "alert_emails": ["[e-mail]", "[e-mail]"],
> +    "bug_numbers": [1413512],
> +    "expires_in_version": "64",
> +    "kind": "boolean",
> +    "description": "Count the number of redirections from HTTP to FTP."

To make it clearer when the probe is fired, I would suggest something like:

"Each time we do a redirect for a document (top-level) channel, note whether or not we are redirecting from HTTP/HTTPS to FTP."

Comment 7

[Amy Chung [:Amy]]

Attachment: Added a telemtry for counting the redirection from http(s) to ftp.

Hi SC,
I have tried to use the telemetry setting like ADDON_FORBIDDEN_CPOW_USAGE, but the compiled error shows the new "count" histograms are not supported on Desktop, and suggested me to use scalars instead.
Base on above reason, I modified the patch as below:
1. Used scalar to record all of statistics on redirection.
2. Only set telemetry on nsHttpHandler::AsyncOnChannelRedirect().

Would you help me to review this patch?
Thanks!

Comment 8

[Amy Chung [:Amy]]

Comment on attachment 8934107 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

Hi Francois,
For this telemetry can record all of statistics on redirection, I modified the patch to use scalar.
Would you help me to review my patch?

Thanks!

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8934107 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +824,5 @@
> +        bool isDocOnNewChan = false,
> +             isDocOnOldChan = false;
> +        newChan->GetIsDocument(&isDocOnNewChan);
> +        oldChan->GetIsDocument(&isDocOnOldChan);
> +        if (isDocOnNewChan && isDocOnOldChan) {

check for the `oldChan` should be enough.

@@ +835,5 @@
> +            newURI->GetScheme(scheme);
> +            if (!isHTTPSorHTTP) {
> +                oldURI->SchemeIs("http", &isHTTPSorHTTP);
> +            }
> +            if (isHTTPSorHTTP) {

You don't need to do the `isHTTPSorHTTP` check since your code is already in `nsHttpHandler`.

::: toolkit/components/telemetry/Scalars.yaml
@@ +999,5 @@
>      record_in_processes:
>        - 'main'
>        - 'content'
>  
> +network.redirection:

s/network.redirection/networking

@@ +1006,5 @@
> +      - 1413512
> +    description: >
> +      Each time we do a redirect for a document (top-level)
> +      channel, note whether or not we are redirecting from
> +      HTTP/HTTPS to any scheme.

This description doesn't match with your code and you'll need to describe what the key and value means in your telemetry.

@@ +1014,5 @@
> +      - [e-mail]
> +      - [e-mail]
> +    release_channel_collection: opt-in
> +    record_in_processes:
> +      - 'main'

missing `keyed: true`?

Comment 10

[Amy Chung [:Amy]]

Attachment: Added a telemtry for counting the redirection from http(s) to ftp.

Hi SC,
I have modified the patch as below:
1. Only confirmed GetIsDocument() from old channe.
2. Removed the condition of isHTTPSorHTTP.
3. Modified "network_redirection" to "network"
4. Added keyed: true.
5. Modified description.

Would you help me to review my patch?
Thanks!

Comment 11

[Amy Chung [:Amy]]

Comment on attachment 8934494 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

Hi Francois,
For this telemetry can record all of statistics on redirection, I modified the patch to use scalar.
Would you help me to review my patch?

Thanks!

Comment 12

[François Marier [:francois]]

Comment on attachment 8934494 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

Looks good, but please also fill out a data review request according to our new process: https://wiki.mozilla.org/Firefox/Data_Collection

You can simply attach the request.md text file (https://github.com/mozilla/data-review/blob/master/request.md) and fill it out. Then attach it to this bug and you can r? me on it.

I will take care of Step 2.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8934494 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

Giving r- simply because I'd like to double check your next revision.
BTW, can you describe how you do the test and verification of your patch?

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +819,5 @@
>                                        nsIChannel* newChan,
>                                        uint32_t flags,
>                                        nsIEventTarget* mainThreadEventTarget)
>  {
> +    if (NS_IsMainThread()) {

use MOZ_ASSERT if this function should never be called on non-main thread.

@@ +823,5 @@
> +    if (NS_IsMainThread()) {
> +        bool isDocOnOldChan = false;
> +        oldChan->GetIsDocument(&isDocOnOldChan);
> +
> +        if (isDocOnOldChan) {

You can use |if (oldChan->IsDocument())| here, see https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/netwerk/base/nsIChannel.idl#361

::: toolkit/components/telemetry/Scalars.yaml
@@ +1005,5 @@
> +    bug_numbers:
> +      - 1413512
> +    description: >
> +      Each time we do a redirect for a document (top-level) channel,
> +      note which scheme is redirecting from HTTP/HTTPS.

You need to describe the meaning of count and key more clearly, see https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/toolkit/components/telemetry/Scalars.yaml#495 as a good example.

Comment 14

[Amy Chung [:Amy]]

Attachment: data collection

Hi Francois,
I have finished the data collection review form,
would you help me to review it?

Thanks!

Comment 15

[François Marier [:francois]]

Comment on attachment 8936408 [details]
data collection

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in scalars.yml

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default-on in pre-release channels, default-off on release.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, telemetry alerts.

Comment 16

[François Marier [:francois]]

Comment on attachment 8934494 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

::: toolkit/components/telemetry/Scalars.yaml
@@ +1005,5 @@
> +    bug_numbers:
> +      - 1413512
> +    description: >
> +      Each time we do a redirect for a document (top-level) channel,
> +      note which scheme is redirecting from HTTP/HTTPS.

To be more clear (and to make sure I understand this description correctly), I would suggest something like:

Each time we do a redirect for a document (top-level) channel on HTTP/HTTPS, note which scheme we are redirecting to.

Comment 17

[Jason Duell]

One new requirement for this bug: we would like to break down this information by whether the redirect happens for a document load (top level page load) or a subresource.  So let's have 2 telemetry probes, one that applies for "channel.isDocument() == true" and one for false.

Comment 18

[François Marier [:francois]]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #17)
> One new requirement for this bug: we would like to break down this
> information by whether the redirect happens for a document load (top level
> page load) or a subresource.  So let's have 2 telemetry probes, one that
> applies for "channel.isDocument() == true" and one for false.

Once that's done, please update the data request form (so it contains everything) and I will give it another r+.

Comment 19

[Amy Chung [:Amy]]

Attachment: Added a telemtry for counting the redirection from http(s) to ftp.

Hi SC,
I have modified this patch as below:
1. Added labels "topLevel" and "subresource" for this telemetry.
2. Added assertion for confirming main thread.
3. I have tested this telemetry by using about:telemetry, the attachment of pic shows the testing result. 

Would you help me to review this patch?
Thanks!

Comment 20

[Amy Chung [:Amy]]

Attachment: Testing result of NETWORK_HTTP_REDIRECT_TO_SCHEME

Comment 21

[Amy Chung [:Amy]]

Attachment: data collection

Hi Francois,
I have modified the data collection form by new design.
Would you help me to review this patch?

Thanks!

Comment 22

[François Marier [:francois]]

Comment on attachment 8936719 [details]
data collection

No changes to the answers from comment 15.

Comment 23

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8936715 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

Sorry to be a bit picky but I'd like to see a nice and clean patch for such small modification.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +831,5 @@
> +        Telemetry::AccumulateCategoricalKeyed(scheme, labelsIsTopLevel);
> +    } else {
> +        labelsIsTopLevel = LABELS_NETWORK_HTTP_REDIRECT_TO_SCHEME::subresource;
> +        Telemetry::AccumulateCategoricalKeyed(scheme, labelsIsTopLevel);
> +    }

This is logically correct, but badly written code IMO. Here are the way to improve the readability:

1) scheme is used before labelsIsTopLevel, so it should be declared and defined first.
2) new line between logic chunks
3) you can use `? :` operator to define the value of labelsIsTopLevel, and it is possible to not use a variable for that.
4) MOZ_ASSERT the return value of |GetURI| and |GetScheme|

::: netwerk/protocol/http/nsHttpHandler.h
@@ +20,5 @@
>  #include "nsIObserver.h"
>  #include "nsISpeculativeConnect.h"
> +#include "mozilla/Telemetry.h"
> +
> +using mozilla::Telemetry::LABELS_NETWORK_HTTP_REDIRECT_TO_SCHEME;

nit: using should only be used in cpp file.

::: toolkit/components/telemetry/Histograms.json
@@ +2425,5 @@
> +    "bug_numbers": [1413512],
> +    "expires_in_version": "64",
> +    "kind": "categorical",
> +    "keyed": true,
> +    "description": "Each time we do a redirect which happens for a document load (top level page load) or a subresource on HTTP/HTTPS, note which scheme we are redirecting to. Keyed by the URL scheme, for example: http, https, ftp...",

I would put the following sentence in the description:

"Count of the HTTP redirection that triggered by top-level document or by subresource, keyed by the URL scheme redirected to."

Comment 24

[Amy Chung [:Amy]]

Attachment: Added a telemtry for counting the redirection from http(s) to ftp.

Hi SC,
I have modified the patch from your suggestions as below:
1. Moved the labelsIsTopLebel after define scheme.
2. Used `? :` operator to define the value of labelsIsTopLevel.
3. Added MOZ_ASSERT on newURI and scheme.

Would you help me to review my patch?
Thanks!

Comment 25

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8936900 [details] [diff] [review]
Added a telemtry for counting the redirection from http(s) to ftp.

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

Thanks for the effort!

One last thing, you'll need to describe your modification in the commit message before check in. This is a common practice for gecko development nowadays. See discussion in https://groups.google.com/d/msg/mozilla.dev.platform/3ig8TZ-0rN8/-dXG587OBAAJ
Feel free to discuss with me if you don't know what should be put in the commit message.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +823,5 @@
>                                        nsIEventTarget* mainThreadEventTarget)
>  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    if (oldChan && newChan) {

`oldChan` and `newChan` should never be null. Use MOZ_ASSERT to do the check instead.

@@ +834,5 @@
> +        MOZ_ASSERT(!scheme.IsEmpty());
> +
> +        Telemetry::AccumulateCategoricalKeyed(scheme, oldChan->IsDocument() ?
> +                   LABELS_NETWORK_HTTP_REDIRECT_TO_SCHEME::topLevel :
> +                   LABELS_NETWORK_HTTP_REDIRECT_TO_SCHEME::subresource);

Not sure if this is the correct indentation. You can run |./mach clang-format| to do the auto indentation on your patch.

Comment 26

[Amy Chung [:Amy]]

Attachment: Added a telemtry for counting the redirection from http(s) to ftp. r=schien

Comment 27

[Amy Chung [:Amy]]

[Try server result]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cabda6ee39de6847030b72bede1a5b427f60ed40

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12981d063095
Added a telemtry for counting of the HTTP redirection that triggered by top-level document or by subresource. r=schien data=francois

Comment 29

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/12981d063095