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

espconn_secure/tls: eliminate funky flash game #3032

Open
nwf opened this issue Feb 2, 2020 · 6 comments
Open

espconn_secure/tls: eliminate funky flash game #3032

nwf opened this issue Feb 2, 2020 · 6 comments
Labels

Comments

@nwf
Copy link
Member

nwf commented Feb 2, 2020

Missing feature

I would like to propose that the current tls.cert.verify and tls.cert.auth API is unfortunate. It directly accesses particular sectors of flash and is holographically embedded within both app/mbedtls/app/espconn_secure.c and app/mbedtls/app/espconn_mbedtls.c. I would prefer, instead, I think, either a more elaborate per-connection configuration mechanism or, failing that,
that the API be such that these two functions take Lua functions that return certificates or keys when invoked:

tls.cert.verify(function(ix) if ix == 1 then return file:read("ca.pem") end end)
tls.cert.auth(function(ix) ... end) -- ix 0: private key, 1: cert, 2+: chain elements

We can encode the current behavior by making the PEM-string forms of these functions write to
a particular name in SPIFFS and make the boolean forms install default functions that read those files.

Justification

At least in the test harness I'm working on, there's a fair bit of juggling of SSL keys and certs, and I'd much rather have these as files in the SPIFFS than in particular sectors of flash.

Workarounds

Lots and lots of flash overwrites.

nwf added a commit to nwf/nodemcu-firmware that referenced this issue Feb 23, 2020
Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing nodemcu#3032
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Feb 23, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Feb 23, 2020
Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing nodemcu#3032
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Feb 23, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Feb 25, 2020
Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing nodemcu#3032
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Feb 25, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Feb 25, 2020
Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing nodemcu#3032
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Feb 25, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Mar 31, 2020
Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing nodemcu#3032
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Mar 31, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Apr 2, 2020
Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing nodemcu#3032
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Apr 2, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Apr 2, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Apr 2, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Apr 3, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Apr 5, 2020
Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing nodemcu#3032
nwf added a commit to nwf/nodemcu-firmware that referenced this issue Apr 5, 2020
The new feature part of nodemcu#3032
Subsequent work will remove the old mechanism.
nwf added a commit that referenced this issue Apr 7, 2020
* espconn: remove unused espconn code, take 1

This is the easiest part of #3004 .
It removes a bunch of functions that were never called in our tree.

* espconn: De-orbit espconn_gethostbyname

Further work on #3004

While here, remove `mqtt`'s charming DNS-retry logic (which is neither
shared with nor duplicated in other modules) and update its :connect()
return value behavior and documentation.

* espconn: remove scary global pktinfo

A write-only global!  How about that.

* net: remove deprecated methods

All the TLS stuff moved over there a long time ago, and
net_createUDPSocket should just do what it says on the tin.

* espconn_secure: remove ESPCONN_SERVER support

We can barely function as a TLS client; being a TLS server seems like a
real stretch.  This code was never called from Lua anyway.

* espconn_secure: more code removal

* espconn_secure: simplify ssl options structure

There is nothing "ssl_packet" about this structure.  Get rid of the
terrifying "pbuffer" pointer.

Squash two structure types together and eliminate an unused field.

* espconn_secure: refactor mbedtls_msg_info_load

Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing #3032

* espconn_secure: introduce TLS cert/key callbacks

The new feature part of #3032
Subsequent work will remove the old mechanism.

* tls: add deprecation warnings

* luacheck: net.ifinfo is a thing now

* tls: remove use of espconn->reverse

* mqtt: stop using espconn->reverse

Instead, just place the espconn structure itself at the top of the user
data.  This enlarges the structure somewhat but removes one more layer
of dynamic heap usage and NULL checks.

While here, simplify the code a bit.

* mqtt: remove redundant pointer to connect_info

Everywhere we have the mqtt_state_t we also have the lmqtt_userdata.

* mqtt: doc fixes

* mqtt: note bug

* tls: allow :on(...,nil) to unregister a callback
marcelstoer pushed a commit that referenced this issue Jun 9, 2020
* espconn: remove unused espconn code, take 1

This is the easiest part of #3004 .
It removes a bunch of functions that were never called in our tree.

* espconn: De-orbit espconn_gethostbyname

Further work on #3004

While here, remove `mqtt`'s charming DNS-retry logic (which is neither
shared with nor duplicated in other modules) and update its :connect()
return value behavior and documentation.

* espconn: remove scary global pktinfo

A write-only global!  How about that.

* net: remove deprecated methods

All the TLS stuff moved over there a long time ago, and
net_createUDPSocket should just do what it says on the tin.

* espconn_secure: remove ESPCONN_SERVER support

We can barely function as a TLS client; being a TLS server seems like a
real stretch.  This code was never called from Lua anyway.

* espconn_secure: more code removal

* espconn_secure: simplify ssl options structure

There is nothing "ssl_packet" about this structure.  Get rid of the
terrifying "pbuffer" pointer.

Squash two structure types together and eliminate an unused field.

* espconn_secure: refactor mbedtls_msg_info_load

Split out espconn_mbedtls_parse, which we can use as part of our effort
towards addressing #3032

* espconn_secure: introduce TLS cert/key callbacks

The new feature part of #3032
Subsequent work will remove the old mechanism.

* tls: add deprecation warnings

* luacheck: net.ifinfo is a thing now

* tls: remove use of espconn->reverse

* mqtt: stop using espconn->reverse

Instead, just place the espconn structure itself at the top of the user
data.  This enlarges the structure somewhat but removes one more layer
of dynamic heap usage and NULL checks.

While here, simplify the code a bit.

* mqtt: remove redundant pointer to connect_info

Everywhere we have the mqtt_state_t we also have the lmqtt_userdata.

* mqtt: doc fixes

* mqtt: note bug

* tls: allow :on(...,nil) to unregister a callback
@TerryE
Copy link
Collaborator

TerryE commented Jun 17, 2020

@nwk, are you aware of the platform_rcr functions (read/write/delete) ? But that aside, I also feel on balance that it is a lot simpler to use SPIFFS as the repository for such data.

@nwf
Copy link
Member Author

nwf commented Jun 17, 2020

I was not aware of the platform_rcr functions, no. If we wanted to store the certs over there, I'd prefer that we expose the RCR records to Lua (perhaps as userdata rather than strings, necessarily) rather than adopt the existing, exciting flash game directly.

I admit I've stalled on actually eliminating the funky TLS flash goo because I haven't yet figured out a good replacement for the Makefile's ability to bake in a certificate:

Because our build system doesn't, historically, construct LFS or SPIFFS images as part of its operation, there's presently no real other way to do this. Given the existence of RCRs.... maybe we should expose those to Lua (at least in a RO way)... hm. This may also be related to #3117 and #3128 and attendant platform support.

@TerryE
Copy link
Collaborator

TerryE commented Jun 17, 2020

Given the existence of RCRs.... maybe we should expose those to Lua (at least in a RO way)...

There are node read and write RCR functions but only on debug builds. The aim of the RCR interface is create a facility for *nix environment like config settings that can persist across reboots. It is fast, lean and simple; using NAND write logic. My preference would be to limit access to C API because if Lua developers abuse this then it is quite easy to leave the firmware unbootable and needing reflashed.

It isn't fully safe in that once the 4Kb page is used, the GC requires it to be erased and rewritten, which might occur every 50-100 config updates. Because it is at a fixed location, you can update it using ESPtool, and this is what nodemcu-partition does, so we could just add the extra logic to this.

@nwf
Copy link
Member Author

nwf commented Jun 17, 2020

Yea, I think having nodemcu-partition.py know how to land SSL certs into the RCR would be fine. I don't particularly see a need to update the in-RCR certs from the on-device application once they're there, given that said application could just as easily pull from SPIFFS (or, better, LFS, which avoids copying into RAM if DER certs are embedded; we could make the RCR path also avoid copying up, but that might be some unnecessary engineering).

@stale
Copy link

stale bot commented Jun 16, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 16, 2021
@nwf nwf removed the stale label Jun 17, 2021
@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants