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

examples/mqttc: add option to use MBED TLS for connections #2643

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

pkarashchenko
Copy link
Contributor

Summary

Add mbedTLS option to examples/mqttc.
I haven't tried to connect with this code exactly to "broker.hivemq.com", so example may not work as is (somebody needs to try), but I used this code to connect to other MQTT server with TLS.

This code has some advantages compared to native MQTT-C mbedtls_sockets.h implementation in the way that it tolerates self-signed certificates on a server (usually useful during development).

Impact

Extend MQTT-C example

Testing

Tested on ESP32-S3, but connecting to custom MQTT provider service

@nuttxpr
Copy link

nuttxpr commented Oct 3, 2024

[Experimental Bot, please feedback here]

The PR summary is missing some key information:

  • Why is this change necessary? Just saying it "adds an option" isn't enough. Explain the benefits of using mbedTLS (e.g., enhanced security) and why this is a good addition to the example.
  • What functional part of the code is being changed? Be more specific than "Extend MQTT-C example." Which files are modified? Are new files added?
  • How does the change exactly work? Briefly describe how the mbedTLS option is integrated and used within the example.

Impact:

  • Impact on user: The PR description claims the example "may not work" with the provided broker. This needs clarification. Will users need to modify the code to connect to different brokers? If so, how? This needs to be clearly documented.
  • Impact on build: Does adding the mbedTLS option require any changes to the build system or configuration? This should be stated explicitly.
  • Impact on documentation: The description mentions that the code tolerates self-signed certificates, which is important for users. This, along with any other necessary usage instructions, needs to be added to the documentation.

Testing:

  • Insufficient information: The PR only mentions testing on ESP32-S3 with a custom MQTT provider. To be considered well-tested, it needs to be validated on other supported platforms and ideally with the provided "broker.hivemq.com" example.
  • Missing logs: The "Testing logs before change" and "Testing logs after change" sections are empty. Provide actual logs demonstrating the functionality before and after your changes.

In conclusion, this PR does not yet meet the NuttX requirements. You need to address the missing information and expand on the testing before it can be considered for merging.

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @pkarashchenko :-)

  • Can you please update also the documentation part so everyone knows that MQTT + TLS is supported?
  • What happens when self-signed certificate is in use? How can we control reaction to that situation? Shall we create a dedicated CONFIG variable to accept self-signed certificates during development? In general self-signed certificates on a production system indicates compromised channel so we should not allow that by default :-)

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @pkarashchenko :-)

Ci check is a GH worker problem.

Please consider adding documentation in a free moment, especially the self-signed certificates handling / warning / errors :-)

@pkarashchenko
Copy link
Contributor Author

Let me update the PR

@xiaoxiang781216
Copy link
Contributor

@pkarashchenko please rebase to the latest master to fix check error.

@pkarashchenko
Copy link
Contributor Author

@cederom I added CONFIG_EXAMPLES_MQTTC_ALLOW_UNVERIFIED_TLS option.

@pkarashchenko
Copy link
Contributor Author

Let's ignore error: Mixed case identifier found

@acassis
Copy link
Contributor

acassis commented Oct 26, 2024

@pkarashchenko please include a board profile, for example sim:mqtt_tls

@acassis acassis merged commit 7f7bbe2 into apache:master Oct 26, 2024
24 of 25 checks passed
@pkarashchenko
Copy link
Contributor Author

@pkarashchenko please include a board profile, for example sim:mqtt_tls

Will do. As well as a docs update

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

Successfully merging this pull request may close these issues.

5 participants