Pass Client parameter to httpUpdate to use any BearSSL security method for a firmware update (with example) #4832

Jeroen88 · 2018-06-20T19:09:01Z

Added two new update() and one updateSpiffs() function that uses the ESP8266HTTPClient adapted by me in PR #4825. With these adaptations, it is possible to do an encrypted and authenticated server firmware or SPIFFS update, using any of the supported security options of the WiFiClientSecureBearSSL library. httpUpdateSecure.ino is a complete example.

To try this, you need either a sketch with enough free memory to allocate the full TLS/SSL-buffer (16k), or a, currently not very widely supported Maximum Fragment Length Negotiation enabled, server (or do the same trick as I do, using a nodeJS server and set maxSendFragmentLength) that sends the correct response headers and serves the binary. You need a certificate store in SPIFFs that includes your root CA, or a self signed certificate, or a certificate fingerprint. In the latter two cases you need to edit the example to use allowSelfSignedCerts() or setFingerprint(). It is also possible to use encryption without server authentication. In this case use setInsecure(). Of course any of the other supported WiFiClientSecureBearSSL functions may be used as well.

I have a question: in ESP8266httpUpdate.cpp with these new functions, a Client& is passed into the update() functions. After the headers have been checked in handleUpdate(), a WiFiClient * is retrieved from the http client :

WiFiClient * tcp = (WiFiClient *) http.getStreamPtr();

This works fine, however: in PR #4825 I designed httpClient as a generic client for any Client. There is no fundamental reason not to use e.g. EthernetClient. But in handleUpdate() a WiFiClient * is necessary to do the actual update. Is it possible to use a base class Client instead?

…ure/httpclient_client_parameter

…com/Jeroen88/Arduino into feature/httpclient_client_parameter

…eBearSSL example using MFLN

Jeroen88 · 2018-06-20T19:55:46Z

Travis build failed because of macro redefinitions not within my scope, e.g.:
In file included from common/Arduino.h:241:0,
from ../../cores/esp8266/StreamString.cpp:23:
../../cores/esp8266/WCharacter.h:24:0: warning: "isascii" redefined [enabled by default]
#define isascii(__c) ((unsigned)(__c)<=0177)
^

Jeroen88 · 2018-06-21T06:24:14Z

Travis build passed :)

…ure/httpupdate_client_parameter

…ture/httpupdate_client_parameter pull origin master

…com/Jeroen88/Arduino into feature/httpupdate_client_parameter update with remote master

earlephilhower

Very nice work! Some concerns about types and the new example, but nothing major. Shouldn't be an issue getting it into 2.5.0.

earlephilhower · 2018-07-24T14:22:16Z

libraries/ESP8266HTTPClient/examples/BasicHttpClient/BasicHttpClient.ino

    HTTPClient http;

    USE_SERIAL.print("[HTTP] begin...\n");
    // configure traged server and url
    //http.begin("https://192.168.1.12/test.html", "7a 9c f4 db 40 d3 62 5a 6e 21 bc 5c cc 66 c8 3e a1 45 59 38"); //HTTPS
-    http.begin("http://192.168.1.12/test.html"); //HTTP
+    if (http.begin((Client &)client, "http://tls.mbed.org/")) {  // HTTP


Any reason this one picked? Isn't tls.mbed.org https-only?

Also, thanks for checking return codes!

I will change this in http://jigsaw.w3.org/HTTP/connection.html

earlephilhower · 2018-07-24T14:27:51Z

libraries/ESP8266HTTPClient/examples/Authorization/Authorization.ino

    HTTPClient http;

    USE_SERIAL.print("[HTTP] begin...\n");
    // configure traged server and url


-    http.begin("http://user:[email protected]/test.html");
+    http.begin((Client&)client, "http://guest:[email protected]/HTTP/Basic/");


Isn't there no need for the cast (Client&) here and in the other examples? Your prototype will make the C++ compiler pass the pointer no matter what, no?

You will be right. I'll check it and then change it

The cast has been removed

earlephilhower · 2018-07-24T14:34:28Z

libraries/ESP8266HTTPClient/examples/StreamHttpClient/StreamHttpClient.ino

@@ -65,7 +67,7 @@ void loop() {
        uint8_t buff[128] = { 0 };

        // get tcp stream


Can we get rid of the stream temp variable completely since it's no longer needed with your fix? i.e. there is no getStreamPtr() needed anymore?

Actually, I see later on that you still have a "getStreamPtr()" method with the new one, too. Any reason not to just keep using it?

I do not quite understand you here.
The getStreamPtr() is embedded in the #ifdef KEEP_PRESENT_API, so it's just there for backward compatibility.
I added the streaming example on purpose, because a server response may be too large to fit into a String, making it impossible to use getString(), but still possible to stream it. Moreover, since it is String getString() instead of String &getString() the response might even be copied (allthough the compiler may optimize this, I don't know)
Could you explain what you are suggesting?

I understand you now. The getStreamPtr() is kept as part of the existing API, however in the examples the unneeded call to getStreamPtr() has been removed.

earlephilhower · 2018-07-24T14:35:33Z

libraries/ESP8266HTTPClient/examples/StreamHttpsClient/StreamHttpsClient.ino

+          uint8_t buff[128] = { 0 };
+
+          // get tcp stream
+          WiFiClient * stream = &client;


Same as above, no more "get tcp stream" with your change.

getStreamPtr() has been removed from the example

earlephilhower · 2018-07-24T14:39:19Z

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

+    }
+
+    _port = (protocol == "https" ? 443 : 80);
+    return beginInternal(url, protocol.c_str());


There shouldn't be any need for the protocol anymore, no? The WiFiCilent* passed in should already either be encrypted or not, and the code paths for both are the same, no?

Yes it is still needed. The ESP8266HTTPClient is setting up (and maintaining with Keep-Alive) the connection, and it should know what port to use. The idea (also in the original version) of this begin() method , is to pass all the needed information to set up the connection through a fully qualified URL.

earlephilhower · 2018-07-24T14:40:06Z

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

+    _host = host;
+    _port = port;
+    _uri = uri;
+    _protocol = (https ? "https" : "http");


Same question as above about the protocol, is it needed anymore?

The _port is needed, see in connect(): if(!_tcp->connect(_host.c_str(), _port)) { ...
If you mean that that _protocol can be dropped as class variable, I think you are right. I'll check that and wrap it into a #ifdef KEEP_PRESENT_API if possible

The _protocol may be dropped in the new class, but beginInternal() is still used, and this function uses _protocol as well. In this function _protocol could be changed into a variable local to that function. I did not do this to stay as close as possible to the original code.

earlephilhower · 2018-07-24T14:46:53Z

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.cpp

-    _tcp->setNoDelay(true);
+#ifdef KEEP_PRESENT_API
+    if(_tcpDeprecated)
+        _tcpDeprecated->setNoDelay(true);


Wouldn't you want to set it on _tcp as well (outside this #define, of course)?

Client::setNoDelay() does not exist. If I change Client in WiFiClient its is possible, see my remark later

Now setNoDelay() is alway called

earlephilhower · 2018-07-24T14:49:10Z

libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h

 #include <WiFiClient.h>
+#else
+#include <Client.h>


Why move to abstract Client from WiFiClient? I don't remember ever seeing anything else take this root class.

The idea was that this class could be used for other Clients as well, eg an EthernetClient as defined here (https://github.com/esp8266/Arduino/blob/master/libraries/Ethernet/src/EthernetClient.h). But because most people will use the WiFiClient, I will change Client to WiFiClient

earlephilhower · 2018-07-24T14:53:32Z

libraries/ESP8266httpUpdate/examples/httpUpdateSecure/httpUpdateSecure.ino

@@ -0,0 +1,138 @@
+/**


While I'm happy to see someone using CertStore, for ESP8266 updates that's probably not what we want people to do since it's saying you'd accept an update from anyone on the internet.

What most people would want to do is have a single, private certificate that they generate and keep secure on their own company's system. It would make this new example shorter and simpler, too, and focus it on the new way of doing the update vs. the overhead for making a CertStore...

I do not understand:
You will not accept an update from anyone on the internet because the URL determines where you fetch the update, and the Client identifies the host. Also, there is no need to populate the cert store with all root certificates, just your company's one is possible too.
Do you want an extra, simpler example just using a cert? Could you provide it?

earlephilhower · 2018-07-24T14:55:12Z

libraries/ESP8266httpUpdate/src/ESP8266httpUpdate.cpp

@@ -276,7 +304,7 @@ HTTPUpdateResult ESP8266HTTPUpdate::handleUpdate(HTTPClient& http, const String&
                ret = HTTP_UPDATE_FAILED;
            } else {

-                WiFiClient * tcp = http.getStreamPtr();
+                WiFiClient * tcp = (WiFiClient *) http.getStreamPtr();


I'm fine with casts like this, but I think @devyte will probably ask for static_cast<>. No change from me, just a warning...

I will try the static_cast

Because http.getStreamPtr() returns a WiFiClient now, casting is no longer necessary

Jeroen88 · 2018-07-25T13:19:35Z

@earlephilhower thnx for the review! I will make updates and give answers to your questions / remarks soon!

Jeroen88 · 2018-07-29T13:37:01Z

Requested review changes are made and posted as a new PR #4980.

Jeroen88 added 14 commits June 18, 2018 17:49

Make HTTPClient suitabe for every Client

8c3edfe

Removed deprecated marking of the current api

1bf21cd

Removed deprecated marking of the current api

7535a40

Updated examples to work with new API and added a https example

0a644f2

Merge branch 'master' into feature/httpclient_client_parameter

3cfa6e9

Corrected minor formatting issues

3ad2b07

Merge branch 'master' of https://github.com/esp8266/Arduino into feat…

57b3a9f

…ure/httpclient_client_parameter

Merge branch 'feature/httpclient_client_parameter' of https://github.…

8e23576

…com/Jeroen88/Arduino into feature/httpclient_client_parameter

Corrected minor formatting issues

a068dca

Corrected minor formatting issues

bf1188e

Corrected minor formatting issues

deb4a5b

Include support for getStreamPtr

b34228f

TLS/SSL httpUpdate passing Client& parameter and with WiFiClientSecur…

6088be9

…eBearSSL example using MFLN

Remove #include <Time.h>

86a2d12

Remove #include <Arduino.h>

1ed6f4a

Jeroen88 changed the title ~~Feature/httpupdate client parameter~~ Pass Client parameter to httpUpdate to use any BearSSL security method for a firmware update (with example) Jun 21, 2018

earlephilhower self-requested a review July 1, 2018 21:29

Jeroen88 added 2 commits July 2, 2018 17:53

Merge branch 'master' of https://github.com/esp8266/Arduino

6cc188e

Merge branch 'master' into feature/httpupdate_client_parameter

48ac7ca

earlephilhower added this to the 2.5.0 milestone Jul 6, 2018

earlephilhower and others added 7 commits July 6, 2018 10:00

Merge branch 'master' into feature/httpupdate_client_parameter

2c00e09

Merge branch 'master' of https://github.com/esp8266/Arduino

e136972

Merge branch 'master' of https://github.com/esp8266/Arduino into feat…

1101809

…ure/httpupdate_client_parameter

Merge branch 'master' of https://github.com/Jeroen88/Arduino into fea…

0fb35d1

…ture/httpupdate_client_parameter pull origin master

Merge branch 'feature/httpupdate_client_parameter' of https://github.…

f100573

…com/Jeroen88/Arduino into feature/httpupdate_client_parameter update with remote master

Merge branch 'master' into feature/httpupdate_client_parameter

e216ce8

Merge branch 'master' into feature/httpupdate_client_parameter

45eb148

earlephilhower requested changes Jul 24, 2018

View reviewed changes

Jeroen88 mentioned this pull request Jul 29, 2018

Feature/http update wi fi client parameter #4980

Merged

Jeroen88 closed this Jul 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass Client parameter to httpUpdate to use any BearSSL security method for a firmware update (with example) #4832

Pass Client parameter to httpUpdate to use any BearSSL security method for a firmware update (with example) #4832

Jeroen88 commented Jun 20, 2018 •

edited

Loading

Jeroen88 commented Jun 20, 2018

Jeroen88 commented Jun 21, 2018

earlephilhower left a comment

earlephilhower Jul 24, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

Jeroen88 Jul 29, 2018

earlephilhower Jul 24, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

Jeroen88 Jul 29, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 29, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

Jeroen88 Jul 29, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

Jeroen88 Jul 29, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

earlephilhower Jul 24, 2018

Jeroen88 Jul 27, 2018

Jeroen88 Jul 29, 2018

Jeroen88 commented Jul 25, 2018

Jeroen88 commented Jul 29, 2018

		@@ -65,7 +67,7 @@ void loop() {
		uint8_t buff[128] = { 0 };

		// get tcp stream

Pass Client parameter to httpUpdate to use any BearSSL security method for a firmware update (with example) #4832

Pass Client parameter to httpUpdate to use any BearSSL security method for a firmware update (with example) #4832

Conversation

Jeroen88 commented Jun 20, 2018 • edited Loading

Jeroen88 commented Jun 20, 2018

Jeroen88 commented Jun 21, 2018

earlephilhower 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

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

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

Choose a reason for hiding this comment

Jeroen88 commented Jul 25, 2018

Jeroen88 commented Jul 29, 2018

Jeroen88 commented Jun 20, 2018 •

edited

Loading