feat(vault): enforce allowedAgents at deploy time instead of documenting it - #659
Closed
ginccc wants to merge 2 commits into
Closed
feat(vault): enforce allowedAgents at deploy time instead of documenting it#659ginccc wants to merge 2 commits into
ginccc wants to merge 2 commits into
Conversation
…ing it SecretMetadata.allowedAgents was 'for visibility only — enforcement is via configuration authorship'. That model assumes a human admin authors agent configs; create_sub_agent lets an LLM author one, so scoping a secret to one agent bought nothing. Enforced at deployment, not resolution, and that is the load-bearing decision: SecretResolver sees only a string and several of its call sites have no agent at all, while ChatModelRegistry caches models keyed on the UNRESOLVED parameters — so a check behind that cache runs for whichever agent built the model first and is skipped for every other one. That would be enforcement that looks real and is not. The agent/secret binding lives in the agent's configuration, so it is checked there: completely, once, with no cache in the way. VaultGrantChecker walks workflows -> llm/apicalls/mcpcalls configs, serializes each and scans for vault references rather than enumerating known credential fields, then checks each against allowedAgents. eddi.vault.grant-enforcement = off|warn|enforce, defaulting to warn: the field has never been enforced, so non-wildcard values in existing deployments are untested configuration and blocking on upgrade could take agents down. Uncertainty never becomes a violation — unreadable metadata, disabled vault, unreadable workflow and wildcard/empty grants all allow. +9 tests.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughVault grant enforcement now checks workflow vault references before agent deployment. ChangesVault grant deployment enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentDeploymentManagement
participant VaultGrantChecker
participant WorkflowStores
participant SecretProvider
AgentDeploymentManagement->>VaultGrantChecker: Validate agent vault references
VaultGrantChecker->>WorkflowStores: Load workflows and extension configurations
VaultGrantChecker->>VaultGrantChecker: Scan serialized configurations
VaultGrantChecker->>SecretProvider: Read grant metadata
SecretProvider-->>VaultGrantChecker: Return metadata or read failure
VaultGrantChecker-->>AgentDeploymentManagement: Return ungranted references
AgentDeploymentManagement->>AgentDeploymentManagement: Apply enforcement mode
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagement.java`:
- Around line 198-204: Ensure every agent deployment path checks vault grants
before deployment, not only checkDeployments. Update the shared deployment flow
or each caller around agentFactory.deployAgent, including the paths near lines
413 and 462, to invoke vaultGrantsSatisfied and prevent deployment when
enforcement rejects ungranted references.
- Around line 266-293: Validate vaultGrantEnforcement during startup
configuration validation, accepting only case-insensitive “off”, “warn”, or
“enforce” values and failing validation for anything else. Update the relevant
startup validation path rather than relying on vaultGrantsSatisfied’s fallback
behavior; preserve the existing enforcement semantics for valid values.
In `@src/test/java/ai/labs/eddi/secrets/VaultGrantCheckerTest.java`:
- Around line 51-63: Extend the VaultGrantChecker test suite around setUp and
the existing workflow-step cases with separate API-call and MCP-call scenarios.
Configure each step using the mocked IApiCallsStore or IMcpCallsStore, place
VAULT_REF in an arbitrary credential field, and assert that an ungranted agent
reports the reference as a violation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 57e19514-b01f-4911-99f4-75cc0229cb93
📒 Files selected for processing (4)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/runtime/internal/AgentDeploymentManagement.javasrc/main/java/ai/labs/eddi/secrets/VaultGrantChecker.javasrc/test/java/ai/labs/eddi/secrets/VaultGrantCheckerTest.java
… enforcement values CodeRabbit on #659, three findings, all valid: - The gate guarded checkDeployments alone. manageAgentDeployments' latest-version redeploy and manageDeploymentOfOldAgent called agentFactory.deployAgent directly, so in enforce mode an agent with ungranted references was blocked by one path and deployed by another a day later. A gate with a way round it is not a gate; there is now a single deployIfGranted entry point instead of three call sites that each have to remember. - Any value other than off/enforce silently meant warn, so the plausible typo 'enforced' produced a security control that was off while appearing on. Parsed strictly into an enum and validated in @PostConstruct, so an unusable value fails startup with the valid values named — the discipline Deployment.Environment.parseStrict already applies. - The test suite configured only LLM steps; IApiCallsStore and IMcpCallsStore were mocked and never reached, so a regression dropping either branch would have passed. Both are now covered. +6 tests.
aisabella-ai
self-requested a review
August 10, 2026 17:40
aisabella-ai
approved these changes
Aug 10, 2026
This was referenced Aug 10, 2026
Member
Author
|
Superseded by a rebuild on current
The content of this PR is not lost. It was verified as still-missing against current
Every review finding raised here — including all of CodeRabbit's — is carried into the replacement PRs along with its test. The discussion here remains the record of the reasoning. |
This was referenced Aug 11, 2026
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
2 participants
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.
Closes the second open item from the Wave-R follow-up review.
SecretMetadata.allowedAgentswas documented as "for visibility only — enforcement is via configuration authorship, not runtime resolution". That access model assumes a human admin authors agent configurations.create_sub_agentlets an LLM author one — so an operator who scoped a secret to a single agent got no enforcement at all. The field was decorative.Why deployment time, and not resolution time
This is the load-bearing decision, so it's worth stating why the obvious place is the wrong one.
SecretResolvercannot do it soundly. It sees only a string — no agent identity — and its ~12 call sites spanApiCallExecutor,ChatModelRegistry,A2AToolProviderManager,McpToolProviderManager,EmbeddingModelFactory,EmbeddingStoreFactoryandChannelTargetRouter. Several legitimately run outside any conversation, andAgentSigningServicebypasses the resolver entirely.The model cache would launder the check.
ChatModelRegistrycaches onModelCacheKey(type, unresolvedParams). Two agents sharing a config share a cache entry, so a check insideresolveSecretsruns for whichever agent builds the model first and is silently skipped for every other one — enforcement that looks real and is not, which is exactly what I declined to ship when I first reported this. Making it sound needs the agent identity in the cache key plus plumbing through sixgetOrCreatecall sites, several of them background services (SummarizationService,ToolResponseTruncator) with no agent at all.The binding between an agent and a secret is established in the agent's configuration. That is where it can be checked completely, once, and with no cache in the way — next to the existing deploy-time
lintInertHitlConfig, which is the same pattern.What it does
VaultGrantCheckerwalks the agent's workflows →llm/apicalls/mcpcallsconfigs, serializes each and scans for${vault:...}references, then verifies each againstallowedAgents.The scan serializes rather than enumerating known credential fields. Enumeration is how this kind of check rots: someone adds a new credential field and the scanner silently stops covering it. Pinned by a test that plants a reference in a field named
someFutureCredentialField.Rollout is warn-first
eddi.vault.grant-enforcement=off|warn(default) |enforce.Warn is deliberate. The field has never been enforced, so any non-wildcard value in an existing deployment is untested configuration — blocking on it during an upgrade could take agents down for a policy nobody has yet had a chance to verify. Operators switch to
enforceonce the warnings are clean.Uncertainty never becomes a violation
Unreadable metadata, a disabled vault, an unreadable workflow, and absent / empty / wildcard grants all allow. A deployment gate that fires on a transient store failure is worse than the hole it closes. Every
AgentSetupService-vaulted key carries["*"], so existing deployments see no change.What it does not do
An agent already deployed before its grant was narrowed keeps resolving until it is redeployed. This is a deploy-time gate, not a revocation mechanism — stated in the class Javadoc rather than left for someone to discover.
Testing
+9 tests; 274 green across the vault, deployment and setup suites.
Worth noting: one test initially passed vacuously. The fixture used toy ids (
w1,l1), andRestUtilities.extractResourceIdrequires ≥18 hex characters — it returns a null id for anything shorter, so the scanner walked nothing and found nothing. The fixture now uses ObjectId-shaped ids, and the failure it produced first is what proved the scanner actually works.Summary by CodeRabbit
New Features
off,warn, andenforce, withwarnas the default.Bug Fixes
Documentation