Add support for trustServerCertificate flag

[thankusWR]

Feature Request

Is your feature request related to a problem? Please describe

In mssql-jdbc library such option is simply available to developer but in r2dbc it needs some struggle (mostly finding out how to enable it) to use SSL with self signed cert.

Describe the solution you'd like

Add new configuration flag which would control trustServerCertificate parameter in underlying mssql-jdbc library.
Best to create new parameter in MssqlConnectionConfiguration which would allow using self signed cert.

Describe alternatives you've considered

Using generic ConnectionFactoryOptions with adding custom Option but that in fact calls Mssql factory which omits it.
After abnalyzing mssql-jdbc driver I found out that in case of using this flag it register dummy trust manager so I mimic that:
configBuilder.sslContextBuilderCustomizer(b -> b.trustManager(TrustAllTrustManager.INSTANCE));

Teachability, Documentation, Adoption, Migration Strategy

MssqlConnectionConfiguration.builder().trustServerCertificate();

[mp911de]

Makes sense. Do you want to submit a PR that introduces the config option to MssqlConnectionFactoryProvider, MssqlConnectionConfiguration, and the actual functionality before sslContextBuilderCustomizer gets called?

[thankusWR]

Hey, not sure if I would have time tio fully implement it but here is some simple implementation which might be sufficient:

diff --git a/src/main/java/io/r2dbc/mssql/MssqlConnectionConfiguration.java b/src/main/java/io/r2dbc/mssql/MssqlConnectionConfiguration.java
index 62df129..6222a35 100644
--- a/src/main/java/io/r2dbc/mssql/MssqlConnectionConfiguration.java
+++ b/src/main/java/io/r2dbc/mssql/MssqlConnectionConfiguration.java
@@ -92,6 +92,8 @@ public final class MssqlConnectionConfiguration {
 
     private final boolean ssl;
 
+    private final boolean trustServerCertificate;
+
     private final Function<SslContextBuilder, SslContextBuilder> sslContextBuilderCustomizer;
 
     @Nullable
@@ -113,7 +115,7 @@ public final class MssqlConnectionConfiguration {
     private final char[] trustStorePassword;
 
     private MssqlConnectionConfiguration(@Nullable String applicationName, @Nullable UUID connectionId, Duration connectTimeout, @Nullable String database, String host, String hostNameInCertificate,
-                                         CharSequence password, Predicate<String> preferCursoredExecution, int port, boolean sendStringParametersAsUnicode, boolean ssl,
+                                         CharSequence password, Predicate<String> preferCursoredExecution, int port, boolean sendStringParametersAsUnicode, boolean ssl, boolean trustServerCertificate,
                                          Function<SslContextBuilder, SslContextBuilder> sslContextBuilderCustomizer,
                                          @Nullable Function<SslContextBuilder, SslContextBuilder> sslTunnelSslContextBuilderCustomizer, boolean tcpKeepAlive, boolean tcpNoDelay,
                                          @Nullable File trustStore, @Nullable String trustStoreType,
@@ -130,6 +132,7 @@ public final class MssqlConnectionConfiguration {
         this.port = port;
         this.sendStringParametersAsUnicode = sendStringParametersAsUnicode;
         this.ssl = ssl;
+        this.trustServerCertificate = trustServerCertificate;
         this.sslContextBuilderCustomizer = sslContextBuilderCustomizer;
         this.sslTunnelSslContextBuilderCustomizer = sslTunnelSslContextBuilderCustomizer;
         this.tcpKeepAlive = tcpKeepAlive;
@@ -173,13 +176,14 @@ public final class MssqlConnectionConfiguration {
         }
 
         return new MssqlConnectionConfiguration(this.applicationName, this.connectionId, this.connectTimeout, this.database, redirectServerName, hostNameInCertificate, this.password,
-            this.preferCursoredExecution, redirect.getPort(), this.sendStringParametersAsUnicode, this.ssl, this.sslContextBuilderCustomizer, this.sslTunnelSslContextBuilderCustomizer,
-            this.tcpKeepAlive, this.tcpNoDelay, this.trustStore, this.trustStoreType, this.trustStorePassword, this.username);
+            this.preferCursoredExecution, redirect.getPort(), this.sendStringParametersAsUnicode, this.ssl, this.trustServerCertificate, this.sslContextBuilderCustomizer,
+                this.sslTunnelSslContextBuilderCustomizer, this.tcpKeepAlive, this.tcpNoDelay, this.trustStore, this.trustStoreType, this.trustStorePassword, this.username);
     }
 
     ClientConfiguration toClientConfiguration() {
-        return new DefaultClientConfiguration(this.connectTimeout, this.host, this.hostNameInCertificate, this.port, this.ssl, this.sslContextBuilderCustomizer,
-            this.sslTunnelSslContextBuilderCustomizer, this.tcpKeepAlive, this.tcpNoDelay, this.trustStore, this.trustStoreType, this.trustStorePassword);
+        return new DefaultClientConfiguration(this.connectTimeout, this.host, this.hostNameInCertificate, this.port, this.ssl, this.trustServerCertificate,
+                this.sslContextBuilderCustomizer, this.sslTunnelSslContextBuilderCustomizer, this.tcpKeepAlive, this.tcpNoDelay, this.trustStore, this.trustStoreType,
+                this.trustStorePassword);
     }
 
     ConnectionOptions toConnectionOptions() {
@@ -201,6 +205,7 @@ public final class MssqlConnectionConfiguration {
         sb.append(", port=").append(this.port);
         sb.append(", sendStringParametersAsUnicode=").append(this.sendStringParametersAsUnicode);
         sb.append(", ssl=").append(this.ssl);
+        sb.append(", trustServerCertificate=").append(this.trustServerCertificate);
         sb.append(", sslContextBuilderCustomizer=").append(this.sslContextBuilderCustomizer);
         sb.append(", sslTunnelSslContextBuilderCustomizer=").append(this.sslTunnelSslContextBuilderCustomizer);
         sb.append(", tcpKeepAlive=\"").append(this.tcpKeepAlive).append("\"");
@@ -344,6 +349,8 @@ public final class MssqlConnectionConfiguration {
 
         private boolean ssl;
 
+        private boolean trustServerCertificate;
+
         private Function<SslContextBuilder, SslContextBuilder> sslContextBuilderCustomizer = Function.identity();
 
         @Nullable
@@ -427,6 +434,16 @@ public final class MssqlConnectionConfiguration {
             return this;
         }
 
+        /**
+         * Allow using SSL when server uses self signed certificate.
+         *
+         * @return this {@link Builder}
+         */
+        public Builder trustServerCertificate() {
+            this.trustServerCertificate = true;
+            return this;
+        }
+
         /**
          * Enable SSL tunnel usage to encrypt all traffic right from the connect phase. This option is required when using a SSL tunnel (e.g. stunnel or other SSL terminator) in front of the SQL
          * server and it is not related to SQL Server's built-in SSL support.
@@ -653,7 +670,8 @@ public final class MssqlConnectionConfiguration {
             }
 
             return new MssqlConnectionConfiguration(this.applicationName, this.connectionId, this.connectTimeout, this.database, this.host, this.hostNameInCertificate, this.password,
-                this.preferCursoredExecution, this.port, this.sendStringParametersAsUnicode, this.ssl, this.sslContextBuilderCustomizer, this.sslTunnelSslContextBuilderCustomizer, tcpKeepAlive,
+                this.preferCursoredExecution, this.port, this.sendStringParametersAsUnicode, this.ssl, this.trustServerCertificate, this.sslContextBuilderCustomizer,
+                this.sslTunnelSslContextBuilderCustomizer, tcpKeepAlive,
                 tcpNoDelay, this.trustStore,
                 this.trustStoreType,
                 this.trustStorePassword, this.username);
@@ -673,6 +691,8 @@ public final class MssqlConnectionConfiguration {
 
         private final boolean ssl;
 
+        private final boolean trustServerCertificate;
+
         private final Function<SslContextBuilder, SslContextBuilder> sslContextBuilderCustomizer;
 
         @Nullable
@@ -692,7 +712,7 @@ public final class MssqlConnectionConfiguration {
         private final char[] trustStorePassword;
 
         DefaultClientConfiguration(Duration connectTimeout, String host, String hostNameInCertificate, int port, boolean ssl,
-                                   Function<SslContextBuilder, SslContextBuilder> sslContextBuilderCustomizer,
+                                   boolean trustServerCertificate, Function<SslContextBuilder, SslContextBuilder> sslContextBuilderCustomizer,
                                    @Nullable Function<SslContextBuilder, SslContextBuilder> sslTunnelSslContextBuilderCustomizer
             , boolean tcpKeepAlive, boolean tcpNoDelay, @Nullable File trustStore,
                                    @Nullable String trustStoreType, @Nullable char[] trustStorePassword) {
@@ -702,6 +722,7 @@ public final class MssqlConnectionConfiguration {
             this.hostNameInCertificate = hostNameInCertificate;
             this.port = port;
             this.ssl = ssl;
+            this.trustServerCertificate = trustServerCertificate;
             this.sslContextBuilderCustomizer = sslContextBuilderCustomizer;
             this.sslTunnelSslContextBuilderCustomizer = sslTunnelSslContextBuilderCustomizer;
             this.tcpKeepAlive = tcpKeepAlive;
@@ -758,7 +779,7 @@ public final class MssqlConnectionConfiguration {
             TrustManager[] trustManagers = tmf.getTrustManagers();
             TrustManager result;
 
-            if (isSslEnabled()) {
+            if (isSslEnabled() && !trustServerCertificate) {
                 result = new ExpectedHostnameX509TrustManager((X509TrustManager) trustManagers[0], this.hostNameInCertificate);
             } else {
                 result = TrustAllTrustManager.INSTANCE;

EDIT
diff
main...thankusWR:trustServerCertificate

[mp911de]

Looks a decent start. However, having diffs in comments makes it hard to review things properly. No worries regarding completion, moving that code into a pull request and adding a test including the connection factory provider change is fine.

[thankusWR]

Wanted to push changes to branch but have no access.
As for test do you have any proposition of how this could be tested?
I started some initial implementation, and try to extract trust manager from created MssqlConnectionConfiguration object but didn't find any way to verify proper behaviour.

[mp911de]

Pull requests work in the way that you fork a repository into your own namespace, apply and push the changes there and then you open a pull request so that we pull in your changes.

[thankusWR]

Ok, still do you have any idea how to test this change?

[mp911de]

I think we don't have tests where we have set SSL enabled because the certificate of the containerized SQL Server instance is self-signed. So enabling SSL and setting trustServerCertificate=true should be fine.

[thankusWR]

I submitted my PR (https://github.com/r2dbc/r2dbc-mssql/pull/186/files) with broken test to be sure its failing properly as locally test are not building for me but integration test are skipped.

2021-01-28T10:57:50.3935426Z [INFO] --- maven-failsafe-plugin:2.22.2:integration-test (default) @ r2dbc-mssql ---
2021-01-28T10:57:50.4587249Z [INFO] Tests are skipped.

Thats done on purpose?

[mp911de]

mvn clean verify should run integration tests which require a local Docker installation. On CI/GitHub Actions we've disabled integration tests as they cause tests to hang and are next to impossible to debug why this is.