Implement HSTS (preload-only)

[samfoo]

Implement HSTS (preload-only) #6105

• Downloads the HSTS preload list from the chromium repo (same as gecko), then convert it to a list appropriate for servo.
• Reads the preload list when creating a resource task, and implements STS for those domains.

Still todo:

• Read Strict-Transport-Security headers from servers and add details to the in-memory HSTS list. (note: this requires hyper or servo to implement an STS header struct. Hyper seems like the appropriate location, so I will create an issue/PR there soon). The work for this is nearly done with the exception of adding a new ControlMsg and the new header.
• Persist HSTS list to disk with known hosts (perhaps a different issue should be raised for this?)

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5393

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[jdm]

Great work @samfoo! I appreciate the way you've split up the commits. Sorry for the delay in looking at this PR; I've skimmed all of the commits so far, and I'll do a thorough review soon.

[jdm]

One significant change I'll request is to switch from including the fetching of the preload list as part of bootstrapping and make it an explicit choice to update the copy that's part of the tree. We should add a mach command to fetch it, and remove the new entry from the gitignore list.

[samfoo]

@jdm Sure. Moving the preload from a bootstrap step to an explicit command shouldn't be too big of a change. I'll take a look at making it sometime this week.

[samfoo]

Also note that since hyperium/hyper#590 (implementing the Strict-Transport-Security header) is now merged, whenever the next version bump of hyper happens, the non-preload STS implementation can be finished.

[bors-servo]

☔ The latest upstream changes (presumably #6591) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

I've started reviewing this. Apologies for the continued delays!

[samfoo]

I've rebased to resolve the conflicts. Let me know if there's something else I can do to help with this.

[jdm]

-S-needs-rebase -S-needs-rebase +S-needs-code-changes
This is such a great PR! Thanks for doing this work :D

---

Reviewed 5 of 5 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, 2 of 2 files at r6, 5 of 5 files at r7, 2 of 2 files at r8, 2 of 2 files at r9, 2 of 2 files at r10, 2 of 2 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, 4 of 4 files at r15, 5 of 5 files at r16.
Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

---

^{components/net/resource_task.rs, line 159 [r1] (raw file):}

read_resource_file(...).ok().and_then(|bytes| {
    from_utf8(bytes).ok().and_then(|content| {
        HSTSList::new_from_preload(content)
    })
})

---

^{components/net/resource_task.rs, line 186 [r1] (raw file):}
I think we should move all of this HSTS-related code into a new hsts.rs file.

---

^{components/net/resource_task.rs, line 198 [r1] (raw file):}
decode(preload_content).ok()

---

^{components/net/resource_task.rs, line 201 [r5] (raw file):}
Let's make this an enum rather than a bool. We can still store a bool internally, though.

---

^{components/net/resource_task.rs, line 221 [r1] (raw file):}
I would just use an if instead.

---

^{components/net/resource_task.rs, line 223 [r1] (raw file):}
Can we not mutate the url in place?

---

^{components/net/resource_task.rs, line 261 [r2] (raw file):}
Why not just use a mutable iterator and update include_subdomains in place, instead of using a fold?

---

^{components/net/resource_task.rs, line 311 [r12] (raw file):}
@SimonSapin, is this necessary?

---

^{components/net/resource_task.rs, line 341 [r1] (raw file):}
if let
This needs to happen in http_loader.rs instead, though, or we won't upgrade any redirects that occur.

---

^{components/net/resource_task.rs, line 370 [r11] (raw file):}
if let

---

^{components/net/resource_task.rs, line 424 [r10] (raw file):}
if let

---

^{components/net/resource_task.rs, line 425 [r8] (raw file):}
I preferred the previous code which wasn't so clone-happy.

---

^{python/servo/bootstrap_commands.py, line 226 [r1] (raw file):}
grammar nit: ; instead of ,

---

^{python/servo/bootstrap_commands.py, line 230 [r1] (raw file):}
A comment explaining this would be useful.

---

^{tests/unit/net/resource_task.rs, line 6 [r1] (raw file):}
nit: so far we've preferred single line imports on multiple lines than {} split across multiple lines.

---

^{tests/unit/net/resource_task.rs, line 24 [r1] (raw file):}
As with the new hsts.rs in net, I think we should have a new test file for it too.

---

^{tests/unit/net/resource_task.rs, line 26 [r1] (raw file):}
assert!(HSTSList::new_from_preload(mock_preload_content).is_some(), "preload list should not have parsed")

---

^{tests/unit/net/resource_task.rs, line 29 [r6] (raw file):}
assert!(entry.is_none(), "able to create HSTSEntry with IPv6 host");

---

^{tests/unit/net/resource_task.rs, line 35 [r1] (raw file):}
assert .is_some()

---

^{tests/unit/net/resource_task.rs, line 41 [r6] (raw file):}
Same as above.

---

^{tests/unit/net/resource_task.rs, line 52 [r1] (raw file):}
[0] instead of .get(0).unwrap()?

---

^{tests/unit/net/resource_task.rs, line 68 [r2] (raw file):}
A test for adding multiple entries (or adding an entry to one with more than one existing entry) would have caught the earlier bug.

---

^{tests/unit/net/resource_task.rs, line 118 [r1] (raw file):}
assert_eq

---

^{tests/unit/net/resource_task.rs, line 129 [r1] (raw file):}
assert_eq

---

Comments from the review on Reviewable.io

[jdm]

Please leave extracting the HSTS code into new files until the very end in a separate commit; it will be easier for me to notice the changes related to my other comment that way :)

[samfoo]

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

---

^{components/net/resource_task.rs, line 215 [r2] (raw file):}
Rust newbie. Fixed.

---

^{components/net/resource_task.rs, line 223 [r1] (raw file):}
There's a "bug"/implementation detail in Url such that even though we update the scheme, it doesn't properly update the port internally. This meant that even when upgrading the URL to https, the connection was still attempted on port 80. Re-parsing the URL is a hack to get around this. I'll add a comment explaining it, and think about submitting a fix to rust-url

---

^{components/net/resource_task.rs, line 341 [r1] (raw file):}
Excellent point. This same problem exists for host file resolution as well, I believe.

---

Comments from the review on Reviewable.io

[jdm]

Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

^{components/net/resource_task.rs, line 449 [r1] (raw file):}
Yep, see #6416 :)

---

Comments from the review on Reviewable.io

[samfoo]

Still need to move the hsts list to the http_loader, but it's a slightly larger task than the rest.

[jdm]

Reviewed 3 of 3 files at r17, 2 of 2 files at r18, 6 of 6 files at r19, 4 of 4 files at r20.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{components/net/hsts.rs, line 18 [r19] (raw file):}
Let's use the regexes from resource_task.rs instead of duplicating them.

---

^{components/net/http_loader.rs, line 89 [r20] (raw file):}
We're going to want this to be an Arc<Mutex<HSTSList>>, or we'll be cloning a large vector for every single request.

---

^{components/net/resource_task.rs, line 26 [r19] (raw file):}
nit: these can be merged into one or multiple lines of {} that aren't longer than 100 chars.

---

^{components/net/resource_task.rs, line 263 [r20] (raw file):}
Why not always have an HSTSList, just make it empty?

---

^{components/net/resource_task.rs, line 363 [r18] (raw file):}
Let's use the enum in this message, too.

---

^{tests/unit/net/hsts.rs, line 137 [r19] (raw file):}
?

---

Comments from the review on Reviewable.io

[jdm]

This is very close! I'm excited!

[bors-servo]

☔ The latest upstream changes (presumably #6593) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit 118122d has been approved by jdm

[bors-servo]

⌛ Testing commit 118122d with merge ab3d6c4...

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3