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

Use TLS module in HTTPS #1560

Merged
merged 1 commit into from
Apr 4, 2018
Merged

Use TLS module in HTTPS #1560

merged 1 commit into from
Apr 4, 2018

Conversation

LaszloLango
Copy link
Contributor

  • 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 LaszloLango force-pushed the tls-rework-and-https branch 2 times, most recently from d4634da to c0c8a9c Compare March 28, 2018 10:16
@zherczeg
Copy link
Member

Quite good patch

@LaszloLango LaszloLango force-pushed the tls-rework-and-https branch from c0c8a9c to 408d3ad Compare March 28, 2018 14:42
@LaszloLango LaszloLango changed the title [WIP] Use TLS module in HTTPS Use TLS module in HTTPS Mar 28, 2018
zherczeg
zherczeg requested changes Mar 29, 2018
View reviewed changes
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good shape. Only a few comments.

src/js/http.js
var ClientRequest = require('http_client').ClientRequest;
var HTTPParser = require('httpparser');
var HTTPServer = require('http_server');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/js/http_client.js Outdated
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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put this after the var declarations.

test/profiles/host-darwin.profile Outdated
@@ -1,2 +1,4 @@
ENABLE_MODULE_IOTJS_BASIC_MODULES
ENABLE_MODULE_IOTJS_CORE_MODULES
ENABLE_MODULE_HTTPS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra newline?

test/run_pass/test_net_http_server.js Outdated
@@ -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) {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/run_pass/test_net_https_server.js Outdated
key: fs.readFileSync('resources/my_key.pem').toString(),
cert: fs.readFileSync('resources/my_crt.pem').toString()
};
try { throw 1; } catch(e) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Just a few notes about this PR:

  1. 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/)
  2. 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
Copy link
Member

What about creating another key?

@LaszloLango
Copy link
Contributor Author

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

@daeyeon
Copy link
Member

daeyeon commented Apr 3, 2018

@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?

@LaszloLango LaszloLango force-pushed the tls-rework-and-https branch from dc437ad to 34d9322 Compare April 3, 2018 11:12
daeyeon
daeyeon reviewed Apr 3, 2018
View reviewed changes
docs/api/IoT.js-API-HTTPS.md Outdated
| 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 | △ ¹ | △ ¹ |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https

@LaszloLango
Copy link
Contributor Author

@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
Copy link
Member

daeyeon commented Apr 3, 2018

@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 LaszloLango force-pushed the tls-rework-and-https branch from 34d9322 to f2f822b Compare April 3, 2018 12:52
@LaszloLango
Copy link
Contributor Author

@daeyeon I've updated the PR.

zherczeg
zherczeg approved these changes Apr 3, 2018
View reviewed changes
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with one minor request.

src/modules.json Outdated
"https_native": {
"native_files": ["modules/iotjs_module_https.c"],
"init": "InitHttps"
"require": ["https_client", "httpparser", "http_server", "net", "tls"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The https_client is deleted. Why do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@LaszloLango
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 LaszloLango force-pushed the tls-rework-and-https branch from f2f822b to fb5f2c3 Compare April 3, 2018 13:49
daeyeon
daeyeon approved these changes Apr 3, 2018
View reviewed changes
Copy link
Member

@daeyeon daeyeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yichoi yichoi merged commit 4e140fb into jerryscript-project:master Apr 4, 2018
@ychuan ychuan mentioned this pull request Apr 8, 2018
Closed
DanielBallaSZTE pushed a commit to DanielBallaSZTE/iotjs that referenced this pull request Apr 17, 2018
@LaszloLango
Use TLS module in HTTPS (jerryscript-project#1560)
6e46254 
* 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 LaszloLango deleted the tls-rework-and-https branch April 20, 2018 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daeyeon daeyeon daeyeon approved these changes

@zherczeg zherczeg zherczeg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LaszloLango @zherczeg @daeyeon @yichoi