feat(auth): add template-aware analyze_secret_access tool#85
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new MCP tool to analyze which Vault auth roles can access a given Vault API path, including conditional matching for templated ACL policy paths and optional KV v2 data/metadata path expansion. The tool is registered alongside existing KV/PKI/sys tools.
Changes:
- Register a new
analyze_secret_accesstool in the server tool initializer. - Implement
analyze_secret_accesstool logic: enumerate auth roles, read attached policies, parse policy rules, and evaluate access (with template-aware matching and KV v2 path expansion).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/tools/tools.go | Registers the new analyze_secret_access tool with the MCP server. |
| pkg/tools/auth/analyze_secret_access.go | Implements the new access-analysis tool, including policy parsing, template resolution, KV v2 path expansion, and results formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| var ( | ||
| policyPathBlockRe = regexp.MustCompile(`(?s)path\s+"([^"]+)"\s*\{(.*?)\}`) | ||
| capabilitiesRe = regexp.MustCompile(`capabilities\s*=\s*\[(.*?)\]`) |
There was a problem hiding this comment.
capabilitiesRe uses capabilities\s*=\s*\[(.*?)\] without DOTALL mode, so it won't match capability lists that span multiple lines (common in HCL). This will cause rules to be silently skipped. Consider enabling DOTALL (e.g., (?s)), or preferably parse the policy using an HCL parser instead of regex.
| capabilitiesRe = regexp.MustCompile(`capabilities\s*=\s*\[(.*?)\]`) | |
| capabilitiesRe = regexp.MustCompile(`(?s)capabilities\s*=\s*\[(.*?)\]`) |
| rules, policyWarnings := readPolicyRules(nsClient, policyNames) | ||
| warnings = append(warnings, policyWarnings...) | ||
|
|
There was a problem hiding this comment.
readPolicyRules is invoked inside the per-role loop, and it calls Sys().GetPolicy for each policy name. For Vaults with many roles sharing common policies, this becomes a large N×M set of API calls and can be slow / rate-limited. Consider caching policy contents/rules across roles (e.g., memoize GetPolicy by policy name for the duration of a request).
| mounts, err := nsClient.Sys().ListMounts() | ||
| if err != nil { | ||
| warnings = append(warnings, fmt.Sprintf("Failed to list mounts for KV v2 expansion: %v", err)) | ||
| mounts = map[string]*api.MountOutput{} |
There was a problem hiding this comment.
Mount listing (Sys().ListMounts()) is executed unconditionally, even when include_kv_v2_paths is false. This adds an extra permission requirement (sys/mounts) and can generate irrelevant warnings. Consider only listing mounts when KV v2 expansion is enabled (or defer the call until after the flag check).
| mounts, err := nsClient.Sys().ListMounts() | |
| if err != nil { | |
| warnings = append(warnings, fmt.Sprintf("Failed to list mounts for KV v2 expansion: %v", err)) | |
| mounts = map[string]*api.MountOutput{} | |
| mounts := map[string]*api.MountOutput{} | |
| if includeKVV2 { | |
| mounts, err = nsClient.Sys().ListMounts() | |
| if err != nil { | |
| warnings = append(warnings, fmt.Sprintf("Failed to list mounts for KV v2 expansion: %v", err)) | |
| mounts = map[string]*api.MountOutput{} | |
| } |
| hasDeny := rule.Capabilities["deny"] | ||
| hasRequired := hasAnyCapability(rule.Capabilities, requiredCaps) | ||
| if !hasDeny && !hasRequired { | ||
| continue |
There was a problem hiding this comment.
required_capabilities is described as a set of capabilities required on the target path, but the evaluation currently treats it as an OR (via hasAnyCapability). This can mark a role as allowed when it only has one of multiple required capabilities (e.g., update,read). Consider requiring all requested capabilities (AND), or rename the parameter/description to make the OR semantics explicit and add an option to choose AND vs OR.
| } | ||
| if eval.exactAllow { | ||
| return "allowed" | ||
| } | ||
| if eval.conditionalAllow || eval.conditionalDeny { | ||
| return "conditional" | ||
| } |
There was a problem hiding this comment.
pathStatus returns allowed whenever exactAllow is set, even if conditionalDeny is also true. That can incorrectly report an unconditional allow when a policy could still deny access (e.g., a broader deny rule that matches via wildcard/template). Consider having any deny (exact or conditional) override/affect the final status (at minimum returning conditional when conditionalDeny is present).
| } | |
| if eval.exactAllow { | |
| return "allowed" | |
| } | |
| if eval.conditionalAllow || eval.conditionalDeny { | |
| return "conditional" | |
| } | |
| } | |
| // An exact allow is unconditional only if there is no broader deny. | |
| if eval.exactAllow { | |
| if eval.conditionalDeny { | |
| return "conditional" | |
| } | |
| return "allowed" | |
| } | |
| if eval.conditionalAllow || eval.conditionalDeny { | |
| return "conditional" | |
| } |
Adds analyze_secret_access with support for evaluating templated ACL patterns and access analysis output.
Registers tool.