Fix vault secret label validation#22257
Conversation
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
product-security-plaid-production
Bot
requested review from
bolekk,
justinkaseman and
pavel-raykov
prashantkumar1982
added
build-publish
prashantkumar1982
dismissed stale reviews from russell-stern and timothyF95
via
a7b10c9
prashantkumar1982
dismissed stale reviews from russell-stern and timothyF95
via
4b55cd0
prashantkumar1982
added
build-publish
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46977cafe4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| vaultCapRequest.RequestId = req.ID | ||
| if err := setAuthorizedIdentityFields(&vaultCapRequest, authResult); err != nil { | ||
| return h.errorResponse(ctx, gatewayID, req, api.FatalError, err) | ||
| } | ||
| for idx, encryptedSecret := range vaultCapRequest.EncryptedSecrets { | ||
| if encryptedSecret != nil && encryptedSecret.Id != nil && normalizeOwner(encryptedSecret.Id.Owner) != normalizeOwner(authorizedOwner) { | ||
| h.lggr.Debugw("create secrets request owner mismatch", "requestID", req.ID, "secretOwner", encryptedSecret.Id.Owner, "authorizedOwner", authorizedOwner, "index", idx) | ||
| return h.errorResponse(ctx, gatewayID, req, api.FatalError, fmt.Errorf("secret ID owner %q does not match authorized owner %q at index %d", encryptedSecret.Id.Owner, authorizedOwner, idx)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
In the node-side gateway handler path (used by core/services/ocr2/delegate.go), write requests are now forwarded immediately after setAuthorizedIdentityFields without any owner-level guard. Because capability-side identity validation now accepts either WorkflowOwner or OrgId (validateOwnerMatchesResolvedIdentity), a JWT-authenticated request that carries both fields can submit workflow-owner-labeled secret IDs/ciphertext and still pass, which weakens the intended JWT strictness for org-scoped writes. This regression is specific to requests processed through this node-side handler, where the removed per-item owner check previously blocked that case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3939b9139
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| validationRequest = proto.Clone(createSecretsRequest).(*vaultcommon.CreateSecretsRequest) | ||
| validationRequest.WorkflowOwner = "" |
There was a problem hiding this comment.
Clearing WorkflowOwner whenever OrgId is present forces create/update validation to accept only org-labeled ciphertext, even when VaultOrgIdAsSecretOwnerEnabled is off. In the configuration where JWT auth is enabled but org-as-owner migration is disabled, this rejects workflow-owner-labeled writes and pushes clients toward org-labeled ciphertexts that the read path later validates against workflow owner only, causing secrets to be unreadable. This strict JWT-only org-label check should be conditioned on the org-owner gate (or equivalent migration mode), not just on OrgId being non-empty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
we're only clearing from gateway, that too from top-level.
top-level field shouldn't be set or used at gateway level
| if orgIDGateEnabled && s.orgID == "" { | ||
| return nil, errors.New("org_id is required when VaultOrgIdAsSecretOwnerEnabled is enabled") |
There was a problem hiding this comment.
This new early return turns an empty s.orgID into a hard failure for every secret fetch whenever VaultOrgIdAsSecretOwnerEnabled is on. The engine startup path explicitly tolerates org-resolution failures and continues without org ID, so after this change a transient resolver/linking failure at startup can leave workflows running but permanently unable to fetch secrets until restart/relink. Either preserve workflow-owner fallback in this case or fail engine initialization immediately when the gate is enabled and org resolution did not succeed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It is ok to fail is orgID is not fetchable on workflow nodes.
… into codex/vault-secret-labeling-bug
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 876a2f513f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if orgIDGateEnabled && s.orgID == "" { | ||
| return nil, errors.New("org_id is required when VaultOrgIdAsSecretOwnerEnabled is enabled") |
There was a problem hiding this comment.
Returning an error as soon as VaultOrgIdAsSecretOwnerEnabled is on and s.orgID is empty makes every GetSecrets call fail for the lifetime of that engine instance, even though startup explicitly tolerates org-resolution failures and continues running. In practice, a transient resolver/linking outage during Engine.start now becomes a persistent secrets outage until restart/relink, instead of falling back to workflow-owner reads as before.
Useful? React with 👍 / 👎.
| if err := setAuthorizedIdentityFields(&vaultCapRequest, authResult); err != nil { | ||
| return h.errorResponse(ctx, gatewayID, req, api.FatalError, err) | ||
| } | ||
| for idx, encryptedSecret := range vaultCapRequest.EncryptedSecrets { | ||
| if encryptedSecret != nil && encryptedSecret.Id != nil && normalizeOwner(encryptedSecret.Id.Owner) != normalizeOwner(authorizedOwner) { | ||
| h.lggr.Debugw("create secrets request owner mismatch", "requestID", req.ID, "secretOwner", encryptedSecret.Id.Owner, "authorizedOwner", authorizedOwner, "index", idx) | ||
| return h.errorResponse(ctx, gatewayID, req, api.FatalError, fmt.Errorf("secret ID owner %q does not match authorized owner %q at index %d", encryptedSecret.Id.Owner, authorizedOwner, idx)) | ||
| } | ||
| } | ||
|
|
||
| h.lggr.Debugf("Processing authorized and normalized create secrets request [%s]", vaultCapRequest.String()) | ||
| vaultCapResponse, err := h.secretsService.CreateSecrets(ctx, &vaultCapRequest) |
There was a problem hiding this comment.
The node-side gateway handler now forwards create requests immediately after setting authorized identity fields, but no longer enforces that each secret identifier owner matches the authorized owner. Because capability-side validation accepts either resolved WorkflowOwner or OrgId, JWT requests that include both identities can now pass with workflow-owner IDs in this path (especially when gateway-side label checks are skipped due to missing cached public key), weakening the intended org-scoped JWT write enforcement.
Useful? React with 👍 / 👎.
| return workflowOwner, requestDigest, nil | ||
| } | ||
|
|
||
| func validateOrgIDOwnedVaultRequest(req jsonrpc.Request[json.RawMessage], orgID string) error { |
There was a problem hiding this comment.
Can we move these to the validator file? I'm assuming other parts of the code will want to use these as well.
|
4 participants
Summary