Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTPS tests break in WPT with certification validation error #6919

Closed
jdm opened this issue Aug 3, 2015 · 9 comments · Fixed by #15784
Closed

HTTPS tests break in WPT with certification validation error #6919

jdm opened this issue Aug 3, 2015 · 9 comments · Fixed by #15784
Labels
A-network A-security A-testing B-high-value Represents work that would have a big impact

Comments

@jdm
Copy link
Member

jdm commented Aug 3, 2015

I assume we need to set up Servo to load certificates from a different location than usual; @jgraham ?
@SimonSapin
Copy link
Member

./mach test-wpt output shows that a certificate is created. I suppose it should be added to the set of trusted roots. (Only for the purpose of running tests of course.)

@jgraham
Copy link
Contributor

jgraham commented Aug 3, 2015

Yes, that's what Firefox does (using certutil; see https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/harness/wptrunner/browsers/firefox.py#177 ). I don't know how to do the same for Servo however.

@SimonSapin
Copy link
Member

net::http_loader::load calls openssl::ssl::SslContext::set_CA_file. Maybe have a command-line option to use an alternative CA file? Or should this be more obscure, to avoid tempting users?

By the way, @Manishearth, where does resources/certs come from? It’s rather opaque.

@jdm jdm mentioned this issue Aug 3, 2015
Merged
@Manishearth
Copy link
Member

https://github.com/servo/servo/blob/master/etc/cert_generator.js run in Firefox's browser debugger shell (Ctrl-Shift-J if enabled)

@Manishearth
Copy link
Member

We can create a new resources dir for testing and pass it via --resources-path

@jdm
Copy link
Member Author

jdm commented Aug 4, 2015

That would also allow us to get rid of a bunch of the guessing at http://mxr.mozilla.org/servo/source/components/util/resource_files.rs#20...

@SimonSapin
Copy link
Member

@jdm do you mean making --resources-path mandatory?

@jdm
Copy link
Member Author

jdm commented Aug 4, 2015

Well, we could default to assuming a path like target/mode/servo I suppose?

bors-servo pushed a commit that referenced this issue Nov 13, 2015
Auto merge of #6935 - jdm:sslverify, r=Manishearth
0735cec 
Make SSL cert verification errors work again. Add a horrible, no-good…

…, very bad regression test.

Here are the list of awful things this test exploits:
- Servo can't load HTTPS content in WPT tests (#6919)
- Our web workers don't report error events to the parent worker object after the initial network load completes
- Our worker resource load don't have a same-origin check

The good news is that this test should start failing if any of those "features" change, so this should not silently break on us.

Other attempts to test this included:
- iframes (didn't work because of #6672 and #3939)
- XMLHttpRequest (I was hit by CORS, I think; maybe I could have made it work if I returned the right headers)

r? @Ms2ger

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6935)
<!-- Reviewable:end -->
@jdm
Copy link
Member Author

jdm commented Jan 11, 2016

https://bugzilla.mozilla.org/show_bug.cgi?id=1025066 was the similar implementation for Gecko.

@jdm jdm mentioned this issue Jan 11, 2016
Merged
@jdm jdm mentioned this issue Feb 20, 2016
Closed
@jdm jdm added the B-high-value Represents work that would have a big impact label May 16, 2016
@jdm jdm mentioned this issue Mar 1, 2017
Merged
@jdm jdm mentioned this issue Mar 21, 2017
Closed
bors-servo pushed a commit that referenced this issue Apr 6, 2017
Auto merge of #15784 - jdm:ssltests, r=nox,jgraham
cd1dbf7 
Make SSL tests work

These changes fix #6919.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15784)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Apr 6, 2017
Auto merge of #15784 - jdm:ssltests, r=nox,jgraham
f3fd823 
Make SSL tests work

These changes fix #6919.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15784)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-network A-security A-testing B-high-value Represents work that would have a big impact
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make SSL tests work jdm/servo
5 participants
@jdm @SimonSapin @jgraham @frewsxcv @Manishearth