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.

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

Upgrade to SDK 2.0.0 #1435

Merged
merged 8 commits into from
Dec 11, 2016
Merged

Upgrade to SDK 2.0.0 #1435

merged 8 commits into from
Dec 11, 2016

Conversation

djphoenix
Copy link
Contributor

@djphoenix djphoenix commented Aug 3, 2016

Fixes #1409, #1415, #1624, #1604

@marcelstoer
Copy link
Member

I'm missing the update from axTLS to mbedTLS.

~/Data/NodeMCU/nodemcu-firmware (dev) > grep -r axTLS *
docs/en/modules/net.md:This specification is imposed by the [axTLS library](http://axtls.sourceforge.net/specifications.htm) used by the SDK. 
tools/makefile.sh:# * Neither the name of the axTLS project nor the names of its

Also, http://nodemcu.readthedocs.io/en/latest/en/modules/net/ lists all supported cipher suites for TLS 1.1 axTLS supports. I guess that needs to be updated as well?

What about IPv6? Espressif has confirmed on Twitter that it's included even though it's not in the release notes. We should mention that somewhere. WiFi module? net module?

@djphoenix
Copy link
Contributor Author

Now only update of SDK included. I work on some tests with mbedtls, so this PR with old ssl module (axTLS-based).

@djphoenix
Copy link
Contributor Author

Also cannot test IPv6 right now. Will work on it some later.

@marcelstoer
Copy link
Member

Thanks Yuri. However, I personally would like this to be a "complete" PR i.e. including mbedTLS. It'd be odd to declare that NodeMCU be based on Espressif SDK 2.0 but retain axTLS. It would certainly provoke questions from our users as they would expect to find mbedTLS.

@djphoenix
Copy link
Contributor Author

Agree, I will extend PR.
So will we use opensource version of mbedtls (like LWIP)? This may help us to optimize memory/flash footprint, and take more flexibility to end users (e.g. turn on/off debug messages).

@pjsg
Copy link
Member

pjsg commented Aug 9, 2016

The lwip code that we are including doesn't have any evidence of IPv6 changes. If the SDK code does support IPv6, then we probably don't want to be including our version of lwip. My suspicion is that we want to dump the espconn layer first, then do the ipv6 work. [Dumping espconn will require a lot of TLS work]

@djphoenix
Copy link
Contributor Author

I was reviewed mbedtls library that Espressif built... There are interesting points:

  1. All sources included, with espconn<=>mbedtls layer.
  2. Sources can be built into similar binary, and libmbedtls.a can be replaced with them.
  3. mbedtls config that Espressif provided is not optimal for embedding... So, "thanks, Esp".

Actually I prefer to use source-based version, maybe with git submodule and config overrides. This way works for my own firmware template (RTOS-based). Also we provide more flexibility to endusers.

@pjsg
Copy link
Member

pjsg commented Aug 10, 2016

I agree that building from source is the way to go -- it allows easier debugging for a start!

@djphoenix
Copy link
Contributor Author

djphoenix commented Aug 14, 2016

Was integrated mbedTLS... It works. Sometimes.

function test()
    local c = net.createConnection(net.TCP,1)
    c:on('connection',function(s)
        print('CONN')
        s:send(
        "GET / HTTP/1.0\n" ..
        "Host: google.com\n" ..
        "Connection: close\n" ..
        "\n"
        )
    end)
    c:on('dns',function(s,a)
        print('DNS',a)
    end)
    c:on('sent',function(s)
        print('SENT')
    end)
    c:on('reconnection',function(s)
        print('ERR')
    end)
    c:on('disconnection',function(s)
        print('CLSD')
    end)
    c:on('receive',function(s,p)
        print(p)
    end)
    c:connect(443,"google.com")
end
test()

This example works OK. But, for example, howsmyssl.com was not loaded - connection closed.
Need some testing...
// Also phoenix.dj loaded normally. Yay!

@djphoenix djphoenix force-pushed the sdk200 branch 3 times, most recently from c58ab76 to 883c55e Compare August 14, 2016 01:00
@devsaurus
Copy link
Member

I've increased irom0 size for travis build and retriggered the build.

@jmattsson
Copy link
Member

I really wish they'd release an RTOS2.0 SDK as well, because I'm not sure how to handle all these differences over on the dev-rtos branch >.< Maybe once they push the ESP32 SDK out the door... I can only hope.

It would seem there's room for some code reduction once mbedTLS is in, as it has SHA{256,384,512} support included. That should mean we could ditch that code from our crypto module and save a bit of flash.

@djphoenix
Copy link
Contributor Author

Just updated SDK2.0.0 to 16_08_10. Good news
Now it looks like mbedTLS layer works properly (over espconn). This code executed as expected:

sock = net.createConnection(net.TCP, 1)
sock:on('connection', function(s)
  print('conn')
  sock:send(
  'GET /a/check HTTP/1.1\n' .. 
  'Host: www.howsmyssl.com\n' ..
  'Connection: close\n' ..
  '\n'
  )
end)
sock:on('receive', function(s,d)
  print(d)
end)
sock:on('disconnection', function(s) print('CLSD') end)
sock:connect(443, '104.196.5.74')

Output (for default config of mbedTLS):

HTTP/1.1 200 OK
Content-Length: 2440
Access-Control-Allow-Origin: *
Connection: close
Content-Type: application/json
Date: Mon, 24 Oct 2016 12:56:12 GMT
Strict-Transport-Security: max-age=631138519; includeSubdomains; preload

{"given_cipher_suites":["TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384","TLS_DHE_RSA_WITH_AES_256_GCM_SHA384","TLS_DHE_RSA_WITH_AES_256_CCM","TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384","TLS_DHE_RSA_WITH_AES_256_CBC_SHA256","TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA","TLS_DHE_RSA_WITH_AES_256_CBC_SHA","TLS_DHE_RSA_WITH_AES_256_CCM_8","TLS_ECDHE_RSA_WITH_CAMELLIA_256_GCM_SHA384","TLS_DHE_RSA_WITH_CAMELLIA_256_GCM_SHA384","TLS_ECDHE_RSA_WITH_CAMELLIA_256_CBC_SHA384","TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA256","TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA","TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256","TLS_DHE_RSA_WITH_AES_128_GCM_SHA256","TLS_DHE_RSA_WITH_AES_128_CCM","TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256","TLS_DHE_RSA_WITH_AES_128_CBC_SHA256","TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA","TLS_DHE_RSA_WITH_AES_128_CBC_SHA","TLS_DHE_RSA_WITH_AES_128_CCM_8","TLS_ECDHE_RSA_WITH_CAMELLIA_128_GCM_SHA256","TLS_DHE_RSA_WITH_CAMELLIA_128_GCM_SHA256","TLS_ECDHE_RSA_WITH_CAMELLIA_128_CBC_SHA256","TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA256","TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA","TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA","TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA","TLS_RSA_WITH_AES_256_GCM_SHA384","TLS_RSA_WITH_AES_256_CCM","TLS_RSA_WITH_AES_256_CBC_SHA256","TLS_RSA_WITH_AES_256_CBC_SHA","TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384","TLS_ECDH_RSA_WITH_AES_256_CBC_SHA384","TLS_ECDH_RSA_WITH_AES_256_CBC_SHA","TLS_RSA_WITH_AES_256_CCM_8","TLS_RSA_WITH_CAMELLIA_256_GCM_SHA384","TLS_RSA_WITH_CAMELLIA_256_CBC_SHA256","TLS_RSA_WITH_CAMELLIA_256_CBC_SHA","TLS_ECDH_RSA_WITH_CAMELLIA_256_GCM_SHA384","TLS_ECDH_RSA_WITH_CAMELLIA_256_CBC_SHA384","TLS_RSA_WITH_AES_128_GCM_SHA256","TLS_RSA_WITH_AES_128_CCM","TLS_RSA_WITH_AES_128_CBC_SHA256","TLS_RSA_WITH_AES_128_CBC_SHA","TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256","TLS_ECDH_RSA_WITH_AES_128_CBC_SHA256","TLS_ECDH_RSA_WITH_AES_128_CBC_SHA","TLS_RSA_WITH_AES_128_CCM_8","TLS_RSA_WITH_CAMELLIA_128_GCM_SHA256","TLS_RSA_WITH_CAMELLIA_128_CBC_SHA256","TLS_RSA_WITH_CAMELLIA_128_CBC_SHA","TLS_ECDH_RSA_WITH_CAMELLIA_128_GCM_SHA256","TLS_ECDH_RSA_WITH_CAMELLIA_128_CBC_SHA256","TLS_RSA_WITH_3DES_EDE_CBC_SHA","TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA","TLS_EMPTY_RENEGOTIATION_INFO_SCSV"],"ephemeral_keys_supported":true,"session_ticket_supported":true,"tls_compression_supported":false,"unknown_cipher_suite_supported":false,"beast_vuln":false,"able_to_detect_n_minus_one_splitting":false,"insecure_cipher_suites":{},"tls_version":"TLS 1.2","rating":"Probably Okay"}

(json at http://pastebin.com/QDBx974K)

Also loaded OK: google.com, yandex.ru, phoenix.dj, github.com

@djphoenix
Copy link
Contributor Author

So MQTT over SSL works too:

m = mqtt.Client('nodemcu-test', 5, nil, nil, 1)
m:on('message', function(c,t,m)
  print('msg', t, m)
end)
m:connect('iot.eclipse.org', 8883, 1, 0, function(c, r)
  print('conn', r)
  c:subscribe('test/ndemcu', 1, function(c)
    print('sub')
  end)
  c:publish('test/ndemcu', 'it works', 1, 0, function(c)
    print('pub')
  end)
end)

@marcelstoer
Copy link
Member

The CI build for pull requests failed because it needs more memory.

@djphoenix
Copy link
Contributor Author

@devsaurus fixed it once before at this thread... Maybe needed to do it again. So irom size is not enough anyway, in local builds I always set it to at least 0xD0000

@marcelstoer
Copy link
Member

After I merged the iram/irom fix I restarted the CI build for this PR -> ok now.

@marcelstoer marcelstoer added this to the 2.0.0 milestone Dec 1, 2016
@djphoenix
Copy link
Contributor Author

Okay, I think we're ready. Or you have another things that should be done before merge?

@@ -7,7 +7,7 @@

### A Lua based firmware for ESP8266 WiFi SOC

NodeMCU is an [eLua](http://www.eluaproject.net/) based firmware for the [ESP8266 WiFi SOC from Espressif](http://espressif.com/en/products/esp8266/). The firmware is based on the [Espressif NON-OS SDK 1.5.4.1](http://bbs.espressif.com/viewtopic.php?f=46&t=2376) and uses a file system based on [spiffs](https://github.com/pellepl/spiffs). The code repository consists of 98.1% C-code that glues the thin Lua veneer to the SDK.
NodeMCU is an [eLua](http://www.eluaproject.net/) based firmware for the [ESP8266 WiFi SOC from Espressif](http://espressif.com/en/products/esp8266/). The firmware is based on the [Espressif NON-OS SDK 2.0.0](http://bbs.espressif.com/viewtopic.php?f=46&t=2451) and uses a file system based on [spiffs](https://github.com/pellepl/spiffs). The code repository consists of 98.1% C-code that glues the thin Lua veneer to the SDK.
Copy link
Member

Choose a reason for hiding this comment

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

Should be http://espressif.com/sites/default/files/sdks/esp8266_nonos_sdk_<version> instead of BBS.

(echo "$(SDK_FILE_SHA1) $@" | sha1sum -c -) || { rm -f "$@"; exit 1; }

$(TOP_DIR)/cache/esp_iot_sdk_v$(SDK_PATCH_VER).zip:
mkdir -p "$(dir $@)"
wget --tries=10 --timeout=15 --waitretry=30 --read-timeout=20 --retry-connrefused http://bbs.espressif.com/download/file.php?id=$(SDK_PATCH_ID) -O $@ || { rm -f "$@"; exit 1; }
wget --tries=10 --timeout=15 --waitretry=30 --read-timeout=20 --retry-connrefused http://espressif.com/sites/default/files/sdks/esp8266_nonos_sdk_v$(SDK_PATCH_VER).zip -O $@ || { rm -f "$@"; exit 1; }
Copy link
Member

Choose a reason for hiding this comment

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

You missed the BBS URL in flash.md.

@djphoenix
Copy link
Contributor Author

Fixed links

@djphoenix
Copy link
Contributor Author

Any notes there?
/cc @marcelstoer @devsaurus @jmattsson

@marcelstoer
Copy link
Member

I haven't had time yet to thoroughly test it yet but I'd like to see this merged sooner rather than later. I won't merge myself though unless I actually tested.

@devsaurus
Copy link
Member

Did some smoke tests, looks good 👍

@pjsg
Copy link
Member

pjsg commented Dec 10, 2016

Can we just merge this so that all the testing that we do is based on 2.0.0? Or is there something that we are waiting for?

@djphoenix
Copy link
Contributor Author

+1 for merge it now from me. This is a key and huge PR that important to test as much as possible before get it to master.
Was played with this for some time, no issues was found.

@marcelstoer
Copy link
Member

marcelstoer commented Dec 10, 2016

https://github.com/nodemcu/nodemcu-firmware/edit/dev/docs/en/modules/net.md lines 8:15 need to be adjusted. And so does https://github.com/nodemcu/nodemcu-firmware/edit/dev/tools/makefile.sh 4:6.

@djphoenix
Copy link
Contributor Author

@marcelstoer Updated net.md. So, about makefile.sh - I not familiar with this one (was never used tools folder at all), how and what I should change?

@marcelstoer
Copy link
Member

That stuff was added by @pjsg in 501bd1f. Lines 4-6 contain some axTLS disclaimer comment which is no longer relevant if axTLS isn't part of the firmware anymore.

@pjsg
Copy link
Member

pjsg commented Dec 11, 2016

I haven't looked at the 2.0.0 documentation -- I hope that they changed the way that certificates worked. It was truly horrible. You can probably comment it out for now. I'll take a look....

@djphoenix
Copy link
Contributor Author

As I can understand, this is enough to just remove it notice. So, this part done.
Any other picks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants