Closed Bug 1009635 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → cviecco
Depends on: 772756
Attached patch explicit-prod-lists (obsolete) — Splinter Review
Attachment #8421820 - Flags: feedback?(mmc)
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.
Attachment #8421820 - Flags: feedback?(mmc) → feedback+
Attached patch explicit-prod-lists (v2) (obsolete) — Splinter Review
Attachment #8421820 - Attachment is obsolete: true
Attachment #8423252 - Flags: review?(dkeeler)
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).
Attachment #8423252 - Flags: review?(dkeeler) → review-
Attached patch explicit-prod-lists (v3) (obsolete) — Splinter Review
Now with excluded pins and arrays.
Attachment #8426512 - Flags: review?(mmc)
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
Attachment #8426512 - Flags: review?(mmc) → review-
Entries suggested by mmc, code OK by mmc.
Attachment #8423252 - Attachment is obsolete: true
Attachment #8426512 - Attachment is obsolete: true
Attachment #8426533 - Flags: review?(dkeeler)
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.
Attachment #8426533 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/67e6b9e8935f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.