Add MQTT server authentication #1816
Conversation
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
I understood the issue to mean that it's not as much about making local MQTT broker secure by default, but about being able to connect even if somebody manually configures server authentication for their broker. If we were to handle handle TLS config for mosquitto in thin-edge, this would more effort required to come up with sensible defaults, making sure everything is secure, also letting people tweak the config, etc. I think just adding more configuration options for MQTT clients can be considered completely separately from adding options or changing current defaults of broker configuration. |
You understood correctly. I would even consider a bad idea to update from thin-edge the broker configuration. This configuration must stay owned by the device owner. |
Thanks for the clarification. |
| @@ -56,6 +60,9 @@ pub struct Config { | |||
| /// | |||
| /// Default: None | |||
| pub initial_message: Option<InitMessageFn>, | |||
|
|
|||
| /// TLS configuration used to connect to the broker. | |||
| pub tls_config: Option<ClientConfig>, | |||
There was a problem hiding this comment.
I would either rename the type or use rustls::ClienConfig as the current name is really vague.
There was a problem hiding this comment.
Ended up changing it to rustls::RootTrustStore
| @@ -27,6 +28,25 @@ pub fn create_tls_config( | |||
| .with_single_cert(cert_chain, pvt_key)?) | |||
| } | |||
|
|
|||
| pub fn create_tls_config_from_single_ca( | |||
There was a problem hiding this comment.
Why only a single CA? It's can be convenient to be given a directory of trusted CA.
There was a problem hiding this comment.
As described in #1816 (comment), will add an option for adding multiple CA.
There was a problem hiding this comment.
Replaced with functions that add certificates from a file and a directory respectively to the trust store.
| const KEY: &'static str = "mqtt.client.cafile"; | ||
|
|
||
| const DESCRIPTION: &'static str = concat!( | ||
| "Path to the CA certificate used by MQTT clients to use when ", |
There was a problem hiding this comment.
I would allow this path to be a directory of trusted certificates.
There was a problem hiding this comment.
I made it a path to a single certificate because when developing I had both CA certificate and server certificate in one directory, and didn't want to include both. But you're right that just 1 certificate might be too restrictive. Both MQTT clients that I used had both cafile and capath options, so perhaps I shall add mqtt.client.capath option?
There was a problem hiding this comment.
The usage is indeed to have two options, one for a ca file and one for a capath directory. However, I wonder if a single option can be used for both as the behavior can be adapted whether the path points to a file or a directory.
@reubenmiller waht do you think about these options?
There was a problem hiding this comment.
However, I wonder if a single option can be used for both as the behavior can be adapted whether the path points to a file or a directory.
Having thought about it, I think with two separate options it is clear from the invocation whether or not we use a single certificate, or multiple certificates. With one option, you would have to look in the filesystem whether or not this path is a file or a directory, making the invocation ambiguous. For this reason I think it would be better to have two options.
There was a problem hiding this comment.
Wrt. the capath option, I have a question. How do we decide which certificates do we read and add? Do we attempt to parse every file in the directory as a certificate? Do we filter by file extensions, like .pem or .crt?
Here's what two example MQTT clients do:
mosquitto_pub--capath Define the path to a directory containing PEM encoded CA certificates that are trusted. Used to enable SSL communication. For --capath to work correctly, the certificate files must have ".crt" as the file ending and you must run "openssl rehash <path to capath>" each time you add/remove a certificate.- MQTT CLI
It does not explain in the manpage, but it seems to include all files with extensions.pem,.cer, and.crt
There was a problem hiding this comment.
I think with two separate options it is clear from the invocation
By this, I meant "invocation" for CLI clients, and forgot that in the case of thin-edge, it is hidden inside configuration files. Still, having a distinction whether or not we use only one or multiple certificates, is what I would be in favour of keeping, even in a config file, unless there is some clear use case where one option that deals with both files and directories is superior.
There was a problem hiding this comment.
It makes senses to have these two options in the configuration file - as proposed by this PR.
| // Not sure if there is a better way to do this, unfortunately rumqttc error | ||
| // reporting is a bit awkward | ||
| impl From<rumqttc::TlsError> for ServerCertVerifyError { |
There was a problem hiding this comment.
Unfortunately, TLS error reporting is tedious in many crates. See
There was a problem hiding this comment.
Yes, the biggest issue is that if we connect via TLS, then all the errors come wrapped inside rumqttc::ConnectionError::Tls(..) even if the issue was not with TLS specifically. If it was a TLS error, there is another layer of rumqttc::TlsError::Io(..) despite the issue not being related to IO at all, and only then inside we have an actual error which is stringified.
This nesting results in the structure of the error just being the copy of rumqttc call stack. All the errors are wrapped in TlsError(..) becuase the errors are returned from a function like TlsClient::read(), which just wraps all the errors, and that's all the way down the stack.
This is I think an antipattern that I want to look into making sure we're not replicating in thin-edge after I complete this issue. Sure we're the only consumers of our APIs, so the impact is limited, but it's still there, and might cause bad error formatting for the users, if one's not careful when reporting errors.
There was a problem hiding this comment.
This is I think an antipattern that I want to look into making sure we're not replicating in thin-edge after I complete this issue. Sure we're the only consumers of our APIs, so the impact is limited, but it's still there, and might cause bad error formatting for the users, if one's not careful when reporting errors.
I would be really happy to have you working on such improvements.
| #[error( | ||
| "Received certificate is not valid for the name it is being validated for. | ||
| \nHint: Be sure to use a hostname specified in the CN field of the certificate and for the certificate to have a Subject Alt Name section." | ||
| )] |
There was a problem hiding this comment.
This error message is written from the point of view of the guy who created the broker certificate. It might be less useful for an end-user who used the wrong hostname to connect the broker.
| #[error( | |
| "Received certificate is not valid for the name it is being validated for. | |
| \nHint: Be sure to use a hostname specified in the CN field of the certificate and for the certificate to have a Subject Alt Name section." | |
| )] | |
| #[error( | |
| "The MQTT server certificate is not valid as not matching the server hostname. | |
| \nHint: Be sure to use a hostname specified in the CN field of the certificate or one the Subject Alt Name section." | |
| )] |
There was a problem hiding this comment.
One correction: We can only connect to a hostname if this hostname is present in subjectAltName section. Connecting to hostname specified in CN is not sufficient if it's not also in the subjectAltName section.
There was a problem hiding this comment.
Thanks for the clarification.
There was a problem hiding this comment.
Error message was reworded according to the recommendations and error structure was changed a bit.
| #[derive(thiserror::Error, Debug)] | ||
| pub enum ServerCertVerifyError { | ||
| /// Received certificate is not in X509v3 format | ||
| #[error("Received certificate is not in X.509 v3 format")] |
There was a problem hiding this comment.
What do you mean by received certificate?
| #[error("Received certificate is not in X.509 v3 format")] | |
| #[error("The MQTT server certificate is not in X.509 v3 format - as required by thin-edge")] |
There was a problem hiding this comment.
I meant "received from the other side of the connection", but you're right that it's a bit unclear. Will fix.
There was a problem hiding this comment.
Reworded this error message as well.
There was a problem hiding this comment.
It can be useful to link this file from the documentation.
There was a problem hiding this comment.
On which documentation page would you like to like to see this file linked?
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
|
Okay, I think I implemented most of the feedback, the only thing remaining is maybe making use of the created script for generating mosquitto server certificates by featuring it in the documentation. As for the client authentication that needs to be done as the other part of the linked issue, I think it should be done in another PR as the scope of the changes for server authentication is large enough. |
There was a problem hiding this comment.
A system test is failing because of a change introduced by this PR:
Invalid value "mqtt.client.cafile" for '<KEY>': Invalid key `mqtt.client.cafile'. See `tedge config list --doc` for a list of valid keys.
The following tests are failing, but I would not bother to fix them. They add really little value (checking that tedge mqtt returns a status of 1 when it fails to connect). So I would simply remove all these tests. Furthermore, robot framework would be more adapted for those tests that require a specific environment (with an MQTT broker up or down).
- tests::mqtt_sub_no_broker_running::none_expects
- tests::mqtt_sub_no_broker_running::some_0_expects
- tests::mqtt_sub_no_broker_running::some_1_expects
- tests::mqtt_sub_no_broker_running::some_2_expects
| file.path() | ||
| .extension() | ||
| .filter(|&extension| { | ||
| ["pem", "cer", "crt"] |
There was a problem hiding this comment.
This is okay. However, such a closed list of accepted names is always a source of surprises. For instance, I would expect to see cert but not cer.
There was a problem hiding this comment.
These 3 extensions are what MQTT CLI uses (mosquitto_pub/sub only allows .crt), but it's largely a matter of convention. More complete list of extensions can be found here, and maybe we could accept more extensions, but I don't think it's an issue as long as we properly document the extensions we accept, which I've just noticed I forgot about. Will add the list of accepted extensions to the description of relevant configuration options.
There was a problem hiding this comment.
Okay. So let's start with these 3 extensions.
| // TODO: should we keep trying to reconnect for all errors, or just | ||
| // if the broker isn't up and abort when e.g. we receive connection | ||
| // refused? | ||
| Err(err) => { | ||
| let err_msg = err.to_string(); | ||
| if err_msg.contains("I/O: Connection refused (os error 111)") { | ||
| return Err(MqttError::ServerConnection(err_msg)); | ||
| } |
There was a problem hiding this comment.
I'm okay with the current proposal (to keep trying reconnect whatever the connection error) as the aim of tedge mqtt is not to replace a full-fledged MQTT command line tool but just to provide a convenient way to interact with thin-edge leveraging the same configuration.
| const KEY: &'static str = "mqtt.client.cafile"; | ||
|
|
||
| const DESCRIPTION: &'static str = concat!( | ||
| "Path to the CA certificate used by MQTT clients to use when ", |
There was a problem hiding this comment.
It makes senses to have these two options in the configuration file - as proposed by this PR.
|
Fixed |
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Bravo555
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Bravo555
force-pushed
the
mqtt-auth
branch
2 times, most recently
from
a147ae5
to
9f2385e
Compare
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
This commit adds to our MQTT clients (tedge mqtt and mqtt_channel) the capability to connect to secure MQTT brokers via TLS. For the broker certificate to be successfully verified, this certificate has to be signed by a CA certificate, and the CA certificate needs to be trusted by the client. To select a CA trusted certificate, new configuration settings, `mqtt.client.ca_file` and `mqtt.client.ca_path` have been added. Additionally the broker certificate needs to be X.509 v3 and have a "X509v3 Subject Alternative Name" section containing its Common Name as a DNS name. This behaviour happens upstream in rumqttc crate, and was documented in [rumqtt#498]. [rumqtt#498]: bytebeamio/rumqtt#498 Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
These tests were flaky as they relied on mosquitto configuration not present in the tests and other things in the environment. For such integration testing we now use Robot Framework tests. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Bravo555
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Proposed changes
This commit adds to our MQTT clients (tedge mqtt and mqtt_channel) the capability to connect to secure MQTT brokers via TLS. For the broker certificate to be successfully verified, this certificate has to be signed by a CA certificate, and the CA certificate needs to be trusted by the client.
To select a CA trusted certificate, a new configuration setting,
mqtt.client.cafile, has been added.Additionally the broker certificate needs to be X.509 v3 and have a "X509v3 Subject Alternative Name" section containing its Common Name as a DNS name. This behaviour happens upstream in rumqttc crate, and was documented in rumqtt#498.
Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments
Did some minimal Robot framework tests to test connecting to a TLS broker. Should I maybe expand them or add unit tests? Also I haven't really updated any documentation, as I don't remember any part that would be made out of date by this PR. Also added a warning when attempting to connect via TLS without CA file and vice versa.