Skip to content

[REST] Add option to configure TLS settings in REST client#13190

Merged
bryanck merged 4 commits into
apache:mainfrom
bryanck:tls-configurer
Jun 4, 2025
Merged

[REST] Add option to configure TLS settings in REST client#13190
bryanck merged 4 commits into
apache:mainfrom
bryanck:tls-configurer

Conversation

@bryanck

@bryanck bryanck commented May 30, 2025

Copy link
Copy Markdown
Contributor

This PR adds an option to the REST client to configure TLS settings via a pluggable configurer class. Java supports setting some TLS parameters via System properties, but doing so will affect all connections, and causes issues with clients such as the S3 client.

Also, while some basic parameters could be set via catalog properties, using a plugin approach allows the most flexibility when configuring mutual authentication, which can involve custom logic for certificate and host name verification. This aligns with the pluggable model currently used for AuthManagers.

If useful, we could follow this PR up with an implementation that is driven off of catalog properties, for cases that don't need special logic. This would allow setting the keystore and the truststore sepcifically for the REST client, for example.

@github-actions github-actions Bot added the core label May 30, 2025
@bryanck bryanck added this to the Iceberg 1.10.0 milestone May 30, 2025
import org.apache.hc.client5.http.ssl.HttpsSupport;
import org.apache.hc.core5.ssl.SSLContexts;

public interface TLSConfigurer {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we have functions for the constructor DefaultClientTlsStrategy which excepts TLS protocol and cipher suites as well, we are 5.5, this will be really helpful in enforcing TLS 1.3 and only certain cipher suits which a maintainer may want to enforce

https://hc.apache.org/httpcomponents-client-5.5.x/current/httpclient5/apidocs/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.html#DefaultClientTlsStrategy-javax.net.ssl.SSLContext-java.lang.String:A-java.lang.String:A-org.apache.hc.core5.reactor.ssl.SSLBufferMode-javax.net.ssl.HostnameVerifier-

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, added. I left HostnameVerificationPolicy and SSLBufferMode at default for now to avoid any Apache HTTP dependencies in the interface.

@singhpk234 singhpk234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @bryanck !

@bryanck bryanck requested review from danielcweeks and nastra June 2, 2025 14:33
Comment thread core/src/main/java/org/apache/iceberg/rest/HTTPClient.java Outdated
}

@Test
public void testLoadTLSConfigurer_NoArgConstructorNotFound() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I know we use this naming scheme with _ in the name in TestCatalogUtil but I feel like just omitting it reads better

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, agreed, the underscore convention isn't used in this test class anyway so I removed it.

@nastra nastra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks great, thanks @bryanck

@bryanck

bryanck commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews @nastra and @singhpk234 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants