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

Updates for #1829 #1873

Merged
merged 17 commits into from
Sep 3, 2019
Merged

Updates for #1829 #1873

merged 17 commits into from
Sep 3, 2019

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Aug 26, 2019

#1829 (comment)
@Niek tested with https://test.mosquitto.org/ - test.mosquitto.org:8883, https://test.mosquitto.org/ssl/mosquitto.org.crt as custom .pem root
I also noticed that we may need to specify it clearer that it is not a client certificate, but root cert of the server. _mqtt_client_ca -> _mqtt_client_server_ca ?

  • clean-up dependencies and headers
  • since MQTT_LIBRARY choices are always commented to explain what they mean, use full names for those and add 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, allow module to define "config" struct with all of the settings as getters. "...scCheck" logic is separated in to internal "checks" struct, running all of the relevant actions based on "config" results.
AsyncMqttClient is still special case, because it handles fingerprinting all by itself. Difference from dev is that fingerprint only added on module setup, not with every configuration reload.
Also notice a little optimization with onconnect verification for axtls. No need to overcomplicate things if it will be removed anyway.

#include <MQTTClient.h>
#ifdef MQTT_MAX_PACKET_SIZE
#if MQTT_LIBRARY == MQTT_LIBRARY_ARDUINOMQTT
#ifdef MQTT_MAX_PACKET_SIZE
MQTTClient _mqtt(MQTT_MAX_PACKET_SIZE);
Copy link
Collaborator Author

@mcspr mcspr Aug 26, 2019

Choose a reason for hiding this comment

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

And what is up with this btw. Does platformio build_flags need comment / bigger value for it? idk if json works at all with the default MQTT_MAX_PACKET_SIZE=400

@mcspr
Copy link
Collaborator Author

mcspr commented Aug 27, 2019

Obviously, httpupdate should use this method.
Thingspeak sync variant too, since I do see SSL as an option.

Later merge of ESPAsyncTCP with proper bssl support would not need anything with before/after logic, only thing what would be required is to have some kind object with the same API as WiFiClientSecureBearSSL (setTrustAnchors, setBufferSizes etc.) that would be somehow exposed on the target client. Template magic will properly handle the rest

@Niek
Copy link
Contributor

Niek commented Aug 27, 2019

Very nice work :) I'll leave some comments on the code, but this looks good - I'll do some testing on my end as well. I agree the naming can be a bit better, for example _mqtt_client_server_ca as you mentioned and the and MQTT_LIBRARY_ prefix like in this PR.

@mcspr
Copy link
Collaborator Author

mcspr commented Sep 3, 2019

Renamed those to ...trusted_root_ca, both headers and symbols.

@mcspr mcspr merged commit e398685 into xoseperez:dev Sep 3, 2019
@mcspr mcspr deleted the mqtt-ssl-rewrite branch September 3, 2019 03:20
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.

None yet

2 participants