Make TLS/SSL version upgrade handling OPTIN for OnPremise (#631)

Co-authored-by: Alexander Dümont <[email protected]>
Co-authored-by: Matthias Kuhr <[email protected]>

cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5FactoryBuilder.java

@@ -10,6 +10,8 @@
 
 import org.apache.hc.client5.http.classic.HttpClient;
 
+import com.google.common.annotations.Beta;
+
 /**
  * Builder class for a default implementation of the {@link ApacheHttpClient5Factory} interface.
  *
@@ -19,9 +21,32 @@ public class ApacheHttpClient5FactoryBuilder
 {
     @Nonnull
     private Duration timeout = DefaultApacheHttpClient5Factory.DEFAULT_TIMEOUT;
+    private TlsUpgrade tlsUpgrade = TlsUpgrade.AUTOMATIC;
     private int maxConnectionsTotal = DefaultApacheHttpClient5Factory.DEFAULT_MAX_CONNECTIONS_TOTAL;
     private int maxConnectionsPerRoute = DefaultApacheHttpClient5Factory.DEFAULT_MAX_CONNECTIONS_PER_ROUTE;
 
+    /**
+     * Enum to control the automatic TLS upgrade feature for insecure connections.
+     *
+     * @since 5.14.0
+     */
+    @Beta
+    public enum TlsUpgrade
+    {
+        /**
+         * Automatic TLS upgrade is enabled.
+         */
+        ENABLED,
+        /**
+         * Automatic TLS upgrade is disabled.
+         */
+        DISABLED,
+        /**
+         * Automatic TLS upgrade is enabled only for {@link ProxyType#INTERNET}, default.
+         */
+        AUTOMATIC;
+    }
+
     /**
      * Sets the timeout {@link HttpClient} instances created by the to-be-built {@link ApacheHttpClient5Factory} should
      * use. This timeout applies to the following concerns:
@@ -89,6 +114,20 @@ public ApacheHttpClient5FactoryBuilder maxConnectionsTotal( final int maxConnect
         return this;
     }
 
+    /**
+     * Sets the automatic TLS upgrade strategy. This strategy controls whether insecure connections should be
+     * automatically upgraded.
+     *
+     * @since 5.14.0
+     */
+    @Beta
+    @Nonnull
+    public ApacheHttpClient5FactoryBuilder tlsUpgrade( @Nonnull final TlsUpgrade tlsUpgrade )
+    {
+        this.tlsUpgrade = tlsUpgrade;
+        return this;
+    }
+
     /**
      * Sets the maximum number of parallel connections <b>per route</b> (e.g. per remote host) that can be established
      * with a {@link HttpClient} created by the to-be-built {@link ApacheHttpClient5Factory}.
@@ -116,6 +155,11 @@ public ApacheHttpClient5FactoryBuilder maxConnectionsPerRoute( final int maxConn
     @Nonnull
     public ApacheHttpClient5Factory build()
     {
-        return new DefaultApacheHttpClient5Factory(timeout, maxConnectionsTotal, maxConnectionsPerRoute, null);
+        return new DefaultApacheHttpClient5Factory(
+            timeout,
+            maxConnectionsTotal,
+            maxConnectionsPerRoute,
+            null,
+            tlsUpgrade);
     }
 }

cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5Factory.java

@@ -50,28 +50,25 @@ class DefaultApacheHttpClient5Factory implements ApacheHttpClient5Factory
     private final Timeout timeout;
     private final int maxConnectionsTotal;
     private final int maxConnectionsPerRoute;
-    // for testing purposes
+
     @Nullable
     private final HttpRequestInterceptor requestInterceptor;
 
-    DefaultApacheHttpClient5Factory(
-        @Nonnull final Duration timeout,
-        final int maxConnectionsTotal,
-        final int maxConnectionsPerRoute )
-    {
-        this(timeout, maxConnectionsTotal, maxConnectionsPerRoute, null);
-    }
+    @Nonnull
+    private final ApacheHttpClient5FactoryBuilder.TlsUpgrade tlsUpgrade;
 
     DefaultApacheHttpClient5Factory(
         @Nonnull final Duration timeout,
         final int maxConnectionsTotal,
         final int maxConnectionsPerRoute,
-        @Nullable final HttpRequestInterceptor requestInterceptor )
+        @Nullable final HttpRequestInterceptor requestInterceptor,
+        @Nonnull final ApacheHttpClient5FactoryBuilder.TlsUpgrade tlsUpgrade )
     {
         this.timeout = toTimeout(timeout);
         this.maxConnectionsTotal = maxConnectionsTotal;
         this.maxConnectionsPerRoute = maxConnectionsPerRoute;
         this.requestInterceptor = requestInterceptor;
+        this.tlsUpgrade = tlsUpgrade;
     }
 
     @Nonnull
@@ -95,7 +92,7 @@ private CloseableHttpClient buildHttpClient( @Nullable final HttpDestinationProp
             HttpClients
                 .custom()
                 .setConnectionManager(getConnectionManager(destination))
-                .setDefaultRequestConfig(getRequestConfig())
+                .setDefaultRequestConfig(getRequestConfig(destination))
                 .setProxy(getProxy(destination));
 
         if( requestInterceptor != null ) {
@@ -162,9 +159,30 @@ private HostnameVerifier getHostnameVerifier( final HttpDestinationProperties de
     }
 
     @Nonnull
-    private RequestConfig getRequestConfig()
+    private RequestConfig getRequestConfig( @Nullable final HttpDestinationProperties destination )
+    {
+        return RequestConfig
+            .custom()
+            .setProtocolUpgradeEnabled(isProtocolUpgradeEnabled(destination))
+            .setConnectionRequestTimeout(timeout)
+            .build();
+    }
+
+    private boolean isProtocolUpgradeEnabled( @Nullable final HttpDestinationProperties destination )
     {
-        return RequestConfig.custom().setConnectionRequestTimeout(timeout).build();
+        return switch( tlsUpgrade ) {
+            case ENABLED -> true;
+            case DISABLED -> false;
+            case AUTOMATIC -> {
+                if( destination == null ) {
+                    yield true;
+                }
+                if( destination.getTlsVersion().isDefined() ) {
+                    yield false;
+                }
+                yield !destination.getProxyType().contains(ProxyType.ON_PREMISE);
+            }
+        };
     }
 
     @Nullable

cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5FactoryBuilderTest.java

@@ -4,8 +4,19 @@
 
 package com.sap.cloud.sdk.cloudplatform.connectivity;
 
+import static com.sap.cloud.sdk.cloudplatform.connectivity.ApacheHttpClient5FactoryBuilder.TlsUpgrade.DISABLED;
+import static com.sap.cloud.sdk.cloudplatform.connectivity.ApacheHttpClient5FactoryBuilder.TlsUpgrade.ENABLED;
+import static com.sap.cloud.sdk.cloudplatform.connectivity.DefaultHttpDestination.builder;
+import static com.sap.cloud.sdk.cloudplatform.connectivity.ProxyType.INTERNET;
+import static com.sap.cloud.sdk.cloudplatform.connectivity.ProxyType.ON_PREMISE;
+import static org.assertj.core.api.Assertions.as;
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatNoException;
+import static org.assertj.core.api.InstanceOfAssertFactories.type;
 
+import javax.annotation.Nonnull;
+
+import org.apache.hc.client5.http.config.Configurable;
 import org.junit.jupiter.api.Test;
 
 class ApacheHttpClient5FactoryBuilderTest
@@ -17,4 +28,59 @@ void testBuilderContainsOptionalParametersOnly()
         // make sure we can build a new factory instance without supplying any parameters
         assertThatNoException().isThrownBy(() -> new ApacheHttpClient5FactoryBuilder().build());
     }
+
+    @Test
+    void testTlsUpgradeToggle()
+    {
+        var service = "https://servive";
+        var proxy = ProxyConfiguration.of("https://proxy");
+
+        var destInternet = builder(service).trustAllCertificates().build();
+        var destOnPremise = builder(service).proxyType(ON_PREMISE).proxyConfiguration(proxy).buildInternal();
+        var destProxy = builder(service).trustAllCertificates().proxyType(INTERNET).proxyConfiguration(proxy).build();
+        var destTlsVersion = builder(service).trustAllCertificates().tlsVersion("TLSv1.1").build();
+
+        ApacheHttpClient5Factory sut;
+
+        // force upgrade=true
+        sut = new ApacheHttpClient5FactoryBuilder().tlsUpgrade(ENABLED).build();
+        assertProtocolUpgradeEnabled(sut, destInternet);
+        assertProtocolUpgradeEnabled(sut, destOnPremise);
+        assertProtocolUpgradeEnabled(sut, destProxy);
+        assertProtocolUpgradeEnabled(sut, destTlsVersion);
+
+        // force upgrade=false
+        sut = new ApacheHttpClient5FactoryBuilder().tlsUpgrade(DISABLED).build();
+        assertProtocolUpgradeDisabled(sut, destInternet);
+        assertProtocolUpgradeDisabled(sut, destOnPremise);
+        assertProtocolUpgradeDisabled(sut, destProxy);
+        assertProtocolUpgradeDisabled(sut, destTlsVersion);
+
+        // default
+        sut = new ApacheHttpClient5FactoryBuilder().build();
+        assertProtocolUpgradeEnabled(sut, destInternet);
+        assertProtocolUpgradeDisabled(sut, destOnPremise);
+        assertProtocolUpgradeEnabled(sut, destProxy);
+        assertProtocolUpgradeDisabled(sut, destTlsVersion);
+    }
+
+    private void assertProtocolUpgradeEnabled(
+        @Nonnull final ApacheHttpClient5Factory factory,
+        @Nonnull final DefaultHttpDestination dest )
+    {
+        assertThat(factory.createHttpClient(dest))
+            .isNotNull()
+            .extracting("httpClient", as(type(Configurable.class)))
+            .satisfies(client -> assertThat(client.getConfig().isProtocolUpgradeEnabled()).isTrue());
+    }
+
+    private void assertProtocolUpgradeDisabled(
+        @Nonnull final ApacheHttpClient5Factory factory,
+        @Nonnull final DefaultHttpDestination dest )
+    {
+        assertThat(factory.createHttpClient(dest))
+            .isNotNull()
+            .extracting("httpClient", as(type(Configurable.class)))
+            .satisfies(client -> assertThat(client.getConfig().isProtocolUpgradeEnabled()).isFalse());
+    }
 }

cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5CacheTest.java

@@ -52,7 +52,9 @@ class DefaultApacheHttpClient5CacheTest
         new DefaultApacheHttpClient5Factory(
             DefaultApacheHttpClient5Factory.DEFAULT_TIMEOUT,
             DefaultApacheHttpClient5Factory.DEFAULT_MAX_CONNECTIONS_TOTAL,
-            DefaultApacheHttpClient5Factory.DEFAULT_MAX_CONNECTIONS_PER_ROUTE);
+            DefaultApacheHttpClient5Factory.DEFAULT_MAX_CONNECTIONS_PER_ROUTE,
+            null,
+            ApacheHttpClient5FactoryBuilder.TlsUpgrade.AUTOMATIC);
     private static final long NANOSECONDS_IN_MINUTE = 60_000_000_000L;
     private static final Duration TEN_MINUTES = Duration.ofMinutes(10L);
 

cloudplatform/connectivity-apache-httpclient5/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultApacheHttpClient5FactoryTest.java

@@ -9,6 +9,7 @@
 import static com.github.tomakehurst.wiremock.client.WireMock.ok;
 import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
 import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
+import static com.sap.cloud.sdk.cloudplatform.connectivity.ApacheHttpClient5FactoryBuilder.TlsUpgrade.AUTOMATIC;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
@@ -87,7 +88,8 @@ void setup()
                 CLIENT_TIMEOUT,
                 MAX_CONNECTIONS,
                 MAX_CONNECTIONS_PER_ROUTE,
-                requestInterceptor);
+                requestInterceptor,
+                AUTOMATIC);
     }
 
     @Test
@@ -101,14 +103,16 @@ void testHttpClientUsesTimeout()
                 Duration.ofSeconds(3L),
                 MAX_CONNECTIONS,
                 MAX_CONNECTIONS_PER_ROUTE,
-                requestInterceptor);
+                requestInterceptor,
+                AUTOMATIC);
 
         final ApacheHttpClient5Factory factoryWithEnoughTimeout =
             new DefaultApacheHttpClient5Factory(
                 Duration.ofSeconds(7L),
                 MAX_CONNECTIONS,
                 MAX_CONNECTIONS_PER_ROUTE,
-                requestInterceptor);
+                requestInterceptor,
+                AUTOMATIC);
 
         final ClassicHttpRequest request = new HttpGet(WIRE_MOCK_SERVER.url("/timeout"));
 
@@ -136,7 +140,8 @@ void testHttpClientUsesMaxConnections()
                 Duration.ofSeconds(3L), // this timeout is also used for the connection lease
                 1,
                 MAX_CONNECTIONS_PER_ROUTE,
-                requestInterceptor);
+                requestInterceptor,
+                AUTOMATIC);
 
         final HttpClient client = sut.createHttpClient();
         final ClassicHttpRequest firstRequest = new HttpGet(WIRE_MOCK_SERVER.url("/max-connections-1"));
@@ -158,7 +163,8 @@ void testHttpClientUsesMaxConnectionsPerRoute()
                 Duration.ofSeconds(3L), // this timeout is also used for the connection lease
                 MAX_CONNECTIONS,
                 1,
-                requestInterceptor);
+                requestInterceptor,
+                AUTOMATIC);
 
         final ClassicHttpRequest firstRequest = new HttpGet(WIRE_MOCK_SERVER.url("/max-connections-per-route"));
         final ClassicHttpRequest secondRequest = new HttpGet(SECOND_WIRE_MOCK_SERVER.url("/max-connections-per-route"));

release_notes.md

@@ -17,6 +17,7 @@
 ### 📈 Improvements
 
 - Minor improvement on OpenAPI code generator to apply JavaDoc on customized model class constructors.
+- Fix a TLS compatibility issue between the latest _Apache HttpClient 5_ and on-premise connectivity (via SAP Cloud Connector).
 - Stabilize ApacheHttpClient5 related API without changes.
   The `@Beta` annotations are removed in most places and consuming applications no longer need to suppress warnings.