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

Remove SD/SPIFFS dependency in CertStoreBearSSL #4740

Closed
earlephilhower opened this issue May 17, 2018 · 26 comments · Fixed by #4760
Closed

Remove SD/SPIFFS dependency in CertStoreBearSSL #4740

earlephilhower opened this issue May 17, 2018 · 26 comments · Fixed by #4760

Comments

@earlephilhower
Copy link
Collaborator

Capturing the points in discussion : e3c9702

  • PlatformIO default optimization barfs on SD.h included in CertStoreSDBearSSL (needs -link=deep+)
  • NO_GLOBAL_INSTANCES breaks the build due to SD.* in the code, but simple #ifdef workaround still leaves other chained dependency problems
  • Having separate but almost identical CertStoreSPIFFS and CertStoreSD definitely sub-optimal (but no compatible class File exists between them)

@everslick and @igrr have suggested using callbacks and Stream to unify the two.

Given the above, a modification to the API to take a structure with several user-supplied callbacks for traversing directories and opening files (as Stream) seems appropriate.

earlephilhower referenced this issue May 17, 2018
…rn SSL (#4273)

BearSSL (https://www.bearssl.org) is a TLS(SSL) library written by
Thomas Pornin that is optimized for lower-memory embedded systems
like the ESP8266. It supports a wide variety of modern ciphers and
is unique in that it doesn't perform any memory allocations during
operation (which is the unfortunate bane of the current axTLS).

BearSSL is also absolutely focused on security and by default performs
all its security checks on x.509 certificates during the connection
phase (but if you want to be insecure and dangerous, that's possible
too).

While it does support unidirectional SSL buffers, like axTLS,
as implemented the ESP8266 wrappers only support bidirectional
buffers. These bidirectional buffers avoid deadlocks in protocols
which don't have well separated receive and transmit periods.

This patch adds several classes which allow connecting to TLS servers
using this library in almost the same way as axTLS:
BearSSL::WiFiClientSecure - WiFiClient that supports TLS
BearSSL::WiFiServerSecure - WiFiServer supporting TLS and client certs

It also introduces objects for PEM/DER encoded keys and certificates:
BearSSLX509List - x.509 Certificate (list) for general use
BearSSLPrivateKey - RSA or EC private key
BearSSLPublicKey - RSA or EC public key (i.e. from a public website)

Finally, it adds a Certificate Authority store object which lets
BearSSL access a set of trusted CA certificates on SPIFFS to allow it
to verify the identity of any remote site on the Internet, without
requiring RAM except for the single matching certificate.
CertStoreSPIFFSBearSSL - Certificate store utility

Client certificates are supported for the BearSSL::WiFiClientSecure, and
what's more the BearSSL::WiFiServerSecure can also *require* remote clients
to have a trusted certificate signed by a specific CA (or yourself with
self-signing CAs).

Maximum Fragment Length Negotiation probing and usage are supported, but
be aware that most sites on the Internet don't support it yet.  When
available, you can reduce the memory footprint of the SSL client or server
dramatically (i.e. down to 2-8KB vs. the ~22KB required for a full 16K
receive fragment and 512b send fragment).  You can also manually set a
smaller fragment size and guarantee at your protocol level all data will
fit within it.

Examples are included to show the usage of these new features.

axTLS has been moved to its own namespace, "axtls".  A default "using"
clause allows existing apps to run using axTLS without any changes.

The BearSSL::WiFi{client,server}Secure implements the axTLS
client/server API which lets many end user applications take advantage
of BearSSL with few or no changes.

The BearSSL static library used presently is stored at
https://github.com/earlephilhower/bearssl-esp8266 and can be built
using the standard ESP8266 toolchain.
@earlephilhower earlephilhower self-assigned this May 17, 2018
@everslick
Copy link
Contributor

This would leave a generic "BearSSL(axTLS?)::CertStore()" class, but each end app would need to re-implement the callback structure, which is not complicated but "feels" half-baked (esp. for folks using Arduino).

yes, it feels 'complicated' and i agree to what you (@earlephilhower ) say, in general.

If we put in a helper function anywhere in core to do it for SPIFFS or SD, we'd be back where we are today

we could move those helpers into the examples. Arduino people tend to copy them verbatim anyway.

anyway, out of pure self interest, I'm not concerned about PlatformIO, but about introducing inter library dependencies that will hurt us in the long run. i'm already confronted with for example, ArduinoOTA calling into MDNS.* methods, that gives me troubles. As a last resort i suggest disabling such code with #ifndef NO_GLOBAL_INSTANCES.

question: what would happen if we removed #ifndef NO_GLOBAL_INSTANCES from the extern declarations in the header files but kept it around the global definitions? could this work?

@igrr
Copy link
Member

igrr commented May 17, 2018 via email

@earlephilhower
Copy link
Collaborator Author

One option might be to combine the certificates into a single file on PC side, and load that one file via Stream, in which case iteration would not be necessary.

Then we'd, unfortunately, need a Java plugin to be installed to do the merge.

Another option might be to convert the iteration function into a template, taking FileSystem and File classes. Specialisations could be provided to deal with the fact that "open" arguments for SPIFFS and SD classes are different.

I was thinking about that. SFINAE might be able to save the day here, but I'm not sure if it's workable in this case. I've never had to use a hammer that big, though, so could be way off base.

A first step of callbacks here would be the low risk, fast turnaround option. Sidesteps the whole SPIFFS vs SD issue.

@igrr
Copy link
Member

igrr commented May 17, 2018 via email

@earlephilhower
Copy link
Collaborator Author

Yes, I guess the script could do the merge. I was hoping the script would just be an example, but I think practically it'll end up being what people use to make a cert store. Then either a seekable Stream (which lives at least the life of the WiFiClient/ServerSecure, or a CB to get the Stream is all that's needed.

@earlephilhower
Copy link
Collaborator Author

Over the weekend I coded up a new version and run into two issues w/a single Get-Stream callback. One is work-aroundable, the other I'm kind of stumped on because of Arduino's habit of passing around objects and not references:

To get the stream I'm using a std::function<Stream*(void*)> which takes a void* context (passed in to the constructor) and returns a Stream*.

Problem 1: class Stream has no seek/skip. I have to read the whole file (up to 700KB+, so 350KB on average) and throw out unused data until I get to the one I want. This is not fast on SPIFFS, (maybe on SD, too), but I can logically make it work. Can't really add an add'l callback to seek as even if you seek'd the *::File, I'm not sure any Stream would honor that (or even if it did, that it wouldn't get confused).

Problem 2: class Stream has a non-virtual destructor (up several levels of class hierarchy, I guess). So there's not safe way for me to call delete stream; in the CertStore code since the Stream will really be a fs::Stream or SD::Stream-ish type. A second callback to destroy the stream would be possible, but it's pretty awkward (it'll need to take the Stream* and coerce to the proper Stream subclass).

I can't have the callback give a plain Stream as it is an abstract class with several methods =0.

Seems like after this look, a set of callbacks (which can be lambdas thanks to std::function, so pretty simple wrappers) may be the most readable way forward.

Any suggestions?

@beegee-tokyo
Copy link

BUZZ!!!

Anything happening here? I have a customer project and my customer needs to be able to compile the code on PlatformIO without deleting files or fiddling around with the PIO command line.

Is there a possibility to add this --project-option=lib_ldf_mode=deep+ into the platformio.ini file?

@earlephilhower
Copy link
Collaborator Author

Sorry, this is a deeper issue than just BearSSL since it involves the incompatibility between SPIFFS and SD and the whole "there is no file standard" problem in Arduino.

Easiest thing to do is rm -f CertStore* from your Git copy. Can't use CertStore, obviously, but they're not using it yet anyway since PIO has that unsafe optimization going on anyway. Or, if there's a global platform.ini setting (but I don't use that setup so can't comment).

I made a whole local branch that uses a custom class you pass in to access the file, and generating a single combined certstore.bin file, but BearSSL internal code uses the SHA256 of the X509 distinguished name as the lookup hash, in binary form. OpenSSL and the Python libs cannot return that (only textual or object forms) so the class needs to support writing as well.

I'm basically ending up with an abstract class File and solving the darn Arduino "no File class" problem just for this, which I don't want to do unless absolutely necessary.

@beegee-tokyo
Copy link

beegee-tokyo commented May 23, 2018

I understand, but I am a little bit under pressure. Hope there will be a solution.

Meanwhile I found a (dirty) solution for PlatformIO users using extra scripts. This way I can at least deliver my product to my customer.

These scripts rename the CertStoreSDBearSSL.* files before compiling into _CertStoreSDBearSSL.*xx_and redo the renaming after compilation.
In platformio.ini of the project I add
extra_scripts = pre:pre_script.py, post:post_script.py
into the [board] options. For a Huzzah board this looks like

[env:huzzah]
platform = https://github.com/platformio/platform-espressif8266.git#feature/stage
board = huzzah
framework = arduino
extra_scripts = pre:pre_script.py, post:post_script.py

The pre_script.py:

import sys
import os

Import("env")

print "========================="
packagePath = env.get("BUILD_SCRIPT")
words = packagePath.split('platforms')
packagePath = words[0] + 'packages\\framework-arduinoespressif8266\\libraries\\ESP8266WiFi\\src'
cppOldName = packagePath + '\\CertStoreSDBearSSL.cpp'
cppNewName = packagePath + '\\CertStoreSDBearSSL.cppxx'
hOldName = packagePath + '\\CertStoreSDBearSSL.h'
hNewName = packagePath + '\\CertStoreSDBearSSL.hxx'
print 'renaming\n'
print cppOldName
print "\nto "
print cppNewName
print 'renaming\n'
print hOldName
print "\nto "
print hNewName
print "========================="
os.rename(cppOldName, cppNewName)
os.rename(hOldName, hNewName)

The post_script.py:

import sys
import os

Import("env")

# print env.Dump()

print "========================="
packagePath = env.get("BUILD_SCRIPT")
words = packagePath.split('platforms')
packagePath = words[0] + 'packages\\framework-arduinoespressif8266\\libraries\\ESP8266WiFi\\src'
cppOldName = packagePath + '\\CertStoreSDBearSSL.cpp'
cppNewName = packagePath + '\\CertStoreSDBearSSL.cppxx'
hOldName = packagePath + '\\CertStoreSDBearSSL.h'
hNewName = packagePath + '\\CertStoreSDBearSSL.hxx'
print 'renaming\n'
print cppNewName
print "\nto "
print cppOldName
print 'renaming\n'
print hNewName
print "\nto "
print hOldName
print "========================="
os.rename(cppNewName, cppOldName)
os.rename(hNewName, hOldName)

@earlephilhower
Copy link
Collaborator Author

@beegee-tokyo Why not use the LDF flag that's supported directly in platformio.ini?

http://docs.platformio.org/en/latest/projectconf/section_env_library.html#projectconf-lib-ldf-mode

[env:myenv]
lib_ldf_mode = deep+

@beegee-tokyo
Copy link

@earlephilhower thats what I was looking for, but had no time for searching. Will try tomorrow and give feedback. Thanks for the hint.

@beegee-tokyo
Copy link

@earlephilhower
lib_ldf_mode = deep+ worked. Thanks. And thanks as well for the new commit.

earlephilhower added a commit to earlephilhower/Arduino that referenced this issue May 24, 2018
Due to popular demand, remove the hardcoded dependency on SPIFFS
or SD from the CertStore by factoring out the file interface into
a new class (CertStoreFile) that the user will need to implement
as a thin wrapper around either a SPIFFS.file or a SD.file

Combine the downloaded certificates into a UNIX "ar" archive
and parse that on-the-fly to allow easy inspection and creation
of the Cert Store database.

Examples updated with a new certificate downloader that creates
the certs.ar archive and with a single sample that can be built
for either SPIFFS or SD with a #define.  Users can copy the
implementation of the CertStoreFile they need to their own code
as it is self-contained.

Also move the CertStore to the BearSSL namespace and remove the
suffix and separate SPIFFS/SD sources.

Fixes esp8266#4740
@earlephilhower
Copy link
Collaborator Author

There's a PR available for people to look at. @everslick, you seem to have a real problem with the existing setup, I think it will solve your problem as well as remove the need for the option @beegee-tokyo added to PIO.

Due to internal BearSSL architecture the Python code can't actually do the necessary processing to pre-generate the index files, so I've just combined everything into an AR archive . To handle the reads, writes, and seeks to the index and DER archive, I've just created a generic File wrapper virtual base class and written SD or SPIFFS subclasses that can be passed back to the library to handle the work.

@everslick
Copy link
Contributor

@earlephilhower Awesome! I will check it out ASAP! Your work is very much appreciated and IMHO going BearSSL is the right thing to do for the ESP8266. Thank you very much!

@chessweb01
Copy link

chessweb01 commented Jun 3, 2018

I added lib_ldf_mode = deep+ to platformio.ini. The SD.h: No such file or directory #4775 error disappeared.

However, now I get

.piolibdeps\SD_ID161/utility/Sd2PinMap.h:371:2: error: #error Architecture or board not supported.
.piolibdeps\SD_ID161/utility/Sd2Card.h:65:37: error: 'SS_PIN' was not declared in this scope
.piolibdeps\SD_ID161/utility/Sd2Card.h:68:31: error: 'MOSI_PIN' was not declared in this scope
.piolibdeps\SD_ID161/utility/Sd2Card.h:72:30: error: 'SCK_PIN' was not declared in this scope

What is the matter with the latest update?

As it stands now, my project is broken and I don't know why.

@chessweb01
Copy link

chessweb01 commented Jun 3, 2018

Update: lib_ldf_mode = deep+ didn't help after all.

I got my project to compile again by physically removing every file matching the pattern *Bear*.* from framework-arduinoespressif8266/libraries/ESP8266WiFi/src.

Additionally I had to comment every #include "*Bear*.h" in ESP8266WiFi.h, WiFiServerSecure.h and WiFiClientSecure.h.

So it seems something is terribly wrong with the Bear SSL integration as far as ESP8266 is concerned.

@earlephilhower
Copy link
Collaborator Author

@chessweb01, It worked for @beegee-tokyo and all the Arduino CI builds since it was checked in, so that's odd. In any case, please try PR #4760 which reworks the structure to work around PlatformIO's limitations and removes any dependencies on SD or SPIFFS unless you use the class.

@beegee-tokyo
Copy link

beegee-tokyo commented Jun 4, 2018

@chessweb01 I can confirm that lib_ldf_mode = deep+ works fine. I use it in all my projects now.
Where in platformio.ini did you place it?

@ivankravets
Copy link
Collaborator

ivankravets commented Jun 4, 2018

Arduino calls GCC Preprocessor on each C/C++ file to determine all dependencies. They can do that because there is no "project" in Arduino. People use 1 INO file in 99% cases. Calling/spawning external process on Windows is an expensive operation.

@platformio has a different philosophy. We propose developers to have multiple source files organized into the project. As result, we have written own Library Dependency Finder (LDF) which does not call real GCC executable per each C/C++ file but has a base understanding of GCC Preprocessor syntax (Python implementation). You can read more about the default behavior of PlatformIO's LDF.

LDF does not parse all source files from a library by default. If you see that some dependencies are not resolved automatically, I recommend helping LDF using lib_deps option in platformio.ini or manually include a missed header file in the one of project files. For example,

main.cpp

#include <SD.h>

@earlephilhower, does https://github.com/esp8266/Arduino/tree/master/libraries/ESP8266WiFi depend on SD library? There is no problem for PlatformIO, we can create library.json manifest for ESP8266WiFi and declare all dependencies or switch LDF to deep+.

P.S: All these things are done for the best performance on the all platforms.

@chessweb01
Copy link

chessweb01 commented Jun 4, 2018

@earlephilhower , @beegee-tokyo : As I wrote before in this thread, lib_ldf_mode = deep+ does indeed solve the SD.h: No such file or directory #4775 error, but then I get the following errors when compiling my project:

.piolibdeps\SD_ID161/utility/Sd2PinMap.h:371:2: error: #error Architecture or board not supported.
.piolibdeps\SD_ID161/utility/Sd2Card.h:65:37: error: 'SS_PIN' was not declared in this scope
.piolibdeps\SD_ID161/utility/Sd2Card.h:68:31: error: 'MOSI_PIN' was not declared in this scope
.piolibdeps\SD_ID161/utility/Sd2Card.h:72:30: error: 'SCK_PIN' was not declared in this scope

Only by completely removing all the Bear SSL related library files could I get my project to compile.

Before the most recent platform.io update the project compiled just fine.

@earlephilhower
Copy link
Collaborator Author

Oh well, sorry to hear that. Please use that PR referenced above (#4760) since I'm pretty sure the current setup is not going to be there at the next release.

@chessweb01
Copy link

chessweb01 commented Jun 4, 2018

Yeah, well, I'm sorry too.

Preferable from a user perspective would be if library modifications didn't nonchalantly break existing applications in user space, though. There is at least one prominent open source project taking pains to adhere to that philosophy.

@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Jun 4, 2018

@ivankravets Whether it needs a change in the esp.conf for PIO is up to you, really.

Right this instant it does depend on SD.h only if the user instantiates a class of BearSSL::CertStoreSD. Due to the odd setup where we have two incompatible "File" classes here on the ESP8266, though, the #include "sd.h" bit is only present in the .cpp file and a simple class File; is used in the header. And it seems like all files in the subdir are compiled (since C++ doesn't have the filename<->classname linkage that Java does).

It works w/the deep+ fine with Platform IO as far as I can tell, since do a PIO-based CI build as part of every PR/commit with that option on the command line).

However, PR #4760 (or another one) will refactor the whole class not to need SD.h (pushing it into the app to handle File IO).

So if you add it, I think you're going to want to remove it once that PR gets merged (before the next official release 2.4.2).

@earlephilhower
Copy link
Collaborator Author

@chessweb01 Your build filesystem has a problem and is not using the SD library ones from the ESP8266 core.

.piolibdeps\SD_ID161/utility/Sd2PinMap.h:371:2: error: #error Architecture or board not supported.
Note that in the core libs that error message is on line 384, not 371 (and has been at that line for 3 years):

#error Architecture or board not supported.

You're trying to use AVR SD libs on the ESP8266 and that's just not possible and not caused by either PIO or this patch. Note the error line on AVR's version is 371:

https://github.com/adafruit/SD/blob/1a24a9486d1dea54fa0c0f689e155bd3e219b305/utility/Sd2PinMap.h#L371

So please either remove any copies of AVR-only sd libraries and try again, or apply the PR I've referenced multiple times.

@ivankravets
Copy link
Collaborator

ivankravets commented Jun 5, 2018

@earlephilhower I see the issue with PIO when we have a library built-in a framework with the same name in a registry (see SD). I've just opened an issue for that platformio/platformio-core#1654

Temporary solution is of one of

  • lib_ldf_mode = deep+ in platformio.in
  • Just add to one of project files #include <SD.h> (PlatformIO will find this built-in library automatically. We parse all files from project directory. Do not forget to remove AVR's library from project_dir/.piolibdeps folder if you install it from a registry)

What do we need to merge #4760 ?

@earlephilhower
Copy link
Collaborator Author

@ivankravets I hope to have a change to a templated implementation instead of a virtual base class in the next day or so, and then get it merged by the 15th.

The one gotcha with the workaround you're using is that if any linked .o file had a #include <SD.h> in it, then the static SDClass SD object will be instantiated with its ~700 bytes of buffering/data structures, even if it's not actually accessed by any functions in the final link graph. It's an artifact of the GCC linking options we're using plus the (IMO) weird way of defining that global object used by the class.

earlephilhower added a commit that referenced this issue Jun 13, 2018
Due to popular demand, remove the hardcoded dependency on SPIFFS
or SD from the CertStore by factoring out the file interface into
a new class (CertStoreFile) that the user will need to implement
as a thin wrapper around either a SPIFFS.file or a SD.file

Combine the downloaded certificates into a UNIX "ar" archive
and parse that on-the-fly to allow easy inspection and creation
of the Cert Store database.

Examples updated with a new certificate downloader that creates
the certs.ar archive and with a single sample that can be built
for either SPIFFS or SD with a #define.  Users can copy the
implementation of the CertStoreFile they need to their own code
as it is self-contained.

Also move the CertStore to the BearSSL namespace and remove the
suffix and separate SPIFFS/SD sources.

Remove the "deep+" change from the CI build as well (no special
options needed on any PIO or makefile build).

We'll revisit the filesystem wrapper for 2.5.0, hopefully having a
unified template for both filesystem usage at a global level.  For
current users, be aware the interface may change (simplify!) in
release 2.5.0.

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

Successfully merging a pull request may close this issue.

6 participants