Ignore SSL Verification #2805
Ignore SSL Verification #2805
Conversation
There was a problem hiding this comment.
Thanks for your contribution, @rohangoli ! Some preliminary comments below.
| WARNING: This should only be used for development and testing environments with self-signed certificates. | ||
| Disabling SSL verification in production environments compromises security. | ||
| example: false | ||
| default: false |
There was a problem hiding this comment.
I'd prefer to avoid an explicit default here. Having a default value in this YAML will cause all clients to receive in in REST API responses. On the other hand this property is not likely to be used in many cases.
We should certainly implement the change such that false is the default behaviour, but I believe it would be preferable to avoid declaring it here as an Open API default (so that clients will not receive this property at all, unless it is set explicitly).
Ignore this comment if you're moving the flag to FeatureConfiguration.
| type: boolean | ||
| description: >- | ||
| Whether SSL certificate verification should be disabled for STS and S3 endpoints (optional). | ||
| WARNING: This should only be used for development and testing environments with self-signed certificates. |
There was a problem hiding this comment.
If the intention is to support dev / test environments only, I believe it would be preferable to have this flag in FeatureConfiguration as opposed to catalog properties.
There was a problem hiding this comment.
Thanks for the review! I thought it would be useful if it can be configurable via dockerfile.
Let me update the code to use ignoreSSLVerification under Feature Configuration!
There was a problem hiding this comment.
docker/helm can set flags in FeatureConfiguration (e.g. via env. variables)
There was a problem hiding this comment.
I think the whole reflection stuff is not needed.
You can test for the S3FileIO class name early in DefaultFileIOFactory.loadFileIOInternal(). It that test yields true, construct it directly via o.a.i.aws.s3.S3FileIO#S3FileIO(o.a.i.util.SerializableSupplier<software.amazon.awssdk.services.s3.S3Client>, o.a.i.util.SerializableSupplier<software.amazon.awssdk.services.s3.S3AsyncClient>) and initialize S3FileIO manually (do what CatalogUtil.loadFileIO() does).
I'd also prefer to not eagerly build the S3Client+S3AsyncClient but only when Supplier.get() is called.
"Blind trust" isn't really great, and it would be much safer to guard the ability to do this via a global option and check it in ProductionReadinessChecks#checkInsecureStorageSettings.
The even better approach would be a change in Iceberg, to configure the SdkHttpConfigurationOption#TRUST_ALL_CERTIFICATES option.
A much safer option than blindly trusting all certificates is to allow configuring custom key and trust stores via ApacheHttpClient.Builder.tlsTrustManagersProvider()/.tlsKeyManagersProvider().
I'd avoid recommending users to configure the global Java key/trust stores, because other external systems (backend database, other object stores) would be affected by such a change.
| */ | ||
| public SdkHttpClient createInsecureHttpClient(S3AccessConfig config) { | ||
| try { | ||
| SSLContext sslContext = |
There was a problem hiding this comment.
software.amazon.awssdk.http.SdkHttpConfigurationOption#TRUST_ALL_CERTIFICATES seems to be a simpler way.
| // Apply configuration options | ||
| config.maxHttpConnections().ifPresent(httpClient::maxConnections); | ||
| config.readTimeout().ifPresent(httpClient::socketTimeout); | ||
| config.connectTimeout().ifPresent(httpClient::connectionTimeout); | ||
| config.connectionAcquisitionTimeout().ifPresent(httpClient::connectionAcquisitionTimeout); | ||
| config.connectionMaxIdleTime().ifPresent(httpClient::connectionMaxIdleTime); | ||
| config.connectionTimeToLive().ifPresent(httpClient::connectionTimeToLive); | ||
| config.expectContinueEnabled().ifPresent(httpClient::expectContinueEnabled); |
There was a problem hiding this comment.
Looks like this is duplicated code, which can be shared w/ sdkHttpClient?
3 participants
What changes were proposed in this pull request?
Why are the changes needed?
Polaris Logs:
Does this PR introduce any user-facing change?
How was this patch tested?
CHANGELOG.md
fcc779b Ignore SSL Verification