vault: integrate linking service identity resolution#21715
vault: integrate linking service identity resolution#21715prashantkumar1982 merged 26 commits intodevelopfrom
Conversation
|
👋 prashantkumar1982, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
CORA - Pending ReviewersAll codeowners have approved! ✅ Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
| s.lggr.Errorw("get secrets request owner mismatch", "index", idx, "secretOwner", req.Id.Owner, "orgID", resolvedIdentity.OrgID) | ||
| return capabilities.CapabilityResponse{}, fmt.Errorf("secret identifier owner %q does not match org_id %q at index %d", req.Id.Owner, resolvedIdentity.OrgID, idx) | ||
| } | ||
| case req.Id.Owner != "": |
There was a problem hiding this comment.
Should this case go first? All other ones depend on it
There was a problem hiding this comment.
No, this case means both workflow and orgID were set as nil in the request, which should never happen.
This is just a saniuty check condition.
There was a problem hiding this comment.
I think this also allows all to be empty though, I think we should just put a default clause here
i.e. either owner or org ID is non-empty, or we error (nothing else is allowed)
| return capabilities.CapabilityResponse{}, fmt.Errorf("secret identifier owner %q does not match workflow owner %q at index %d", req.Id.Owner, resolvedIdentity.WorkflowOwner, idx) | ||
| } | ||
| case resolvedIdentity.OrgID != "": | ||
| if req.Id.Owner != resolvedIdentity.OrgID { |
There was a problem hiding this comment.
I'm trying to understand the difference between case normalizedWorkflowOwner != "" and case resolvedIdentity.OrgID != "". Is there any case when both the normalizedWorkflowOwner and resolvedIdentity.OrgID are non-empty?
There was a problem hiding this comment.
Well, I see this logic is pretty convoluted.
This code path is within Execute(), meaning it will only be called from a workflow, while fetching secrets via GetSecret().
So CLI will never use this code path.
This call is already trusted because comes from within workflow engine with F+1 consensus.
Now, the workflow engine, for a given request, will have both workflowOwner and OrgID. Today, it only sends the workflowOwner, but I am considering it can send both workflowOwner and orgID. This will be helpful for the time we are in auto-migration mode.
So theoretically we don't need linking service to be even invoked for this code path, as we trust inputs here.
@cedric-cordenier what do you think? Should we invoke Linking Service for each GetSecret() path and validate the mapping?
I think it adds extra rpc for each read call, whicih you wanted to avoid?
There was a problem hiding this comment.
Yes I don't think we need to invoke the linking service here, we should be able to trust the input coming from the engine if we land in this part of the code
| if workflowOwner == "" { | ||
| return LinkedVaultRequestIdentity{OrgID: orgID, WorkflowOwner: workflowOwner}, nil | ||
| } |
There was a problem hiding this comment.
This can probably be extracted as an independent condition about this if statement, because it will be checked again on L95.
|
|
||
| if req.Id != nil && normalizeOwner(req.Id.Owner) != normalizedWorkflowOwner { | ||
| return capabilities.CapabilityResponse{}, fmt.Errorf("secret identifier owner %q does not match workflow owner %q at index %d", req.Id.Owner, request.Metadata.WorkflowOwner, idx) | ||
| if req.Id != nil && normalizeOwner(req.Id.Owner) != normalizeOwner(r.WorkflowOwner) { |
There was a problem hiding this comment.
It's safer to use the request.Metadata.WorkflowOwner because that is always provided by the engine; I think using the request.WorkflowOwner is fine because we provide that in the SecretsFetcher (but please check!) but that might change in the future.
There was a problem hiding this comment.
Well this method Execute() is only called by the workflows, and they will set this field after picking it up from request.Metadata.WorkflowOwner. So it is the same thing imo.
But still switching to request.Metadata.WorkflowOwner.
| if strings.TrimSpace(resolvedOrgID) == "" { | ||
| return LinkedVaultRequestIdentity{}, fmt.Errorf("resolved empty org_id for workflow owner %q", workflowOwner) | ||
| } | ||
|
|
There was a problem hiding this comment.
🤔 Why are we fetching the orgID here? Does this duplicate what we're doing on line 86?
There was a problem hiding this comment.
ya, refactored now
| } | ||
|
|
||
| // Link resolves or verifies the request identity from the caller-provided org and workflow owner. | ||
| func (l *OrgIDToWorkflowOwnerLinker) Link(ctx context.Context, orgID string, workflowOwner string) (LinkedVaultRequestIdentity, error) { |
There was a problem hiding this comment.
Can we clean up some of the defensive coding throughout this file? I think we've taken it way too far.
see line 53 -- vaultOrgIDAsSecretOwnerEnabled will not be nil if created via the constructor since we'll error if that the case. Same with checking l == nil and the orgResolver; let's just do this check once in the constructor if we need to.
There was a problem hiding this comment.
At the moment it's very difficult to understand what the important error conditions are and what we're trying to guard
There was a problem hiding this comment.
agreed. cleaned up the fn now.
| return common.HexToAddress(owner).Hex(), nil | ||
| } | ||
|
|
||
| func vaultOrgIDAsSecretOwnerEnabled(ctx context.Context, gate limits.GateLimiter) (bool, error) { |
There was a problem hiding this comment.
nit: maybe LimitOrFalse since there's nothing vault specific about this, it's a helper
| @@ -66,15 +70,18 @@ func NewSecretsFetcher( | |||
| lggr = logger.Named(lggr, "WorkflowEngine.SecretsFetcher") | |||
| lggr = logger.With(lggr, "workflowID", workflowID, "workflowName", workflowName, "workflowOwner", workflowOwner, "phaseID", phaseID) | |||
There was a problem hiding this comment.
Worth adding the orgID here :) ?
core/scripts/cre/environment/examples/workflows/v1/proof-of-reserve/cron-based/go.mod
Outdated
Show resolved
Hide resolved
|




Summary
Integrates Vault with the linking service flow so workflow-driven Vault requests can carry both
OrgIdandWorkflowOwner.The workflow engine now propagates
OrgIDthroughRequestMetadataand forwards it into VaultGetSecretsRequestpayloads. ThatOrgIDforwarding is gated byVaultOrgIdAsSecretOwnerEnabled, so when the gate is disabled the workflow side omitsOrgID.On the Vault capability side, all gateway request paths (
create,update,delete, andlist) go through the linker layer. The linker resolves or verifies theorg_id/workflow_ownerpair and rejects requests when the values do not match correctly.Execute(GetSecrets)is kept simpler: it treats the incomingGetSecretsRequestidentity fields as trusted and validates secret owners against the requestWorkflowOwnerdirectly.