Repository navigation
Conversation
…y holds The encrypt walk accepted any value a password field arrived with: a Fernet token was decrypted with the server's key and stored as the submitting entity's credential, and a secret: reference was stored verbatim. No legitimate round trip feeds an encoded value into that walk from a user path - the masker restores masked fields from the stored original, and unmask-permitted users echo back the value the unmask handed them - so storing anything else leaves the entity owning a value it was never given as a plain credential. Two validation layers, a caller policy, and a re-storage pass keep those round trips honest: - SecretsManager.requireOwnCredential rejects, on strict paths (user create, bulk create), a Fernet token outright and a secret: reference when it falls inside the deployment's managed namespace and outside the entity's own accepted bases. References into a vault the submitter manages themselves stay allowed. Server-side re-storage passes CredentialPolicy with internalReStorage and is exempt. - PasswordEntityMasker.requireOwnStoredValue accepts an encoded value on a user update only when it matches the field's stored or resolved value, and the unmask walk now covers the fields the encrypt walk encrypts (DO_NOT_UNMASK_CLASSES mirrors DO_NOT_ENCRYPT_CLASSES), so a bot's JWT token is validated too. - Repositories and the SecretsManagerUpdateService migrations store their already-held copies under internal re-storage; creates and bulk creates stay strict. WorkflowResource builds the TEST_CONNECTION comparison base from the stored service's connection instead of the submission itself. - The PUT create-or-update path prepares an entity twice: once at the resource boundary on the user's submission, and once more in the create pipeline on what the first pass encrypted. A strict policy cannot tell the first pass's own output from a foreign credential, so the second pass is marked as a re-storage (EntityRepository.createPrepared -> prepare(entity, update, alreadyPrepared)) and the service connection is re-stored as the server's copy. Without it, every PUT that creates a service with a plain password 400s on the server's own output. A rejection returns a 400 naming the field and never echoes the submitted value. Existing provider, lifecycle and delete-walker fixtures pass unchanged: references outside the managed namespace remain a supported feature. ADR: 2026-10-22-an-encoded-credential-needs-an-owner
github-actions
Bot
added
Ingestion
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
DBSecretsManager caches a JVM-wide singleton with the first caller's configuration; other classes in the unit lane bootstrap it with no prefix, so the hard-coded managed-reference fixture failed whenever this class ran after them. The test now resets the singleton around itself and rebuilds it from its own configuration.
…nd the HTTP workflow path The PATCH pipeline's prepare pass runs after the masker pass, which has already validated every encoded value against the stored copy, so it now stores what the masker accepted instead of re-validating it against a strict policy. Without that, a patch that changes a service's name rejects the connection's stored reference, because the accepted bases are rebuilt from the new name before the server reverts the rename. A TEST_CONNECTION request that arrives over HTTP is typed at the top of the resource's unmask, so the stored-service lookup finds the tested service and the request can echo its own stored reference. A connection a patch adds where none was stored is now walked by the masker with nothing to compare against, so an encoded value it carries is rejected instead of reaching the re-storage pass unvalidated.
| return encryptContextPluginConnection( | ||
| connectionConfig, | ||
| pluginName, | ||
| CredentialPolicy.strictWithReferenceBases( |
There was a problem hiding this comment.
Since this default is now strict, Collate's context plugins and query runner will start getting 400s once this lands.
ContextPluginRepository.encryptConnection copies the stored secrets into the payload on update (an OAuth plugin's refresh token always gets carried over, since the form never sends it) and then calls this overload. On the db secrets manager that stored value is a Fernet token, so editing any OAuth plugin fails. On AWS-style managers a rename fails too, because the stored reference sits under the old name. QueryRunnerConfigService re-saves already-encrypted configs through encryptQueryRunnerConfig in a similar way.
Collate builds against OSS main, so the Collate companion PR needs to land together with this one.
|
|
||
| for (IngestionPipeline ingestionPipeline : entities) { | ||
| if (secretsManager != null) { | ||
| // The batch store is a user create path: encoded values it does not own are rejected. |
There was a problem hiding this comment.
The bulk update leg (updateWithDeferredStore) still stores pipelines, workflows and bot mechanisms without this check, so a foreign token can still get in that way. Could we cover that path in this PR as well, so the fix is complete?
- [high] AWS SSM parameter ARNs bypass managed-namespace validation (F-7af61dec34) - [high] Entity-name prefixes allow another entity's managed credentials (F-fa978fcf80) - [medium] Secrets-manager migration updates workflows through the ingestion-pipeline DAO, so workflows are never migrated (F-814266bdd6) - [medium] Owned-reference check is a prefix match, so an entity can claim the secret reference of any entity whose name extends its own (F-b0f089c900) Automated by pr-supervisor using glm-5.3-flash:cloud via ollama; reviewed by gpt-6.1-sol.
- [high] TEST_CONNECTION echo accepts a guessable reference to the named service's secret and can run it against an attacker-controlled host (F-31f7346e4b) - [high] Saved AWS credentials block saves (G-a6034a0b52) - [medium] If-Match PATCH stores the masker-decrypted workflow or bot credential in plaintext, bypassing storeEntity (F-11490a8d77) - [medium] AWS SSM trims leading/trailing spaces, so a space-padded reference bypasses the managed-namespace classification (F-16f8a4309c) Automated by pr-supervisor using glm-5.3-flash:cloud via ollama; reviewed by gpt-6.1-sol.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
- [high] If-Match PATCH on an ingestion pipeline persists masker-decrypted sourceConfig credentials as plaintext (F-b2ac1351f1) - [high] Stored TEST_CONNECTION workflows can still carry and run another service's reference; the trigger and the migration do not revalidate them (F-e676d413a4) - [high] Rejected edits change bot credentials (G-48db171092) - [medium] Unmask errors lose their cause (G-281daf48f4) - [medium] Require EditAll on the service only when the service exists. Otherwise fall back to the pipeline-create rule.: (G-bbd8521720) Automated by pr-supervisor using glm-5.3-flash:cloud via ollama; reviewed by gpt-6.1-sol.
github-actions
Bot
added
the
merge conflict
🚦 Removed from the merge queue —
|
# Conflicts: # openmetadata-service/src/main/java/org/openmetadata/service/resources/automations/WorkflowResource.java
github-actions
Bot
removed
the
merge conflict
| authorizeNamedServiceEdit(securityContext, testRequest); | ||
| } | ||
| Workflow originalWorkflow; | ||
| if (WorkflowType.TEST_CONNECTION.equals(workflow.getWorkflowType())) { |
There was a problem hiding this comment.
⚠️ Bug: Non-admin test of a not-yet-created service NPEs on null original
In this commit, buildFromOriginalServiceConnection returns storedServiceConnection(workflow) for a new workflow, and that returns null when the named service doesn't exist yet. That is the add-service flow, where the user tests a connection before saving the service. For a non-admin, non-bot caller, unmask then runs requireSecretsReenteredOnRetarget(workflow, originalWorkflow). In that method, converted.getRequest() instanceof TestServiceConnectionRequest is true, so the next condition, original.getRequest(), is evaluated on null and throws a NullPointerException. The caller gets a 500 instead of a test-connection workflow. Before this commit, originalWorkflow was never null on this path because it fell back to a converted copy. The authorization comment says this flow intentionally keeps the pipeline-create rule, so it is meant to work.
Fix: skip the retarget check when there is no stored original. With nothing stored, there is no secret to borrow, and the masker already rejects encoded values that have no stored base.
Guard the retarget check against a missing comparison base:
originalWorkflow = buildFromOriginalServiceConnection(workflow);
if (originalWorkflow != null && !readsStoredSecrets(securityContext)) {
requireSecretsReenteredOnRetarget(workflow, originalWorkflow);
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
Describe your changes:
The credential round trips in
SecretsManagerandPasswordEntityMaskerassumed their input was either plaintext or a copy the server already held — nothing checked that. This PR closes the round trip: a credential field now accepts a plain value, or an encoded value the entity itself already holds; anything else is a client error. No legitimate round trip produces the rejected inputs, so no existing behavior depends on them.I worked on the encrypt walk, the masker and their callers because every path that stores a credential validated its input's shape but never its provenance.
No linked issue — self-contained input validation, with the behavior change and its scope described below.
Type of change:
High-level design:
Three pieces:
SecretsManager#requireOwnCredential, driven by a newCredentialPolicy. On strict paths (user create, bulk create) an encoded token is always rejected, and asecret:reference only when it falls inside the deployment's managed namespace (buildSecretId(true)) and outside the entity's accepted bases. A reference outside the namespace points into a vault the submitter manages themselves — those resolve at runtime in the ingestion framework and are never touched by the delete walker — so they stay allowed. Internal re-storage (a stored copy the server already holds) is exempt viaCredentialPolicy.INTERNAL_RE_STORAGE.PasswordEntityMasker#requireOwnStoredValue: a user update that carries an encoded value is accepted only when it matches, byte for byte, the value the field already holds, as stored or as resolved. A submission with nothing stored to compare against — a connection added where none was stored, a TEST_CONNECTION request with no stored service behind it — is still walked, and an encoded value with no owner is rejected there. The unmask walk now covers the fields the encrypt walk encrypts (onlyBasicAuthMechanismremains excluded), so bot JWT tokens are validated and masked server JWT credentials can be restored.storeEntity(update=true)and theSecretsManagerUpdateServicemigrations re-store copies the server already holds under internal re-storage (safe only because the masker validated first); creates and bulk creates stay strict.WorkflowResourcenow types the request body before comparing it (an HTTP submission arrives as raw JSON), and builds the TEST_CONNECTION comparison base from the stored service's connection when the workflow does not exist yet, instead of from the submission itself — a base built from the submission validates it against itself.EntityRepository#createPrepared/prepare(entity, update, reStore)). Without this, two things break: every PUT that creates a service with a plain password 400s on the server's own first-pass output, and a PATCH that renames a service rejects the connection's stored reference — an edit that never touched the connection — because the accepted bases are rebuilt from the new name. The resource boundary's own prepare stays strict; it runs before the create-or-update decision, so it cannot be keyed off the update flag.Backward compatibility: the values that now get a 400 are exactly the encoded values the entity did not already hold. Plaintext submissions,
PASSWORD_MASKround trips and references into the submitter's own vault behave as before. No schema, storage-format or migration changes.A rejection returns a 400 naming the field and never echoes the submitted value — the value may itself be a credential, and it does not belong in the logs.
Known limitation: the bulk create-or-update update leg persists through the deferred-store path (
EntityUpdater#updateWithDeferredStore→updateMany), which never runs the encrypt walk — a pre-existing shape this PR does not change. Services are still validated on that leg (their connection is encrypted at the resource-boundary prepare, which is strict), but entities whose credentials encrypt instoreEntity— ingestion pipelines, workflows, bot mechanisms — are not re-walked there. Covering that path is a follow-up.Downstream companion: Collate #7960 moves query-runner, context-plugin and migration callers to the explicit internal re-storage overloads. The legacy overloads in this PR retain strict defaults.
Decision record:
docs/decisions/2026-10-22-an-encoded-credential-needs-an-owner.md(ADR:2026-10-22-an-encoded-credential-needs-an-owner).Tests:
Use cases covered
secret:reference it does not own returns a 400 naming the field; sending the plain value works as before.PASSWORD_MASKplaceholder still saves it — the create pipeline's second prepare pass re-stores the first pass's own output instead of rejecting it.SecretsManagerUpdateServicemigrations) keeps working with already-encrypted values.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/secrets/DBSecretsManagerTest.java— strict encrypt rejects an encoded token (fails onmain); internal re-storage round-trips an already-encrypted value.openmetadata-service/src/test/java/org/openmetadata/service/secrets/ExternalSecretsManagerTest.java— runs on AWS/SSM/GCP: a service accepts its own stored reference echoed back; a reference outside the entity's own base is rejected; same pair for the workflow test-connection path (all four fail onmain).openmetadata-service/src/test/java/org/openmetadata/service/secrets/masker/PasswordEntityMaskerForeignCredentialTest.java(new) — rejects a token the entity does not hold, a reference outside its own base and an encoded value with no original to compare against; accepts the stored token echoed back, a plain value with no stored original and a reference into the submitter's own vault; validates the bot's JWT token; covers explicit strict and internal re-storage policies. Database-backed secrets also accept literal passwords beginning withsecret:.Backend integration tests
openmetadata-integration-tests/for new/changed API endpoints.openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/SecretsManagerCredentialValidationIT.java(new) — end to end over the service PUT API: a create carrying an encoded token minted outside the request returns a 400 naming the field and never echoing the value; the same rejection on the update leg; the plain-value andPASSWORD_MASKround trips keep working (the rejection tests fail onmain).openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/SecretsManagerNullSecretIT.java— under the external secrets manager, a service created with a managedsecret:reference it does not own is rejected with a 400 (fails onmain); a PATCH that changes the service's name keeps its stored credential in place (fails on the first version of this PR, which re-validated the untouched connection against the new name's bases); a TEST_CONNECTION workflow submitted over HTTP echoes the tested service's own stored reference (same — the raw request body was never typed, so there was nothing to compare against).Ingestion integration tests
Playwright (UI) tests
openmetadata-ui/.../ui/playwright/for UI changes.Current head:
e91fc1490ab6c07c4319c9879c1d0e2d97a0c55c.secret:passwords with the database secrets manager, and masked server JWT restoration. The first two failures were reproduced before the fixes.Manual testing performed
No manual UI testing; this is a backend-only change. Automated local and remote evidence is listed above.
Functional review: merged current main and resolved the workflow-test and ingestion patch conflicts. Fixed persisted encrypted workflow-reference validation, database-manager plaintext classification, and masked JWT restoration. Independent review found no remaining blocking functional regressions. No cosmetic changes requested or added.
UI screen recording:
Not applicable — backend-only change, no UI surface touched.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.REVIEW.md(thepr-checklistskill runs it) and fixed every 🔴 finding, or answered it on the line below.docs/decisions/2026-10-22-an-encoded-credential-needs-an-owner.md.🤖 Generated with Claude Code
The PR needs changes before merging because encrypted stored references bypass the new workflow trigger check.
Findings
Summary
The PR limits encoded credentials to values the entity already holds, while preserving plain credentials and external-vault references.
TeddyCr explicitly defers the bulk update path that bypasses encryption and the downstream re-storage callers outside this repository.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Credential submitted] --> B[Check ownership] B --> C[Store secret or keep reference] C --> D[Encrypt field with Fernet] D --> E[(Saved workflow)] E --> F[Trigger reads encrypted field] F --> G[Reference prefix check skips token] G --> H[Worker fetches workflow] H --> I[Server decrypts reference] I --> J[Worker resolves referenced secret]Reviews (6) · Last reviewed commit: "fix: address automated review findings (..." · Reviewed by Greptile