support plain text credentials by zane-neo · Pull Request #4782 · opensearch-project/ml-commons

zane-neo · 2026-04-07T03:02:19Z

Description

Currently, ml-commons requires all connector credentials to be encrypted using EncryptorImpl with AWS Encryption SDK. This creates challenges in certain use cases where encryption is unnecessary, redundant, or problematic. This issue proposes adding support for optionally storing credentials in plaintext, controlled by a cluster-level setting (plugins.ml_commons.allow_plaintext_credentials, default: false) to ensure security by default.

Related Issues

#4704

Testing

Below is an example request body for different testing cases:

{
    "name": "Cohere Connector: embedding",
    "description": "The connector to cohere embedding model",
    "version": 1,
    "protocol": "http",
    "parameters": {
      "model_name": "embed-english-v3.0"
    },
    "credential": {
      "encrypted": "xxx",
      "cohere_key": "YOUR_COHERE_KEY"
    },
    "actions": [
      {
        "action_type": "predict",
        "method": "POST",
        "url": "https://api.cohere.com/v2/embed",
        "headers": {
          "content-type": "application/json",
          "Authorization": "Bearer ${credential.cohere_key}"
        },
        "request_body": "{ \"texts\": ${parameters.texts}, \"truncate\": \"END\", \"model\": \"${parameters.model_name}\", \"embedding_types\": [\"float\", \"int8\", \"uint8\", \"binary\", \"ubinary\"], \"input_type\": \"classification\"}",
        "pre_process_function": "connector.pre_process.cohere.embedding",
        "post_process_function": "connector.post_process.cohere_v2.embedding.float"
      }
    ]
}

Allow plain text credentials not enabled

Keep the cluster setting: plugins.ml_commons.allow_plaintext_credentials unchanged and try to create a connector with encrypted to false like below:

...
  "credential": {
      "encrypted": "false",
      "cohere_key": "YOUR_COHERE_KEY"
    },
...

The result should like below:

{
    "error": {
        "root_cause": [
            {
                "type": "illegal_argument_exception",
                "reason": "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage."
            }
        ],
        "type": "illegal_argument_exception",
        "reason": "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage."
    },
    "status": 400
}

Remove the key encrypted or change the key's value to true can get successful response.

Allow plain text credentials enabled

Use plain text credentials with request body like below:

...
  "credential": {
      "encrypted": "false",
      "cohere_key": "YOUR_COHERE_KEY"
    },
...

Response like below:

{
    "connector_id": "6VzuZp0B-y9qd6CWXP-L"
}

If you have permission to check the raw documents you will see:

...
"_source": {
    "name": "Cohere Connector: embedding",
     ...
    "credential": {
        "encrypted": "false",
        "cohere_key": "YOUR_COHERE_KEY"
    },
...

Use encrypted credentials with request body like below:

...
  "credential": {
      "cohere_key": "YOUR_COHERE_KEY"
    },
...

Response like below:

{
    "connector_id": "6lzvZp0B-y9qd6CWZv95"
}

If you have permission to read raw documents you will see:

...
"_source": {
  "name": "Cohere Connector: embedding",
  ...
  "credential": {
      "cohere_key": "AgV4djTpfnlByArLgWmAo7Y30CBFX9k/jB5ZirpeeccveYcAXwABABVhd3MtY3J5cHRvLXB1YmxpYy1rZXkAREExZlFTRFNtbHlTVnNCSEpWdHA0TDBZV0FQblp1eWJTS0NGL3FVVHhPdlk5VEp1ODhyejVjL2djS0VsUGJsSWVFQT09AAEABkN1c3RvbQAUAAAAgAAAAAxeqnZKVagWpsqAHm4AMASNOiiwECY1Rfvj2gHrqHKZ8v3y/k6kYl36YlISczDq8//Ry4wrbSroT0RAG/y6CwIAABAAL8BD5fT5HlQkSCvFtJKbgb8zZKn4cLR+TWhFQbg3WJT8m5TmM6Sx++aM5+GjRXjF/////wAAAAEAAAAAAAAAAAAAAAEAAAAoVbbRKUon+0+E7KxbnkLF1le/N2x+EKhcym8gAvHfsSWWUqjU097BWzDMYe9kHobdrDv5mXEcfJwAZzBlAjEAisLXWdOCzHbQra0X0u91DZGB4V3iWySEvHMxmvZ4QRZWc/tZkuR5xWBKAwMj01e8AjB33WjvCQ9kwEu7lUf/k8faOx26D4dTYRjA4baTbn+Czl5Z5MXPUv11hDFzsXZVlHM="
  },
...

Allow plain text credentials changed to disabled

If allow plain text credentials are set to enabled and registered with several connectors with plain text credentials and the setting are changed back to disabled after that, then the plain text credentials should still work.
Predict model

POST /_plugins/_ml/models/BmuyZp0BfDnPLd6M65JO/_predict
{
    "parameters": {
        "texts": ["Say this is a test", "world"]
    }
}

Response:

{
    "inference_results": [
        {
            "output": [
                {
                    "name": "sentence_embedding",
                    "data_type": "FLOAT32",
                    "shape": [
                        1024
                    ],
                    "data": [
                        -0.015640259,
                        -0.0048446655,
                        -0.02684021,
                        -0.020339966,
                        -0.014762878,
                        0.004890442,
                        -0.003250122,
                        ...
                 ]
             }
          ]
      }
   ]
}

Update connector when allow plain text credentials are set to false

Update Failed

POST /_plugins/_ml/connectors/AGuyZp0BfDnPLd6MaJLr
{
    "credential": {
        "encrypted": "false",
        "cohere_key": "YOUR_COHERE_KEY"
    },
    "description": "Change the connector credentials to use plain text credential"
}

Response:

{
    "error": {
        "root_cause": [
            {
                "type": "illegal_argument_exception",
                "reason": "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage."
            }
        ],
        "type": "illegal_argument_exception",
        "reason": "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage."
    },
    "status": 400
}

Update succeed

POST /_plugins/_ml/connectors/AGuyZp0BfDnPLd6MaJLr
{
    "credential": {
        "cohere_key": "YOUR_COHERE_KEY"
    },
    "description": "Change the connector credentials to use plain text credential"
}

Response:

{
    "_index": ".plugins-ml-connector",
    "_id": "AGuyZp0BfDnPLd6MaJLr",
    "_version": 2,
    "result": "updated",
    "forced_refresh": true,
    "_shards": {
        "total": 1,
        "successful": 1,
        "failed": 0
    },
    "_seq_no": 5,
    "_primary_term": 3
}

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

github-actions · 2026-04-07T03:03:19Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 8ce8f73.

Path	Line	Severity	Description
common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java	186	medium	The encryption bypass in AbstractConnector.encrypt() and decrypt() is controlled solely by the connector's own 'encrypted' field, without checking the cluster-level policy (ML_COMMONS_ALLOW_PLAINTEXT_CREDENTIALS). The cluster setting is only enforced at connector creation/update time in the transport actions. This means a connector created with 'encrypted=false' while the setting was enabled will permanently skip encryption even if the cluster setting is later disabled, and any direct index write bypassing the transport action would also circumvent the policy check.
common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java	551	low	The new ML_COMMONS_ALLOW_PLAINTEXT_CREDENTIALS setting introduces an intentional mechanism to store connector credentials unencrypted in the cluster state. While it defaults to false and is documented as intended for managed-service use, enabling it via a dynamic cluster settings API call requires no restart and no re-authentication, meaning a compromised admin account could silently downgrade credential security at runtime across all new connectors without code changes.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-04-07T03:04:20Z

PR Reviewer Guide 🔍

(Review updated until commit `8ce8f73`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 Security concerns Sensitive information exposure: Plaintext credentials are stored in the credential map alongside an `encrypted=false` flag. When the connector is serialized to XContent (e.g., stored in OpenSearch index or returned via GET API), the plaintext credentials will be visible in the response. There is no masking or redaction of credential values when `encrypted=false`. This means API keys and secrets stored in plaintext will be fully exposed in GET connector responses, audit logs, and index documents, which is a significant security concern even when the feature is intentionally enabled.
✅ No TODO sections
🔀 Multiple PR themes Sub-PR theme: Core plaintext credentials support in common module Relevant files: common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java common/src/main/java/org/opensearch/ml/common/settings/MLFeatureEnabledSetting.java common/src/test/java/org/opensearch/ml/common/connector/HttpConnectorTest.java common/src/test/java/org/opensearch/ml/common/settings/MLFeatureEnabledSettingTests.java Sub-PR theme: Plugin-level enforcement and integration tests for plaintext credentials Relevant files: plugin/src/main/java/org/opensearch/ml/action/connector/TransportCreateConnectorAction.java plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java plugin/src/main/java/org/opensearch/ml/plugin/MachineLearningPlugin.java plugin/src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java plugin/src/test/java/org/opensearch/ml/action/connector/UpdateConnectorTransportActionTests.java plugin/src/test/java/org/opensearch/ml/settings/MLFeatureEnabledSettingTests.java plugin/src/test/java/org/opensearch/ml/rest/RestMLPlaintextCredentialsIT.java
⚡ Recommended focus areas for review Security Risk The `isPlaintextCredentials()` method checks if `credential.get("encrypted")` equals "false" (case-insensitive). This means any connector that explicitly sets `encrypted=false` will bypass encryption. However, there is no validation that the value is a recognized boolean string — values like "FALSE", "False", "0", or other variants could behave unexpectedly. More critically, the `encrypted` field is stored as a plain string in the credential map alongside actual secrets, which could be confusing and error-prone. Consider using a dedicated field outside the credential map for this flag. public boolean isPlaintextCredentials() { return credential != null && "false".equalsIgnoreCase(credential.get(ENCRYPTED_FIELD)); } Policy Bypass The plaintext credentials check in the update action is performed after `connector.update(mlUpdateConnectorAction.getUpdateContent())` is called. This means the connector's in-memory state is already mutated before the policy check. If the check fails, the listener receives an error, but the connector object has already been modified. While this doesn't persist to the index, it could cause subtle issues if the object is referenced elsewhere. The policy check should be performed before applying the update. connector.update(mlUpdateConnectorAction.getUpdateContent()); // Enforce cluster-level plaintext credentials policy if (connector instanceof AbstractConnector) { AbstractConnector abstractConnector = (AbstractConnector) connector; if (abstractConnector.isPlaintextCredentials()) { if (!mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()) { // Cluster setting does not allow plaintext credentials - throw exception listener .onFailure( new IllegalArgumentException( "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage." ) ); return; } else { // Cluster setting allows plaintext credentials log .warn( "Connector '{}' is configured to store credentials in plaintext (unencrypted). " + "This is less secure. Only use this for testing or when encryption is handled externally.", connectorId ); } } } Instanceof Check The plaintext credentials enforcement uses `instanceof AbstractConnector` check. Since all connectors are expected to extend `AbstractConnector`, this check should always be true. If it ever evaluates to false (e.g., a new connector type not extending `AbstractConnector`), the plaintext credentials policy would be silently bypassed without any warning or error. Consider adding an else branch to log a warning or enforce the policy at the `Connector` interface level. if (connector instanceof AbstractConnector) { AbstractConnector abstractConnector = (AbstractConnector) connector; if (abstractConnector.isPlaintextCredentials()) { if (!mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()) { // Cluster setting does not allow plaintext credentials listener .onFailure( new IllegalArgumentException( "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage." ) ); return; } else { // Cluster setting allows plaintext credentials log .warn( "Connector '{}' is configured to store credentials in plaintext (unencrypted). " + "This is less secure. Only use this for testing or when encryption is handled externally.", connectorName ); } } } Missing @Test The test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation. Without this annotation, these tests will not be executed by JUnit and will silently pass without running. public void test_execute_plaintext_credentials_blocked_when_not_allowed() { when(connectorAccessControlHelper.accessControlNotEnabled(any(User.class))).thenReturn(true); when(mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()).thenReturn(false); List<ConnectorAction> actions = new ArrayList<>(); actions .add( ConnectorAction .builder() .actionType(ConnectorAction.ActionType.PREDICT) .method("POST") .url("https://api.openai.com/v1/completions") .build() ); // Include "encrypted": "false" in credentials to request plaintext storage Map<String, String> credential = ImmutableMap.of("access_key", "mockKey", "secret_key", "mockSecret", "encrypted", "false"); MLCreateConnectorInput mlCreateConnectorInput = MLCreateConnectorInput .builder() .name("test_connector") .version("1") .protocol(ConnectorProtocols.HTTP) .credential(credential) .actions(actions) .build(); MLCreateConnectorRequest request = new MLCreateConnectorRequest(mlCreateConnectorInput); action.doExecute(task, request, actionListener); ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class); verify(actionListener).onFailure(argumentCaptor.capture()); assertTrue(argumentCaptor.getValue().getMessage().contains("Plaintext credentials are not allowed")); } public void test_execute_plaintext_credentials_allowed_when_setting_enabled() { when(connectorAccessControlHelper.accessControlNotEnabled(any(User.class))).thenReturn(true); when(mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()).thenReturn(true); doAnswer(invocation -> { ActionListener<Boolean> listener = invocation.getArgument(0); listener.onResponse(true); return null; }).when(mlIndicesHandler).initMLConnectorIndex(isA(ActionListener.class)); doAnswer(invocation -> { ActionListener<IndexResponse> listener = invocation.getArgument(1); listener.onResponse(indexResponse); return null; }).when(client).index(any(IndexRequest.class), isA(ActionListener.class)); List<ConnectorAction> actions = new ArrayList<>(); actions .add( ConnectorAction .builder() .actionType(ConnectorAction.ActionType.PREDICT) .method("POST") .url("https://api.openai.com/v1/completions") .build() ); // Include "encrypted": "false" in credentials to request plaintext storage Map<String, String> credential = ImmutableMap.of("access_key", "mockKey", "secret_key", "mockSecret", "encrypted", "false"); MLCreateConnectorInput mlCreateConnectorInput = MLCreateConnectorInput .builder() .name("test_connector") .version("1") .protocol(ConnectorProtocols.HTTP) .credential(credential) .actions(actions) .build(); MLCreateConnectorRequest request = new MLCreateConnectorRequest(mlCreateConnectorInput); action.doExecute(task, request, actionListener); // Capture and verify the response - should succeed ArgumentCaptor<MLCreateConnectorResponse> captor = ArgumentCaptor.forClass(MLCreateConnectorResponse.class); verify(actionListener).onResponse(captor.capture()); MLCreateConnectorResponse actualResponse = captor.getValue(); assertNotNull(actualResponse); assertEquals(CONNECTOR_ID, actualResponse.getConnectorId()); }

github-actions · 2026-04-07T03:04:59Z

PR Code Suggestions ✨

Latest suggestions up to 8ce8f73

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add missing test annotations to new test methods The two new test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation. Without this annotation, JUnit will not recognize and execute these tests, meaning the new functionality will not actually be tested. plugin/src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java [651] +@Test public void test_execute_plaintext_credentials_blocked_when_not_allowed() { Suggestion importance[1-10]: 8 __ Why: The two new test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation, which means JUnit will not execute them. This is a real bug that would cause the new test coverage to be silently skipped.	Medium
Possible issue	Validate policy before mutating connector state The plaintext credentials check in `UpdateConnectorTransportAction` happens after `connector.update()` is called, meaning the connector's in-memory state is already mutated with the new (potentially plaintext) credentials before the policy check. If the check fails, the update is rejected, but the connector object has already been modified. The policy check should be performed before calling `connector.update()` to avoid any unintended side effects or partial state changes. plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java [124-149] -connector.update(mlUpdateConnectorAction.getUpdateContent()); +// Enforce cluster-level plaintext credentials policy BEFORE applying update +// Check if the update content contains plaintext credentials +MLCreateConnectorInput updateContent = mlUpdateConnectorAction.getUpdateContent(); +if (updateContent.getCredential() != null + && "false".equalsIgnoreCase(updateContent.getCredential().get(AbstractConnector.ENCRYPTED_FIELD))) { + if (!mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()) { + listener.onFailure(new IllegalArgumentException( + "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage." + )); + return; + } else { + log.warn("Connector '{}' is configured to store credentials in plaintext (unencrypted). " + + "This is less secure. Only use this for testing or when encryption is handled externally.", connectorId); + } +} +connector.update(updateContent); -// Enforce cluster-level plaintext credentials policy -if (connector instanceof AbstractConnector) { - Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that `connector.update()` is called before the plaintext credentials check, meaning the connector's in-memory state is mutated before the policy is enforced. However, the proposed fix checks the update content directly rather than using `isPlaintextCredentials()` after the update, which changes the approach significantly. The current code still works correctly since the listener fails before any persistence occurs, but the ordering issue is a valid concern for code clarity and correctness.	Low
General	Validate encrypted flag value to prevent silent misconfiguration The `isPlaintextCredentials()` method uses the string `"false"` to indicate plaintext (unencrypted) storage, which is semantically confusing — `encrypted=false` means plaintext. Consider renaming the field or the method to make the intent clearer, or at minimum add validation to reject unexpected values (anything other than `"true"` or `"false"`) to prevent misconfiguration where a typo silently falls back to encryption. common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java [170-172] public boolean isPlaintextCredentials() { - return credential != null && "false".equalsIgnoreCase(credential.get(ENCRYPTED_FIELD)); + if (credential == null) return false; + String encryptedValue = credential.get(ENCRYPTED_FIELD); + if (encryptedValue == null) return false; + if (!"true".equalsIgnoreCase(encryptedValue) && !"false".equalsIgnoreCase(encryptedValue)) { + throw new IllegalArgumentException( + "Invalid value for '" + ENCRYPTED_FIELD + "' in credentials: '" + encryptedValue + "'. Must be 'true' or 'false'." + ); + } + return "false".equalsIgnoreCase(encryptedValue); } Suggestion importance[1-10]: 4 __ Why: The suggestion to validate unexpected values for the `encrypted` field is a reasonable defensive programming improvement, but throwing an exception from `isPlaintextCredentials()` could break existing connectors that have unexpected values in their credential map. The current behavior of silently treating non-`"false"` values as encrypted is actually safer.	Low

Previous suggestions

Suggestions up to commit 04e5bbb

Category	Suggestion	Impact
Possible issue	Add missing test annotations to new test methods The two new test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation. Without it, JUnit will not recognize them as test cases and they will never be executed, providing no test coverage. plugin/src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java [651] +@Test public void test_execute_plaintext_credentials_blocked_when_not_allowed() { Suggestion importance[1-10]: 8 __ Why: The two new test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation, which means JUnit will not execute them and the new feature will have no actual test coverage from these methods.	Medium
Possible issue	Validate policy before mutating connector state The plaintext credentials check in `UpdateConnectorTransportAction` happens after `connector.update()` is called, meaning the connector's in-memory state is already mutated with the new (potentially plaintext) credentials before the policy check. If the check fails, the update is rejected, but the connector object has already been modified. The policy check should be performed before calling `connector.update()` to avoid any side effects. plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java [124-149] +// Enforce cluster-level plaintext credentials policy before applying update +if (connector instanceof AbstractConnector) { + AbstractConnector abstractConnector = (AbstractConnector) connector; + // Check the incoming update content for plaintext credentials flag + MLCreateConnectorInput updateContent = mlUpdateConnectorAction.getUpdateContent(); + Map<String, String> newCredential = updateContent.getCredential(); + if (newCredential != null && "false".equalsIgnoreCase(newCredential.get(AbstractConnector.ENCRYPTED_FIELD))) { + if (!mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()) { + listener.onFailure(new IllegalArgumentException( + "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage." + )); + return; + } else { + log.warn("Connector '{}' is configured to store credentials in plaintext (unencrypted). " + + "This is less secure. Only use this for testing or when encryption is handled externally.", connectorId); + } + } +} connector.update(mlUpdateConnectorAction.getUpdateContent()); -// Enforce cluster-level plaintext credentials policy -if (connector instanceof AbstractConnector) { - Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `connector.update()` is called before the plaintext credentials check, meaning the connector's in-memory state is already mutated before the policy is enforced. Moving the check before the update avoids this side effect and is a cleaner design. However, the `improved_code` introduces a different approach (checking `updateContent.getCredential()` directly) rather than simply reordering the existing check, which changes the logic significantly.	Medium
General	Ensure plaintext credential headers exclude the encrypted flag When plaintext credentials are used, the `decryptedCredential` map is populated with a copy of `credential` (minus the `ENCRYPTED_FIELD`). However, the credential values themselves are stored as-is without any substitution or processing. If any credential values contain template variables (e.g., `${...}`), they won't be resolved. More importantly, this is consistent with the encrypted path, but it should be verified that `createDecryptedHeaders` correctly uses `decryptedCredential` rather than the raw `credential` map to avoid leaking the `encrypted` flag into headers. common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java [227-235] if (isPlaintextCredentials()) { log.debug("Connector credentials are in plaintext mode, using as-is"); decryptedCredential = new HashMap<>(credential); // Remove the encrypted flag from decrypted credentials decryptedCredential.remove(ENCRYPTED_FIELD); + // Use decryptedCredential (without encrypted flag) for header substitution this.decryptedHeaders = createDecryptedHeaders(getAllHeaders(action)); listener.onResponse(true); return; } Suggestion importance[1-10]: 1 __ Why: The `improved_code` is functionally identical to the `existing_code` — only a comment is changed. The suggestion raises a valid concern about whether `createDecryptedHeaders` uses `decryptedCredential`, but the code change itself provides no actual fix, making this a documentation-only change with no functional impact.	Low

Suggestions up to commit 1ced2ed

Category	Suggestion	Impact
Possible issue	Missing @test annotation on test methods The test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation. Without it, JUnit will not recognize and execute these tests, meaning the plaintext credentials enforcement logic will not be tested. plugin/src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java [651] +@Test public void test_execute_plaintext_credentials_blocked_when_not_allowed() { Suggestion importance[1-10]: 8 __ Why: The `test_execute_plaintext_credentials_blocked_when_not_allowed` method is missing the `@Test` annotation, meaning JUnit will never execute it. This is a real bug that would leave the plaintext credentials blocking logic untested.	Medium
Possible issue	Missing @test annotation on test method This test method is also missing the `@Test` annotation, so it will never be executed by JUnit. Add the annotation to ensure the test runs and validates the allowed plaintext credentials path. plugin/src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java [685] +@Test public void test_execute_plaintext_credentials_allowed_when_setting_enabled() { Suggestion importance[1-10]: 8 __ Why: The `test_execute_plaintext_credentials_allowed_when_setting_enabled` method is also missing the `@Test` annotation, so it will never run. This leaves the allowed plaintext credentials path untested.	Medium
General	Policy check occurs after connector state mutation The plaintext credentials policy check happens after `connector.update(...)` has already mutated the connector state. If the policy check fails and returns early, the connector object is left in a partially updated state. The policy check should be performed before applying the update to avoid inconsistent state. plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java [124-131] -connector.update(mlUpdateConnectorAction.getUpdateContent()); - -// Enforce cluster-level plaintext credentials policy +// Enforce cluster-level plaintext credentials policy before applying update if (connector instanceof AbstractConnector) { AbstractConnector abstractConnector = (AbstractConnector) connector; + // Temporarily apply update to check resulting credential state + connector.update(mlUpdateConnectorAction.getUpdateContent()); if (abstractConnector.isPlaintextCredentials()) { if (!mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()) { Suggestion importance[1-10]: 5 __ Why: The plaintext credentials check happens after `connector.update()` has already mutated the connector. If the check fails, the connector object is left in a modified state. However, the `improved_code` still calls `connector.update()` before the check, so it doesn't actually fix the issue it describes, making the suggestion partially inaccurate.	Low
General	Validate encrypted field values to prevent misconfiguration The `isPlaintextCredentials()` method uses the string value `"false"` to indicate plaintext (unencrypted) storage, which is semantically confusing — the field is named `encrypted` but `"false"` means "not encrypted" (i.e., plaintext). Any value other than explicitly `"false"` (e.g., `"true"`, missing, or typos) will be treated as encrypted. Consider documenting this clearly or validating that only `"true"` and `"false"` are accepted values to prevent silent misconfigurations. common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java [170-172] public boolean isPlaintextCredentials() { - return credential != null && "false".equalsIgnoreCase(credential.get(ENCRYPTED_FIELD)); + if (credential == null) return false; + String encryptedValue = credential.get(ENCRYPTED_FIELD); + if (encryptedValue != null && !"true".equalsIgnoreCase(encryptedValue) && !"false".equalsIgnoreCase(encryptedValue)) { + throw new IllegalArgumentException("Invalid value for 'encrypted' field: '" + encryptedValue + "'. Must be 'true' or 'false'."); + } + return "false".equalsIgnoreCase(encryptedValue); } Suggestion importance[1-10]: 4 __ Why: While adding validation for the `encrypted` field value is a reasonable defensive measure, the current implementation is simple and consistent. Throwing an exception in `isPlaintextCredentials()` would be a behavioral change that could break existing connectors with unexpected values, and the improved_code introduces a more complex flow that may not align with the PR's intent.	Low

Suggestions up to commit 66783d8

Category	Suggestion	Impact
Possible issue	Add missing test annotations to new tests The two new test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation. Without it, these tests will not be executed by the test runner, making the test coverage ineffective. plugin/src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java [651] +@Test public void test_execute_plaintext_credentials_blocked_when_not_allowed() { Suggestion importance[1-10]: 8 __ Why: The two new test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation, which means they will not be executed by JUnit and the test coverage they provide will be completely ineffective.	Medium
Possible issue	Validate policy before mutating connector state The plaintext credentials policy check happens after `connector.update()` is called, meaning the connector's in-memory state has already been mutated with the new (potentially plaintext) credentials before the policy is enforced. If the check fails, the listener receives an error, but the connector object has already been modified. The policy check should be performed before calling `connector.update()` to avoid partial state changes. plugin/src/main/java/org/opensearch/ml/action/connector/UpdateConnectorTransportAction.java [124-149] +// Enforce cluster-level plaintext credentials policy before applying update +if (connector instanceof AbstractConnector) { + AbstractConnector abstractConnector = (AbstractConnector) connector; + // Check the incoming update content for plaintext credentials flag + MLCreateConnectorInput updateContent = mlUpdateConnectorAction.getUpdateContent(); + Map<String, String> newCredential = updateContent.getCredential(); + if (newCredential != null && "false".equalsIgnoreCase(newCredential.get("encrypted"))) { + if (!mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()) { + listener.onFailure(new IllegalArgumentException( + "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage." + )); + return; + } else { + log.warn("Connector '{}' is configured to store credentials in plaintext (unencrypted). " + + "This is less secure. Only use this for testing or when encryption is handled externally.", connectorId); + } + } +} connector.update(mlUpdateConnectorAction.getUpdateContent()); -// Enforce cluster-level plaintext credentials policy -if (connector instanceof AbstractConnector) { - Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `connector.update()` is called before the plaintext credentials check, meaning the connector's in-memory state is mutated before the policy is enforced. While the failure path still returns an error to the caller, checking the policy before mutating state is cleaner and avoids partial state changes. However, since the connector object is not persisted until later in the flow, the practical impact is limited.	Medium
General	Ensure plaintext credentials are substituted into headers In the plaintext decrypt path, `decryptedCredential` is populated with a copy of `credential` (minus the `ENCRYPTED_FIELD`), but the credential values are used as-is without substitution into headers. The `createDecryptedHeaders` call uses `getAllHeaders(action)` which may reference `${credential.xxx}` placeholders. These placeholders won't be resolved unless `decryptedCredential` is used during header creation, similar to how the normal decrypt path works. Verify that `createDecryptedHeaders` correctly uses `decryptedCredential` for substitution in this path. common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java [227-235] if (isPlaintextCredentials()) { log.debug("Connector credentials are in plaintext mode, using as-is"); decryptedCredential = new HashMap<>(credential); // Remove the encrypted flag from decrypted credentials decryptedCredential.remove(ENCRYPTED_FIELD); + // Use decryptedCredential for header substitution this.decryptedHeaders = createDecryptedHeaders(getAllHeaders(action)); listener.onResponse(true); return; } Suggestion importance[1-10]: 2 __ Why: The `existing_code` and `improved_code` are functionally identical - the suggestion only adds a comment but makes no actual code change. The concern about header substitution is valid to verify, but the proposed fix doesn't actually change anything, making this suggestion ineffective.	Low

Suggestions up to commit 4c84012

Category	Suggestion	Impact
Possible issue	Use listener for error propagation consistency In `TransportCreateConnectorAction`, the exception is thrown directly instead of being passed to `listener.onFailure()`. This means the exception will propagate up the call stack uncaught rather than being handled gracefully by the action listener, potentially causing unhandled exceptions. Use `listener.onFailure()` consistent with how the update action handles this case. plugin/src/main/java/org/opensearch/ml/action/connector/TransportCreateConnectorAction.java [129-131] -throw new IllegalArgumentException( +listener.onFailure(new IllegalArgumentException( "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage." -); +)); +return; Suggestion importance[1-10]: 8 __ Why: In `TransportCreateConnectorAction`, throwing the exception directly instead of using `listener.onFailure()` is inconsistent with the update action and can cause unhandled exceptions propagating up the call stack. The `UpdateConnectorTransportAction` correctly uses `listener.onFailure()` for the same scenario.	Medium
Possible issue	Add missing test annotations to new tests The two new test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation. Without this annotation, JUnit will not recognize and execute these tests, meaning the new functionality will not actually be tested. plugin/src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java [651] +@Test public void test_execute_plaintext_credentials_blocked_when_not_allowed() { Suggestion importance[1-10]: 7 __ Why: The test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing `@Test` annotations, meaning JUnit will not execute them and the new functionality won't actually be tested.	Medium
General	Ensure credential state consistency after skipping encryption The `encrypt` method skips encryption entirely when `isPlaintextCredentials()` is true, but it does not set `decryptedCredential` or any encrypted credential state. If any downstream code relies on the encrypted credential map being populated after `encrypt()` is called, this could lead to null pointer exceptions or incorrect behavior. Consider at minimum documenting this assumption or ensuring the credential state is consistent. common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java [185-189] if (isPlaintextCredentials()) { log.warn("Connector credentials are configured for plaintext storage (not encrypted)"); + // Plaintext credentials are stored as-is; no encryption performed + decryptedCredential = new HashMap<>(credential); + decryptedCredential.remove(ENCRYPTED_FIELD); listener.onResponse(true); return; } Suggestion importance[1-10]: 5 __ Why: When encryption is skipped for plaintext credentials, `decryptedCredential` is not populated, which could cause null pointer exceptions if downstream code expects it to be set after `encrypt()` is called. However, this may be an intentional design choice since `decryptedCredential` is typically set during `decrypt()`.	Low
General	Clarify inverted boolean credential flag logic The method name `isPlaintextCredentials()` is semantically confusing — it returns `true` when `encrypted` is `"false"`, which is a double-negative logic. Additionally, the field name `ENCRYPTED_FIELD` with value `"encrypted"` and checking for `"false"` means the logic is inverted. Consider renaming the method or the field to make the intent clearer and reduce the risk of future misuse. common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java [170-172] public boolean isPlaintextCredentials() { - return credential != null && "false".equalsIgnoreCase(credential.get(ENCRYPTED_FIELD)); + return credential != null && Boolean.FALSE.toString().equalsIgnoreCase(credential.get(ENCRYPTED_FIELD)); } Suggestion importance[1-10]: 2 __ Why: The `improved_code` is functionally identical to the `existing_code` since `Boolean.FALSE.toString()` returns `"false"`, making this suggestion a no-op change that doesn't address the naming concern raised.	Low

Suggestions up to commit de4ce40

Category	Suggestion	Impact
Possible issue	Missing @test annotation on test methods The test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation. Without it, these tests will not be executed by the test runner, making the test coverage ineffective. plugin/src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java [651] +@Test public void test_execute_plaintext_credentials_blocked_when_not_allowed() { Suggestion importance[1-10]: 8 __ Why: The test methods `test_execute_plaintext_credentials_blocked_when_not_allowed` and `test_execute_plaintext_credentials_allowed_when_setting_enabled` are missing the `@Test` annotation, meaning they will never be executed by JUnit. This is a real bug that makes the test coverage ineffective.	Medium
Possible issue	Use listener for error propagation consistency In `TransportCreateConnectorAction`, the plaintext credentials check throws an `IllegalArgumentException` directly, which will propagate as an unhandled exception rather than being passed to the `ActionListener`. This is inconsistent with the pattern used in `UpdateConnectorTransportAction` where `listener.onFailure(...)` is called. The exception should be passed to the listener to ensure proper error handling and response to the client. plugin/src/main/java/org/opensearch/ml/action/connector/TransportCreateConnectorAction.java [124-132] if (connector instanceof AbstractConnector) { AbstractConnector abstractConnector = (AbstractConnector) connector; if (abstractConnector.isPlaintextCredentials()) { if (!mlFeatureEnabledSetting.isPlaintextCredentialsAllowed()) { - // Cluster setting does not allow plaintext credentials - throw exception - throw new IllegalArgumentException( + listener.onFailure(new IllegalArgumentException( "Plaintext credentials are not allowed. Please set cluster setting 'plugins.ml_commons.allow_plaintext_credentials' to true to enable plaintext credentials storage." - ); + )); + return; } Suggestion importance[1-10]: 7 __ Why: Throwing an exception directly in `doExecute` instead of calling `listener.onFailure()` is inconsistent with the pattern used in `UpdateConnectorTransportAction` and may not properly propagate the error to the client in all transport action contexts. Using `listener.onFailure()` with a `return` is the correct pattern.	Medium
General	Validate encrypted field value strictly The `isPlaintextCredentials()` method only checks for `"false"` as the value of the `encrypted` field, but does not validate that the value is a recognized boolean string. An attacker or misconfigured connector could set `encrypted` to an arbitrary string (e.g., `"maybe"`) which would be treated as encrypted (returning `false`), potentially causing unexpected behavior. Consider explicitly checking for both `"true"` and `"false"` and throwing an error for unrecognized values. common/src/main/java/org/opensearch/ml/common/connector/AbstractConnector.java [170-172] public boolean isPlaintextCredentials() { - return credential != null && "false".equalsIgnoreCase(credential.get(ENCRYPTED_FIELD)); + if (credential == null) return false; + String encryptedValue = credential.get(ENCRYPTED_FIELD); + if (encryptedValue == null) return false; + if ("false".equalsIgnoreCase(encryptedValue)) return true; + if ("true".equalsIgnoreCase(encryptedValue)) return false; + throw new IllegalArgumentException("Invalid value for 'encrypted' field: '" + encryptedValue + "'. Must be 'true' or 'false'."); } Suggestion importance[1-10]: 4 __ Why: While validating unrecognized values for the `encrypted` field could improve robustness, the current behavior of treating any non-`"false"` value as encrypted is a safe default. This is a minor improvement with limited security impact since unrecognized values default to the more secure encrypted path.	Low

github-actions · 2026-04-07T06:03:21Z

Persistent review updated to latest commit 5291e54

github-actions · 2026-04-07T06:44:38Z

Persistent review updated to latest commit de4ce40

github-actions · 2026-04-08T05:24:05Z

Persistent review updated to latest commit 1ced2ed

github-actions · 2026-04-13T02:56:52Z

Persistent review updated to latest commit 04e5bbb

Signed-off-by: zane-neo <zaniu@amazon.com>

github-actions · 2026-04-15T03:05:59Z

Persistent review updated to latest commit 8ce8f73

zane-neo requested review from HenryL27, Zhangxunmt, austintlee, b4sjoo, dhrubo-os, jngz-es, mingshl, model-collapse, pyek-bot, rbhavna, sam-herman, xinyual and ylwu-amzn as code owners April 7, 2026 03:02

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 03:05 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 03:05 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 03:05 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 03:05 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 06:02 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 06:02 — with GitHub Actions Failure

zane-neo force-pushed the support-non-encrypted-credentials branch from a45dd89 to de4ce40 Compare April 7, 2026 06:43

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 06:45 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 06:45 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 7, 2026 06:45 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 8, 2026 05:55 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 8, 2026 05:55 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 8, 2026 05:55 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 8, 2026 05:55 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 8, 2026 09:19 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 8, 2026 09:19 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 8, 2026 09:19 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 8, 2026 09:19 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 9, 2026 07:31 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 9, 2026 07:31 — with GitHub Actions Error

zane-neo had a problem deploying to ml-commons-cicd-env April 9, 2026 07:31 — with GitHub Actions Failure

zane-neo had a problem deploying to ml-commons-cicd-env April 9, 2026 07:31 — with GitHub Actions Error

zane-neo force-pushed the support-non-encrypted-credentials branch from fc0768e to 04e5bbb Compare April 13, 2026 02:54

zane-neo had a problem deploying to ml-commons-cicd-env April 13, 2026 02:56 — with GitHub Actions Failure

zane-neo temporarily deployed to ml-commons-cicd-env April 13, 2026 02:56 — with GitHub Actions Inactive

zane-neo had a problem deploying to ml-commons-cicd-env April 13, 2026 02:56 — with GitHub Actions Error

zane-neo temporarily deployed to ml-commons-cicd-env April 13, 2026 02:56 — with GitHub Actions Inactive

zane-neo temporarily deployed to ml-commons-cicd-env April 13, 2026 03:18 — with GitHub Actions Inactive

zane-neo deployed to ml-commons-cicd-env April 13, 2026 05:58 — with GitHub Actions Active

zane-neo temporarily deployed to ml-commons-cicd-env April 13, 2026 05:58 — with GitHub Actions Inactive

zane-neo added 5 commits April 15, 2026 11:02

support plain text credentials

f941739

Signed-off-by: zane-neo <zaniu@amazon.com>

format code

d65eb7c

Signed-off-by: zane-neo <zaniu@amazon.com>

optimize code

6512866

Signed-off-by: zane-neo <zaniu@amazon.com>

fix failure UTs

bfef919

Signed-off-by: zane-neo <zaniu@amazon.com>

address llm comments

8ce8f73

Signed-off-by: zane-neo <zaniu@amazon.com>

zane-neo force-pushed the support-non-encrypted-credentials branch from 04e5bbb to 8ce8f73 Compare April 15, 2026 03:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support plain text credentials#4782

support plain text credentials#4782
zane-neo wants to merge 5 commits intoopensearch-project:mainfrom
zane-neo:support-non-encrypted-credentials

zane-neo commented Apr 7, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 7, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 7, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 7, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 7, 2026

Uh oh!

github-actions bot commented Apr 7, 2026

Uh oh!

github-actions bot commented Apr 8, 2026

Uh oh!

github-actions bot commented Apr 13, 2026

Uh oh!

github-actions bot commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

zane-neo commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Testing

Allow plain text credentials not enabled

Allow plain text credentials enabled

Allow plain text credentials changed to disabled

Update connector when allow plain text credentials are set to false

Check List

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 8ce8f73)

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions bot commented Apr 7, 2026

Uh oh!

github-actions bot commented Apr 7, 2026

Uh oh!

github-actions bot commented Apr 8, 2026

Uh oh!

github-actions bot commented Apr 13, 2026

Uh oh!

github-actions bot commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

zane-neo commented Apr 7, 2026 •

edited

Loading

github-actions bot commented Apr 7, 2026 •

edited

Loading

github-actions bot commented Apr 7, 2026 •

edited

Loading

(Review updated until commit `8ce8f73`)

github-actions bot commented Apr 7, 2026 •

edited

Loading