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

Enable users to configure authentication for the mqtt clients used in each thin-edge component #1785

Closed
reubenmiller opened this issue Mar 2, 2023 · 4 comments
Assignees
Labels
improvement User value theme:mqtt Theme: mqtt and mosquitto related topics
Milestone

Comments

@reubenmiller
Copy link
Contributor

reubenmiller commented Mar 2, 2023

Is your feature improvement request related to a problem? Please describe.

If a user enables enforcement of authentication on the mosquitto MQTT broker used by thin-edge, then it currently results in the thin-edge components not being able to connect to the broker as the mqtt clients used by each component are not configurable.

This limitations prevents users from being able to enable authentication on their MQTT broker.

Describe the solution you'd like

Each component should configurable mqtt broker connection settings, for example the following should be configurable:

  • Select connection mode to MQTT broker (e.g. username/password or certificate based)
  • Username/password configuration (used when connecting via username/password authentication)
  • Certificate paths for private, public and ca certificates (used when connecting via cert base authentication)

Notes

For a first implementation there does not need to be one certificate per thin-edge.io component as the mosquitto MQTT broker settings can be configured using the following properties (which separates the identity/username/clientid)

use_identity_as_username true
use_username_as_clientid false

But in other cases where using thin-edge in a single-process per container setup, it allows users to use different certificate per device. Together with #1783, it should make it very flexible for users.

Describe alternatives you've considered

Avoid the effort supporting one property per component as the refactoring will be combining multiple services making there essentially only two mqtt clients (one for the mapper and one for the device management component)

Additional context

Related tickets

@reubenmiller reubenmiller added improvement User value theme:mqtt Theme: mqtt and mosquitto related topics labels Mar 2, 2023
@didier-wenzek
Copy link
Contributor

I fully agree @reubenmiller to avoid one specific set of connection properties per component (plugins/mappers/agent/child devices) in a shared configuration file - and this even without a plan to reduce the number of components. This would be a burden to manage. Furthermore, the connection code must be the same independently on how the components are deployed.

So I see this ticket as follow-up task for #1773,
adding the following settings to tedge.conf to be used by plugins/mappers/agent to connect the broker:

  • mqtt.client.capath path to a file or directory with trusted certificates used to validate the broker certificate
  • mqtt.client.cert path to the certificate of the client
  • mqtt.client.key path to the private key of the client
  • I would avoid password identification.

@reubenmiller reubenmiller added this to the 0.10.0 milestone Mar 8, 2023
@Bravo555
Copy link
Contributor

This ticket is made of 2 parts: server authentication and client authentication. This comment will be about server authentication, which is arguably easier of the two.

Server authentication

Requirements

  1. A broker MUST use a certificate signed by a CA certificate.
  2. A client MUST know the CA certificate used to verify the broker.
  3. A client MUST connect to the broker using a hostname that is the same as in CN record of the certificate.
  4. A client connecting to port 1883 SHOULD NOT attempt TLS communication, and vice versa, client connecting to port 8883 SHOULD NOT attempt plaintext communication. We should probably enforce this. In case of any other port do we keep TCP transport as the default?

Steps

  1. Existing API of mqtt_channel will have to be changed to support connecting via TLS.
  2. For testing the TLS codepath, we'll have to handle creating certificate for test brokers. As the TLS codepath invokes a plaintext codepath and TLS is plenty fast, perhaps we should only test the TLS codepath and use TLS as much as possible internally, unless using TLS is not widespread enough in the deployments that we target (LAN and container networks) to be a first-class default.

@didier-wenzek
Copy link
Contributor

@gligorisaev: I let you assess/improve the coverage of the tests written by @Bravo555.

From @Bravo555, own words:

When I started working on this PR, I did a small RobotFramework suite Tests.Mqtt.Basic Pub Sub which verified all the MQTT clients work correctly with each: no authentication, server authentication, server + client authentication. Then, because we ideally want to test all clients, I've edited the test suite so all the tests work with server +client authentication, testing the largest possible surface. Now that all the tests work with authentication, should the original Tests.Mqtt.Basic Pub Sub suite be removed? It's somewhat trivial and things it tests are already tested by the rest of the tests.

The feature to be tested is documented by the PR itself: https://github.com/thin-edge/thin-edge.io/pull/1864/files#diff-c6383548a956d044a2724ebabbf7179ce4ff307898d505a0b2ed699fda24eb5f

@gligorisaev
Copy link
Contributor

If @reubenmiller agrees I would not remove this test, it should be excluded from the pipeline not to be executed everytime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement User value theme:mqtt Theme: mqtt and mosquitto related topics
Projects
None yet
Development

No branches or pull requests

4 participants