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

MQTT rewrite with SSL fixes (see also: #1751) #1829

Merged
merged 6 commits into from
Aug 26, 2019

Conversation

Niek
Copy link
Contributor

@Niek Niek commented Jul 17, 2019

  • 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

This PR kind of depends on #1751, might be better to wait for that one to be merged first. Update: #1751 is merged now.

- 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
@mcspr
Copy link
Collaborator

mcspr commented Jul 20, 2019

Quick questions:

  • any particular reason not to use DEBUG_MSG_P?
  • can we reduce the size of connect method, avoiding the nested ifdef tree?
  • should Experimental support of HTTPUpdate for OTA #1751 implement common setup method for wificlient?
  • why combine async server dependencies and secure_client_axtls?

@Niek
Copy link
Contributor Author

Niek commented Jul 23, 2019

Quick questions:

  • any particular reason not to use DEBUG_MSG_P?

I'm using DEBUG_MSG instead of DEBUG_MSG_P if the debug message consist of static text only - please let me know if this doesn't make sense.

  • can we reduce the size of connect method, avoiding the nested ifdef tree?

I agree, it's getting quite messy for sure - I can split up in a separate WifiClient and AsyncMqttClient one if you want? It'll be a bit more verbose obviously and have some duplication, but the code will be more readable.

I think it makes sense, but if we want things like separate MFLN/CA/etc settings per module it will be hard to make it a common method. And there may be more exceptions: for example, we might always want to attempt MFLN on MQTT but not on OTA.

  • why combine async server dependencies and secure_client_axtls?

I don't understand this question, can you elaborate? When MQTT_LIBRARY == MQTT_ASYNC, it's using ESPAsyncTCP and thus AxTLS by definition, since you can't use BearSSL with ESPAsyncTCP.

@mcspr
Copy link
Collaborator

mcspr commented Jul 24, 2019

You can just use PSTR always. For any debug message it makes sense to place those there, since they will be kept in heap otherwise; after esp loads the binary, all char strings are placed in heap (unless specified
otherwise with PROGMEM macro). And only the format is in PROGMEM anyways.
sidenote: but, with 2.5.0+ (not sure on the exact version, since earlephilhower/newlib-xtensa@61f08d0. unless it was broken on release), libc was patched so that printf will accept PSTR's as arguments too, simply always using aligned reads for %s

It'll be a bit more verbose obviously and have some duplication, but the code will be more readable.

Duplication can be minimized through? At the very least, maybe some flow can be split up into functions. If marked as explicitly inline, binary code would be exactly the same, but (i hope) more readable.

I think it makes sense, but if we want things like separate MFLN/CA/etc settings per module it will be hard to make it a common method. And there may be more exceptions: for example, we might always want to attempt MFLN on MQTT but not on OTA.

When I get around to updating the HTTPupdate PR, I will try to implement sort of a wrapper for wificlient struct and helpers for some of the flow stages (i.e. here we are checking mfln, here we are setting up security checks). Some minimal config, giving cert pointer and fp string for example.

I don't understand this question, can you elaborate? When MQTT_LIBRARY == MQTT_ASYNC, it's using ESPAsyncTCP and thus AxTLS by definition, since you can't use BearSSL with ESPAsyncTCP.

I meant changing ASYNC_TCP_SSL_ENABLED && WEB_SSL_ENABLED parts.

@Niek
Copy link
Contributor Author

Niek commented Jul 25, 2019

I have cleaned up the code a bit, using PSTR everywhere and made the connect method a lot more concise (still not spit up though). Having the WiFiClient(Secure) wrapper would help a lot in cleaning up the code I think.

I don't understand this question, can you elaborate? When MQTT_LIBRARY == MQTT_ASYNC, it's using ESPAsyncTCP and thus AxTLS by definition, since you can't use BearSSL with ESPAsyncTCP.

I meant changing ASYNC_TCP_SSL_ENABLED && WEB_SSL_ENABLED parts.

Ah yes, I probably should have put that in a separate PR - I assume that after introducing SECURE_CLIENT in the codebase we should get rid of all ASYNC_TCP_SSL_ENABLED ifdefs in the code, hence those changes.

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

Niek commented Aug 13, 2019

Web UI is still TBD as per comment #1829 (comment)

@mcspr
Copy link
Collaborator

mcspr commented Aug 22, 2019

Regarding my previous comment about the fixed flash location for cert data, this is exactly what happened in tasmota aws iot integration:
Special conversion .pem -> .der -> base64 (...which looks like easier than awk/sed'ing PEM into a single line): https://github.com/arendst/Sonoff-Tasmota/wiki/AWS-IoT#step-8-prepare-your-aws-iot-credentials (certificate section, first one is for the key)
Ofc that required base64 converter and some hard-coded (probably temporary) assumptions about the flash layout: arendst/Tasmota#6179

@mcspr
Copy link
Collaborator

mcspr commented Aug 25, 2019

Little follow up on client configuration, based on this branch.
dev...mcspr:mqtt-ssl-rewrite
I wanted to test it first though...

Gist of what's changed:

  • include headers are in prototypes, .ino has the objects
  • since MQTT_LIBRARY choices are always commented to explain what they mean, use full names for those. I am also a fan of adding MQTT_LIBRARY prefix.
  • remove some logging from runtime, move some into compile time
  • split connect method into two stages: setup and async/sync_connect
  • use the same mfln logic as httpupdate does (see mfln.probe terminal command for probing)

And SecureClientHelpers do the rest. Since we want to have up-to-date config, there's new "config" struct with all of the settings as getters. "...scCheck" logic is separated in to "checks" struct, running all of the relevant actions.
I also tried to utilize those for AsyncMqttClient, but I am not sure how to clearly separate those besides using "before" and "after" callbacks. Could also just forget about axtls altogether and focus on bssl integration everywhere. Notice a little optimization with onconnect verification.

Continuing off-topic about the certs...
I had also noticed that we can override X509List class with something that returns progmem pointers on getTrustAnchors(), so there's no need to create a whole separate wificlientsecure class. After make on bearssl-esp8266 repo, brssl ta file.pem util can generate the required struct, only thing needed is to add PROGMEM attribute.

@mcspr mcspr merged commit b48f8c1 into xoseperez:dev Aug 26, 2019
This was referenced Aug 26, 2019
@Niek
Copy link
Contributor Author

Niek commented Aug 27, 2019

For reference: discussion continued in #1873 and #1465

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.

3 participants