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

OTA support encapsulated to ArduinoOTA class #883

Merged
merged 1 commit into from Oct 18, 2015

Conversation

mangelajo
Copy link
Contributor

This work encapsulates the basic OTA functionality to an ArduinoOTA class.
It also adds callbacks for OTA events so the application can react. SPIFFS
support to come in next patches.

@mangelajo
Copy link
Contributor Author

Thanks @hallard. It's exactly what I wanted to avoid too :)

@mangelajo
Copy link
Contributor Author

@s2kinfra, you already have one class for doing that HttpUpdate or something like that.

@s2kinfra
Copy link

@mangelajo yeah thanks, I noticed that after writing the suggestion.
Can just say your class is working perfectly with esp-12, is it also working on esp-01?

@grahamehorner
Copy link

nice work; 👍 !SHIP IT! 💃

@ikeyasu
Copy link

ikeyasu commented Oct 15, 2015

great work!

@grahamehorner
Copy link

Q: should the receiving of a udp packet on the 8266 port stop all other udp listeners? prior to validating of the message and/or client connectivity?

or am I reading this wrong?

void ArduinoOTA::handle() {

if (!_udp_ota->parsePacket())
return;

IPAddress remote = _udp_ota->remoteIP();
int cmd = _udp_ota->parseInt();
int port = _udp_ota->parseInt();
int size = _udp_ota->parseInt();

if (_serial_debug){
Serial.print("Update Start: ip:");
Serial.print(remote);
Serial.printf(", port:%d, size:%d\n", port, size);
}

WiFiUDP::stopAll();

...

@mangelajo
Copy link
Contributor Author

@grahamehorner I was just refactoring the example we have for OTA https://github.com/esp8266/Arduino/blob/esp8266/hardware/esp8266com/esp8266/libraries/ESP8266mDNS/examples/DNS_SD_Arduino_OTA/DNS_SD_Arduino_OTA.ino

But I guess it can make sense to put it after the TCP connection back to updater is done?

BTW, I'm not sure what's the real reason of it, I guess that if the UDP listeners stay open, and they keep receiving packets we end up in a deadlock because nobody takes them and we have no more buffers for the normal TCP firmware download.

@grahamehorner
Copy link

@mangelajo I think I'll do a fork of your work on this and some testing; may be add a mutex with in the handle function after the code has done the validation of the udp packet will prevent race conditions where a system send many udp packets on the network.

Q: I'm wondering if the esp8266 is in sleep/deep sleep mode would a udp packet wake the processor? WOL (wake up on LAN) style? so OTA updates would work on devices that are battery powered without consuming power polling/waking on a time based interval?

@mangelajo
Copy link
Contributor Author

@grahamehorner sounds good, may be we could get this merged, and improve iteratively. As soon as the API stays stable I guess it should be good :)

@grahamehorner
Copy link

@mangelajo I think we should ask the forum about the UDP packet that triggers the OTA; how do they see it working; I guess it should have some type of security so the devices know it has come for an authorised source? don't what a packet triggering OTA from a hacker ;) IoT security :O

@mangelajo
Copy link
Contributor Author

@grahamehorner , I agree, but one step at a time, I'm just encapsulating the example in the mDNS directory, and using the protocol already provided by the espota.py. We can upgrade that later, to include a password for example. That's a good idea.

The ones without password would start regardless of the password, and when password support is there it won't start without it.

Btw, we're far far far from any IOT security without full certificate support in the TLS part, that'd be the only fully secure way to do things, for example when connecting to download the firmware, do it in an encrypted way, and, only from a server which has a certificate we trust.

@grahamehorner
Copy link

@mangelajo definitely one step at a time; I believe we are in agile; I think the ArduionOTA should be made a separate GitHub repo to allow the library to be developed as the first step; the community can then raise product feature request on that repo which would be developed/placed in the sprint according to priority and integrated accordingly into the release of the library.

This would certainly allow for adding features like password, certificate validation and https/sftp/ftps downloading if the community ask/like/vote for those features.

@mangelajo
Copy link
Contributor Author

@grahamehorner, more important than ask/like/vote is implement ;-), and btw, to implement certificate validation we need the underlaying support for it, and AFAIK that is still not ready.

You're asking to slow down something potentially very useful for the community in general, in favour of having "production" features which the Arduino IDE doesn't seem to be ready for.

@mangelajo
Copy link
Contributor Author

@grahamehorner , thinking of our conversation here, I'm not sure if you are unaware that support for this is already integrated into the IDE. If you select the OTA mode, and then select the remote device, you're able to flash it with the ArduinoOTA protocol.

https://github.com/esp8266/Arduino/blob/esp8266/hardware/esp8266com/esp8266/tools/espota.py

@grahamehorner: @pgollor and @igrr worked on it ;), I guess they should have a saying here.

@mangelajo
Copy link
Contributor Author

@pgollor, @igrr, do you know why do we stop all UDPs ? shouldn't we stop just ours?, I guessed it was there to avoid other UDP receivers to fill up the buffer space in the IP stack, and get everything stuck..

@pgollor
Copy link
Contributor

pgollor commented Oct 17, 2015

if i remember correctly it has safety reasons to stop all UDP connections. Other connections can interrupt the update and that could cause problems.
But i am not realy sure.

@mangelajo
Copy link
Contributor Author

@pgollor, yeah, I imagined that, I found errors where unattended connections fill the IP stack buffer, and other connections cannot receive anything more. I agree with @grahamehorner we should at least move it to the point we know TCP connection is established back to espota.py.

@grahamehorner
Copy link

@majenkotech

You're asking to slow down something potentially very useful for the community in general, in favour > of having "production" features which the Arduino IDE doesn't seem to be ready for.

exactly the opposite; the cadence of the ArduinoOTA shouldn't be coupled to that of the IDE, yes features like secured download https may be requested and voted for by the community, but these remain block until the feature or the library for TLS/SSL support is available.

thinking of our conversation here, I'm not sure if you are unaware that support for this is already > integrated into the IDE. If you select the OTA mode, and then select the remote device, you're able
to flash it with the ArduinoOTA protocol.

@igrr @pgollor

I wasn't aware but as above and like other tools IMHO should not be tied to the cadence of the IDE; these are supporting tools/libraries and should have their own release cycle; which allows quicker implementation/bug fixes and easier regression should a developer wish/need.

again IMHO the ESP8622/Arduino project should be focused on integration of the esp8266 tool chain with the IDE for compilation/uploading and serial monitor; not core libraries or supporting libraries these should be other projects that don't impact the IDE integration of the tool chain.

but these are topics not directly relating to the ArduinoOTA, which BTW and I do like your work on this thusfar.

@igrr
Copy link
Member

igrr commented Oct 17, 2015

@mangelajo Great work! The code looks good to me, aside for a few nitpicks which may be fixed later on.

Regarding WiFiUDP::stopAll — that's actually important stuff. You need to add WiFiClient::stopAll there as well.
Reason for this is that once OTA starts, loop is no longer being called. This means that if there are any connections created by user code which are normally handled in the loop, these will no longer be handled. Therefore incoming data (udp/tcp pbufs) will keep piling up, and this will likely lead to a condition where new packets from OTA will no longer be handled. Therefore we need to terminate all connections other than OTA related ones.

@igrr
Copy link
Member

igrr commented Oct 17, 2015

@grahamehorner
This project is less about the IDE (which is developed by core Arduino team) and more about providing a set of tools, libraries, and APIs which allow people to run Arduino-like code on the ESP8266. It's not obvious to me that all libraries should have their own release cycle. It should mostly be up to the library's author/maintainer to make this decision. So for instance @mangelajo might decide to maintain ArduinoOTA library outside of this repository, and I would be okay with that. At the same time I have little incentive to work on IDE integration, ESP8266 core, and essential libraries in three different repositories.

@grahamehorner
Copy link

@igrr @mangelajo I understand the explanation of why WiFiUdp.stopAll and WiFiClient::stopAll are needed but surely these should be done at the latest opportunity after validatinf the UDP packet/OTA request and the validating for connection to the download server?

@mangelajo
Copy link
Contributor Author

@grahamehorner, I guess if we "stopAll" after connection we would be killing our own connection, that's not cool.

Let's look for a compromise solution, I can add an optional password to the UDP, and to espota.py, and kill all connections right before connecting back to the updater.

Then, if something fails, you always get the callback "onError", and you can reset the board, or reconnect whatever was disconnected.

@mangelajo
Copy link
Contributor Author

@igrr, I personally prefer to contribute to the work here, and make sure the work is widely available for everyone. You're doing an awesome work with this. :)

igrr added a commit that referenced this pull request Oct 18, 2015
OTA support encapsulated to ArduinoOTA class
@igrr igrr merged commit 2aa4bba into esp8266:esp8266 Oct 18, 2015
@mangelajo
Copy link
Contributor Author

Thanks for merging, I will follow up with the suggested changes: optional password support for a tiny bit of extra security, and stopping also TCP connections.

I guess we could also integrate SPIFFS update support too as the changes are very minimal.

@igrr
Copy link
Member

igrr commented Oct 20, 2015

Regarding password-protected OTA, the way I was originally planning to do
it is by using HMAC to sign the firmware image. HMAC-MD5 is easy to compute
and it would fit the bill perfectly.

On Tue, Oct 20, 2015 at 11:03 AM, Miguel Ángel Ajo <notifications@github.com

wrote:

Thanks for merging, I will follow up with the suggested changes: optional
password support for a tiny bit of extra security, and stopping also TCP
connections.

I guess we could also integrate SPIFFS update support too as the changes
are very minimal.


Reply to this email directly or view it on GitHub
#883 (comment).

@grahamehorner
Copy link

@igrr @mangelajo greatwork; @mangelajo it would also be great if a CRC or HASH of the update file was given so the receive device can get the file has downloaded correctly prior to update; not sure if that's a big ask or if its even possible?

@igrr
Copy link
Member

igrr commented Oct 20, 2015

Actually HMAC will serve both authentication and integrity validation
purposes.

On Tue, Oct 20, 2015 at 11:09 AM, Grahame Horner notifications@github.com
wrote:

@igrr https://github.com/igrr @mangelajo https://github.com/mangelajo
greatwork; @mangelajo https://github.com/mangelajo it would also be
great if a CRC or HASH of the update file was given so the receive device
can get the file has downloaded correctly prior to update; not sure if
that's a big ask or if its even possible?


Reply to this email directly or view it on GitHub
#883 (comment).

@grahamehorner
Copy link

@igrr indeed; just saw your post seconds after I posted 👍 HMAC

@grahamehorner
Copy link

@igrr @mangelajo oh and include the version of the update in the request along with the HMAC to project the request; so devices that have the current version don't need to do anything, they simply discard the request.

I realise these are changes to features and may/may not be a big ask for the library, I do IMHO think these are worth investing in and should you need me to do any work or fork the code and implement, I'd be happy to do so.

@pgollor
Copy link
Contributor

pgollor commented Oct 20, 2015

@mangelajo i added the SPIFFS update support here #802

@mangelajo
Copy link
Contributor Author

@igrr, so HMAC-MD5 would validate the first UDP packet?, and we could also include an MD5 sum of the firmware itself?

We must avoid the entry on the update loop if the UDP request is not authenticated (to avoid denial of service attacks).

@mangelajo
Copy link
Contributor Author

exactly @pgollor, I mean, leveraging that too from the ArduinoOTA class, if we receive a SPIFFS update, we handle it, if we receive a firmware update, we handle it as firmware... :)

@pgollor
Copy link
Contributor

pgollor commented Oct 22, 2015

Okay. I am not sure if i understand your plan completely. But I'm very excited. :)

@igrr
Copy link
Member

igrr commented Oct 22, 2015

@mangelajo you add firmware hash to the UDP packet, and then sign it with HMAC.
We also need to add some nonce to work around replay attacks, but this isn't easy because currently UDP handshake is one-way.

@grahamehorner
Copy link

@mangelajo @igrr the nonce could simply be the build version/date time of the firmware + the number of seconds since 01/01/1970 00:00:00 when the UDP was sent

@igrr
Copy link
Member

igrr commented Oct 22, 2015

@grahamehorner ESP does not necessarily know local time (i.e. I don't want to add dependency on NTP).
My current thinking is that we should probably avoid inventing custom protocols here, and use something like CRAM-MD5 or SCRAM (both are simple enough to be implemented on ESP). This will require exchanging two more UDP datagrams.

@Juppit
Copy link
Contributor

Juppit commented Oct 22, 2015

@igrr the ESP already 'knows' time, just call

sntp_init
sntp_setservername
sntp_get_current_timestamp

If you like, find more in https://github.com/Juppit/esp8266-SNTPClock
_

On 22.10.2015 at 16:32 wrote Ivan Grokhotkov:

@grahamehorner https://github.com/grahamehorner ESP does not
necessarily know local time (i.e. I don't want to add dependency on NTP).
My current thinking is that we should probably avoid inventing custom
protocols here, and use something like CRAM-MD5 or SCRAM (both are
simple enough to be implemented on ESP). This will require exchanging
two more UDP datagrams.


Reply to this email directly or view it on GitHub
#883 (comment).

@igrr
Copy link
Member

igrr commented Oct 22, 2015

Yes, you can get ESP to obtain time, but doing so uses some resources,
namely UDP PCBs and RAM. These resources are not used unless you call
sntp_init, sntp_setservername, etc.
I don't think it's okay to use these resources 'by default' just to have
OTA working.

On Thu, Oct 22, 2015, 18:36 Juppit notifications@github.com wrote:

@igrr the ESP already 'knows' time, just call

sntp_init
sntp_setservername
sntp_get_current_timestamp

If you like, find more in https://github.com/Juppit/esp8266-SNTPClock
_

On 22.10.2015 at 16:32 wrote Ivan Grokhotkov:

@grahamehorner https://github.com/grahamehorner ESP does not
necessarily know local time (i.e. I don't want to add dependency on NTP).
My current thinking is that we should probably avoid inventing custom
protocols here, and use something like CRAM-MD5 or SCRAM (both are
simple enough to be implemented on ESP). This will require exchanging
two more UDP datagrams.


Reply to this email directly or view it on GitHub
#883 (comment).


Reply to this email directly or view it on GitHub
#883 (comment).

@drmpf
Copy link

drmpf commented Oct 22, 2015

For a simple fast "secure hash" and protection against message replay you might like to check out my page on http://www.forward.com.au/pfod/secureChallengeResponse/index.html
which uses SipHash and message sequence numbers etc.

@grahamehorner
Copy link

@Juppit @igrr I would think that while NTP does consume some resources, so would two/more udp packet exchange; and NTP time packet can be more useful generally to the ESP that a exchange of udp packets for CRAM-MD5 or SCRAM.

but if the OTA did a general callback; the developer could do this with out implementing something that was to restrictive.

@igrr igrr mentioned this pull request Oct 29, 2015
igrr added a commit that referenced this pull request Oct 29, 2015
OTA support encapsulated to ArduinoOTA class
@hallard
Copy link
Contributor

hallard commented Nov 2, 2015

Hi @mangelajo,

I'm starting to use encapsulated ArduinoOTA class. Unfortunately all my OTA setting was (and still) stored in EEP and I would like to be able to change them after OTA instantiation.

But all parameters are passed on class init, would it be possible to also add these parameters to the setup() or any new such as OTA.setMDNSHost(host) or whatever so we can change them after instanciation ?

What do you think ?

I can do it if you want, not a problem

@pgollor
Copy link
Contributor

pgollor commented Nov 2, 2015

@hallard I'm not fully sure if it is what you want, but you can set the OTA parameters by calling ArduinoOTA ota_server(HOSTNAME, APORT);

@igrr
Copy link
Member

igrr commented Nov 2, 2015

@hallard as a temporary solution, I think you should be able to do this:

ArduinoOTA ota("esp8266", 8266);

void setup() {
    // read config
    // ota_host_name = ...
    // ota_port = ...
    ota = ArduinoOTA(ota_host_name, ota_port);
}

@mangelajo
Copy link
Contributor Author

@hallard, yes I guess that could be possible.

Also you could do:

ArduinoOTA *ota;

void setup() {

ota = new ArduinoOTA(ota_host_name, ota_port);

}

@igrr, we may need to look into the authentication thing, any idea of how to integrate that with the IDE?

As an extra measure, we could include a hash of the firmware itself into the UDP packet, and at the end of reception, before triggering reset/update sequence, we could check the hash received in the UDP packet (and signed with pass) matches the hash of the firmware.

@igrr
Copy link
Member

igrr commented Nov 7, 2015

Firmware hash should most certainly be included into handshake, that's the
plan.

Regarding passwords and IDE integration, you might want to coordinate with
@me-no-dev, he is working on a pull request to the IDE regarding OTA
support.

It would also make sense to open a new issue to track these improvements.

On Sat, Nov 7, 2015, 21:53 Miguel Ángel Ajo notifications@github.com
wrote:

@hallard https://github.com/hallard, yes I guess that could be possible.

Also you could do:

ArduinoOTA *ota;

void setup() {

ota = new ArduinoOTA(ota_host_name, ota_port);

}

@igrr https://github.com/igrr, we may need to look into the
authentication thing, any idea of how to integrate that with the IDE?

As an extra measure, we could include a hash of the firmware itself into
the UDP packet, and at the end of reception, before triggering reset/update
sequence, we could check the hash received in the UDP packet (and signed
with pass) matches the hash of the firmware.


Reply to this email directly or view it on GitHub
#883 (comment).

@me-no-dev
Copy link
Collaborator

here is the IDE part: me-no-dev/Arduino-1@2b75aec
Here is my current sketch, but i'm still working in the mdns side and changes are not pushed yet.

#include <ESP8266WiFi.h>
#include <ESP8266mDNS.h>

const char* host = "esp8266-8m";
const char* ssid = "nbis-test";
const char* password = "1234567890";
const uint16_t ota_port = 8266;
const char* ota_pass = "1234";

WiFiUDP OTA;

void initOTA(){
  MDNS.enableArduino(ota_port, true);
  OTA.begin(ota_port);
}

void checkOTA(){
  if (OTA.parsePacket()) {
    IPAddress remote = OTA.remoteIP();
    String pass = OTA.readStringUntil(' ');
    int cmd  = OTA.parseInt();
    int port = OTA.parseInt();
    int size   = OTA.parseInt();

    Serial.printf("Update Pass: %s\n", pass.c_str());
    if(!pass.equals(String(ota_pass))){
      Serial.println("ERROR: Password Failed");
      return;
    }
    Serial.printf("Update Start: %d\n", size);
    if(!Update.begin(size)){
      Update.printError(Serial);
      return;
    }

    WiFiUDP::stopAll();
    WiFiClient::stopAll();
    WiFiClient client;
    if (client.connect(remote, port)) {
      Serial.setDebugOutput(true);
      uint32_t written;
      while(!Update.isFinished()){
        written = Update.write(client);
        if(written > 0) client.print(written, DEC);
      }
      Serial.setDebugOutput(false);
      if(Update.end()){
        client.print("OK");
        Serial.printf("Update Success\n");
        ESP.restart();
      } else {
        Update.printError(client);
        Update.printError(Serial);
      }
    } else {
      Serial.printf("Connect Failed\n");
    }
  }
}

void setup() {
  Serial.begin(115200);
  Serial.setDebugOutput(true);
  Serial.println();

  Serial.println("booting...");
  WiFi.mode(WIFI_STA);
  WiFi.begin(ssid, password);
  if(WiFi.waitForConnectResult() == WL_CONNECTED){
    MDNS.begin(host);
    initOTA();
    Serial.println("ready");
  } else {
    Serial.println("failed");
    delay(10000);
    ESP.reset();
  }
}

void loop() {
  checkOTA();
}

@me-no-dev
Copy link
Collaborator

ESP Changes: #980

Normola pushed a commit to Normola/Arduino that referenced this pull request Feb 19, 2020
Update czech language file based on suggestion
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.

None yet

10 participants