Fix secret handling for map-valued connection properties by harshach · Pull Request #36740 · open-metadata/OpenMetadata

harshach · 2026-10-31T05:03:57Z

Summary by Gitar

Map secret handling:
- Encrypts and masks string entries of password-annotated maps like Kafka consumerConfig
- Annotates additionalProperties holders in PasswordAnnotator so secret maps are visible to walkers
- Deletes per-entry secrets from the store when a service is hard-deleted
Schema changes:
- Marks Kafka, Redpanda, dbt header, and custom LLM header map values as format: password
Ingestion secret resolution:
- Resolves secret: references in Kafka config maps before they reach librdkafka
- Unwraps dbt HTTP header secrets so secret: references resolve before requests
Tests:
- Adds masking, lifecycle, and Kafka round-trip tests in Java and Python
Retarget guard:
- Treats typed values at masked map entries as retargets, since masked maps hide non-secret edits

_{This will update automatically on new commits.}

github-actions · 2026-10-31T05:04:07Z

❌ 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.

github-actions · 2026-10-31T05:05:08Z

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 · 2026-10-31T05:24:16Z

Jest test Coverage

UI tests summary

Lines	Statements	Branches	Functions
	81.16% (137649/169600)	68.08% (49438/72612)	71.15% (27831/39111)

github-actions · 2026-10-31T05:55:43Z

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 · 2026-10-31T06:03:13Z

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.

Map-valued connection properties (Kafka/Redpanda consumerConfig and schemaRegistryConfig, dbt dbtHttpHeaders, CustomLLM headers) were invisible to the secret framework: PasswordAnnotator only marked scalar format:password fields, and the SecretsManager/PasswordEntityMasker walkers had no branch for java.util.Map. Their values were stored unencrypted and returned unmasked on reads. Declare these maps as additionalProperties with format: password, teach the annotator to mark the generated getAdditionalProperties() accessor, and handle annotated maps entry-by-entry in the encrypt, decrypt, get-secret, delete and mask/unmask paths, keying each entry by its own key. The ingestion client resolves secret: references in the Kafka maps and unwraps dbt header values before use.

Masking every string entry of a password-valued map hid edits to non-secret entries (e.g. an OAuth token endpoint URL) from ConnectionSecretsGuard's masked comparison, so a stored map secret could be restored onto a redirected connection - the redirect ADR:2026-10-24 exists to block. The guard now treats a typed value at a masked password-map entry as a retarget unless it still equals the stored entry, scoped to map entries so re-entering a scalar secret stays accepted. Also record the map-of-secrets contract (per-entry masking, encryption and secret-id format, client-side resolution) as ADR:2026-10-30, and isolate KafkaConnectionMapSecretsTest from the SecretsManagerFactory singleton leaked by other tests.

…entry A typed value at a masked map entry is indistinguishable from a redirected setting, so the guard refuses it while other secrets stay masked - the same rule the unmasked maps produced before they were declared secret, and the only alternative is a per-key secret declaration. Pin the behavior in ConnectionSecretsGuardTest and name the cost in the ADR amendments.

Copilot

🟡 Changes recommended

Unresolved flaws allow OAuth endpoint retargeting with restored credentials and external-secret overwrites from colliding header keys, while additional regressions alter empty headers and literal secret: credentials.

4 open issues

What changed in this PR

This PR extends OpenMetadata’s secret handling to map-valued connection properties, covering server-side protection and ingestion-time resolution for Kafka, Redpanda, dbt, and custom LLM headers.

Changes:

Marks map values as passwords and adds per-entry encryption, masking, restoration, and deletion.
Resolves secret references before Kafka connections and dbt HTTP requests, and extends the retarget guard.
Adds regression tests and decision records documenting the map-secret contract.

File	Description
`openmetadata-ui/src/main/resources/ui/src/jsons/ingestionSchemas/workflow.json`	Updates embedded messaging and dbt password-map schemas.
`openmetadata-ui/src/main/resources/ui/src/jsons/ingestionSchemas/testSuitePipeline.json`	Updates embedded messaging password-map schemas.
`openmetadata-ui/src/main/resources/ui/src/jsons/ingestionSchemas/dbtPipeline.json`	Marks dbt header values as passwords.
`openmetadata-ui/src/main/resources/ui/src/jsons/ingestionSchemas/dbtconfig/dbtHttpConfig.json`	Updates the dbt HTTP form schema.
`openmetadata-ui/src/main/resources/ui/public/jsons/connectionSchemas/connections/serviceConnection.json`	Updates bundled messaging connection schemas.
`openmetadata-ui/src/main/resources/ui/public/jsons/connectionSchemas/connections/messaging/redpandaConnection.json`	Declares Redpanda configuration maps as password-valued.
`openmetadata-ui/src/main/resources/ui/public/jsons/connectionSchemas/connections/messaging/kafkaConnection.json`	Declares Kafka configuration maps as password-valued.
`openmetadata-ui/src/main/resources/ui/public/jsons/connectionSchemas/connections/llm/customLLMConnection.json`	Marks custom LLM headers as passwords.
`openmetadata-spec/src/main/resources/json/schema/metadataIngestion/dbtconfig/dbtHttpConfig.json`	Defines password-valued dbt headers.
`openmetadata-spec/src/main/resources/json/schema/entity/services/connections/messaging/redpandaConnection.json`	Defines password-valued Redpanda configuration maps.
`openmetadata-spec/src/main/resources/json/schema/entity/services/connections/messaging/kafkaConnection.json`	Defines password-valued Kafka configuration maps.
`openmetadata-spec/src/main/resources/json/schema/entity/services/connections/llm/customLLMConnection.json`	Defines password-valued custom LLM headers.
`openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java`	Tests map-entry storage, decryption, and deletion.
`openmetadata-service/src/test/java/org/openmetadata/service/secrets/masker/PasswordEntityMaskerTest.java`	Tests masking and restoration across affected configurations.
`openmetadata-service/src/test/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuardTest.java`	Tests map edits, masked echoes, and secret rotation.
`openmetadata-service/src/test/java/org/openmetadata/service/secrets/KafkaConnectionMapSecretsTest.java`	Tests Kafka map encryption and read masking.
`openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java`	Adds per-entry secret lifecycle handling.
`openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/PasswordEntityMasker.java`	Visits and updates string-valued secret-map entries.
`openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuard.java`	Detects edits hidden by blanket map masking.
`ingestion/tests/unit/test_dbt_http_config.py`	Tests header secret-reference resolution.
`ingestion/tests/unit/source/messaging/kafka/test_connection.py`	Tests configuration-map reference resolution and passthrough.
`ingestion/src/metadata/ingestion/source/messaging/kafka/connection.py`	Resolves map references before constructing clients.
`ingestion/src/metadata/ingestion/source/database/dbt/dbt_config.py`	Unwraps header secrets before HTTP requests.
`docs/decisions/2026-10-30-password-format-on-additionalproperties-makes-a-map-of-secrets.md`	Records map-secret behavior and tradeoffs.
`docs/decisions/2026-10-24-a-masked-connection-secret-is-kept-only-while-the-endpoint-is-unchanged.md`	Documents retarget rules for masked map edits.
`common/src/main/java/org/openmetadata/annotations/PasswordAnnotator.java`	Annotates generated password-map holders and accessors.

_{Review effort: Balanced}

sonarqubecloud · 2026-11-03T16:51:10Z

Quality Gate passed for 'open-metadata-ingestion'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

…xternal stores Entry keys that normalize to the same secret id (X.Auth.Token and X_Auth_Token) are now refused with a 400 before any entry is written, instead of overwriting each other. An empty map entry stays an empty string while its previously stored secret is removed, rather than becoming null. A deliberate 400 from the secrets manager now reaches the caller instead of the generic Fernet-key message.

Only the mask, the stored value itself, or a value whose local cleartext equals the stored entry's now keep a map entry unchanged. A different secret: reference or a Fernet token of another value counts as an edit, so the guard asks for the secrets again.

…ovider The default DB provider never hands out references, so Kafka config map values and dbt HTTP headers that start with secret: are passed through as written instead of having the prefix stripped.

gitar-bot · 2026-11-03T17:34:34Z

Code Review ⚠️ Changes requested 2 closed / 3 findings

🔴 High risk · Encrypting, masking, and resolving password-map entries across connections and ingestion could expose credentials or send requests with unintended secrets if handling is wrong.

Encrypts, masks, and resolves secrets held in map-valued connection properties such as Kafka consumerConfig, and annotates additionalProperties holders so secret maps are visible to walkers. Changes requested because rotating a single secret inside a password map is rejected as a retarget, even when the endpoint is unchanged; the other masked secrets on the connection would have to be re-entered. Resolved this session: the retarget guard now sees edits inside masked config maps and the map-secret contract has a decision record.

⚠️

Edge Case: Rotating one secret inside a password map is rejected as a retarget

📄 openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuard.java:80-83 📄 openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuard.java:131-143 📄 openmetadata-service/src/test/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuardTest.java:241-255 📄 docs/decisions/2026-10-24-a-masked-connection-secret-is-kept-only-while-the-endpoint-is-unchanged.md:44-54

maskedPasswordMapEntryEdited counts any typed value at a masked map entry as an edit unless it matches the stored cleartext. A rotated secret never matches, so rotating a real secret inside the map also counts as an edit. Example: a user replaces sasl.oauthbearer.client.secret in consumerConfig and leaves the other secrets (saslPassword, ssl.key.password, …) masked as the UI echoes them. Then maskedPasswordMapEntryEdited returns true, keepsStoredSecret sees the other masked secrets, and the update fails with "The connection now points somewhere else". The endpoint never changed, yet the user must re-type every other secret on the connection. This differs from the scalar-field behaviour the javadoc describes ("a typed value is a re-entered secret, which the guard has always accepted"). No test covers it, and the v3 amendment does not name this cost. One fix: treat an entry as an edit only when its stored cleartext does not look like a secret, e.g. only count a typed value as a retarget when at least one other map entry was echoed masked AND the edited entry's stored cleartext is non-secret. Simpler: exempt the edited entry from keepsStoredSecret, so a lone rotated entry passes while other kept secrets still block real redirects. If the strict behaviour is kept on purpose, add a test for it and state in the ADR amendment that rotating a map secret requires re-entering all of the connection's secrets.

Add a test that pins the intended behaviour for rotating a single map secret (here it shows the current rejection; flip it to assertDoesNotThrow if rotation should pass).

@Test
void rotatingOneSecretInsideTheConfigMapKeepsTheOthersMasked() {
  Map<String, Object> stored = storedKafka(IDP_URL);
  Map<String, Object> rotated =
      Map.of(
          "type", "Kafka",
          "bootstrapServers", "broker:9092",
          "saslPassword", PASSWORD_MASK,
          "consumerConfig",
              Map.of(
                  "sasl.oauthbearer.token.endpoint.url", PASSWORD_MASK,
                  "sasl.oauthbearer.client.secret", "rotated-secret"));
  assertThrows(BadRequestException.class, () -> guardKafka(rotated, stored));
}

✅ 2 closed

✅ Security: Retarget guard can't see edits inside masked consumer/registry config maps

📄 openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/PasswordEntityMasker.java:244-258 📄 openmetadata-spec/src/main/resources/json/schema/entity/services/connections/messaging/kafkaConnection.json:76-87
ConnectionSecretsGuard.requireSecretsReenteredOnRetarget decides whether a connection has been retargeted by comparing masked copies of the incoming and stored configs (ADR:2026-10-24-a-masked-connection-secret-is-kept-only-while-the-endpoint-is-unchanged). This PR masks every string entry of consumerConfig and schemaRegistryConfig, not only the secret ones (visitSecretMap). After masking, a change to a string entry shows up as ********* on both sides, so nonExemptFieldChanged reports no change.

Concrete case: an editor who cannot read secrets changes consumerConfig["sasl.oauthbearer.token.endpoint.url"] to a host they control. They leave sasl.oauthbearer.client.secret (or saslPassword) masked. The guard passes, the unmasker restores the real secret, and the runner sends it to the attacker's endpoint. Before this PR the URL entry was not masked, so the guard caught the edit. That is the redirect the Accepted ADR is meant to block.

Fix: have the guard compare the unmasked (converted) values of password-map entries, or treat any differing map entry as a non-exempt change. Add a ConnectionSecretsGuardTest case that edits a string entry inside consumerConfig while another secret stays masked.

✅ Quality: No decision record for the map-secret contract and its secret-id format

📄 common/src/main/java/org/openmetadata/annotations/PasswordAnnotator.java:38-52 📄 openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java:542-556 📄 ingestion/src/metadata/ingestion/source/messaging/kafka/connection.py:84-92
This PR adds a cross-component contract, but no record in docs/decisions/ describes it (there is no secrets/map ADR). The contract has two parts. First, a schema property whose additionalProperties carry format: password is a map of secrets: every string entry is masked, encrypted and stored as its own secret, and non-string entries are left alone. Second, each entry's secret id has the format <service path>/<field>/additionalproperties/<entry_key with '.' → '_'>.

Java (PasswordAnnotator, SecretsManager, PasswordEntityMasker) and Python (_resolve_secret_values in the Kafka connection, dbt_config) both have to agree on it. The Python tests hard-code that id path. The PR description is empty, so this rule exists nowhere outside the code.

Add a record stating: "A property whose additionalProperties declare format: password is a map of secrets; each string entry is masked, encrypted and stored under <map id>/additionalproperties/<entry key>, and clients resolve secret: references in such maps themselves."

🤖 Prompt for agents

Code Review: Encrypts, masks, and resolves secrets held in map-valued connection properties such as Kafka `consumerConfig`, and annotates `additionalProperties` holders so secret maps are visible to walkers. Changes requested because rotating a single secret inside a password map is rejected as a retarget, even when the endpoint is unchanged; the other masked secrets on the connection would have to be re-entered. Resolved this session: the retarget guard now sees edits inside masked config maps and the map-secret contract has a decision record.

1. ⚠️ Edge Case: Rotating one secret inside a password map is rejected as a retarget
   Files: openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuard.java:80-83, openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuard.java:131-143, openmetadata-service/src/test/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuardTest.java:241-255, docs/decisions/2026-10-24-a-masked-connection-secret-is-kept-only-while-the-endpoint-is-unchanged.md:44-54

   `maskedPasswordMapEntryEdited` counts any typed value at a masked map entry as an edit unless it matches the stored cleartext. A rotated secret never matches, so rotating a real secret inside the map also counts as an edit. Example: a user replaces `sasl.oauthbearer.client.secret` in `consumerConfig` and leaves the other secrets (`saslPassword`, `ssl.key.password`, …) masked as the UI echoes them. Then `maskedPasswordMapEntryEdited` returns true, `keepsStoredSecret` sees the other masked secrets, and the update fails with "The connection now points somewhere else". The endpoint never changed, yet the user must re-type every other secret on the connection. This differs from the scalar-field behaviour the javadoc describes ("a typed value is a re-entered secret, which the guard has always accepted"). No test covers it, and the v3 amendment does not name this cost. One fix: treat an entry as an edit only when its stored cleartext does not look like a secret, e.g. only count a typed value as a retarget when at least one other map entry was echoed masked AND the edited entry's stored cleartext is non-secret. Simpler: exempt the edited entry from `keepsStoredSecret`, so a lone rotated entry passes while other kept secrets still block real redirects. If the strict behaviour is kept on purpose, add a test for it and state in the ADR amendment that rotating a map secret requires re-entering all of the connection's secrets.

   Fix (Add a test that pins the intended behaviour for rotating a single map secret (here it shows the current rejection; flip it to assertDoesNotThrow if rotation should pass).):
   @Test
   void rotatingOneSecretInsideTheConfigMapKeepsTheOthersMasked() {
     Map<String, Object> stored = storedKafka(IDP_URL);
     Map<String, Object> rotated =
         Map.of(
             "type", "Kafka",
             "bootstrapServers", "broker:9092",
             "saslPassword", PASSWORD_MASK,
             "consumerConfig",
                 Map.of(
                     "sasl.oauthbearer.token.endpoint.url", PASSWORD_MASK,
                     "sasl.oauthbearer.client.secret", "rotated-secret"));
     assertThrows(BadRequestException.class, () -> guardKafka(rotated, stored));
   }

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Not enabled

Set up

_{For more verification details, comment gitar display:verbose.}

Options

Display: compact → Showing compact verification results.

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}

Copilot

🟡 Changes recommended

Removed map credentials leave orphaned external-store secrets even after hard deletion, while unchanged Fernet-wrapped references can incorrectly cause saves to return 400.

2 open issues

4 resolved since last review

_{Review effort: Balanced}

+    new ArrayList<>(secretMap.entrySet())
+        .forEach(
+            entry -> {
+              if (entry.getValue() instanceof String stringValue) {


+  /**
+   * The cleartext a map entry holds when it can be recovered locally: the value itself for a
+   * plaintext entry (rows saved before maps were encrypted), a Fernet decrypt for a token. An
+   * external {@code secret:} reference, which the server never fetches, or a token that does not
+   * decrypt yields null: such a value cannot be shown to equal anything.
+   */
+  private static String localCleartext(String value) {
+    if (value == null || value.startsWith(SecretsManager.SECRET_FIELD_PREFIX)) {


sonarqubecloud · 2026-11-03T18:10:17Z

Quality Gate passed for 'open-metadata-ui'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

harshach requested review from a team as code owners October 31, 2026 05:04

harshach requested review from Khairajani and sonika-shah October 31, 2026 05:04

github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Oct 31, 2026

gitar-bot Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread ...data-service/src/main/java/org/openmetadata/service/secrets/masker/PasswordEntityMasker.java Resolved

gitar-bot Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread common/src/main/java/org/openmetadata/annotations/PasswordAnnotator.java Resolved

github-actions Bot added safe to test Add this label to run secure Github workflows on PRs and removed safe to test Add this label to run secure Github workflows on PRs labels Oct 31, 2026

gitar-bot Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread ...ta-service/src/main/java/org/openmetadata/service/secrets/masker/ConnectionSecretsGuard.java Resolved

github-actions Bot added safe to test Add this label to run secure Github workflows on PRs and removed safe to test Add this label to run secure Github workflows on PRs labels Oct 31, 2026

manerow approved these changes Oct 31, 2026

View reviewed changes

harshach force-pushed the agent/secrets-in-map-fields-like-k branch from 62b5fb9 to 53e8b9c Compare November 1, 2026 15:55

github-actions Bot added safe to test Add this label to run secure Github workflows on PRs and removed safe to test Add this label to run secure Github workflows on PRs labels Nov 1, 2026

pmbrull assigned harshach Nov 3, 2026

harshach added 4 commits November 3, 2026 08:13

Fix G004: lazy %-style logging in kafka connection secret resolution

74c9b61

harshach force-pushed the agent/secrets-in-map-fields-like-k branch from 53e8b9c to 74c9b61 Compare November 3, 2026 16:19

Copilot AI balanced review requested due to automatic review settings November 3, 2026 16:19

github-actions Bot added safe to test Add this label to run secure Github workflows on PRs and removed safe to test Add this label to run secure Github workflows on PRs labels Nov 3, 2026

Copilot started reviewing on behalf of harshach November 3, 2026 16:20

View session

Copilot AI reviewed Nov 3, 2026

View reviewed changes

harshach added 3 commits November 3, 2026 09:31

fix: keep literal secret:-prefixed map values under the DB secrets pr…

e05b1e1

…ovider The default DB provider never hands out references, so Kafka config map values and dbt HTTP headers that start with secret: are passed through as written instead of having the prefix stripped.

Copilot AI balanced review requested due to automatic review settings November 3, 2026 17:32

github-actions Bot added safe to test Add this label to run secure Github workflows on PRs and removed safe to test Add this label to run secure Github workflows on PRs labels Nov 3, 2026

Copilot started reviewing on behalf of harshach November 3, 2026 17:33

View session

Copilot AI reviewed Nov 3, 2026

View reviewed changes

Conversation

harshach commented Oct 31, 2026 • edited by gitar-bot Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Gitar

Uh oh!

github-actions Bot commented Oct 31, 2026

❌ PR checklist incomplete

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jest test Coverage

UI tests summary

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

Uh oh!

github-actions Bot commented Oct 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud Bot commented Nov 3, 2026

Quality Gate passed for 'open-metadata-ingestion'

Uh oh!

gitar-bot Bot commented Nov 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

sonarqubecloud Bot commented Nov 3, 2026

Quality Gate passed for 'open-metadata-ui'

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

harshach commented Oct 31, 2026 •

edited by gitar-bot Bot

Loading

github-actions Bot commented Oct 31, 2026 •

edited

Loading