espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
591 stars 254 forks source link

Added support to set server common name. (IDFGH-10225) #254

Closed Erlkoenig90 closed 1 year ago

Erlkoenig90 commented 1 year ago

I would like to suggest adding the possibility to configure the server TLS certificate's expected common_name. This would allow us to connect to a server where the hostname differs from the CN of the server certificate, or if DNS is not available and we connect via IP address but know the server's hostname/CN.

For example, if both a server's hostname and its CN is server, but we can't resolve its hostname via DNS and connect via .uri = "mqtts://192.168.2.123" we get a TLS error and connection failure. If we can manually set the expected CN to server the connection succeeds. Setting skip_cert_common_name_check=true works too but reduces security.

This has already been implemented for the http_client. This pull request does exactly the same thing for MQTT. This requires using the function esp_transport_ssl_set_common_name, which has been added on IDF master. Therefore, the pull request checks for IDF version v5.1.0 for enabling the feature.

I added a field common_name for esp_mqtt_client_config_t::broker.verification. If it is not NULL, it will be passed to esp_transport_ssl_set_common_name, analog to skip_cert_common_name_check and esp_transport_ssl_skip_common_name_check.

Usage is:

esp_mqtt_client_config_t {
    .broker = {
        .address = {
             .uri =  "mqtts://192.168.2.123" // If the server doesn't have a DNS entry, we can't pass "mqtts://server" but have to use the IP address
        },
        .verification = {
            .skip_cert_common_name_check = false, // We want to avoid setting this to true
            .common_name = "server"
        }
    },

If common_name is set to NULL, behavior is as before (API backwards compatible).

Thanks for looking into this!

euripedesrocha commented 1 year ago

Closing this since the PR was merged in 6195762d28a61b627297ee1b4d77fa13adc2742a