Add MQTT client authentication #1864
Conversation
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
75116b9
to
1fa640e
Compare
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
1fa640e
to
6287258
Compare
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
There is a misunderstanding here. This would be a too strong constraint. See mosquitto documentation: This means that any CA certificates you include in cafile or capath will be able to issue client certificates that are valid for connecting to your broker. It means that to accept connections from some client, one must add in the mosquitto
These two settings are required but for another reason. One needs to use different certificates and signing authority to authenticate the device in the cloud and to authenticate a local process or child-device. |
You're right, CAs trusted by the broker and the clients can actually be different.
Okay, then |
| @@ -10,25 +10,45 @@ Force Tags theme:mqtt | |||
| *** Test Cases *** | |||
| Publish on a local insecure broker | |||
| Start Service mosquitto | |||
| Execute Command tedge mqtt pub topic message | |||
| Execute Command tedge mqtt pub topic1 message1 | |||
There was a problem hiding this comment.
I would add a check that the message is actually received - here and for the two other tests using authentication
There was a problem hiding this comment.
To verify that the message was received, I was thinking of starting tedge mqtt pub and tedge mqtt sub commands, but as tedge sub blocks, I don't really know how should I handle it using our ThinEdgeIO Robot Framework library. Could you give me some pointers? Because from the glimpse into the library, I don't see any way to start a background process on the test device.
There was a problem hiding this comment.
@Bravo555 Sorry I'm a bit late...There is a mqtt logger which is running in the integration test device where you can use the Should Have MQTT Messages keyword. The mqtt logger is running all the time, so no need to explicitly do a mqtt sub.
Here is an example:
@{listen}= Should Have MQTT Messages topic=tedge/${CHILD_SN}/commands/req/config_update date_from=-5s
Indeed, we start to have quite a lot of MQTT related options ! The two added by this PR make sense and are related to the former ones
The main issue is more that PS: Writing this comment, I noticed that something needs to be fixed: the client and external settings use the same names except an extra underscore ( |
I think there's one issue with I think that instead of starting with For the time being, I'll take the long road and just update all usages adding server and client authentication (in server authentication PR i updated |
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
6287258
to
c58c770
Compare
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
I would not be as negative as saying that "when we add something that changes the behaviour of the broker, all the clients using However, you are correct in saying that "any new MQTT configuration option impacts all the tedge services" and that the very same boilerplate code has to be added for each of them to extract the MQTT config from tedge config.
I agree that we need some |
There was a problem hiding this comment.
The changes on mqtt-channel to add client authentication support are correct.
The missing point is now to use this setting from all the tedge services. This might be an opportunity to add an impl From<TEdgeConfig> for mqtt_channel::Config as discussed here: #1864 (comment). I will let you choose the steps you prefer to go there.
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
c58c770
to
a48bdbc
Compare
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
|
In a48bdbc, I did a first try in extracting common MQTT configuration options into one place. Made it just another impl function, because only a part of the configuration is extracted, and it involves calling builder methods on Still, I'm not really satisfied with this, because by generating MQTT config from tedge config, now we need to pass |
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
a48bdbc
to
37cbc1d
Compare
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
37cbc1d
to
b90568f
Compare
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
This link is broken.
The function
Here, there is something I don't understand. Why I agree that constructing Also keep in mind that |
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
b90568f
to
34f0726
Compare
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
34f0726
to
31e6c77
Compare
| Custom Test Teardown | ||
|
|
||
| Check if a service using configured service type | ||
| [Arguments] ${service_name} | ||
| Execute Command tedge config set service.type thinedge | ||
| Custom Test Setup | ||
| ThinEdgeIO.Start Service ${service_name} | ||
| ThinEdgeIO.Restart Service ${service_name} |
There was a problem hiding this comment.
Why restart is needed here?, is it already started by the setup?
There was a problem hiding this comment.
The service starts when the connection to Cumulocity is not yet established, and fails with c8y_api::http_proxy: An error occurred while retrieving internal Id, operation will retry in 60 seconds and mapper will reinitialize. This error is mentioned earlier in the file, but bumping the timeout didn't really fix the problem. This did. If not having to restart the service is a hard requirement in this test, then I'll note it and be sure to find a solution that does not involve restarting, later.
There was a problem hiding this comment.
If not having to restart the service is a hard requirement in this test, then I'll note it and be sure to find a solution that does not involve restarting, later.
Restarting the service is okay.
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
41df15b
to
29ada96
Compare
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
|
So I think it's ready for review, core functionality is implemented, now just some potential tweaks:
|
|
I realised that I haven't updated any markdown documentation, but it would probably be good to inform users about this new functionality there. As such, one more commit with documentation will likely be coming. |
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
There was a problem hiding this comment.
Nicely and thoroughly done! Thank you! Approved with some comments.
settings naming: there's
mqtt.client.ca_filefor server authentication, butmqtt.client.auth.cert_file/key_filefor client authentication. All of them are related to authentication, so I think perhaps all of them should be undermqtt.client.auth. Additionally, forcert_file,key_filewe could remove_filesuffix, at for anybody familiar with authentication, it's evident that we only provide one cert-private key pair. Forca_file, we could perhaps do something else to eliminate the underscore.
- I would keep the same names as mosquitto e.g
certfileandkeyfile. - We also need to be consistent with the misc
mqtt.external.*settings.
config organisation: because of current config architecture, I had to include some additional boilerplate for checking invariants like whether or not if client certificate was included, was the private key also included and vice-versa. As described here I think this aspect of validation configuration states should be moved from config clients to config provider.
Indeed there is some boilerplate, but this okay for now.
RobotFramework tests: When I started working on this PR, I did a small RobotFramework suite
Tests.Mqtt.Basic Pub Subwhich 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 originalTests.Mqtt.Basic Pub Subsuite be removed? It's somewhat trivial and things it tests are already tested by the rest of the tests.
Are you speaking about these tests?
I would keep them even if redundant. Indeed there are simple and cover well the 3 levels of authentication.
| integration-test: | ||
| #!/usr/bin/env bash | ||
| ci/build_scripts/build.sh x86_64-unknown-linux-musl | ||
| cd tests/RobotFramework | ||
| source .venv/bin/activate | ||
| invoke build --local | ||
| invoke tests |
There was a problem hiding this comment.
It's really a good idea to add the integration tests here! Thanks.
| ```conf | ||
| listener 8883 | ||
| certfile PATH_TO_SERVER_CERTIFICATE | ||
| keyfile PATH_TO_SERVER_PRIVATE_KEY | ||
| ``` | ||
|
|
||
| - `listener 8883`: defines a new listener on port 8883 | ||
| - `certfile`: points to a certificate that will be used by the broker to | ||
| authenticate itself to connecting clients | ||
| - `keyfile`: points to a private key of the specified certificate, necessary for | ||
| encrypted communication |
There was a problem hiding this comment.
What is document here is correct. However, I would like to stress that preparing such a configuration file is the point of the mqtt.external.* settings of tedge config.
tedge config list --doc | grep mqtt.external
mqtt.external.port Mqtt broker port, which is used by the external mqtt clients to publish or subscribe. Example: 8883
mqtt.external.bind_address IP address / hostname, which the mqtt broker limits incoming connections on. Example: 0.0.0.0
mqtt.external.bind_interface Name of network interface, which the mqtt broker limits incoming connections on. Example: wlan0
mqtt.external.capath Path to a file containing the PEM encoded CA certificates that are trusted when checking incoming client certificates. Example: /etc/ssl/certsNote: If the capath is not set, then no certificates are required for the external connections.
mqtt.external.certfile Path to the certificate file, which is used by external MQTT listenerExample: /etc/tedge/device-certs/tedge-certificate.pemNote: This setting shall be used together with `mqtt.external.keyfile` for external connections.
mqtt.external.keyfile Path to the private key file, which is used by external MQTT listenerExample: /etc/tedge/device-certs/tedge-private-key.pemNote: This setting shall be used together with `mqtt.external.certfile` for external connections.
- See https://thin-edge.github.io/thin-edge.io/html/howto-guides/013_connect_external_device.html.
- @Bravo555 Change nothing for your current PR. In a follow-up task, we will have to assess if something need to be change for this
mqtt.externalsetting. - @reubenmiller I think this
mqtt.externalsetting is badly named making things confusing.
There was a problem hiding this comment.
@reubenmiller I think this mqtt.external setting is badly named making things confusing.
Yes I fully agree. It is really just a binding address, it does not matter if it is "internal" or "external"...but yes it can be done as a later stage.
|
|
||
| ## Generating certificates | ||
|
|
||
| You can use the following script to generate all required certificates: |
There was a problem hiding this comment.
@reubenmiller We can consider to extend the tedge cert create command to create such certificates.
| Custom Test Teardown | ||
|
|
||
| Check if a service using configured service type | ||
| [Arguments] ${service_name} | ||
| Execute Command tedge config set service.type thinedge | ||
| Custom Test Setup | ||
| ThinEdgeIO.Start Service ${service_name} | ||
| ThinEdgeIO.Restart Service ${service_name} |
There was a problem hiding this comment.
If not having to restart the service is a hard requirement in this test, then I'll note it and be sure to find a solution that does not involve restarting, later.
Restarting the service is okay.
|
Added fixups addressing review comments. Also renamed |
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
|
Thank you for the fixups. Please, rebase and squash the fixup commits. And I will merge this PR. |
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
307c9bb
to
8591542
Compare
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
|
@didier-wenzek squashed |
Unfortunately, two system tests are failing on a certificate issue. Cargo audit is also failing: did you rebase once your fix merged? |
My bad, forgot to rebase, will fix |
This commit adds 2 new config options: `mqtt.client.auth.certfile` and `mqtt.client.auth.keyfile` to facilitate MQTT client authentication. The client certificates need to be signed by a CA. To authorise these clients, the certificate of the CA which signed client certificates needs to be specified in mosquitto config with cafile or cadir options. Description of these options can be found in [mosquitto.conf]. These new settings were added because we need different certificates to identify a device on a remote broker, and to identify plugins or extensions on a local broker. Also rewrapped some doc comments to 80 columns in tedge_config_dto.rs [mosquitto.conf]: https://mosquitto.org/man/mosquitto-conf-5.html Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
This commit adds MQTT authentication support to the `mqtt_channel` crate used by different plugins to access the local MQTT broker, as well as changes the call sites in all the different plugins to start with MQTT config partially filled by settings related to the current broker, e.g. hostname, port, and authentication settings, which are the same for all the plugins. After loading the pre-filled config, the client is then free to provide settings for the client like subscriptions or last_will messages, or even to override previously set broker settings. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
8591542
to
a63936c
Compare
To make sure that all the plugins can work properly with server and client authentication enabled, the RobotFramework test suite setup was edited to create a secure mosquitto listener on port 8883 with server and client authentication enabled. This was achieved by adding --secure/--no-secure options to bootstrap.sh script, with --secure being the default. The default port 1883 listener was not removed because it's created by thin-edge with seemingly no easy way to disable editing mosquitto config at runtime, thus it would require more source code changes which I've decided are not worth it at the moment. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
There was a [bug] in version 0.5.11 of toml crate which caused serialization to fail if a serialized struct had fields that serialized to plain value keys after fields that serialized to tables (like vecs or hashmaps). In 0.7 this bug is solved and additionally `toml` crate is now a thin wrapper over `toml-edit` crate, just providing serde compatibility. `toml-edit` now is able to better edit toml without fully serializing it, potentially helping with the partial serialization problem described in this [comment]. [bug]: toml-rs/toml#356 [comment]: thin-edge#1812 (comment) Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Bravo555
force-pushed
the
mqtt-client-auth
branch
from
a63936c
to
023bce5
Compare
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Proposed changes
This commit adds 2 new config options:
mqtt.client.auth.cert_fileandmqtt.client.auth.key_fileto facilitate MQTT client authentication. The client certificate needs to be signed by the same CA as the server certificate for the authentication to work, that's why new settings were added instead of using a thin-edge device certificate located in /etc/tedge/device-certs, which is self-signed and adding the option of using CA totedge certcommand will not be trivial.Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments
Some more thought needs to be put into how to integrate client authentication with device certificates generated by the
tedge cert createcommand. The certificates generated by the command are currently self-signed, but for client authentication to work, both server and client need to use certificates signed by the same CA. A CA certificate is usually located on another device, to which the subject sends a CSR and receives back a signed certificate. So unless we want to keep it as it is now, where we use a certificate different from thin-edge device certificate, and which needs to be deployed by the user manually, thetedge certcommand would have to be heavily modified.