Skip to content

Replace deprecated SSLConnectionSocketFactory with recommended API #6281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Jul 18, 2025

Motivation and Context

Apache HttpClient 5.x deprecated ConnectionSocketFactory in favor of the new TlsSocketStrategy interface. This change updates the AWS SDK's Apache5 HTTP client to fully adopt the modern TLS configuration approach, completing the migration by removing the deprecated API entirely.

Historical Context: Earlier versions of this implementation incorrectly used SSLConnectionSocketFactory instead of the more general ConnectionSocketFactory interface. The correct API should have been ConnectionSocketFactory, which was consistent with the SDK's Apache4 client implementation. However, Apache HttpClient 5.x has since deprecated ConnectionSocketFactory entirely as part of a broader architectural redesign to better separate concerns between socket creation and TLS upgrade operations.

The new TlsSocketStrategy interface provides a cleaner abstraction specifically for TLS upgrade operations, moving away from the socket factory pattern that mixed plain socket creation with TLS layering concerns.

Modifications

Added

  • New tlsSocketStrategy() method in Apache5HttpClient.Builder to support the modern TLS configuration approach

Modified

  • SdkTlsSocketFactory now extends DefaultClientTlsStrategy instead of SSLConnectionSocketFactory
  • Connection manager creation updated to use setTlsSocketStrategy() instead of deprecated setSSLSocketFactory()
  • Socket initialization updated to use initializeSocket() instead of prepareSocket()
  • Method signatures updated to use upgrade() for TLS socket upgrades instead of connectSocket()

Removed

  • Deprecated socketFactory() method - completely removed to eliminate legacy API surface
  • Legacy socket wrapper classes (DelegateSocket, SdkSocket) - replaced with SSL-specific SdkSslSocket
  • Backward compatibility adapter - no longer needed as deprecated API is fully removed

Breaking Changes

  • socketFactory() method removed: Users must migrate to tlsSocketStrategy()
  • Legacy socket factory support discontinued: Only TlsSocketStrategy is supported

Migration Path

Users must migrate from the deprecated approach to the modern API:

Before (Deprecated API):

import org.apache.hc.client5.http.socket.ConnectionSocketFactory;
import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory;
import org.apache.hc.client5.http.ssl.DefaultHostnameVerifier;

// OLD: Using ConnectionSocketFactory (no longer supported)
ConnectionSocketFactory sslSocketFactory = new SSLConnectionSocketFactory(
    customSslContext,
    new String[] {"TLSv1.2", "TLSv1.3"}, // Supported protocols
    null, // Default cipher suites
    new DefaultHostnameVerifier()
);

Apache5HttpClient httpClient = Apache5HttpClient.builder()
    .socketFactory(sslSocketFactory) // ❌ Method removed
    .build();

After (Modern API):

import org.apache.hc.client5.http.ssl.TlsSocketStrategy;
import org.apache.hc.client5.http.ssl.DefaultClientTlsStrategy;
import org.apache.hc.client5.http.ssl.DefaultHostnameVerifier;
import org.apache.hc.core5.reactor.ssl.SSLBufferMode;

// NEW: Using TlsSocketStrategy (required)
TlsSocketStrategy tlsSocketStrategy = new DefaultClientTlsStrategy(
    customSslContext,
    new String[] {"TLSv1.2", "TLSv1.3"}, // Supported protocols
    null, // Default cipher suites
    SSLBufferMode.STATIC, // SSL buffer mode
    new DefaultHostnameVerifier()
);

Apache5HttpClient httpClient = Apache5HttpClient.builder()
    .tlsSocketStrategy(tlsSocketStrategy) // ✅ Modern method
    .build();

Testing

  • Added new Test cases

License

  • I confirm that this pull request can be released under the Apache 2 license

License

  • I confirm that this pull request can be released under the Apache 2 license

@joviegas joviegas requested a review from a team as a code owner July 18, 2025 23:18
@joviegas joviegas force-pushed the joviegas/apache5_replace_DeprecatedAPIs branch from 724e1b2 to 034f7b6 Compare July 18, 2025 23:43
@joviegas joviegas changed the base branch from master to feature/master/apache5x July 21, 2025 23:49
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@@ -452,12 +454,27 @@ public interface Builder extends SdkHttpClient.Builder<Apache5HttpClient.Builder
Builder dnsResolver(DnsResolver dnsResolver);

/**
* @deprecated this has been replaced with {{@link #tlsSocketStrategy(TlsSocketStrategy)}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we explain that this is here to ease migration from 4.5.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -252,4 +258,76 @@ private HttpExecuteResponse makeRequestWithHttpClient(SdkHttpClient httpClient)
return httpClient.prepareRequest(request).call();
}

@Test
public void tls_strategy_configuration() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: can we fix the test names so they match our normal conventions? i.e. methodToTest_when_expectedBehavior

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 313 to 314
.socketFactory(legacyFactorySpy)
.tlsSocketStrategy(tlsStrategySpy) // This should override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just disallow setting both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@joviegas joviegas force-pushed the joviegas/apache5_replace_DeprecatedAPIs branch from 2bcd1e2 to dcaaaf6 Compare July 29, 2025 14:59
@joviegas joviegas merged commit 5c6cdb2 into feature/master/apache5x Aug 1, 2025
17 of 26 checks passed
Copy link

github-actions bot commented Aug 1, 2025

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants