Implement HSTS fetch step #14716

nmvk · 2016-12-24T17:55:55Z

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and
current URL's host is domain and if current URL's host matched with Known
HSTS Host Domain Name Matching results in either a superdomain match with
an asserted includeSubDomains directive or a congruent match then we
change request scheme to 'https'. This change has been made in method.rs

A test case to validate this has been added in fetch.rs. For asserting
https scheme, a https localhost was required. For this purpose I have
created a self-signed certificate and refactored fetch-context and
connector.rs to programmatically trust this certificate for running this
test case.

This should fix #14363

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14363

There are tests for these changes

This change is

highfive · 2016-12-24T17:56:00Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/fetch/methods.rs, components/net/resource_thread.rs, components/net/http_loader.rs, components/net/connector.rs

nmvk · 2016-12-25T09:16:28Z

Travis failure The command "./mach test-stylo" exited with 101. not due to the current change.

jdm · 2016-12-25T15:46:50Z

That's #14723 .

bors-servo · 2016-12-27T17:10:20Z

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

nmvk · 2016-12-28T05:34:15Z

I have addressed conflicts. @emilio did you get a chance to review the changes?

emilio · 2016-12-28T09:40:32Z

I think @jdm or @Ms2ger are more suited to review this than me.

There should probably be a WPT test or similar for this.

@bors-servo try

bors-servo · 2016-12-28T09:40:36Z

⌛ Trying commit e8099df with merge 1ccb92f...


        Auto merge of #14716 - mrnayak:hsts, r=<try>

Implement HSTS fetch step Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and current URL's host is domain and if current URL's host matched with Known HSTS Host Domain Name Matching results in either a superdomain match with an asserted includeSubDomains directive or a congruent match then we change request scheme to 'https'. This change has been made in method.rs A test case to validate this has been added in fetch.rs. For asserting https scheme, a https localhost was required. For this purpose I have created a self-signed certificate and refactored fetch-context and connector.rs to programmatically trust this certificate for running this test case. This should fix #14363  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14363  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14716)

emilio · 2016-12-28T09:40:39Z

@bors-servo r? @Ms2ger

bors-servo · 2016-12-28T11:41:54Z

💔 Test failed - linux-rel-wpt

nmvk · 2016-12-28T12:05:43Z

Is the test failure due to network issue?

emilio

Seems like the failures may be legit? We can retry to verify it.

Other option would be that that check makes stuff immensely slower, in which case we'd need a faster data-structure for HstsList.

emilio · 2016-12-28T12:38:13Z

components/net/fetch/methods.rs

@@ -189,7 +189,15 @@ pub fn main_fetch(request: Rc<Request>,
    }

    // Step 9
-    // TODO this step (HSTS)
+    if request.current_url().scheme() == "http" && request.current_url().domain() != None {


nit: use request.current_url().domain().is_some()?

emilio · 2016-12-28T12:38:26Z

@bors-servo retry

bors-servo · 2016-12-28T12:38:30Z

⌛ Trying commit e8099df with merge 35d62a5...


        Auto merge of #14716 - mrnayak:hsts, r=<try>

Implement HSTS fetch step Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and current URL's host is domain and if current URL's host matched with Known HSTS Host Domain Name Matching results in either a superdomain match with an asserted includeSubDomains directive or a congruent match then we change request scheme to 'https'. This change has been made in method.rs A test case to validate this has been added in fetch.rs. For asserting https scheme, a https localhost was required. For this purpose I have created a self-signed certificate and refactored fetch-context and connector.rs to programmatically trust this certificate for running this test case. This should fix #14363  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14363  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14716)

emilio · 2016-12-28T12:41:11Z

IMO the hsts list seems pretty big to use a vec as a backing store, but maybe other people have different opinions.

nmvk · 2016-12-28T13:16:35Z

Thanks, @emilio, Maybe I could refactor HstsList to use HashSet or HashMap which could provide faster lookup?

bors-servo · 2016-12-28T13:50:42Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

emilio · 2016-12-28T14:04:36Z

Tests seem green, so it's likely to have been another kind of error. I've opened #14756 to change the underlying storage of the HSTS list. I think it's worth to land that refactoring separately as a followup.

jdm

Great work!

jdm · 2016-12-28T20:00:57Z

resources/certificate.crt

@@ -0,0 +1,22 @@
+-----BEGIN CERTIFICATE-----


This file should be called self_signed_certificate_for_testing.crt to make its purpose clear ;)

jdm · 2016-12-28T20:00:57Z

resources/privatekey.key

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----


This should be called privatekey_for_testing.key.

jdm · 2016-12-28T20:00:57Z

tests/unit/net/fetch.rs

+        list.push(HstsEntry::new("localhost".to_owned(), IncludeSubdomains::NotIncluded, None)
+            .unwrap());
+    }
+    let do_fetch = |url: ServoUrl| {


I don't think having this closure makes the test any more readable, so let's remove it.

jdm · 2016-12-28T20:00:57Z

tests/unit/net/fetch.rs

+fn test_fetch_with_hsts() {
+    static MESSAGE: &'static [u8] = b"";
+    let handler = move |_: HyperRequest, response: HyperResponse| {
+        response.send(MESSAGE).unwrap();


Let's have the response contain the scheme of the request's URL instead. This will let us verify that the server never receives a non-HTTPS connection.

If I correctly understand, you want request.url.scheme() to be sent as part of response body?. Here we use hyper::server::request::Request which does not capture information about request scheme. If we make a non-HTTPS connection to the server, then we get response error saying "Connection reset"

Ah, good point. This test is fine as-is, in that case.


        Implement HSTS fetch step

Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and current URL's host is domain and if current URL's host matched with Known HSTS Host Domain Name Matching results in either a superdomain match with an asserted includeSubDomains directive or a congruent match then we change request scheme to 'https'. This change has been made in method.rs A test case to validate this has been added in fetch.rs. For asserting https scheme, a https localhost was required. For this purpose I have created a self-signed certificate and refactored fetch-context and connector.rs to programmatically trust this certificate for running this test case.

jdm · 2016-12-29T08:35:05Z

@bors-servo: r+
Thanks for fixing this and figuring out how to use HTTPS servers in unit tests!

bors-servo · 2016-12-29T08:35:06Z

📌 Commit 6020b4c has been approved by jdm

bors-servo · 2016-12-29T08:35:10Z

⌛ Testing commit 6020b4c with merge c7991d5...


        Auto merge of #14716 - mrnayak:hsts, r=jdm

Implement HSTS fetch step Implemented step nine of the main fetch. If current URL scheme is 'HTTP' and current URL's host is domain and if current URL's host matched with Known HSTS Host Domain Name Matching results in either a superdomain match with an asserted includeSubDomains directive or a congruent match then we change request scheme to 'https'. This change has been made in method.rs A test case to validate this has been added in fetch.rs. For asserting https scheme, a https localhost was required. For this purpose I have created a self-signed certificate and refactored fetch-context and connector.rs to programmatically trust this certificate for running this test case. This should fix #14363  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14363  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14716)

bors-servo · 2016-12-29T09:38:17Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned emilio Dec 24, 2016

highfive added the S-awaiting-review label Dec 24, 2016

nmvk force-pushed the nmvk:hsts branch 2 times, most recently from 8fb4c4d to 24a883c Dec 25, 2016

highfive added the S-needs-rebase label Dec 27, 2016

nmvk force-pushed the nmvk:hsts branch from 24a883c to e8099df Dec 28, 2016

highfive assigned Ms2ger and unassigned emilio Dec 28, 2016

highfive added the S-tests-failed label Dec 28, 2016

emilio reviewed Dec 28, 2016

View changes

emilio mentioned this pull request Dec 28, 2016

Use another backing store for the HSTS list. #14756

Closed

nmvk force-pushed the nmvk:hsts branch from e8099df to 8730f09 Dec 28, 2016

highfive removed the S-tests-failed label Dec 28, 2016

nmvk force-pushed the nmvk:hsts branch from 8730f09 to d5d9e89 Dec 28, 2016

jdm requested changes Dec 28, 2016

View changes

jdm assigned jdm and unassigned Ms2ger Dec 28, 2016

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Dec 28, 2016

nmvk force-pushed the nmvk:hsts branch from d5d9e89 to 6020b4c Dec 29, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 29, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 29, 2016

bors-servo merged commit 6020b4c into servo:master Dec 29, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 29, 2016

bors-servo mentioned this pull request Dec 29, 2016

Implemented nosniff for fetch algorithm #14620

Merged

4 of 4 tasks complete

servo / servo

Implement HSTS fetch step #14716

Implement HSTS fetch step #14716

nmvk commented Dec 24, 2016 •

edited

highfive commented Dec 24, 2016

nmvk commented Dec 25, 2016 •

edited

jdm commented Dec 25, 2016

bors-servo commented Dec 27, 2016

nmvk commented Dec 28, 2016

emilio commented Dec 28, 2016

bors-servo commented Dec 28, 2016

emilio commented Dec 28, 2016

bors-servo commented Dec 28, 2016

nmvk commented Dec 28, 2016

emilio left a comment

This comment has been minimized.

emilio commented Dec 28, 2016

bors-servo commented Dec 28, 2016

emilio commented Dec 28, 2016

nmvk commented Dec 28, 2016 •

edited

bors-servo commented Dec 28, 2016

emilio commented Dec 28, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Dec 29, 2016

bors-servo commented Dec 29, 2016

bors-servo commented Dec 29, 2016

bors-servo commented Dec 29, 2016

servo / servo

Join GitHub today

Implement HSTS fetch step #14716

Implement HSTS fetch step #14716

Conversation

nmvk commented Dec 24, 2016 • edited

highfive commented Dec 24, 2016

nmvk commented Dec 25, 2016 • edited

jdm commented Dec 25, 2016

bors-servo commented Dec 27, 2016

nmvk commented Dec 28, 2016

emilio commented Dec 28, 2016

bors-servo commented Dec 28, 2016

emilio commented Dec 28, 2016

bors-servo commented Dec 28, 2016

nmvk commented Dec 28, 2016

emilio left a comment

This comment has been minimized.

emilio Dec 28, 2016

emilio commented Dec 28, 2016

bors-servo commented Dec 28, 2016

emilio commented Dec 28, 2016

nmvk commented Dec 28, 2016 • edited

bors-servo commented Dec 28, 2016

emilio commented Dec 28, 2016

jdm left a comment

This comment has been minimized.

jdm Dec 28, 2016

This comment has been minimized.

jdm Dec 28, 2016

This comment has been minimized.

jdm Dec 28, 2016

This comment has been minimized.

jdm Dec 28, 2016

This comment has been minimized.

nmvk Dec 29, 2016

This comment has been minimized.

jdm Dec 29, 2016

jdm commented Dec 29, 2016

bors-servo commented Dec 29, 2016

bors-servo commented Dec 29, 2016

bors-servo commented Dec 29, 2016

nmvk commented Dec 24, 2016 •

edited

nmvk commented Dec 25, 2016 •

edited

nmvk commented Dec 28, 2016 •

edited