thin-edge / thin-edge.io

The open edge framework for lightweight IoT devices
https://thin-edge.io
Apache License 2.0
221 stars 54 forks source link

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

Closed reubenmiller closed 1 year ago

reubenmiller commented 1 year ago

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:

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

didier-wenzek commented 1 year ago

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 https://github.com/thin-edge/thin-edge.io/issues/1773, adding the following settings to tedge.conf to be used by plugins/mappers/agent to connect the broker:

Bravo555 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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