BlackZork / mqmgateway

MQTT gateway for modbus networks
GNU Affero General Public License v3.0
42 stars 18 forks source link

Add TLS support for MQTT #60

Closed git-developer closed 1 month ago

git-developer commented 1 month ago

This PR adds basic TLS support for MQTT as requested in https://github.com/BlackZork/mqmgateway/discussions/51.

Supported Features

The only supported feature is secure communication between application and broker. The application needs to know a trusted CA certificate for this. Users may choose between one of the following options:

  1. Add an empty mqtt.broker.tls option to trust all OS provided certificates (usually located in /etc/ssl/certs)
    mqtt:
      client_id: mqm-tls
      broker:
        host: broker
        tls:
  2. Add the path to a CA file in option mqtt.broker.tls.cafile:
    mqtt:
      client_id: mqm-tls
      broker:
        host: broker
        tls:
          cafile: /usr/local/lib/ca-certificates/ca.crt

Try it

A demo project is attached where functionality can be tested. It boots a container for the application and a second container for mosquitto, listening via TLS on default port 8883. Certificates are part of the demo project. To try it:

  1. Unpack and enter the demo project: tar -xvzf mqm-tls.tar.gz && cd mqm-tls
  2. Build a local image that contains demo certificates: docker compose build
  3. Configure the modbus settings in config.yaml
  4. Start the containers: docker compose up -d
  5. Check that everything is up and running: docker compose logs
  6. Subscribe via TLS to the broker and receive messages: docker compose exec broker mosquitto_sub -v -L mqtts://broker/# --cafile /mosquitto/certs/ca.crt

Resources

git-developer commented 1 month ago

This PR is now ready for review. What I noticed so far:

BlackZork commented 1 month ago

Many thanks for this contribution!

One of my rules for unit tests is that I do not test dependent libraries. Mosquitto handles TLS, so we do not need test it here. I think it would be good to add at least one simple test that checks if mosquitto_tls_set is called and a default port is set correctly - for regression.

Looking at the code I realized that mosquitto api is not mocked, only IMqttImpl interface is. So you could just check if a MqttBrokerConfig has expected values in the MockedMqttImpl::connect(). I would add it in new mqtt_config_tests.cpp file just like config tests are done for modbus section in modbus_config_tests.cpp

I think that no read access to cafile or certificate will be a common configuration problem. I would add a read test for those files in MqttBrokerConfig constructor and throw ConfigurationException with a better message. You could add a second test for it in mqtt_config_tests.cpp - passing an non existing file path for example. I usually do not assert error message, only initOk() from mocked server. It is still helpful to see what message is generated from test in log file.

git-developer commented 1 month ago

Thanks for your feedback. I added checks and tests as suggested.