[FEATURE] Add an option to turn on and off the certificate validation of llm connectors #4394
Conversation
… in ML Commons Resolves opensearch-project#4371 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
akolarkunnu
requested review from
HenryL27,
Zhangxunmt,
austintlee,
b4sjoo,
dhrubo-os,
jngz-es,
mingshl,
model-collapse,
pyek-bot,
rbhavna,
sam-herman,
xinyual,
ylwu-amzn and
zane-neo
as code owners
|
Thanks for publishing this PR. |
Another option: control enable/disable SSL on connector level |
rithin-pullela-aws
left a comment
rithin-pullela-aws
left a comment
There was a problem hiding this comment.
Thanks for the changes @akolarkunnu !!
Just one high level concern, we call the flag: connector.ssl_verification_enabled
So it should be applicable to all the connectors.
Can we update the MCP and MCPStreamableHTTP connectors with the same logic for SSL validation? We are using java http client behind the scenes:
// Create streamable HTTP transport
McpClientTransport transport = HttpClientStreamableHttpTransport
.builder(mcpServerUrl)
.endpoint(endpoint)
.customizeClient(clientBuilder -> {
clientBuilder.connectTimeout(connectionTimeout);
clientBuilder.followRedirects(HttpClient.Redirect.NORMAL);
})
.customizeRequest(headerConfig)
.build();
WalkthroughAdds a new boolean parameter Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java (1)
487-489: Global connector SSL verification flag is wired correctlyThe setting name, default (
true), scope, and dynamic behavior are consistent with other ML Commons flags and the stated requirements. Be sure to clearly document that setting this tofalsedisables certificate verification cluster‑wide for connector traffic, as this is a high‑risk security toggle.plugin/src/test/java/org/opensearch/ml/settings/MLFeatureEnabledSettingTests.java (1)
31-31: ClusterSettings wiring for connector SSL flag looks correctRegistering
ML_COMMONS_CONNECTOR_SSL_VERIFICATION_ENABLEDinClusterSettingsmatches how other feature flags are handled and is required for dynamic updates. You may also want to extend these tests to assertmlFeatureEnabledSetting.isConnectorSslVerificationEnabled()in the “all features enabled / some disabled” scenarios, similar to other flags.Also applies to: 84-85
common/src/main/java/org/opensearch/ml/common/settings/MLFeatureEnabledSetting.java (1)
25-25: Import ordering is inconsistent with the rest of the file.The new static import should be placed alphabetically between
ML_COMMONS_CONNECTOR_PRIVATE_IP_ENABLEDandML_COMMONS_CONTROLLER_ENABLED(around line 11) rather than at the end.ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/HttpJsonConnectorExecutor.java (1)
77-78: Consider addingvolatilefor consistency withconnectorPrivateIpEnabled.The
connectorPrivateIpEnabledfield (line 76) is markedvolatile, butconnectorSslVerificationEnabledis not. While the HTTP client is lazily initialized once and subsequent changes may not take effect immediately, usingvolatilewould be consistent with the existing pattern and ensure visibility across threads.@Setter -private boolean connectorSslVerificationEnabled; +private volatile boolean connectorSslVerificationEnabled;common/src/main/java/org/opensearch/ml/common/httpclient/MLHttpClientFactory.java (1)
29-35: Include SSL verification status in debug log.The debug log would benefit from including the
connectorSslVerificationEnabledparameter to aid in troubleshooting connection issues.Apply this diff:
log .debug( - "Creating MLHttpClient with connectionTimeout: {}, readTimeout: {}, maxConnections: {}", + "Creating MLHttpClient with connectionTimeout: {}, readTimeout: {}, maxConnections: {}, sslVerificationEnabled: {}", connectionTimeout, readTimeout, - maxConnections + maxConnections, + connectorSslVerificationEnabled );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
common/src/main/java/org/opensearch/ml/common/httpclient/MLHttpClientFactory.java(3 hunks)common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java(1 hunks)common/src/main/java/org/opensearch/ml/common/settings/MLFeatureEnabledSetting.java(5 hunks)common/src/test/java/org/opensearch/ml/common/httpclient/MLHttpClientFactoryTests.java(1 hunks)common/src/test/java/org/opensearch/ml/common/settings/MLCommonsSettingsTests.java(1 hunks)common/src/test/java/org/opensearch/ml/common/settings/MLFeatureEnabledSettingTests.java(5 hunks)ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java(2 hunks)ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/HttpJsonConnectorExecutor.java(2 hunks)ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteConnectorExecutor.java(1 hunks)ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteModel.java(2 hunks)plugin/src/main/java/org/opensearch/ml/action/connector/ExecuteConnectorTransportAction.java(1 hunks)plugin/src/main/java/org/opensearch/ml/model/MLModelManager.java(2 hunks)plugin/src/main/java/org/opensearch/ml/plugin/MachineLearningPlugin.java(1 hunks)plugin/src/test/java/org/opensearch/ml/settings/MLFeatureEnabledSettingTests.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
common/src/test/java/org/opensearch/ml/common/settings/MLCommonsSettingsTests.java (1)
common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java (1)
MLCommonsSettings(24-490)
common/src/test/java/org/opensearch/ml/common/settings/MLFeatureEnabledSettingTests.java (1)
common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java (1)
MLCommonsSettings(24-490)
plugin/src/test/java/org/opensearch/ml/settings/MLFeatureEnabledSettingTests.java (1)
common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java (1)
MLCommonsSettings(24-490)
🪛 GitHub Actions: Build and Test ml-commons
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteModel.java
[error] 128-128: spotlessJavaCheck failed: formatting violations detected in RemoteModel.java. Run './gradlew :opensearch-ml-algorithms:spotlessApply' to fix.
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/HttpJsonConnectorExecutor.java
[error] 185-185: spotlessJavaCheck failed: formatting violations detected in HttpJsonConnectorExecutor.java. Run './gradlew :opensearch-ml-algorithms:spotlessApply' to fix.
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java
[error] 196-196: spotlessJavaCheck failed: formatting violations detected in AwsConnectorExecutor.java. Run './gradlew :opensearch-ml-algorithms:spotlessApply' to fix.
🔇 Additional comments (11)
plugin/src/main/java/org/opensearch/ml/model/MLModelManager.java (1)
45-45: Connector SSL verification flag correctly propagated to remote modelsImporting
CONNECTOR_SSL_VERIFICATION_ENABLEDand adding it tosetUpParameterMapfrommlFeatureEnabledSetting.isConnectorSslVerificationEnabled()cleanly wires the new setting into remote model deployment. Note this value is captured when the params map is built (deploy/update‑cache time); changing the cluster setting later won’t affect already‑initializedRemoteModelinstances until they’re redeployed or their cache is refreshed.Also applies to: 1512-1514
plugin/src/main/java/org/opensearch/ml/plugin/MachineLearningPlugin.java (1)
1367-1368: Exposing connector SSL verification setting via plugin is correctAdding
ML_COMMONS_STREAM_ENABLEDandML_COMMONS_CONNECTOR_SSL_VERIFICATION_ENABLEDtogetSettings()correctly surfaces these feature flags as configurable OpenSearch settings. No issues noted.common/src/test/java/org/opensearch/ml/common/settings/MLCommonsSettingsTests.java (1)
111-114: Good default-coverage test for connector SSL verification flagThe new test correctly verifies that
ML_COMMONS_CONNECTOR_SSL_VERIFICATION_ENABLEDdefaults totrue, matching the intended secure‑by‑default behavior.plugin/src/main/java/org/opensearch/ml/action/connector/ExecuteConnectorTransportAction.java (1)
84-88: SSL verification flag correctly applied to connector executorsPassing
mlFeatureEnabledSetting.isConnectorSslVerificationEnabled()intosetConnectorSslVerificationEnabledensures execute‑connector requests honor the global toggle, consistent with the remote‑model path. Behavior for implementations that don’t override the setter remains unchanged.ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteConnectorExecutor.java (1)
187-188: Default SSL verification setter on RemoteConnectorExecutor is appropriateAdding a no-op default
setConnectorSslVerificationEnabled(boolean)keeps existing implementations binary-compatible while allowing HTTP-based executors to opt in and honor the flag. This aligns with how other optional setters on the interface are handled.common/src/main/java/org/opensearch/ml/common/settings/MLFeatureEnabledSetting.java (1)
67-68: LGTM!The feature flag implementation follows the established pattern: volatile field, initialization from settings, cluster settings update consumer, and getter. The wiring is consistent with other feature flags in this class.
Also applies to: 89-89, 116-116, 253-256
common/src/test/java/org/opensearch/ml/common/settings/MLFeatureEnabledSettingTests.java (1)
51-52: LGTM!Test coverage appropriately validates:
- Setting registration in ClusterSettings
- Enabled state when explicitly set to
true- Disabled state when explicitly set to
falseThis is consistent with the testing patterns for other feature flags in this class.
Also applies to: 78-78, 98-98, 121-121, 141-141
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java (1)
85-87: LGTM!The new field and setter follow the same pattern as
connectorPrivateIpEnabledin this class. The implementation correctly integrates with the SSL verification feature.common/src/test/java/org/opensearch/ml/common/httpclient/MLHttpClientFactoryTests.java (1)
18-24: LGTM!The test appropriately validates both
true(SSL verification enabled) andfalse(SSL verification disabled) configurations for the new parameter. This provides basic coverage for the factory method's new signature.common/src/main/java/org/opensearch/ml/common/httpclient/MLHttpClientFactory.java (2)
13-13: LGTM!The new imports are necessary for implementing SSL certificate verification control.
Also applies to: 16-16
21-27: Verify all callers have been updated for the new parameter.The method signature now includes
connectorSslVerificationEnabled. Ensure all callers across the codebase have been updated to pass this parameter. This requires manual inspection of all invocations ofgetAsyncHttpClientto confirm the parameter is correctly supplied at each call site.
common/src/main/java/org/opensearch/ml/common/httpclient/MLHttpClientFactory.java
Outdated
Show resolved
Hide resolved
...lgorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java
Show resolved
Hide resolved
...thms/src/main/java/org/opensearch/ml/engine/algorithms/remote/HttpJsonConnectorExecutor.java
Show resolved
Hide resolved
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteModel.java
Outdated
Show resolved
Hide resolved
… in ML Commons Resolves opensearch-project#4371 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
…ommons into sslverification
akolarkunnu
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Failure
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
akolarkunnu
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Failure
akolarkunnu
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
common/src/main/java/org/opensearch/ml/common/httpclient/MLHttpClientFactory.java (2)
29-35: Consider enhancing debug logging.The debug log message doesn't include the new
skipSslVerificationparameter, which would be helpful for troubleshooting SSL-related connection issues.Apply this diff to include the parameter in the log:
log .debug( - "Creating MLHttpClient with connectionTimeout: {}, readTimeout: {}, maxConnections: {}", + "Creating MLHttpClient with connectionTimeout: {}, readTimeout: {}, maxConnections: {}, skipSslVerification: {}", connectionTimeout, readTimeout, - maxConnections + maxConnections, + skipSslVerification );
41-43: Consider adding security warning when SSL verification is disabled.A past review suggested adding a warning-level log when
skipSslVerification=trueto alert operators about the security implications. While the implementation is correct, adding such a warning would improve operational visibility.Based on past review comments, consider adding:
return doPrivileged(() -> { + if (skipSslVerification) { + log.warn( + "SSL certificate verification is DISABLED. This connection is vulnerable to man-in-the-middle attacks. " + + "Only use this setting in trusted environments." + ); + } log
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
common/src/main/java/org/opensearch/ml/common/httpclient/MLHttpClientFactory.java(3 hunks)common/src/test/java/org/opensearch/ml/common/httpclient/MLHttpClientFactoryTests.java(1 hunks)ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java(1 hunks)ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/HttpJsonConnectorExecutor.java(1 hunks)ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteConnectorExecutor.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/HttpJsonConnectorExecutor.java
🔇 Additional comments (6)
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java (3)
193-196: Logic is correct and follows secure-by-default principle.The implementation correctly defaults to
false(SSL verification enabled) and only disables verification when explicitly configured. The use ofBoolean.parseBooleansafely handles null or invalid values by returningfalse.
200-201: Method signature change propagated correctly.The call to
MLHttpClientFactory.getAsyncHttpClientcorrectly includes the new fifth parameterskipSslVerification.
193-196: The referenced constantSKIP_SSL_VERIFICATIONdoes not exist in RemoteConnectorExecutor or elsewhere in the ml-commons repository.The code snippet at lines 193-196 references a constant that is not defined in the codebase. Verify that this constant either exists in the current codebase or needs to be defined. While AbstractConnectorExecutor correctly implements the RemoteConnectorExecutor interface, the constant accessibility issue cited in the review is not valid since the constant does not exist in RemoteConnectorExecutor.
Likely an incorrect or invalid review comment.
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/RemoteConnectorExecutor.java (1)
68-68: LGTM! Clean constant definition.The constant follows Java naming conventions and provides a centralized definition for the SSL verification parameter key, making it available to all connector implementations.
common/src/test/java/org/opensearch/ml/common/httpclient/MLHttpClientFactoryTests.java (1)
20-24: Test covers both SSL verification modes.The test correctly exercises both
skipSslVerificationvalues (false and true), ensuring the factory method handles both configurations without errors. The assertions verify successful client creation for both modes.common/src/main/java/org/opensearch/ml/common/httpclient/MLHttpClientFactory.java (1)
25-26: Core SSL verification logic is correct.The implementation correctly wires
skipSslVerificationtoTRUST_ALL_CERTIFICATES, enabling the requested functionality to disable certificate validation when needed. The parameter defaults to secure behavior (verification enabled) when callers passfalse.Also applies to: 41-43
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4394 +/- ##
============================================
- Coverage 80.22% 80.22% -0.01%
- Complexity 10245 10255 +10
============================================
Files 858 858
Lines 44552 44558 +6
Branches 5158 5160 +2
============================================
+ Hits 35742 35745 +3
+ Misses 6639 6635 -4
- Partials 2171 2178 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Resolves opensearch-project#4371 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
akolarkunnu
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
akolarkunnu
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
akolarkunnu
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
akolarkunnu
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
Thanks @ylwu-amzn for the suggestion, this approach looks more suitable and convenient here. Introduced a new connector parameter "skip_ssl_verification" to enable/disable ssl verification of llm connectors. |
@rithin-pullela-aws This PR is especially for llm connectors. We will address MCP connectors in separate task, I created task for it #4504 |
3 participants
Description
Introduced a new connector parameter "skip_ssl_verification" to enable/disable ssl verification of llm connectors.
Tested with and without setting this parameter and I am able to connect a lll server by disabling ssl verification.
Related Issues
Resolves #4371
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
New Features
API Updates
Tests
✏️ Tip: You can customize this high-level summary in your review settings.