Fixes 32693: Encrypt and mask ingestion pipeline secrets

[IceS2]

Describe your changes:

Fixes #32693

Storage metadata manifest credentials in sourceConfig.config bypassed password-field encryption and masking because the nested configuration remained an untyped map. Convert these configurations before traversing secrets so stored credentials are encrypted and API responses are masked. Keep application private configuration out of persisted pipelines and API responses, and restore it when building runner payloads.

Type of change:

• Bug fix

High-level design:

• Add a storage metadata converter using the existing converter registry and password annotations. Cover S3, ADLS, and GCS credentials, including nested GCP private keys; preserve local and HTTP manifest configurations and existing DBT behavior.
• Apply encryption to both ordinary and version-checked persistence. Encrypt a copy for version history without writing historical values to the external secrets manager, and mask version-list and timestamp-history responses.
• Strip application private configuration from storage, history, and masked responses. Airflow and Kubernetes payload construction share the loader that resolves runtime configuration from the owning application's relationship.
• Compare normalized, decrypted source configurations and deploy only during the initial update pass. Acquire the pipeline row lock and check its version before deployment or managed-secret writes, so a rejected concurrent save cannot change the accepted credential or deploy its configuration. Slow external calls hold this lock longer for updates to the same pipeline.

No schema or API signature changes. Existing plaintext records are encrypted on their next write; legacy history is masked on read. No bulk data migration is included.

Tests:

Use cases covered

• Encrypt, mask, and decrypt storage credentials after JSON round trips; retain credentials during masked edits and accept replacements.
• Preserve non-secret configurations and DBT encryption behavior.
• Remove application private configuration from persisted and returned pipelines; restore it in runner payloads without mutating the stored entity.
• Protect history responses and prevent history serialization from overwriting active managed credentials.
• Preserve separate credentials for generated pipeline names with identical display names.
• Reject concurrent stale saves with 412 while preserving the accepted credential and deployment; avoid extra backend deployments during history consolidation.

Unit tests

226 focused Java tests passed, with no failures or skips. Added/updated: IngestionPipelineSecretsTest, IngestionPipelinePersistenceSecretsTest, IngestionPipelineStorageStrippingTest, IngestionPipelineHistorySecretsTest, ApplicationWorkflowConfigTest, and AirflowRESTClientTest. The run also included existing secrets, converter, Airflow, and Kubernetes tests.

Combined JaCoCo evidence from focused tests and instrumented API execution covers 130/130 changed executable lines. ApplicationWorkflowConfig, StorageServiceMetadataPipelineClassConverter, ClassConverterFactory, and IngestionPipelineBuilder have 100% line coverage.

The 90% whole-class coverage guideline is not met for six existing files in this focused run: AirflowRESTClient 85.6%, EntityRepository 31.8%, IngestionPipelineRepository 34.4%, IngestionPipelineResource 16%, SecretsManager 44.1%, and PasswordEntityMasker 81.4%.

Backend integration tests

openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IngestionPipelineSecretsIT.java: 4 cases passed against real MySQL, with controlled runner and secrets-manager boundaries. Covers consolidation, matching display names, and concurrent accepted/rejected saves for deployed and undeployed pipelines.

Ingestion integration tests

Not applicable: no Python ingestion changes. Actual scheduler execution and cloud authentication were not validated.

Playwright (UI) tests

No UI source changes or committed Playwright specifications. Local Chromium validation exercised the real UI, API, and MySQL with a controlled deployment boundary.

Manual testing performed

1. Start an isolated OpenMetadata stack, sign in as an administrator, and create an S3 storage metadata pipeline using dummy manifest credentials.
2. Open the pipeline edit page and inspect its API response: the credential is masked. Change its display name without replacing the credential, save, and reload; the original credential reaches the controlled runner.
3. Replace the credential, save, and reload; responses remain masked, the runner receives the replacement, and database readback confirms an encrypted stored value.
4. Request the baseline fixture's version list, individual version, and timestamp history: credentials are absent. Repeat without authentication: requests return 401. API integration tests independently exercise history after updates.

Validation limits and guideline findings

• Application ownership lookup is mocked in current tests; a database-backed application-to-pipeline relationship test remains missing.
• Some tests partially mock internal repositories/DAOs. The real API tests supplement them, but these patterns do not satisfy the stricter test-enforcement guidance.
• Some new parameters lack final, and some test methods exceed the approximate method-length guideline. Spotless passes and does not enforce these rules.
• The full repository suite and PostgreSQL were not run. Java Spotless and git diff --check passed.

UI screen recording / screenshots:

Not applicable: no UI source changes.

Checklist:

• Read the repository's contributing guidance.
• PR title follows Fixes <issue-number>: <short explanation>.
• Linked to the issue with Fixes #32693.
• Added comments for non-obvious constraints.
• Added regression and integration tests, listed above.
• JSON Schema migrations: not applicable; no schema changes.
• UI recording: not applicable; no UI source changes.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

❌ PR checklist incomplete

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

• Linked issue [Detail Bug] Ingestion pipelines: non-DBT configs aren’t encrypted/masked, leaking secrets in plaintext #32693 is not added to the Shipping project.

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.

[gitar-bot]

💡 Performance: Row lock held across external deploy HTTP call

lockPipelineForUpdate() takes a SELECT ... FOR UPDATE row lock (and pins the DB connection) at the start of the baseline pass, and the same @Transaction later runs deployIfRequired, which makes a synchronous HTTP call to Airflow/K8s. The lock and the pooled connection are therefore held for the full duration of the external deploy. Concurrent updates to the same pipeline serialize behind this lock, and a slow or hung pipeline service extends lock-hold time, risking lock-wait timeouts and connection-pool pressure. This is largely intentional per the PR description, but consider bounding the deploy call with an aggressive timeout so a stuck runner cannot pin the row lock indefinitely.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Edge Case: Possible NPE in lockPipelineForUpdate on concurrent delete

dao.jsonToEntity(dao.findJsonByIdForUpdate(id, ALL), id) returns null when the row no longer exists (jsonToEntity returns null for null json). If the pipeline is deleted concurrently before the lock is acquired, stored.getVersion() throws a NullPointerException that surfaces as a 500 instead of a clean 404/409. Guard against a null stored and translate it into an appropriate not-found/conflict response.

_{Was this helpful? React with 👍 / 👎}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[gitar-bot]

Code Review ⚠️ Changes requested 0 resolved / 4 findings

Encrypts and masks ingestion pipeline storage credentials, preventing plaintext leakage through API responses and version history. The fix includes 226 passing tests with 100% coverage of new logic, but three issues must be resolved before merge:

• Row lock held across external HTTP deploy calls risks connection-pool exhaustion if the runner is slow or hung; add an aggressive timeout to bound the deploy call.
• Concurrent pipeline deletion can cause an unguarded null dereference in lockPipelineForUpdate, surfacing as a 500 instead of a clean 404/409.
• Bulk import paths bypass certification validation and stamping, leaving unvalidated classifications and arbitrary dates in persisted records despite the code comment claiming all create paths are covered; route validation through the batch apply method or call it per entity in import methods.

⚠️ Edge Case: Bulk import bypasses new certification validation/stamping

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:3028-3033 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:3932-3937 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:6258-6267 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:6348-6362

prepareCertification is only invoked from prepareInternal(entity, false), which is reached by createInternal and createMany but NOT by the import paths createManyEntitiesForImport/updateManyEntitiesForImport. Those go storeEntities → storeRelationshipsInternal(List) → applyCertificationBatch, which writes the certification tag using the client-supplied expiryDate and without ever calling validateCertification against allowedClassification. So a certification imported (create or update) via the bulk/CSV path is persisted with an unvalidated classification and arbitrary dates — the exact gap the new comment claims to close for "bulk-create". Route validation/date-stamping through applyCertificationBatch (or call prepareCertification per entity in the import methods) so all create paths are covered, and correct the misleading comment.

💡 Performance: Row lock held across external deploy HTTP call

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/IngestionPipelineRepository.java:1246-1260 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/IngestionPipelineRepository.java:1286-1300

lockPipelineForUpdate() takes a SELECT ... FOR UPDATE row lock (and pins the DB connection) at the start of the baseline pass, and the same @Transaction later runs deployIfRequired, which makes a synchronous HTTP call to Airflow/K8s. The lock and the pooled connection are therefore held for the full duration of the external deploy. Concurrent updates to the same pipeline serialize behind this lock, and a slow or hung pipeline service extends lock-hold time, risking lock-wait timeouts and connection-pool pressure. This is largely intentional per the PR description, but consider bounding the deploy call with an aggressive timeout so a stuck runner cannot pin the row lock indefinitely.

💡 Edge Case: Possible NPE in lockPipelineForUpdate on concurrent delete

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/IngestionPipelineRepository.java:1276-1284

dao.jsonToEntity(dao.findJsonByIdForUpdate(id, ALL), id) returns null when the row no longer exists (jsonToEntity returns null for null json). If the pipeline is deleted concurrently before the lock is acquired, stored.getVersion() throws a NullPointerException that surfaces as a 500 instead of a clean 404/409. Guard against a null stored and translate it into an appropriate not-found/conflict response.

💡 Bug: validateAndStampCertification can NPE on null tagLabel in update path

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:10181-10195 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:6269-6280

In updateCertification, after the updatedCertification == null guard, if updatedCertification.getTagLabel() is null the certificationTagUnchanged check is false and execution falls through to validateAndStampCertification(updatedCertification), which dereferences certification.getTagLabel().getTagFQN() and throws an NPE. The new create-path prepareCertification explicitly tolerates a null/empty tag label, but the update path does not — an inconsistency now visible in the refactored code. Add the same null/nullOrEmpty tag-label guard before calling validateAndStampCertification on the update path.

🤖 Prompt for agents

Code Review: Encrypts and masks ingestion pipeline storage credentials, preventing plaintext leakage through API responses and version history. The fix includes 226 passing tests with 100% coverage of new logic, but three issues must be resolved before merge:
  
  - Row lock held across external HTTP deploy calls risks connection-pool exhaustion if the runner is slow or hung; add an aggressive timeout to bound the deploy call.
  - Concurrent pipeline deletion can cause an unguarded null dereference in `lockPipelineForUpdate`, surfacing as a 500 instead of a clean 404/409.
  - Bulk import paths bypass certification validation and stamping, leaving unvalidated classifications and arbitrary dates in persisted records despite the code comment claiming all create paths are covered; route validation through the batch apply method or call it per entity in import methods.

1. 💡 Performance: Row lock held across external deploy HTTP call
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/IngestionPipelineRepository.java:1246-1260, openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/IngestionPipelineRepository.java:1286-1300

   `lockPipelineForUpdate()` takes a `SELECT ... FOR UPDATE` row lock (and pins the DB connection) at the start of the baseline pass, and the same `@Transaction` later runs `deployIfRequired`, which makes a synchronous HTTP call to Airflow/K8s. The lock and the pooled connection are therefore held for the full duration of the external deploy. Concurrent updates to the same pipeline serialize behind this lock, and a slow or hung pipeline service extends lock-hold time, risking lock-wait timeouts and connection-pool pressure. This is largely intentional per the PR description, but consider bounding the deploy call with an aggressive timeout so a stuck runner cannot pin the row lock indefinitely.

2. 💡 Edge Case: Possible NPE in lockPipelineForUpdate on concurrent delete
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/IngestionPipelineRepository.java:1276-1284

   `dao.jsonToEntity(dao.findJsonByIdForUpdate(id, ALL), id)` returns null when the row no longer exists (jsonToEntity returns null for null json). If the pipeline is deleted concurrently before the lock is acquired, `stored.getVersion()` throws a NullPointerException that surfaces as a 500 instead of a clean 404/409. Guard against a null `stored` and translate it into an appropriate not-found/conflict response.

3. ⚠️ Edge Case: Bulk import bypasses new certification validation/stamping
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:3028-3033, openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:3932-3937, openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:6258-6267, openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:6348-6362

   `prepareCertification` is only invoked from `prepareInternal(entity, false)`, which is reached by `createInternal` and `createMany` but NOT by the import paths `createManyEntitiesForImport`/`updateManyEntitiesForImport`. Those go `storeEntities` → `storeRelationshipsInternal(List)` → `applyCertificationBatch`, which writes the certification tag using the client-supplied `expiryDate` and without ever calling `validateCertification` against `allowedClassification`. So a certification imported (create or update) via the bulk/CSV path is persisted with an unvalidated classification and arbitrary dates — the exact gap the new comment claims to close for "bulk-create". Route validation/date-stamping through `applyCertificationBatch` (or call `prepareCertification` per entity in the import methods) so all create paths are covered, and correct the misleading comment.

4. 💡 Bug: validateAndStampCertification can NPE on null tagLabel in update path
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:10181-10195, openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:6269-6280

   In `updateCertification`, after the `updatedCertification == null` guard, if `updatedCertification.getTagLabel()` is null the `certificationTagUnchanged` check is false and execution falls through to `validateAndStampCertification(updatedCertification)`, which dereferences `certification.getTagLabel().getTagFQN()` and throws an NPE. The new create-path `prepareCertification` explicitly tolerates a null/empty tag label, but the update path does not — an inconsistency now visible in the refactored code. Add the same null/`nullOrEmpty` tag-label guard before calling `validateAndStampCertification` on the update path.

Review coverage

Rules No rules evaluated

Functional validation Not enabled · Set up

Auto-approval Not enabled · Set up

Options

Display: compact → Counting what did not apply, without listing it.

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}

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ingestion'

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