fix(ingestion): enhance SSL safety and log sanitization

[RinZ27]

Describe your changes:

This update introduces configurable SSL verification for SAS and Elasticsearch connectors and improves log sanitization.

Key improvements:

• Configurable Security: Added a verifySSL property to both SASConnection and ElasticSearchConnection schemas. I've set the default to ignore based on maintainer feedback to ensure we don't break existing setups, while allowing users to opt-in to validate for production hardening.
• Log Sanitization: I've sanitized sensitive details in aws_secrets_manager.py debug logs. Based on reviewer feedback, I've ensured the secret_id is preserved in error logs to facilitate troubleshooting of misconfigured secrets.
• Robust Error Handling: Improved SAS token retrieval with better exception handling and status code reporting, avoiding confusing tracebacks.

These changes provide a path toward better security standards while maintaining backward compatibility for current deployments.

Type of change:

• Bug fix
• Improvement

Checklist:

• I have read the CONTRIBUTING document.
• My PR title is Fixes :
• I have commented on my code, particularly in hard-to-understand areas.

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[RinZ27]

The py-checkstyle and Integration Tests failures in CI are expected consequences of this security-focused update.

1. Security by Default: SSL verification has been restored to True for both SAS and Elasticsearch connectors. While this causes current integration tests to fail (likely due to missing CA bundles in the test runners), it ensures that production environments are protected against MITM attacks by default.

2. Log Sanitization: A sensitive clear-text password leak in the SAS client was removed, and Secret IDs were sanitized across both successful and error paths in the AWS Secrets Manager utility, as suggested by the Gitar bot.

Maintainers may need to either update the CI runner's trust store or explicitly allow verify=False through configuration settings if they prefer to keep the insecure behavior for specific test environments.

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[ayush-shah]

Thanks for the PR. This needs to be updated against the latest main before we can move it forward.

Could you please rebase on main, resolve any conflicts if present, push the updated branch, and let CI rerun? Once the checks are green, we can re-check merge readiness.

[github-actions]

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

[github-actions]

⚠️ TypeScript Types Need Update

The generated TypeScript types are out of sync with the JSON schema changes.

Since this is a pull request from a forked repository, the types cannot be automatically committed.
Please generate and commit the types manually:

cd openmetadata-ui/src/main/resources/ui
./json2ts-generate-all.sh -l true
git add src/generated/
git commit -m "Update generated TypeScript types"
git push

After pushing the changes, this check will pass automatically.

[github-actions]

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

[ayush-shah]

Thanks for addressing the earlier review items. Gitar is now approved, so the remaining blockers look mechanical from CI:

• TypeScript generated types are still out of sync with the JSON schema changes. Please run the type generation step from the bot comment and commit the generated UI files.
• Python checkstyle is still failing. Please run make py_format and make py_format_check from the repository root and commit any formatting changes.

Once those outputs are committed, let CI rerun. After the checks are green, this can move back to reviewer/maintainer review.

[github-actions]

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

• No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ui'

Issues
1371 New issues
0 Accepted issues

Measures
18 Security Hotspots
0.0% Coverage on New Code
3.6% Duplication on New Code

See analysis details on SonarQube Cloud

[ayush-shah]

Thanks for the SSL/log-sanitization hardening. One security-default question before merge: the new verifySSL fields default to ignore, and the runtime maps that to disabled certificate verification.

If backward compatibility is the intent, could we make the docs/description explicit that SSL validation remains opt-in? If the goal is secure-by-default hardening, could we default to validation and override only the fixtures/tests that need ignore? That distinction matters because users may otherwise assume the new setting improves TLS verification by default.

[github-actions]

This PR has had no activity for 30 days and will be closed in 7 days if no further activity occurs.
Feel free to reopen it if you'd like to continue working on it.

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[greptile-apps]

Too many files changed for review (897 files, 100 file limit).

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[IceS2]

Hey @RinZ27,

Thanks for pushing on this! The direction is right, but a few things need fixing before it can merge.

1. The Java doesn't compile. SearchServiceResourceIT.java still has three stray lines after the final } (>= 3); }}), and both Java files use VerifySSL.IGNORE without importing org.openmetadata.schema.security.ssl.VerifySSL.

Suggestion: drop both Java files from the PR. They don't test anything SSL-related, it is all related to the search service the application uses.

2. The new verifySSL fields aren't read by any code. Adding a field to a JSON schema creates a UI form field and a model attribute. Right now setting verifySSL: validate on a SAS or Elasticsearch service does nothing.

For SAS, use the existing helper:

from metadata.utils.ssl_registry import get_verify_ssl_fn

verify = get_verify_ssl_fn(config.verifySSL)(config.sslConfig)

Pass verify to both places that currently use _VERIFY_SSL — client.py:45 and client.py:178.

While you're there, please delete the envvar: remove sas/settings.py entirely (verify_ssl is its only field), drop the client.py:22 import and the client.py:28 constant, and run make -C ingestion generate_settings_docs to drop the OM_SAS_VERIFY_SSL row from SETTINGS.md. That envvar was only ever a quick Snyk fix; the connection field is where this belongs.

For Elasticsearch, mirror opensearch/connection.py:123-125.

3. Change the default from ignore to no-ssl.

This matters: ignore makes the helper return False. no-ssl returns None, which leaves requests' default (True) alone. It's also what openSearchConnection.json:43 and tableauConnection.json:71 use.

4. Please revert the ssl.create_default_context() change in elasticsearch/connection.py:138, or gate it on verifySSL == validate. As written it forces verification on regardless of the setting, which is the opposite of making it configurable.

5. It needs a test. ingestion/tests/unit/source/database/sas/test_connection.py already exists
   Three cases asserting the verify value passed to ClientConfig (False for ignore, the CA cert for validate, None for no-ssl) would cover it. They'll fail on current main, which is what makes them worth having.

6. Small cleanups: the aws_secrets_manager.py error message ends up back where it started after commit 3
   Please revert that line (keep the ingestion_logger() change at line 27, that one's a real fix). And the description mentions improved SAS token error handling that isn't in the diff.

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[github-actions]

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

[gitar-bot]

Code Review ⚠️ Changes requested 21 resolved / 25 findings

Enhances SSL safety and log sanitization for connectors, but introduces critical issues including an AttributeError on non-dict responses, a TypeError in APIError construction, missing error handling in get_token, and an ignored verifySSL setting in Elasticsearch connections.

⚠️ Bug: response.keys() raises AttributeError on non-dict responses

📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:77 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:109 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:121 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:131 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:146 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:156 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:163

The guard was changed from if response and isinstance(response, dict) and "error" in response to if "error" in response.keys(). TrackedREST.get/post can return a list, None, or a requests.Response, and calling .keys() on any of those raises AttributeError, crashing these methods instead of returning data. Restore the dict/None guard, e.g. if isinstance(response, dict) and "error" in response:.

Guard against non-dict responses and construct APIError with the required dict shape.

if isinstance(response, dict) and "error" in response:
    raise APIError({"message": response["error"], "code": 0})

⚠️ Bug: APIError constructed with string arg raises TypeError

📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:78 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:110 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:122 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:132 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:147 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:157 📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:164

APIError.__init__ does super().__init__(error["message"]) and exposes self._error["code"], so it requires a dict with message/code keys. Passing APIError(response["error"]) (often a string) or APIError(f"{response}") raises TypeError/KeyError during construction, masking the real API error with a confusing exception. Build the dict, e.g. raise APIError({"message": response["error"], "code": 0}).

Pass the dict shape APIError expects instead of a raw string.

raise APIError({"message": response["error"], "code": 0})

⚠️ Bug: get_token has no error handling, crashes on bad response

📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:170-179

get_token now returns response.json()["access_token"] with no status check or JSON validation. On an auth failure or non-JSON response this raises a ValueError/KeyError with a confusing traceback and leaks nothing useful for troubleshooting — directly contradicting the PR's stated goal of 'avoiding confusing tracebacks'. Call response.raise_for_status(), guard response.json(), and raise a clear RuntimeError when access_token is missing.

Validate HTTP status and JSON body, and raise a clear error when the token is missing.

response = requests.request("POST", url, headers=headers, data=payload, verify=_VERIFY_SSL, timeout=10)
response.raise_for_status()
try:
    token = response.json().get("access_token")
except ValueError as exc:
    raise RuntimeError(f"SAS token endpoint returned non-JSON response (HTTP {response.status_code})") from exc
if not token:
    raise RuntimeError(f"Failed to retrieve access_token from SAS (HTTP {response.status_code})")
return token

⚠️ Edge Case: ElasticSearch connection now ignores verifySSL setting

📄 ingestion/src/metadata/ingestion/source/search/elasticsearch/connection.py:107-117 📄 ingestion/src/metadata/ingestion/source/search/elasticsearch/connection.py:171-176

get_ssl_context dropped the verify_ssl parameter and the _get_client path only builds an SSL context when sslConfig is set. A user who sets verifySSL=ignore (or no_ssl) without an sslConfig now gets the default verifying context (ssl_context stays None → Elasticsearch verifies certs), which will break existing self-signed/ignore deployments the PR claims to preserve. Re-honor connection.verifySSL when deciding whether to return an unverified/None context.

Respect verifySSL=ignore by producing an unverified context, preserving prior behavior.

if connection.verifySSL == VerifySSL.ignore:
    ssl_context = ssl._create_unverified_context()
elif connection.sslConfig:
    certificates = connection.sslConfig.certificates
    if isinstance(certificates, SslCertificatesByValues):
        staging_dir = certificates.stagingDir
        self._on_close(lambda: _cleanup_staging_dir(staging_dir))
    ssl_context = get_ssl_context(connection.sslConfig)

✅ 21 resolved

✅ Security: Secret ID still logged in error path, inconsistent sanitization

📄 ingestion/src/metadata/utils/secrets/aws_secrets_manager.py:56 📄 ingestion/src/metadata/utils/secrets/aws_secrets_manager.py:59
The debug log on line 56 was sanitized to remove secret_id, but the error log on line 59 still includes it: f"Couldn't get value for secret [{secret_id}]: {err}". If the goal is to prevent infrastructure/secret-name leakage in logs, this should be consistent. The error path is arguably more likely to appear in collected logs since it's at ERROR level.

✅ Edge Case: No error handling before accessing access_token from response

📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:176-184
In get_token, after switching to verify=True, SSL handshake failures and HTTP error responses become more likely (e.g., untrusted certs, misconfigured endpoints). If the server returns a non-200 response or a body without access_token, response.json()["access_token"] will raise a KeyError (or JSONDecodeError) with no actionable context, making it difficult to diagnose in production.

The new debug log on line 179-182 already captures the status code, which shows awareness of possible failure — but the code proceeds unconditionally to parse the token.

✅ Quality: Hardcoded Basic auth header in get_token

📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:173
Line 173 contains a hardcoded Base64-encoded Authorization header (c2FzLmNsaTo=, which decodes to sas.cli:). Per custom review instructions, raw strings should not be used for constants — this should be a named constant to clarify its purpose and make it easier to maintain.

✅ Bug: APIError raised with string arg will TypeError on construction

📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:189
APIError expects a dict with "message" and "code" keys (it calls error["message"] in __init__), but get_token passes a plain string. This means when the access token is missing from the SAS response, the code will raise an unhandled TypeError instead of the intended APIError, masking the real problem and producing a confusing traceback.

✅ Security: Raw response body in exception may leak sensitive data

📄 ingestion/src/metadata/ingestion/source/database/sas/client.py:189
Line 189 embeds response.text in the exception message. In a PR focused on log sanitization, this is counter-productive: the SAS token endpoint response could contain tokens, session details, or internal error messages that end up in logs or UI when the exception propagates. Prefer logging only the HTTP status code.

...and 16 more resolved from earlier reviews

🤖 Prompt for agents

Code Review: Enhances SSL safety and log sanitization for connectors, but introduces critical issues including an AttributeError on non-dict responses, a TypeError in APIError construction, missing error handling in get_token, and an ignored verifySSL setting in Elasticsearch connections.

1. ⚠️ Bug: response.keys() raises AttributeError on non-dict responses
   Files: ingestion/src/metadata/ingestion/source/database/sas/client.py:77, ingestion/src/metadata/ingestion/source/database/sas/client.py:109, ingestion/src/metadata/ingestion/source/database/sas/client.py:121, ingestion/src/metadata/ingestion/source/database/sas/client.py:131, ingestion/src/metadata/ingestion/source/database/sas/client.py:146, ingestion/src/metadata/ingestion/source/database/sas/client.py:156, ingestion/src/metadata/ingestion/source/database/sas/client.py:163

   The guard was changed from `if response and isinstance(response, dict) and "error" in response` to `if "error" in response.keys()`. TrackedREST.get/post can return a list, None, or a requests.Response, and calling `.keys()` on any of those raises AttributeError, crashing these methods instead of returning data. Restore the dict/None guard, e.g. `if isinstance(response, dict) and "error" in response:`.

   Fix (Guard against non-dict responses and construct APIError with the required dict shape.):
   if isinstance(response, dict) and "error" in response:
       raise APIError({"message": response["error"], "code": 0})

2. ⚠️ Bug: APIError constructed with string arg raises TypeError
   Files: ingestion/src/metadata/ingestion/source/database/sas/client.py:78, ingestion/src/metadata/ingestion/source/database/sas/client.py:110, ingestion/src/metadata/ingestion/source/database/sas/client.py:122, ingestion/src/metadata/ingestion/source/database/sas/client.py:132, ingestion/src/metadata/ingestion/source/database/sas/client.py:147, ingestion/src/metadata/ingestion/source/database/sas/client.py:157, ingestion/src/metadata/ingestion/source/database/sas/client.py:164

   `APIError.__init__` does `super().__init__(error["message"])` and exposes `self._error["code"]`, so it requires a dict with `message`/`code` keys. Passing `APIError(response["error"])` (often a string) or `APIError(f"{response}")` raises TypeError/KeyError during construction, masking the real API error with a confusing exception. Build the dict, e.g. `raise APIError({"message": response["error"], "code": 0})`.

   Fix (Pass the dict shape APIError expects instead of a raw string.):
   raise APIError({"message": response["error"], "code": 0})

3. ⚠️ Bug: get_token has no error handling, crashes on bad response
   Files: ingestion/src/metadata/ingestion/source/database/sas/client.py:170-179

   `get_token` now returns `response.json()["access_token"]` with no status check or JSON validation. On an auth failure or non-JSON response this raises a ValueError/KeyError with a confusing traceback and leaks nothing useful for troubleshooting — directly contradicting the PR's stated goal of 'avoiding confusing tracebacks'. Call `response.raise_for_status()`, guard `response.json()`, and raise a clear RuntimeError when `access_token` is missing.

   Fix (Validate HTTP status and JSON body, and raise a clear error when the token is missing.):
   response = requests.request("POST", url, headers=headers, data=payload, verify=_VERIFY_SSL, timeout=10)
   response.raise_for_status()
   try:
       token = response.json().get("access_token")
   except ValueError as exc:
       raise RuntimeError(f"SAS token endpoint returned non-JSON response (HTTP {response.status_code})") from exc
   if not token:
       raise RuntimeError(f"Failed to retrieve access_token from SAS (HTTP {response.status_code})")
   return token

4. ⚠️ Edge Case: ElasticSearch connection now ignores verifySSL setting
   Files: ingestion/src/metadata/ingestion/source/search/elasticsearch/connection.py:107-117, ingestion/src/metadata/ingestion/source/search/elasticsearch/connection.py:171-176

   `get_ssl_context` dropped the `verify_ssl` parameter and the `_get_client` path only builds an SSL context when `sslConfig` is set. A user who sets `verifySSL=ignore` (or `no_ssl`) without an sslConfig now gets the default verifying context (ssl_context stays None → Elasticsearch verifies certs), which will break existing self-signed/ignore deployments the PR claims to preserve. Re-honor `connection.verifySSL` when deciding whether to return an unverified/None context.

   Fix (Respect verifySSL=ignore by producing an unverified context, preserving prior behavior.):
   if connection.verifySSL == VerifySSL.ignore:
       ssl_context = ssl._create_unverified_context()
   elif connection.sslConfig:
       certificates = connection.sslConfig.certificates
       if isinstance(certificates, SslCertificatesByValues):
           staging_dir = certificates.stagingDir
           self._on_close(lambda: _cleanup_staging_dir(staging_dir))
       ssl_context = get_ssl_context(connection.sslConfig)

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source}