Remove SD/SPIFFS dependency in CertStoreBearSSL

[earlephilhower]

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.

[everslick]

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]

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. 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. The awkward part of this approach is how to pass File class to the template without requiring users to do so explicitly. Maybe the File class can be deduced automatically from file system (SD/SPIFFS) class somehow...

…

On Fri, May 18, 2018, 01:09 Clemens Kirchgatterer ***@***.***> wrote: 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 ***@***.*** <https://github.com/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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4740 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJcej174uQYEM7OEbI64amJWyiQbX-Qks5tza66gaJpZM4UDby6> .

[earlephilhower]

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]

Don't we already require Python runtime to download the certificates? Could the script (or another script) do the merging part?

…

On Fri, May 18, 2018, 04:16 Earle F. Philhower, III < ***@***.***> wrote: 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4740 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJceip_3XhEU6y9vsr42u5uujGDoiG_ks5tzdqPgaJpZM4UDby6> .

[earlephilhower]

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]

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]

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]

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]

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]

@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+