1009635 - PreloadedHPKP.json should contain test/production information about chromium imported data

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Camilo Viecco (:cviecco)]

Currently we import all domains from google in a naive way:
1. We include them all (even tough there is some shadowing and some explicit rejections).
2. We put them all in test mode
3. We have no way to move what is introduction/test without modifying js.

This tries to:
1. Move the source of chromium/chrome urls from the js file to the json file (facilitating testing).
2. Have explicit lists of domains that are in production or test mode so that we can actually go forward for those domains that have agreed on  FF putting them in producion.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: explicit-prod-lists

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8421820 [details] [diff] [review]
explicit-prod-lists

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

Hi Camilo,

Thanks for working on this, I was thinking we needed something similar. Re: Tor, I understand that they said they want to pin. Nevertheless, I still think the safest rollout is to put them in test mode first, then promote them to production.

The other thing is that the list of Google sites looks arbitrary to me. I don't understand what you mean by "shadow": having pinning enabled for google.com with subdomains isn't going to catch any of the cc TLDs that were removed. I am fine with it being smaller, I just want to understand your reasoning here.

Finally, if you decide to include google.com on the test list, please also include play.google.com with subdomain turned off. I figure they had it there for a reason.

Thanks,
Monica

::: security/manager/tools/PreloadedHPKPins.json
@@ +46,5 @@
> +                      "dev.twitter.com",
> +                      "business.twitter.com",
> +                      "platform.twitter.com",
> +                      "twimg.com",
> +                      // google has asked is not to pin

They never said this in public, so let's not put words in their mouth.

@@ +47,5 @@
> +                      "business.twitter.com",
> +                      "platform.twitter.com",
> +                      "twimg.com",
> +                      // google has asked is not to pin
> +                      // google.com shadows many entries in the list (great)

What's shadow?

::: security/manager/tools/genHPKPStaticPins.js
@@ +314,5 @@
>    entries.forEach(function(entry) {
>      if (entry.pins && chromeImportedPinsets[entry.pins]) {
> +      let testMode = false;
> +      let productionMode = false;
> +      if (-1 != testDomains.indexOf(entry.name)) {

Whenever you have a choice between using a linear-access structure (array) and an O(1) lookup (map), you should generally use the map, esp. in languages like JS.

I think it would be better to restructure these 2 arrays as a single map, e.g.

gStaticPins.chromium_data = {
  "torproject.org": { "test_mode": true },
  ...
}

That way it will be impossible for a domain to accidentally appears in both test_domains and production_domains, and this code becomes:

if (entry.name in gStaticPins.chromium_data) {
  // We are importing the pinset from Chrome
  chromeImportedEntries.push({
    name: entry.name,
    include_subdomains: entry.include_subdomains,
    test_mode: gStaticPins.chromium_data[entry.name].test_mode,
    pins: entry.pins });

and references to productionMode and testMode go away. Please ping me if this doesn't make sense.

Comment 3

[Camilo Viecco (:cviecco)]

Attachment: explicit-prod-lists (v2)

Comment 4

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8423252 [details] [diff] [review]
explicit-prod-lists (v2)

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

This looks good. I have a few formatting nits. Also, I have concerns similar to Monica - how did you choose what domains to include?

::: security/manager/tools/PreloadedHPKPins.json
@@ +25,5 @@
>  // Geotrust Primary -> www.mozilla.org
>  // Geotrust Global -> *. addons.mozilla.org
>  {
> +  "chromium_data" : {
> +     "cert_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.certs",

nit: indent one less space

@@ +28,5 @@
> +  "chromium_data" : {
> +     "cert_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.certs",
> +     "json_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.json",
> +     "imported_domains": {
> +                          "torproject.org": {"test_mode":true},

The format of this data should be documented. Also, by what method did you choose these domain names? I notice for example www.gmail.com isn't on here.
nit: space after/before "{"/"}" and after ":" (in each '"test_mode":')
additional nit: indent each entry here one more space

@@ +33,5 @@
> +                          "blog.torproject.org": {"test_mode":true},
> +                          "check.torproject.org": {"test_mode":true},
> +                          "dist.torproject.org": {"test_mode":true},
> +                          "www.torproject.org": {"test_mode":true},
> +                          // Twitter

If we're going to comment each block of entries, we should be consistent (i.e. add a "// Tor" comment to the top)

::: security/manager/tools/genHPKPStaticPins.js
@@ +317,3 @@
>          name: entry.name,
>          include_subdomains: entry.include_subdomains,
> +        test_mode: gStaticPins.chromium_data.imported_domains[entry.name].test_mode,

This is a bit cumbersome - maybe factor out gStaticPins.chromium_data.imported_domains?

@@ +320,1 @@
>          pins: entry.pins });

nit: indent this whole block (inside the .push).

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: explicit-prod-lists (v3)

Now with excluded pins and arrays.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8426512 [details] [diff] [review]
explicit-prod-lists (v3)

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

Looks good, modulo coverage loss.

::: security/manager/tools/PreloadedHPKPins.json
@@ +28,5 @@
> +  "chromium_data" : {
> +     "cert_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.certs",
> +     "json_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.json",
> +     "excluded_pinsets": [
> +                          "google"

Are you planning on switching this over to http://pki.google.com/roots.pem in the same change so we don't lose telemetry coverage?

@@ +30,5 @@
> +     "json_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.json",
> +     "excluded_pinsets": [
> +                          "google"
> +                         ],
> +     "production_domains": [

pinningtest.appspot.com

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: explicit-prod-lists (v3.1)

Entries suggested by mmc, code OK by mmc.

Comment 8

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8426533 [details] [diff] [review]
explicit-prod-lists (v3.1)

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

Looks good!

::: security/manager/tools/PreloadedHPKPins.json
@@ +28,5 @@
> +  "chromium_data" : {
> +     "cert_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.certs",
> +     "json_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.json",
> +     "excluded_pinsets": [
> +                         ],

nit: indent to the same level as the beginning of '"excluded_pinsets"'

@@ +30,5 @@
> +     "json_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.json",
> +     "excluded_pinsets": [
> +                         ],
> +     "production_domains": [
> +                           "pinningtest.appspot.com"

nit: indent two spaces from the beginning of '"production_domains"'

@@ +31,5 @@
> +     "excluded_pinsets": [
> +                         ],
> +     "production_domains": [
> +                           "pinningtest.appspot.com"
> +                           ]

nit: indent to the same level as the beinning of '"production_domains"'
(basically, match the formatting of sha256_hashes).

::: security/manager/tools/genHPKPStaticPins.js
@@ +312,5 @@
>    // ours, except theirs includes a HSTS mode.
>    let entries = chromePreloads.entries;
>    entries.forEach(function(entry) {
> +    let isExcludedPinset =
> +      (-1 != gStaticPins.chromium_data.excluded_pinsets.indexOf(entry.pins));

nit: switch the order of the operands of != (i.e. put "-1" on the right-hand side)

@@ +314,5 @@
>    entries.forEach(function(entry) {
> +    let isExcludedPinset =
> +      (-1 != gStaticPins.chromium_data.excluded_pinsets.indexOf(entry.pins));
> +    let isProductionDomain =
> +      (-1 != gStaticPins.chromium_data.production_domains.indexOf(entry.name));

nit: same
Also, "gStaticPins.chromium_data" is used twice here - you might consider factoring it out so these lines aren't so long.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/67e6b9e8935f