Use TLS module in HTTPS #1560

LaszloLango · 2018-03-28T08:12:15Z

Removed the curl dependency
Updated the mbedtls configuration
Removed the code duplications of HTTP and HTTPS modules
Updated the related test files

IoT.js-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com

zherczeg · 2018-03-28T14:12:48Z

Quite good patch

zherczeg

Very good shape. Only a few comments.

zherczeg · 2018-03-29T06:22:56Z

src/js/http.js

 var ClientRequest = require('http_client').ClientRequest;
 var HTTPParser = require('httpparser');
+var HTTPServer = require('http_server');


"http underscore server" and "httpparser" sounds inconsistent. Although this is legacy so you can keep it.

Yes, It bothers me too. :) I didn't want to change it in this PR, because it is quite unrelated.

zherczeg · 2018-03-29T06:25:04Z

src/js/http_client.js

  OutgoingMessage.call(this);

  // get port, host and method.
-  var port = options.port = options.port || 80;
-  var host = options.host = options.hostname || options.host || '127.0.0.1';
+  options.host = options.hostname || options.host || '127.0.0.1';


I would put this after the var declarations.

zherczeg · 2018-03-29T06:32:48Z

test/profiles/host-darwin.profile

@@ -1,2 +1,4 @@
 ENABLE_MODULE_IOTJS_BASIC_MODULES
 ENABLE_MODULE_IOTJS_CORE_MODULES
+ENABLE_MODULE_HTTPS
+


Extra newline?

zherczeg · 2018-03-29T06:34:13Z

test/run_pass/test_net_http_server.js

@@ -181,7 +183,7 @@ finalReq.end();
 var server2 = http.createServer();

 // Create server instance without new keyword.
-var server3 = Server(function(request, response) {});
+// var server3 = Server(function(request, response) {});


Why this is commented out?

I forgot to uncomment it, but it works. I'll fix it.

zherczeg · 2018-03-29T06:35:11Z

test/run_pass/test_net_https_server.js

+  key: fs.readFileSync('resources/my_key.pem').toString(),
+  cert: fs.readFileSync('resources/my_crt.pem').toString()
+};
+try { throw 1; } catch(e) {}


Indeed, it is a leftover. I'll will fix it.

LaszloLango · 2018-03-29T08:13:35Z

Just a few notes about this PR:

It enables HTTPS and TLS modules by default
a) so Travis can check the related tests
b) but it will increase the binary size (also on https://samsung.github.io/iotjs-test-results/)
This reveals an AddressSanitizer bug in the mbedtls itself.
a) We can disable HTTPS by default to make Travis happy, but it can make hard to catch bugs in this module. Improve precommit testing #1341 can be a good solution to be able to add new job for testing HTTPS and TLS.
b) We might try to turn off asan checks in mbedtls
c) Try to create a minimized testcase and report to the ARMmbed team
d) other idea?

zherczeg · 2018-03-30T03:26:08Z

What about creating another key?

LaszloLango · 2018-04-03T06:48:40Z

@zherczeg unfortunately it does not work. I tried with both openssl and mbedtls, but every generated key fails.

daeyeon · 2018-04-03T11:04:48Z

@LaszloLango @zherczeg It seems better to include HTTPS tests. We are fine with 2-(b), disabling asan in mbedtls, for now. However, 2-(c) is also needed. Once it's fixed, let's re-enable 2-(b). Could you open an issue for 2-(c) so that we can track it?

daeyeon · 2018-04-03T11:19:08Z

docs/api/IoT.js-API-HTTPS.md

-| https.get  | X | X | X | X | O |
+ |  | Linux<br/>(Ubuntu) | Raspbian<br/>(Raspberry Pi) | NuttX<br/>(STM32F4-Discovery) | TizenRT<br/>(Artik053) |
+ | :---: | :---: | :---: | :---: | :---: |
+ | http.createServer | O | O | △ ¹ | △ ¹ |


LaszloLango · 2018-04-03T11:21:05Z

@daeyeon I've reported the issue: Mbed-TLS/mbedtls#1550
I've updated the PR to ignore the address sanitizer flag in mbedtls.

Since #1341 landed we can test the HTTPS module without enabling it on default build. We can add it to test/profiles/*.profile and testsets-*.json (e.g: testsets-host-linux.json, etc.). So Travis could run the tests, but the default build (tools/build.py --clean) won't change. Which way do you prefer?

daeyeon · 2018-04-03T11:45:43Z

@LaszloLango It's right not to include HTTPS on the default build. Please add it to *.profile and testsets.json. Having each json per each purpose is a question mark for now.

LaszloLango · 2018-04-03T13:34:49Z

@daeyeon I've updated the PR.

zherczeg

LGTM, with one minor request.

zherczeg · 2018-04-03T13:42:42Z

src/modules.json

-    "https_native": {
-      "native_files": ["modules/iotjs_module_https.c"],
-      "init": "InitHttps"
+      "require": ["https_client", "httpparser", "http_server", "net", "tls"]


The https_client is deleted. Why do we need it?

Good catch, it is a typo. It should be "http_client" I'll fix it.

* Removed the curl dependency * Updated the mbedtls configuration * Removed the code duplications of HTTP and HTTPS modules * Updated the related test files IoT.js-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com

daeyeon

LGTM. A bit different from what we prefer though. We can fix it later.

* Removed the curl dependency * Updated the mbedtls configuration * Removed the code duplications of HTTP and HTTPS modules * Updated the related test files IoT.js-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com

LaszloLango force-pushed the tls-rework-and-https branch 2 times, most recently from d4634da to c0c8a9c Compare March 28, 2018 10:16

LaszloLango force-pushed the tls-rework-and-https branch from c0c8a9c to 408d3ad Compare March 28, 2018 14:42

LaszloLango changed the title ~~[WIP] Use TLS module in HTTPS~~ Use TLS module in HTTPS Mar 28, 2018

zherczeg requested changes Mar 29, 2018

View reviewed changes

LaszloLango force-pushed the tls-rework-and-https branch from 408d3ad to dc437ad Compare March 29, 2018 07:22

LaszloLango force-pushed the tls-rework-and-https branch from dc437ad to 34d9322 Compare April 3, 2018 11:12

daeyeon reviewed Apr 3, 2018

View reviewed changes

LaszloLango force-pushed the tls-rework-and-https branch from 34d9322 to f2f822b Compare April 3, 2018 12:52

zherczeg approved these changes Apr 3, 2018

View reviewed changes

Use TLS module in HTTPS

fb5f2c3

* Removed the curl dependency * Updated the mbedtls configuration * Removed the code duplications of HTTP and HTTPS modules * Updated the related test files IoT.js-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com

LaszloLango force-pushed the tls-rework-and-https branch from f2f822b to fb5f2c3 Compare April 3, 2018 13:49

daeyeon approved these changes Apr 3, 2018

View reviewed changes

yichoi merged commit 4e140fb into jerryscript-project:master Apr 4, 2018

ychuan mentioned this pull request Apr 8, 2018

Merge pull request #1 from Samsung/master #1572

Closed

LaszloLango deleted the tls-rework-and-https branch April 20, 2018 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use TLS module in HTTPS #1560

Use TLS module in HTTPS #1560

LaszloLango commented Mar 28, 2018

zherczeg commented Mar 28, 2018

zherczeg left a comment

zherczeg Mar 29, 2018

LaszloLango Mar 29, 2018

zherczeg Mar 29, 2018

zherczeg Mar 29, 2018

zherczeg Mar 29, 2018

LaszloLango Mar 29, 2018

zherczeg Mar 29, 2018

LaszloLango Mar 29, 2018

LaszloLango commented Mar 29, 2018

zherczeg commented Mar 30, 2018

LaszloLango commented Apr 3, 2018

daeyeon commented Apr 3, 2018

daeyeon Apr 3, 2018

LaszloLango commented Apr 3, 2018

daeyeon commented Apr 3, 2018

LaszloLango commented Apr 3, 2018

zherczeg left a comment

zherczeg Apr 3, 2018

LaszloLango Apr 3, 2018

daeyeon left a comment

Use TLS module in HTTPS #1560

Use TLS module in HTTPS #1560

Conversation

LaszloLango commented Mar 28, 2018

zherczeg commented Mar 28, 2018

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaszloLango commented Mar 29, 2018

zherczeg commented Mar 30, 2018

LaszloLango commented Apr 3, 2018

daeyeon commented Apr 3, 2018

Choose a reason for hiding this comment

LaszloLango commented Apr 3, 2018

daeyeon commented Apr 3, 2018

LaszloLango commented Apr 3, 2018

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daeyeon left a comment

Choose a reason for hiding this comment