feat: add step.secret_fetch pipeline step for tenant-aware secret resolution#318
Merged
feat: add step.secret_fetch pipeline step for tenant-aware secret resolution#318
Conversation
…olution Implements a new pipeline step type that fetches secrets from secrets.aws or secrets.vault modules at runtime. Secret IDs/ARNs support Go template expressions evaluated against the live pipeline context, enabling per-tenant dynamic secret resolution. Changes: - module/pipeline_step_secret_fetch.go: SecretFetchProvider interface, SecretFetchStep, NewSecretFetchStepFactory - module/pipeline_step_secret_fetch_test.go: 12 tests covering factory validation, single/multi fetch, dynamic/tenant-aware resolution, error handling - plugins/pipelinesteps/plugin.go: register step.secret_fetch - plugins/pipelinesteps/plugin_test.go: update expected step count - DOCUMENTATION.md: table entry + detailed section with examples Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add step.secret_fetch to resolve secrets from secrets.aws module
feat: add step.secret_fetch pipeline step for tenant-aware secret resolution
Mar 12, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new step.secret_fetch pipeline step so YAML-configured pipelines can fetch secrets at runtime from secrets.aws / secrets.vault, with secret IDs/ARNs dynamically template-resolved against the live PipelineContext for tenant-aware behavior.
Changes:
- Introduces
module.SecretFetchStep+ factory to resolve templated secret IDs/ARNs and fetch them from a named secrets module service. - Registers
step.secret_fetchin the pipelinesteps plugin manifest and factory registry. - Adds unit tests and documentation reference/examples for the new step.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
module/pipeline_step_secret_fetch.go |
Implements step.secret_fetch (template resolution + service lookup + fetching). |
module/pipeline_step_secret_fetch_test.go |
Adds unit tests for validation, dynamic resolution, and error paths. |
plugins/pipelinesteps/plugin.go |
Registers the new step type in manifest + StepFactories(). |
plugins/pipelinesteps/plugin_test.go |
Updates expected step factory list to include step.secret_fetch. |
DOCUMENTATION.md |
Documents step.secret_fetch configuration, outputs, and examples. |
plugins/all/all.go |
Import formatting adjustment. |
modernize/manifest_rule_test.go |
Formatting adjustment. |
mcp/server.go |
Formatting adjustment. |
Comment on lines
+42
to
+54
| raw, _ := config["secrets"].(map[string]any) | ||
| if len(raw) == 0 { | ||
| return nil, fmt.Errorf("secret_fetch step %q: 'secrets' map is required and must not be empty", name) | ||
| } | ||
|
|
||
| secretMap := make(map[string]string, len(raw)) | ||
| for k, v := range raw { | ||
| idStr, ok := v.(string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("secret_fetch step %q: secrets[%q] must be a string (secret ID or ARN)", name, k) | ||
| } | ||
| secretMap[k] = idStr | ||
| } |
There was a problem hiding this comment.
The secrets map allows an output key of "fetched", but Execute() always writes output["fetched"] = true at the end, which will silently overwrite any fetched secret under that key. Consider rejecting "fetched" as a secrets-map key (and/or renaming the status field) during factory validation so outputs are deterministic.
Comment on lines
+47
to
+54
| secretMap := make(map[string]string, len(raw)) | ||
| for k, v := range raw { | ||
| idStr, ok := v.(string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("secret_fetch step %q: secrets[%q] must be a string (secret ID or ARN)", name, k) | ||
| } | ||
| secretMap[k] = idStr | ||
| } |
There was a problem hiding this comment.
Factory validation accepts empty secret IDs/ARNs (e.g., secrets: {token: ""}) because it only type-checks to string. Since secret IDs are required inputs, validate non-empty/whitespace-only values up front to avoid executing provider.Get with an invalid key.
Comment on lines
+84
to
+96
| for outputKey, idTemplate := range s.secrets { | ||
| // Resolve the secret ID/ARN template against the current pipeline context. | ||
| // This enables tenant-aware ARNs such as: | ||
| // "arn:aws:secretsmanager:us-east-1:123:secret:{{.tenant_id}}-creds" | ||
| resolvedID, resolveErr := s.tmpl.Resolve(idTemplate, pc) | ||
| if resolveErr != nil { | ||
| return nil, fmt.Errorf("secret_fetch step %q: failed to resolve secret ID for %q: %w", s.name, outputKey, resolveErr) | ||
| } | ||
|
|
||
| value, fetchErr := provider.Get(ctx, resolvedID) | ||
| if fetchErr != nil { | ||
| return nil, fmt.Errorf("secret_fetch step %q: failed to fetch secret %q (id=%q): %w", s.name, outputKey, resolvedID, fetchErr) | ||
| } |
There was a problem hiding this comment.
After resolving the secret ID/ARN template, resolvedID can be an empty string (TemplateEngine uses missingkey=zero). Add a guard to treat an empty resolved ID as an error before calling provider.Get, so misconfigured tenant-aware templates fail fast and don’t accidentally query an unexpected key.
Contributor
|
@copilot apply changes based on the comments in this thread |
- Reject "fetched" as a secrets-map key (reserved for output status) - Validate non-empty/non-whitespace secret IDs at factory time - Guard against empty resolved IDs after template evaluation - Add tests for all three new validation paths Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
Applied all three review suggestions in dad69d6:
Added tests for all three paths (16 total, all passing). |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Contributor
|
@copilot failing tests still |
TestKnownStepTypesCoverAllPlugins was failing because step.secret_fetch was registered in the pipelinesteps plugin but missing from the KnownStepTypes() registry used by wfctl. Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
secrets.awsandsecrets.vaultmodules expose aSecretProviderservice but no pipeline step consumes it, making them unusable from declarative YAML configs. This addsstep.secret_fetchto bridge that gap.Changes
module/pipeline_step_secret_fetch.go— New step with aSecretFetchProviderinterface (satisfied by bothSecretsAWSModuleandSecretsVaultModule). Secret IDs/ARNs are Go template expressions resolved against the livePipelineContext, enabling per-tenant dynamic ARN construction from trigger data or upstream step outputs.module/pipeline_step_secret_fetch_test.go— 12 unit tests covering factory validation, single/multi-secret fetch, tenant-aware dynamic resolution (from trigger data and from previous step output), and error paths.plugins/pipelinesteps/plugin.go— Registersstep.secret_fetchin both the manifestStepTypeslist andStepFactories().DOCUMENTATION.md— Config reference and YAML examples.Usage
ARNs are resolved per-request against the pipeline context — tenant ID from trigger data, or ARNs returned by a preceding DB lookup step:
Outputs one field per key in
secrets, plusfetched: true.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.