Fix/vault reset and passthrough - #535
Merged
Merged
Conversation
- Add resetTenant() to ISecretProvider and VaultSecretProvider
Deletes all secrets + DEK for a tenant, bypassing DEK decryption.
Safe to call when master key has changed and old key is unavailable.
- Add POST /{tenantId}/reset REST endpoint in IRestSecretStore/RestSecretStore
with input validation, cache invalidation, and proper error responses.
- Extract handleDekDecryptionFailure() with actionable 3-option recovery message
including secret count context (0/N/unknown) for informed decision-making.
- Extract generateAndPersistDek() helper from getOrCreateDek().
- Fix vaultApiKey() in AgentSetupService to detect existing vault refs
and pass them through, preventing double-vaulting bug.
- Fix LOGGER.errorf -> LOGGER.error to preserve stack traces in error logs.
…yptionFailure, vaultApiKey - VaultSecretProviderBranchTest: 8 new tests covering resetTenant (happy path, empty tenant, persistence failure, vault unavailable), handleDekDecryptionFailure (with N/0/unknown secrets), and generateAndPersistDek (new DEK generation). - RestSecretStoreTest: 6 new tests for resetTenant endpoint (200 success, 503 unavailable, 400 invalid/blank tenantId, 500 provider exception, cache invalidation). - AgentSetupServiceBranchCoverageTest: 3 new tests for vaultApiKey passthrough (vault reference, null, blank). - AgentSigningServiceTest: add resetTenant() stub to InMemorySecretProvider. All 141 tests pass. Coverage: RestSecretStore 100%/95.7%, VaultSecretProvider 78.1%/80.4% (new methods fully covered, remaining gaps in pre-existing code).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds a destructive ChangesVault resetTenant feature with DEK refactor and AgentSetupService guard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| EncryptedDek dek = new EncryptedDek(UUID.randomUUID().toString(), tenantId, encResult.ciphertext(), encResult.iv(), Instant.now()); | ||
|
|
||
| persistence.upsertDek(dek); | ||
| LOGGER.infof("Generated new DEK for tenant: %s", tenantId); |
![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: 1
🧹 Nitpick comments (1)
src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java (1)
710-720: ⚡ Quick winAdd a passthrough test for legacy
${eddivault:...}references.Current coverage only asserts
${vault:...}passthrough. Adding the legacy prefix case will lock in compatibility with existing reference semantics and prevent future regressions.🤖 Prompt for 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. In `@src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java` around lines 710 - 720, The passthroughVaultReference test only covers the modern ${vault:...} format but lacks coverage for the legacy ${eddivault:...} format. Add a new test method (similar to passthroughVaultReference) that tests the same passthrough behavior but using the legacy ${eddivault:anthropic-api-key} reference format. This new test should also invoke invokeVaultApiKey with the legacy format, assert that the same reference is returned unchanged, and verify that secretProvider has no interactions, ensuring backward compatibility with existing vault references is maintained.
🤖 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/setup/AgentSetupService.java`:
- Around line 499-503: The condition in the vault reference guard check only
validates the new `${vault:...}` format but ignores the legacy
`${eddivault:...}` format, causing legacy references to be re-vaulted instead of
passed through. Update the if statement condition to also check for the legacy
`${eddivault:` prefix pattern in addition to the current `${vault:` check,
ensuring both formats are recognized as already-vaulted references and returned
as-is without re-vaulting.
---
Nitpick comments:
In
`@src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java`:
- Around line 710-720: The passthroughVaultReference test only covers the modern
${vault:...} format but lacks coverage for the legacy ${eddivault:...} format.
Add a new test method (similar to passthroughVaultReference) that tests the same
passthrough behavior but using the legacy ${eddivault:anthropic-api-key}
reference format. This new test should also invoke invokeVaultApiKey with the
legacy format, assert that the same reference is returned unchanged, and verify
that secretProvider has no interactions, ensuring backward compatibility with
existing vault references is maintained.
🪄 Autofix (Beta)
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
Run ID: 4cf810ce-75e7-43ad-9e0d-254a7baca09c
📒 Files selected for processing (9)
src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.javasrc/main/java/ai/labs/eddi/secrets/ISecretProvider.javasrc/main/java/ai/labs/eddi/secrets/impl/VaultSecretProvider.javasrc/main/java/ai/labs/eddi/secrets/rest/IRestSecretStore.javasrc/main/java/ai/labs/eddi/secrets/rest/RestSecretStore.javasrc/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.javasrc/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.javasrc/test/java/ai/labs/eddi/secrets/impl/VaultSecretProviderBranchTest.javasrc/test/java/ai/labs/eddi/secrets/rest/RestSecretStoreTest.java
There was a problem hiding this comment.
Pull request overview
Adds a destructive “reset tenant vault” operation to the Secrets Vault subsystem to recover from unrecoverable master-key (KEK) changes, while improving REST-side logging and extending test coverage for the new and existing vault branches.
Changes:
- Introduce
ISecretProvider.resetTenant()and implement it inVaultSecretProviderto delete all tenant secrets and the tenant DEK. - Expose
POST /secretstore/secrets/{tenantId}/reset(admin-only) inIRestSecretStore/RestSecretStore, with validation + cache invalidation. - Expand unit/branch-coverage tests around vault error paths, key rotation, reset behavior, and AgentSetupService vault-reference passthrough.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/secrets/ISecretProvider.java | Adds the resetTenant SPI method for destructive tenant vault resets. |
| src/main/java/ai/labs/eddi/secrets/impl/VaultSecretProvider.java | Implements tenant reset and improves DEK decryption failure messaging + DEK creation factoring. |
| src/main/java/ai/labs/eddi/secrets/rest/IRestSecretStore.java | Adds the admin REST contract for POST /{tenantId}/reset. |
| src/main/java/ai/labs/eddi/secrets/rest/RestSecretStore.java | Implements the reset endpoint and standardizes error logging to include stack traces. |
| src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java | Avoids re-vaulting API keys that are already vault references (passthrough behavior). |
| src/test/java/ai/labs/eddi/secrets/rest/RestSecretStoreTest.java | Adds nested tests for additional REST error branches + resetTenant behavior. |
| src/test/java/ai/labs/eddi/secrets/impl/VaultSecretProviderBranchTest.java | New extended branch-coverage tests for VaultSecretProvider (DEK/KEK rotation, reset, failure branches). |
| src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java | New branch-coverage tests for AgentSetupService helpers and vault passthrough behavior. |
| src/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.java | Updates test double to implement the new SPI method. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+499
to
+503
| // Already a vault reference — use it directly, don't re-vault | ||
| if (apiKey.startsWith("${vault:") && apiKey.endsWith("}")) { | ||
| LOGGER.infof("API key for agent '%s' is already a vault reference — using as-is.", agentName); | ||
| return apiKey; | ||
| } |
Comment on lines
+399
to
+409
| // Delete all secrets first, then the DEK | ||
| var secrets = persistence.listSecretsByTenant(tenantId); | ||
| int secretCount = secrets.size(); | ||
|
|
||
| for (var secret : secrets) { | ||
| persistence.deleteSecret(secret.getTenantId(), secret.getKeyName()); | ||
| } | ||
| persistence.deleteDek(tenantId); | ||
|
|
||
| LOGGER.infof("[VAULT] Tenant '%s' reset: %d secret(s) deleted, DEK removed.", tenantId, secretCount); | ||
| return secretCount; |
- vaultApiKey: use SecretReference.isVaultReference() + compiledPattern() to also handle legacy references (CodeRabbit + Copilot). - resetTenant: count actual successful deletions via deleteSecret() return value instead of assuming secrets.size() (Copilot). - InMemorySecretProvider.resetTenant: return actual deleted count (Copilot). - Add tests for legacy eddivault and full-form vault reference passthrough. - Fix deleteSecret mock to return true for accurate count assertions.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/ai/labs/eddi/secrets/impl/VaultSecretProviderBranchTest.java (1)
387-396: ⚡ Quick winAdd a partial-delete test for
resetTenantcount semantics.
happyPathnow correctly stubs all deletions astrue, but there’s still no branch that proves the returned count excludes failed deletes (deleteSecret == false). Since this endpoint’s contract is “count successful deletions,” add a mixed-outcome test (true,false) and assert count1while still verifyingdeleteDekis called.🤖 Prompt for 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. In `@src/test/java/ai/labs/eddi/secrets/impl/VaultSecretProviderBranchTest.java` around lines 387 - 396, The current test for resetTenant only validates the successful deletion path where all deleteSecret calls return true. Add a new test method to verify the partial-delete scenario where some deleteSecret calls return false and others return true, to ensure the returned count from resetTenant correctly reflects only successful deletions. Configure the mock for persistence.deleteSecret to return mixed outcomes (true for one key and false for the other), call provider.resetTenant(TENANT_ID), assert the result equals 1 (counting only successful deletes), and verify that deleteDek is still invoked to ensure all cleanup operations are attempted regardless of individual deleteSecret outcomes.
🤖 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.
Nitpick comments:
In `@src/test/java/ai/labs/eddi/secrets/impl/VaultSecretProviderBranchTest.java`:
- Around line 387-396: The current test for resetTenant only validates the
successful deletion path where all deleteSecret calls return true. Add a new
test method to verify the partial-delete scenario where some deleteSecret calls
return false and others return true, to ensure the returned count from
resetTenant correctly reflects only successful deletions. Configure the mock for
persistence.deleteSecret to return mixed outcomes (true for one key and false
for the other), call provider.resetTenant(TENANT_ID), assert the result equals 1
(counting only successful deletes), and verify that deleteDek is still invoked
to ensure all cleanup operations are attempted regardless of individual
deleteSecret outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49204f4c-9e15-4c74-8d4c-a728d5aab196
📒 Files selected for processing (5)
src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.javasrc/main/java/ai/labs/eddi/secrets/impl/VaultSecretProvider.javasrc/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.javasrc/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.javasrc/test/java/ai/labs/eddi/secrets/impl/VaultSecretProviderBranchTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/ai/labs/eddi/configs/agents/AgentSigningServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java
- src/main/java/ai/labs/eddi/secrets/impl/VaultSecretProvider.java
- src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java
- Replace high-entropy dummy value 'newkey12345678' with clearly-fake 'test-new-master-key' in RestSecretStoreTest rotateKek fixtures. - Add historical fingerprints to .gitleaksignore for the already-pushed commit (06ef33a) — these are unit test fixture values, not real secrets.
…verage tests - fromObjectPath: String, Map, List, Integer, Float, Boolean value types - fromObjectPath + toObjectPath: PathNavigator.setValue integration - scope=secret: auto-vault store + graceful degradation on vault failure - valueFloat, valueBoolean, valueList: typed property instruction coverage - TemplateEngineException: wraps in LifecycleException - CATCH_ANY_INPUT_AS_PROPERTY: empty input skips property - Previous step null actions: null-safe actions data handling - configure: URI-based config loading + ServiceException wrapping Total: 45 tests (28 existing + 17 new), all passing.
- Fix CodeQL log injection finding: sanitize(tenantId) in resetTenant log message and exception message using LogSanitizer. - Add partial-delete test proving returned count excludes failed deletes (mixed true/false outcome from deleteSecret). PR nitpick feedback.
Gitleaks only supports full-line # comments. Inline comments after fingerprints were being treated as part of the fingerprint string, preventing the suppression from matching.
rolandpickl
approved these changes
Jun 18, 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
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.
Summary
This pull request introduces a new "reset vault" operation for tenant secrets management, improves error handling and logging, and updates the test infrastructure to support the new functionality. The most significant changes are the addition of a destructive vault reset endpoint, robust handling for master key changes, and improved clarity in error messages.
Vault reset functionality:
resetTenantmethod to theISecretProviderinterface and implemented it inVaultSecretProvider, allowing administrators to delete all secrets and the DEK for a specific tenant, enabling recovery when the master key changes and the old key is unavailable. [1] [2]POST /secretstore/secrets/{tenantId}/resetinIRestSecretStoreandRestSecretStorefor admins to trigger the vault reset operation. [1] [2]Robust handling of master key changes:
VaultSecretProvider: if the DEK cannot be decrypted due to a master key change, the user receives a clear error with actionable recovery steps, including using the new reset endpoint.Logging and error handling improvements:
RestSecretStoreto useLOGGER.errorwith exception details instead of custom formatted error messages, providing better stack traces and easier debugging. (F1384645L137R137, [1] [2] [3] [4] [5]Test updates:
AgentSigningServiceTestto implement the newresetTenantmethod, and extendedRestSecretStoreTestwith new imports and test structure to support the new functionality. [1] [2]Other improvements:
AgentSetupService, improved handling of API keys that are already vault references by detecting and using them as-is, avoiding unnecessary re-vaulting.Type of Change
Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements