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

Experimental support of HTTPUpdate for OTA #1751

Merged
merged 36 commits into from
Aug 12, 2019
Merged

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Jun 2, 2019

  • select OTA_CLIENT (ESPAsyncTCP or HTTPUpdate)
  • select SSL_CLIENT (WiFiClientSecure variant)
  • partial support for CA certs with SSL_CLIENT_BEARSSL (example letsencrypt isrg x1 root as default. might not work 50% of the time)

Configuration might need to be OTA specific though, otaSslCheck?

@Niek This is what I meant by separate implementations. I think this builds 🌊
ArduinoOTA and OTA client are split into separate modules, Core HTTP updater added in a similar manner. Both axtls and bearssl should be supported. Only fingerprint support with 2.4.2, because travis envs do not use 2.5.. yet (and at this point, idk what to do with it. Core git broke spiffs symbols.... agrh)

CA support is trickier. The most robust way is to allow multiple of them (as common CA storage works on general-purpose oses), but rn it is only a single one. Either some personal CA entity and that surely will work, or we hope that LE cert is properly signed (because I do see that some of my certs are signed with DST root instead).

edit: This is more of a reference implementation and a dumping ground for some nice things found on the way, which will be merged asap (URL class, ArduinoOTA support flag...) before this

@mcspr
Copy link
Collaborator Author

mcspr commented Jun 5, 2019

For example, X1 root from LE is ~1.4KB when stored as .der
With spiffs it could be pretty straightforward thing to manage - just use CertStore
One simple option here is to just reserve a single flash sector for it and give some API to overwrite it
(4KB, up to 2 certs! aka root+intermediate. plus some storage left over)

@Niek
Copy link
Contributor

Niek commented Jun 5, 2019

Very nice, great work @mcspr! I will be leaving some comments on the code, but this looks much nicer split up this my PR - I'll be closing that one now in favor of this.

// -----------------------------------------------------------------------------

#ifndef SSL_CLIENT
#define SSL_CLIENT SSL_CLIENT_NONE // What variant of WiFiClient to use (no SSL support by default)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to mention the option here, so: SSL_CLIENT_NONE, SSL_CLIENT_AXTLS and SSL_CLIENT_BEARSSL. Also maybe do some auto detection based on the core version?

Copy link
Collaborator Author

@mcspr mcspr Jun 5, 2019

Choose a reason for hiding this comment

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

Autodetection? One interesting problem with version detection:
Ciphers blow up stack (fixed by stack thunk size increase in git) but can be mitigated locally by using hand-made cipher list. And that affects 2.5.0-2.5.2. It kind of tricky to define what works, so maybe it is better to leave this to the user.

I will have updated comments with possible clients options + Core from which they work / become deprecated

// TODO: add DigiCert CA for github?
#if SSL_CLIENT_BEARSSL && OTA_SSL_CLIENT_INCLUDE_CA
#include "static/ota_ssl_client_ca.h"
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

#else if SSL_CLIENT_BEARSSL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I wanted to add something here using .cpp files and weaken the progmem char symbol, but that did not quite work. This probably can even be just a check for include_ca flag, progmem'ed array will be discarded anyways as unused variable.

code/espurna/ota_asynctcp.ino Outdated Show resolved Hide resolved
@Niek
Copy link
Contributor

Niek commented Jun 17, 2019

Are you planning to merge this soon @mcspr? I will base the MQTT changes I'm working on on this PR (e.g. SSL_CLIENT).

@mcspr
Copy link
Collaborator Author

mcspr commented Jun 17, 2019

I thought to clean up 1.13.6 first.
Still some things waiting (#1705 , #1769 and others)

@xoseperez did specify that we use semantic versioning, but I think we need to release more versions for that to apply. However, since we don't break existing functionality and only add optional features, maybe it still works for this PR.

@Niek
Copy link
Contributor

Niek commented Jun 17, 2019

Maybe it's an idea to make a "SSL improvements" branch? That makes it at least a bit easier to base other PRs on.

@mcspr
Copy link
Collaborator Author

mcspr commented Jun 17, 2019

It can. I'd sleep on it, but I think just adding it to dev marked as experimental would be less of a hassle. There could be a separate module/feature line in info banner to denote that. Maybe even in the same line, with some marker.

And some minor things:

  • ssl... settings should be namespaced per module? mqttSsl..., otaSsl... ...
  • isn't it TLS now? SECURE_...?
  • something bugging me about URL class, need to check how memory is allocated when chunks are substring'ed

@mcspr mcspr changed the title [wip] Experimental support of HTTPUpdate for OTA Experimental support of HTTPUpdate for OTA Jun 20, 2019
@mcspr
Copy link
Collaborator Author

mcspr commented Jun 22, 2019

Can merge this finally.

So the one thing I have forgot is to support basic urls when ssl client is built. Turns out esp8266/Arduino#4980 marked all but update(WiFiClient, ...) methods as deprecated.

Both HTTPCLIENT_1_1_COMPATIBLE and HTTPUPDATE_1_2_COMPATIBLE look more suitable for internal Core dependency, not something supporting both sides of deprecation (when there were none of those methods present). That and different update() signatures per fingerprinting, it is kind of a pain to support all of the methods. Hence combining deprecated axTLS and update(url, fp) methods together
Bearssl is upped to >=2.5.0, since we don't want to support basic update(...) and want to directly use update(WiFiClient, ...)

@Niek
Copy link
Contributor

Niek commented Jun 26, 2019

Looks good! Once it's merged I will change the MQTT code to look at the SECURE_CLIENT_CHECK defines.

code/espurna/ota_httpupdate.ino Outdated Show resolved Hide resolved
@mcspr mcspr merged commit 9156eb1 into xoseperez:dev Aug 12, 2019
@mcspr mcspr deleted the ota/wificlient branch August 12, 2019 20:16
mcspr pushed a commit that referenced this pull request Aug 26, 2019
* MQTT rewrite with SSL fixes

- Added Arduino MQTT library support (actively maintained)
- Added support for BearSSL (core >= 2.5)
- BearSSL validation: insecure, fingerprinting and CA validation
- AxTLS validation: insecure and fingerprinting
- Support MFLN in order to reduce heap usage

* Better header incl, fix building w/ no NTP_SUPPORT

* Clean up code, use DEBUG_MSG_P

* Fix compile error
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.

2 participants