Hyper sync rustls 0.6.1 #17938

SimranGujral · 2017-08-01T22:31:41Z

This change removes Hyper-OpenSSL and integrates HyperSyncRustls v 0.6.1 with servo.
It also removes ciphers depending on DHE and SHA1

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-08-01T22:31:47Z

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

highfive · 2017-08-01T22:31:48Z

Heads up! This PR modifies the following files:

@edunham: servo-tidy.toml
@KiChjang: components/net/Cargo.toml, components/net/lib.rs, components/net/http_loader.rs, components/net/connector.rs

highfive · 2017-08-01T22:31:49Z

Warning

These commits modify net code, but no tests are modified. Please consider adding a test!

SimranGujral · 2017-08-01T22:32:33Z

r? @avadacatavra

avadacatavra · 2017-08-02T00:05:08Z

servo-tidy.toml

@@ -36,6 +36,8 @@ packages = [
  "error-chain",
  "bitflags",
  "libloading", # Conflicting version is only used at build-time by geckolib.
+  "base64", # Conflicting version is used for ring
+  "rayon", # Conflicting verswon is used for rustls


nit: "verswon"

avadacatavra · 2017-08-02T00:05:08Z

components/net/connector.rs

+pub fn create_ssl_client(ca_file: &PathBuf) -> TlsClient {
+    let mut ca = {
+        let f = fs::File::open(ca_file).unwrap();
+        let rd = io::BufReader::new(f);


you should be able to change this to just
io::BufReader::new(f)
and omit the semicolon on this line, the rd, and the end of the nested scope

Additionally, do we want to unwrap here? The old code uses expect() instead of unwrap().

avadacatavra · 2017-08-02T00:05:08Z

components/net/connector.rs

+    let mut tls = rustls::ClientConfig::new();
+    tls.root_store.add_pem_file(&mut ca).unwrap();
+    let tls = Arc::new(tls);
+    let client = TlsClient { cfg: tls };


TlsClient { cfg: Arc::new(tls) }
Rust automatically returns the last expression of a function: https://rustbyexample.com/fn.html

avadacatavra · 2017-08-02T00:06:40Z

This also needs to be squashed into one commit -- we can go over that tomorrow

http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

SimranGujral · 2017-08-02T23:11:45Z

All changes have been made and are ready for review.

avadacatavra

The squash is good--I would clean up the commit comments to be more succinct. Currently they restate the same thing multiple times

avadacatavra · 2017-08-03T15:25:39Z

components/net/connector.rs

@@ -1,32 +1,31 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-


nit: put this line back in

avadacatavra · 2017-08-03T15:25:39Z

components/net/connector.rs

-    let ssl_connector = ssl_connector_builder.build();
-    OpensslClient::from(ssl_connector)
+pub fn create_ssl_client(ca_file: &PathBuf) -> TlsClient {
+    let mut ca = {


let mut ca = { let f = fs::File::open(ca_file).expect("cannot open CA file"); io::BufReader::new(f) }

avadacatavra · 2017-08-03T15:25:39Z

components/net/lib.rs

@@ -5,15 +5,14 @@
 #![deny(unsafe_code)]
 #![feature(box_syntax)]
 #![feature(iterator_step_by)]
-


nit: put this line back in

SimranGujral · 2017-08-03T20:56:41Z

Changes have been made. Ready for review.

jdm · 2017-08-04T14:10:11Z

test fetch::test_fetch_with_hsts has been running for over 60 seconds
That's worrying! I bet that running any HTTPS WPT test will end up timing out.
@bors-servo: try

bors-servo · 2017-08-04T14:10:16Z

⌛ Trying commit ad07861 with merge 90aac87...


        Auto merge of #17938 - SimranGujral:HyperSyncRustls_0.6.1, r=<try>

Hyper sync rustls 0.6.1  This change removes Hyper-OpenSSL and integrates HyperSyncRustls v 0.6.1 with servo. It also removes ciphers depending on DHE and SHA1 ---  - [X] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17938)

bors-servo · 2017-08-04T14:16:59Z

💔 Test failed - android

jdm · 2017-08-04T15:21:29Z

	thread 'fetch::test_fetch_with_hsts' panicked at 'called `Option::unwrap()` on a `None` value', /checkout/src/libcore/option.rs:335:20
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
             at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at /checkout/src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at /checkout/src/libstd/sys_common/backtrace.rs:60
             at /checkout/src/libstd/panicking.rs:380
   3: std::panicking::default_hook
             at /checkout/src/libstd/panicking.rs:390
   4: std::panicking::rust_panic_with_hook
             at /checkout/src/libstd/panicking.rs:611
   5: std::panicking::begin_panic_new
             at /checkout/src/libstd/panicking.rs:553
   6: std::panicking::begin_panic_fmt
             at /checkout/src/libstd/panicking.rs:521
   7: rust_begin_unwind
             at /checkout/src/libstd/panicking.rs:497
   8: core::panicking::panic_fmt
             at /checkout/src/libcore/panicking.rs:71
   9: core::panicking::panic
             at /checkout/src/libcore/panicking.rs:51
  10: <core::option::Option<T>>::unwrap
             at /checkout/src/libcore/macros.rs:22
  11: net_tests::fetch::test_fetch_with_hsts
             at ./fetch.rs:557
  12: <F as test::FnBox<T>>::call_box
             at /checkout/src/libtest/lib.rs:1477
             at /checkout/src/libcore/ops/function.rs:143
             at /checkout/src/libtest/lib.rs:138
  13: __rust_maybe_catch_panic
             at /checkout/src/libpanic_unwind/lib.rs:98

WPT tests worked, so that's good. The android build appears to break while building ring.

avadacatavra · 2017-08-07T13:57:17Z

@jdm my suspicion is that we'll need to specify android to ring in the build command based on https://github.com/briansmith/ring/blob/master/build.rs#L613

jdm · 2017-08-07T15:06:42Z

The target is provided automatically since we're invoking Cargo with --target, and the C commands have android-specific include directories appended to them:

running "arm-linux-androideabi-gcc" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "--sysroot" "/home/servo/android/ndk/current/platforms/android-18/arch-arm" "-I/home/servo/android/ndk/current/sources/android/support/include" "-g" "-march=armv7-a" "-march=armv7-a" "-mfpu=vfpv3-d16" "-mfloat-abi=softfp" "-I" "include" "-std=c1x" "-Wbad-function-cast" "-Wmissing-prototypes" "-Wnested-externs" "-Wstrict-prototypes" "-fdata-sections" "-ffunction-sections" "-pedantic" "-pedantic-errors" "-Wall" "-Wextra" "-Wcast-align" "-Wcast-qual" "-Wenum-compare" "-Wfloat-equal" "-Wformat=2" "-Winline" "-Winvalid-pch" "-Wmissing-declarations" "-Wmissing-field-initializers" "-Wmissing-include-dirs" "-Wredundant-decls" "-Wshadow" "-Wsign-compare" "-Wundef" "-Wuninitialized" "-Wwrite-strings" "-fno-strict-aliasing" "-fvisibility=hidden" "-Wno-cast-align" "-fstack-protector" "-g3" "-D_XOPEN_SOURCE=700" "-D__ANDROID_API__=18" "-c" "-o/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/ring-a621dfbd7d7ae5c7/out/ecp_nistz256.o" "crypto/ec/ecp_nistz256.c"

jdm · 2017-08-07T15:09:18Z

https://github.com/briansmith/ring/blob/5f8d332ed0f38415ee003712d1bb93647ed34afe/build.rs#L560 is the line that is adding the include directory that contains the standard library headers that clang does not like:

In file included from include/GFp/base.h:60:0,
                 from include/GFp/bn.h:126,
                 from crypto/bn/montgomery_inv.c:15:
/home/servo/android/ndk/current/sources/android/support/include/stdint.h:31:2: error: #include_next is a GCC extension
 #include_next <stdint.h>
  ^
thread '<unnamed>' panicked at 'execution failed', /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/ring-0.11.0/build.rs:658:8


        Servo using Hyper-Rustls for verification

Removed OpenSSL imports Using Hyper-Sync-Rustls instead of Rust-OpenSSL Servo integrated with HyperSyncRustls- v 0.6.1 Making Formatting changes after review

SimranGujral · 2017-08-07T22:01:16Z

The android issue is still in progress, Other issues have been resolved. Ready for review.
r? @avadacatavra

jdm · 2017-08-08T12:47:11Z

test fetch::test_fetch_with_hsts has been running for over 60 seconds. TravisCI still does not like running that test now.

Manishearth · 2017-08-10T21:48:16Z

Apparently if we pass --disable-werror as a cflag when compiling this should go away.

But I'm not sure, since the warning doesn't actually mention Werror.

jdm · 2017-08-10T22:01:58Z

@Manishearth What should go away?

Manishearth · 2017-08-10T22:02:48Z

Oh. The GCC extension warning.

bors-servo · 2017-08-18T00:11:20Z

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

avadacatavra · 2017-11-02T10:00:37Z

I'm going to close this until I have more time to bring it up to date :)

highfive assigned pcwalton Aug 1, 2017

highfive added the S-awaiting-review label Aug 1, 2017

highfive assigned avadacatavra and unassigned pcwalton Aug 1, 2017

avadacatavra requested changes Aug 2, 2017

View changes

avadacatavra added the S-needs-squash label Aug 2, 2017

SimranGujral force-pushed the SimranGujral:HyperSyncRustls_0.6.1 branch from f8f4a2d to e28cf76 Aug 2, 2017

avadacatavra requested changes Aug 3, 2017

View changes

SimranGujral force-pushed the SimranGujral:HyperSyncRustls_0.6.1 branch from e28cf76 to ad07861 Aug 3, 2017

avadacatavra added S-needs-code-changes and removed S-awaiting-review S-needs-squash labels Aug 3, 2017

highfive added the S-tests-failed label Aug 4, 2017

Servo using Hyper-Rustls for verification

Loading status checks…

31133c0

Removed OpenSSL imports Using Hyper-Sync-Rustls instead of Rust-OpenSSL Servo integrated with HyperSyncRustls- v 0.6.1 Making Formatting changes after review

SimranGujral force-pushed the SimranGujral:HyperSyncRustls_0.6.1 branch from ad07861 to 31133c0 Aug 7, 2017

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Aug 7, 2017

avadacatavra approved these changes Aug 7, 2017

View changes

highfive added the S-needs-rebase label Aug 18, 2017

avadacatavra closed this Nov 2, 2017

Darkspirit mentioned this pull request Nov 17, 2019

Add `./mach build --with-rustls` #24764

Closed

3 of 3 tasks complete

servo / servo

Hyper sync rustls 0.6.1 #17938

Hyper sync rustls 0.6.1 #17938

SimranGujral commented Aug 1, 2017 •

edited

highfive commented Aug 1, 2017

highfive commented Aug 1, 2017

highfive commented Aug 1, 2017

SimranGujral commented Aug 1, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

avadacatavra commented Aug 2, 2017

SimranGujral commented Aug 2, 2017

avadacatavra left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimranGujral commented Aug 3, 2017

jdm commented Aug 4, 2017

bors-servo commented Aug 4, 2017

bors-servo commented Aug 4, 2017

jdm commented Aug 4, 2017

avadacatavra commented Aug 7, 2017

jdm commented Aug 7, 2017

jdm commented Aug 7, 2017

SimranGujral commented Aug 7, 2017

jdm commented Aug 8, 2017

Manishearth commented Aug 10, 2017

jdm commented Aug 10, 2017

Manishearth commented Aug 10, 2017

bors-servo commented Aug 18, 2017

avadacatavra commented Nov 2, 2017

servo / servo

Join GitHub today

Hyper sync rustls 0.6.1 #17938

Hyper sync rustls 0.6.1 #17938

Conversation

SimranGujral commented Aug 1, 2017 • edited

highfive commented Aug 1, 2017

highfive commented Aug 1, 2017

highfive commented Aug 1, 2017

SimranGujral commented Aug 1, 2017

This comment has been minimized.

avadacatavra Aug 2, 2017

This comment has been minimized.

avadacatavra Aug 2, 2017

This comment has been minimized.

KiChjang Aug 2, 2017

This comment has been minimized.

avadacatavra Aug 2, 2017

avadacatavra commented Aug 2, 2017

SimranGujral commented Aug 2, 2017

avadacatavra left a comment

This comment has been minimized.

avadacatavra Aug 3, 2017

This comment has been minimized.

avadacatavra Aug 3, 2017

This comment has been minimized.

avadacatavra Aug 3, 2017

SimranGujral commented Aug 3, 2017

jdm commented Aug 4, 2017

bors-servo commented Aug 4, 2017

bors-servo commented Aug 4, 2017

jdm commented Aug 4, 2017

avadacatavra commented Aug 7, 2017

jdm commented Aug 7, 2017

jdm commented Aug 7, 2017

SimranGujral commented Aug 7, 2017

jdm commented Aug 8, 2017

Manishearth commented Aug 10, 2017

jdm commented Aug 10, 2017

Manishearth commented Aug 10, 2017

bors-servo commented Aug 18, 2017

avadacatavra commented Nov 2, 2017

SimranGujral commented Aug 1, 2017 •

edited