Implement hostnameValidated flag for secure MQTT connections (openhab…

…#2348)

Fixes openhab#2346

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

bundles/org.openhab.core.io.transport.mqtt/src/main/java/org/openhab/core/io/transport/mqtt/MqttBrokerConnection.java

@@ -57,6 +57,7 @@
  * @author David Graeff - All operations are async now. More flexible sslContextProvider and reconnectStrategy added.
  * @author Markus Rathgeb - added connection state callback
  * @author Jan N. Klug - changed from PAHO to HiveMQ client
+ * @author Mark Herwege - Added flag for hostname validation
  */
 @NonNullByDefault
 public class MqttBrokerConnection {
@@ -85,6 +86,7 @@ public enum MqttVersion {
     protected final String host;
     protected final int port;
     protected final boolean secure;
+    protected final boolean hostnameValidated;
     protected final MqttVersion mqttVersion;
 
     private @Nullable TrustManagerFactory trustManagerFactory = InsecureTrustManagerFactory.INSTANCE;
@@ -197,7 +199,25 @@ public CompletableFuture<Boolean> createFuture() {
      * @throws IllegalArgumentException If the client id or port is not valid.
      */
     public MqttBrokerConnection(String host, @Nullable Integer port, boolean secure, @Nullable String clientId) {
-        this(Protocol.TCP, MqttVersion.V3, host, port, secure, clientId);
+        this(host, port, secure, true, clientId);
+    }
+
+    /**
+     * Create a new TCP MQTT3 client connection to a server with the given host and port.
+     *
+     * @param host A host name or address
+     * @param port A port or null to select the default port for a secure or insecure connection
+     * @param secure A secure connection
+     * @param hostnameValidated Validate hostname from certificate against server hostname for secure connection
+     * @param clientId Client id. Each client on a MQTT server has a unique client id. Sometimes client ids are
+     *            used for access restriction implementations.
+     *            If none is specified, a default is generated. The client id cannot be longer than 65535
+     *            characters.
+     * @throws IllegalArgumentException If the client id or port is not valid.
+     */
+    public MqttBrokerConnection(String host, @Nullable Integer port, boolean secure, boolean hostnameValidated,
+            @Nullable String clientId) {
+        this(Protocol.TCP, MqttVersion.V3, host, port, secure, hostnameValidated, clientId);
     }
 
     /**
@@ -216,9 +236,30 @@ public MqttBrokerConnection(String host, @Nullable Integer port, boolean secure,
      */
     public MqttBrokerConnection(Protocol protocol, MqttVersion mqttVersion, String host, @Nullable Integer port,
             boolean secure, @Nullable String clientId) {
+        this(protocol, mqttVersion, host, port, secure, true, clientId);
+    }
+
+    /**
+     * Create a new MQTT client connection to a server with the given protocol, host and port.
+     *
+     * @param protocol The transport protocol
+     * @param mqttVersion The version of the MQTT client (v3 or v5)
+     * @param host A host name or address
+     * @param port A port or null to select the default port for a secure or insecure connection
+     * @param secure A secure connection
+     * @param hostnameValidated Validate hostname from certificate against server hostname for secure connection
+     * @param clientId Client id. Each client on a MQTT server has a unique client id. Sometimes client ids are
+     *            used for access restriction implementations.
+     *            If none is specified, a default is generated. The client id cannot be longer than 65535
+     *            characters.
+     * @throws IllegalArgumentException If the client id or port is not valid.
+     */
+    public MqttBrokerConnection(Protocol protocol, MqttVersion mqttVersion, String host, @Nullable Integer port,
+            boolean secure, boolean hostnameValidated, @Nullable String clientId) {
         this.protocol = protocol;
         this.host = host;
         this.secure = secure;
+        this.hostnameValidated = hostnameValidated;
         this.mqttVersion = mqttVersion;
         String newClientID = clientId;
         if (newClientID == null) {
@@ -320,6 +361,13 @@ public boolean isSecure() {
         return secure;
     }
 
+    /**
+     * Return true if hostname in certificate is validated against server hostname for secure connection
+     */
+    public boolean isHostnameValidated() {
+        return secure & hostnameValidated;
+    }
+
     /**
      * Set the optional user name and optional password to use when connecting to the MQTT broker.
      * The connection needs to be restarted for the new settings to take effect.
@@ -665,11 +713,11 @@ public CompletableFuture<Boolean> start() {
 
     protected MqttAsyncClientWrapper createClient() {
         if (mqttVersion == MqttVersion.V3) {
-            return new Mqtt3AsyncClientWrapper(host, port, clientId, protocol, secure, connectionCallback,
-                    trustManagerFactory);
+            return new Mqtt3AsyncClientWrapper(host, port, clientId, protocol, secure, hostnameValidated,
+                    connectionCallback, trustManagerFactory);
         } else {
-            return new Mqtt5AsyncClientWrapper(host, port, clientId, protocol, secure, connectionCallback,
-                    trustManagerFactory);
+            return new Mqtt5AsyncClientWrapper(host, port, clientId, protocol, secure, hostnameValidated,
+                    connectionCallback, trustManagerFactory);
         }
     }
 

bundles/org.openhab.core.io.transport.mqtt/src/main/java/org/openhab/core/io/transport/mqtt/MqttBrokerConnectionConfig.java

@@ -19,15 +19,17 @@
  * Contains configuration for a MqttBrokerConnection. Necessary to add a new broker connection the {@link MqttService}.
  *
  * @author David Graeff - Initial contribution
+ * @author Mark Herwege - Added flag for hostname validation
  */
 @NonNullByDefault
 public class MqttBrokerConnectionConfig {
     // Optional connection name
     public @Nullable String name;
-    // Connection parameters (host+port+secure)
+    // Connection parameters (host+port+secure+hostnameValidated)
     public @Nullable String host;
     public @Nullable Integer port;
     public boolean secure = true;
+    public boolean hostnameValidated = true;
     // Authentication parameters
     public @Nullable String username;
     public @Nullable String password;
@@ -68,7 +70,7 @@ public String getBrokerID() {
     }
 
     /**
-     * Output the name, host, port and secure flag
+     * Output the name, host, port, secure flag and hostname validation flag
      */
     @Override
     public String toString() {
@@ -88,6 +90,9 @@ public String toString() {
         if (secure) {
             b.append(":s");
         }
+        if (hostnameValidated) {
+            b.append(":v");
+        }
         return b.toString();
     }
 }

bundles/org.openhab.core.io.transport.mqtt/src/main/java/org/openhab/core/io/transport/mqtt/internal/MqttServiceImpl.java

@@ -90,7 +90,8 @@ public boolean addBrokerConnection(String brokerID, MqttBrokerConnection connect
             }
             String host = config.host;
             if (host != null && !host.isBlank()) {
-                connection = new MqttBrokerConnection(host, config.port, config.secure, config.clientID);
+                connection = new MqttBrokerConnection(host, config.port, config.secure, config.hostnameValidated,
+                        config.clientID);
                 brokerConnections.put(brokerID, connection);
             } else {
                 throw new ConfigurationException("host", "You need to provide a hostname/IP!");

bundles/org.openhab.core.io.transport.mqtt/src/main/java/org/openhab/core/io/transport/mqtt/internal/client/Mqtt3AsyncClientWrapper.java

@@ -37,21 +37,29 @@
  * The {@link Mqtt3AsyncClientWrapper} provides the wrapper for Mqttv3 async clients
  *
  * @author Jan N. Klug - Initial contribution
+ * @author Mark Herwege - Added flag for hostname validation
  */
 @NonNullByDefault
 public class Mqtt3AsyncClientWrapper extends MqttAsyncClientWrapper {
     private final Mqtt3AsyncClient client;
 
     public Mqtt3AsyncClientWrapper(String host, int port, String clientId, Protocol protocol, boolean secure,
-            ConnectionCallback connectionCallback, @Nullable TrustManagerFactory trustManagerFactory) {
+            boolean hostnameValidated, ConnectionCallback connectionCallback,
+            @Nullable TrustManagerFactory trustManagerFactory) {
         Mqtt3ClientBuilder clientBuilder = Mqtt3Client.builder().serverHost(host).serverPort(port).identifier(clientId)
                 .addConnectedListener(connectionCallback).addDisconnectedListener(connectionCallback);
 
         if (protocol == Protocol.WEBSOCKETS) {
             clientBuilder.webSocketWithDefaultConfig();
         }
         if (secure) {
-            clientBuilder.sslWithDefaultConfig().sslConfig().trustManagerFactory(trustManagerFactory).applySslConfig();
+            if (hostnameValidated) {
+                clientBuilder.sslWithDefaultConfig().sslConfig().trustManagerFactory(trustManagerFactory)
+                        .applySslConfig();
+            } else {
+                clientBuilder.sslWithDefaultConfig().sslConfig().trustManagerFactory(trustManagerFactory)
+                        .hostnameVerifier(this).applySslConfig();
+            }
         }
 
         client = clientBuilder.buildAsync();

bundles/org.openhab.core.io.transport.mqtt/src/main/java/org/openhab/core/io/transport/mqtt/internal/client/Mqtt5AsyncClientWrapper.java

@@ -38,21 +38,29 @@
  * The {@link Mqtt5AsyncClientWrapper} provides the wrapper for Mqttv5 async clients
  *
  * @author Jan N. Klug - Initial contribution
+ * @author Mark Herwege - Added flag for hostname validation
  */
 @NonNullByDefault
 public class Mqtt5AsyncClientWrapper extends MqttAsyncClientWrapper {
     private final Mqtt5AsyncClient client;
 
     public Mqtt5AsyncClientWrapper(String host, int port, String clientId, Protocol protocol, boolean secure,
-            ConnectionCallback connectionCallback, @Nullable TrustManagerFactory trustManagerFactory) {
+            boolean hostnameValidated, ConnectionCallback connectionCallback,
+            @Nullable TrustManagerFactory trustManagerFactory) {
         Mqtt5ClientBuilder clientBuilder = Mqtt5Client.builder().serverHost(host).serverPort(port).identifier(clientId)
                 .addConnectedListener(connectionCallback).addDisconnectedListener(connectionCallback);
 
         if (protocol == Protocol.WEBSOCKETS) {
             clientBuilder.webSocketWithDefaultConfig();
         }
         if (secure) {
-            clientBuilder.sslWithDefaultConfig().sslConfig().trustManagerFactory(trustManagerFactory).applySslConfig();
+            if (hostnameValidated) {
+                clientBuilder.sslWithDefaultConfig().sslConfig().trustManagerFactory(trustManagerFactory)
+                        .applySslConfig();
+            } else {
+                clientBuilder.sslWithDefaultConfig().sslConfig().trustManagerFactory(trustManagerFactory)
+                        .hostnameVerifier(this).applySslConfig();
+            }
         }
 
         client = clientBuilder.buildAsync();

bundles/org.openhab.core.io.transport.mqtt/src/main/java/org/openhab/core/io/transport/mqtt/internal/client/MqttAsyncClientWrapper.java

@@ -14,6 +14,9 @@
 
 import java.util.concurrent.CompletableFuture;
 
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLSession;
+
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.core.io.transport.mqtt.MqttWillAndTestament;
@@ -29,7 +32,7 @@
  */
 
 @NonNullByDefault
-public abstract class MqttAsyncClientWrapper {
+public abstract class MqttAsyncClientWrapper implements HostnameVerifier {
     /**
      * connect this client
      *
@@ -97,4 +100,9 @@ protected MqttQos getMqttQosFromInt(int qos) {
                 throw new IllegalArgumentException("QoS needs to be 0, 1 or 2.");
         }
     }
+
+    @Override
+    public boolean verify(@Nullable String hostname, @Nullable SSLSession sslSession) {
+        return true;
+    }
 }

bundles/org.openhab.core.io.transport.mqtt/src/main/resources/OH-INF/config/brokerConnectionInstance.xml

@@ -42,6 +42,12 @@
 			<description>A broker connection is either a non-secure TCP connection or a TLS secure connection.</description>
 			<default>false</default>
 		</parameter>
+		<parameter name="hostnameValidated" type="boolean" required="true" groupName="group_connection">
+			<label>Hostname Validated?</label>
+			<description>For a secure TLS connection, defines if the server hostname is validated against the hostname in the
+				certificate.</description>
+			<default>true</default>
+		</parameter>
 		<parameter name="username" type="text" groupName="group_connection">
 			<label>Broker Username</label>
 			<description>Broker username.</description>

bundles/org.openhab.core.io.transport.mqtt/src/main/resources/OH-INF/i18n/brokerConnectionInstance.properties

@@ -13,6 +13,8 @@ mqtt.config.systemBrokerConnectionInstance.port.label = Broker Port
 mqtt.config.systemBrokerConnectionInstance.port.description = A custom broker connection port. Leave empty to use the default MQTT ports for secure or non-secure connections.
 mqtt.config.systemBrokerConnectionInstance.secure.label = Secure Connection?
 mqtt.config.systemBrokerConnectionInstance.secure.description = A broker connection is either a non-secure TCP connection or a TLS secure connection.
+mqtt.config.systemBrokerConnectionInstance.hostnameValidated.label = Hostname Validated?
+mqtt.config.systemBrokerConnectionInstance.hostnameValidated.description = For a secure TLS connection, defines if the server hostname is validated against the hostname in the certificate.
 mqtt.config.systemBrokerConnectionInstance.username.label = Broker Username
 mqtt.config.systemBrokerConnectionInstance.username.description = Broker username.
 mqtt.config.systemBrokerConnectionInstance.password.label = Broker Password

bundles/org.openhab.core.io.transport.mqtt/src/test/java/org/openhab/core/io/transport/mqtt/MqttBrokerConnectionEx.java

@@ -45,8 +45,9 @@ public class MqttBrokerConnectionEx extends MqttBrokerConnection {
     public boolean connectSuccess = true;
     public boolean connectTimeout = false;
 
-    public MqttBrokerConnectionEx(String host, @Nullable Integer port, boolean secure, String clientId) {
-        super(host, port, secure, clientId);
+    public MqttBrokerConnectionEx(String host, @Nullable Integer port, boolean secure, boolean hostnameValidated,
+            String clientId) {
+        super(host, port, secure, hostnameValidated, clientId);
     }
 
     public Map<String, Subscription> getSubscribers() {

bundles/org.openhab.core.io.transport.mqtt/src/test/java/org/openhab/core/io/transport/mqtt/MqttBrokerConnectionTests.java

@@ -55,7 +55,7 @@ private static byte[] eqHelloBytes() {
     @Test
     public void subscribeBeforeOnlineThenConnect()
             throws ConfigurationException, MqttException, InterruptedException, ExecutionException, TimeoutException {
-        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false,
+        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false, false,
                 "MqttBrokerConnectionTests");
 
         // Add a subscriber while still offline
@@ -76,7 +76,7 @@ public void subscribeBeforeOnlineThenConnect()
     @Test
     public void subscribeToWildcardTopic()
             throws ConfigurationException, MqttException, InterruptedException, ExecutionException, TimeoutException {
-        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false,
+        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false, false,
                 "MqttBrokerConnectionTests");
 
         // Add a subscriber while still offline
@@ -107,7 +107,7 @@ public void subscribeToWildcardTopic()
     @Test
     public void subscriber()
             throws ConfigurationException, MqttException, InterruptedException, ExecutionException, TimeoutException {
-        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false,
+        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false, false,
                 "MqttBrokerConnectionTests");
 
         // Expect no subscribers
@@ -142,7 +142,7 @@ public void subscriber()
 
     @Test
     public void reconnectPolicyDefault() throws ConfigurationException, MqttException, InterruptedException {
-        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false,
+        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false, false,
                 "MqttBrokerConnectionTests");
 
         // Check if the default policy is set and that the broker within the policy is set.
@@ -155,7 +155,7 @@ public void reconnectPolicyDefault() throws ConfigurationException, MqttExceptio
     public void reconnectPolicy()
             throws ConfigurationException, MqttException, InterruptedException, ConfigurationException {
         MqttBrokerConnectionEx connection = spy(
-                new MqttBrokerConnectionEx("123.123.123.123", null, false, "MqttBrokerConnectionTests"));
+                new MqttBrokerConnectionEx("123.123.123.123", null, false, false, "MqttBrokerConnectionTests"));
         connection.setConnectionCallback(connection);
 
         // Check setter
@@ -188,7 +188,7 @@ public void reconnectPolicy()
     @Test
     public void timeoutWhenNotReachable() throws ConfigurationException, MqttException, InterruptedException {
         MqttBrokerConnectionEx connection = spy(
-                new MqttBrokerConnectionEx("10.0.10.10", null, false, "MqttBrokerConnectionTests"));
+                new MqttBrokerConnectionEx("10.0.10.10", null, false, false, "MqttBrokerConnectionTests"));
         connection.setConnectionCallback(connection);
 
         ScheduledThreadPoolExecutor s = new ScheduledThreadPoolExecutor(1);
@@ -222,7 +222,7 @@ public void connectionStateChanged(MqttConnectionState state, @Nullable Throwabl
     public void timeoutWhenNotReachableFuture()
             throws ConfigurationException, MqttException, InterruptedException, ExecutionException, TimeoutException {
         MqttBrokerConnectionEx connection = spy(
-                new MqttBrokerConnectionEx("10.0.10.10", null, false, "MqttBrokerConnectionTests"));
+                new MqttBrokerConnectionEx("10.0.10.10", null, false, false, "MqttBrokerConnectionTests"));
         connection.setConnectionCallback(connection);
 
         ScheduledThreadPoolExecutor s = new ScheduledThreadPoolExecutor(1);
@@ -244,7 +244,7 @@ public void timeoutWhenNotReachableFuture()
     @Test
     public void connectionObserver() throws ConfigurationException, MqttException {
         MqttBrokerConnectionEx connection = spy(
-                new MqttBrokerConnectionEx("123.123.123.123", null, false, "connectionObserver"));
+                new MqttBrokerConnectionEx("123.123.123.123", null, false, false, "connectionObserver"));
         connection.setConnectionCallback(connection);
 
         // Add an observer
@@ -275,7 +275,7 @@ public void connectionObserver() throws ConfigurationException, MqttException {
 
     @Test
     public void lastWillAndTestamentTests() throws ConfigurationException {
-        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false,
+        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false, false,
                 "lastWillAndTestamentTests");
 
         assertNull(connection.getLastWill());
@@ -295,17 +295,19 @@ public void lastWillAndTestamentConstructorTests() {
 
     @Test
     public void qosInvalid() throws ConfigurationException {
-        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false, "qosInvalid");
+        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false, false,
+                "qosInvalid");
         assertThrows(IllegalArgumentException.class, () -> connection.setQos(10));
     }
 
     @Test
     public void setterGetterTests() {
-        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false,
+        MqttBrokerConnectionEx connection = new MqttBrokerConnectionEx("123.123.123.123", null, false, false,
                 "setterGetterTests");
         assertEquals(connection.getHost(), "123.123.123.123", "URL getter");
         assertEquals(connection.getPort(), 1883, "Name getter"); // Check for non-secure port
         assertFalse(connection.isSecure(), "Secure getter");
+        assertFalse(connection.isHostnameValidated(), "HostnameValidated getter");
         assertEquals("setterGetterTests", connection.getClientId(), "ClientID getter");
 
         connection.setCredentials("user@!", "password123@^");
@@ -333,7 +335,7 @@ public void setterGetterTests() {
     public void gracefulStop()
             throws ConfigurationException, MqttException, InterruptedException, ExecutionException, TimeoutException {
         MqttBrokerConnectionEx connection = spy(
-                new MqttBrokerConnectionEx("123.123.123.123", null, false, "MqttBrokerConnectionTests"));
+                new MqttBrokerConnectionEx("123.123.123.123", null, false, false, "MqttBrokerConnectionTests"));
 
         assertTrue(connection.start().get(200, TimeUnit.MILLISECONDS));