Remove vault.secrets.get from gateway; add workflow-based secret retrieval and cross-namespace E2E tests#21660
Open
prashantkumar1982 wants to merge 7 commits intodevelopfrom
Open
Remove vault.secrets.get from gateway; add workflow-based secret retrieval and cross-namespace E2E tests#21660prashantkumar1982 wants to merge 7 commits intodevelopfrom
prashantkumar1982 wants to merge 7 commits intodevelopfrom
Conversation
product-security-plaid-production
bot
requested review from
bolekk,
justinkaseman and
patrickhuie19
Contributor
|
✅ No conflicts with other open PRs targeting |
Contributor
|
I see you updated files related to
|
…etrieval tests Remove the `vault.secrets.get` method from the gateway surface entirely: - Remove `GetSupportedMethods()` dev-build conditional that exposed the method - Remove `handleSecretsGet()` and `getEncryptionKeys()` from the gateway handler - Remove the `MethodSecretsGet` case from the gateway-side handler and aggregator - Remove the now-unused `capRegistry` field from `GatewayHandler` Replace the commented-out gateway-based get test with a WASM workflow that calls `runtime.GetSecret()`, and add cross-namespace E2E coverage proving that secrets with the same ID in different namespaces are fully independent (create, get, update, list, delete all scoped to their namespace). Made-with: Cursor
- In the vaultsecret WASM workflow, check that the GetSecret error specifically contains "key does not exist" instead of accepting any error as proof the secret was deleted. This prevents config-propagation, transport, or decryption failures from masking real bugs. - In updateVaultCapabilityConfigInRegistry, replace the fire-and-sleep pattern with sethClient.WaitMined + receipt status assertion so a reverted or stuck tx fails the test immediately instead of causing a downstream workflow flake. Made-with: Cursor
- Fix goimports import ordering in v2_vault_don_test.go - Add vaultsecret module to go.md dependency graph - Fix updateVaultCapabilityConfigInRegistry to dynamically find the DON that has vault@1.0.0 instead of hardcoding "workflow-don", which fails in the workflow-gateway-capabilities topology where vault lives on "capabilities-don" Made-with: Cursor
…n in multi-DON topology In the workflow-gateway-capabilities topology, the vault capability runs on a separate capabilities-don. Without MethodConfigs, the launcher treats vault as a V1 capability and passes nil transmissionConfig. Since secrets.go doesn't set Config on the CapabilityRequest, the V1 fallback path fails with "cannot unwrap nil values.Map" when extracting transmission config. Adding RemoteExecutableConfig for the vault.secrets.get method ensures the V2 Don2Don framework is used, which sets transmissionConfig from the on-chain registry config rather than requiring it per-request.
prashantkumar1982
force-pushed
the
vault-get-secret-workflow-test
branch
from
652a213 to
4ba906e
Compare
Tofel
reviewed
Mar 24, 2026
| deployerKey, err := crypto.HexToECDSA(ctfblockchain.DefaultAnvilPrivateKey) | ||
| require.NoError(t, err, "failed to parse deployer private key") | ||
| deployerOpts, err := bind.NewKeyedTransactorWithChainID(deployerKey, big.NewInt(sethClient.ChainID)) | ||
| require.NoError(t, err, "failed to create deployer transact opts") |
Contributor
There was a problem hiding this comment.
you can delete this and use seth directly, check the comment below
Tofel
reviewed
Mar 24, 2026
|
|
||
| receipt, err := sethClient.WaitMined(t.Context(), testLogger, sethClient.Client, tx) | ||
| require.NoError(t, err, "UpdateDONByName tx was not mined") | ||
| require.Equal(t, uint64(1), receipt.Status, "UpdateDONByName tx reverted on-chain") |
Contributor
There was a problem hiding this comment.
you can replace these 5 lines above + deployerOpts with:
_, err = sethClient.Decode(capReg.UpdateDONByName(sethClient.NewTxOpts(), don.Name, updateParams)`
It will return only once tx is mined. if it reverted then err != nil.
Tofel
requested changes
Mar 24, 2026
Contributor
Tofel
left a comment
Tofel
left a comment
There was a problem hiding this comment.
Please parallelise the newly added tests inside that big Vault test. We can't merge as-is, because that test becomes by far the longest running one and goes against our efforts to speed up CI.
cedric-cordenier
previously approved these changes
Mar 24, 2026
…duce sleeps - Split ExecuteVaultTest into two parallel subtests (basic_crud + cross_namespace), each with its own per-test keys, ChIP sink, and channels. Follows the same pattern as HTTP Action tests. When parallelEnabled && fanoutEnabled, both subtests run concurrently. - Replace manual deployerKey/deployerOpts + WaitMined + receipt check with a deployer seth client and sethClient.Decode() per Tofel's review. - Remove redundant 30s "Vault DON ready" sleep. - Reduce registry syncer wait from 30s to 15s (polls every 12s). - Reduce allowlist sleep from 10s to 6s (polls every 5s). - Remove unused consensus workflow deployment from vault test setup.
… into vault-get-secret-workflow-test
Each subtest creates a new per-test key that hasn't completed the linkOwner flow on the workflow registry. Call creworkflow.LinkOwner() explicitly before any allowlistRequest operations. Made-with: Cursor
|
cedric-cordenier
approved these changes
Mar 25, 2026
Tofel
approved these changes
Mar 25, 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
3 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.
Summary
vault.secrets.getfrom the gateway surface entirely. The method was conditionally enabled for dev builds viaGetSupportedMethods(), but secrets should only be retrieved through workflows callingruntime.GetSecret(). This removeshandleSecretsGet(),getEncryptionKeys(), theMethodSecretsGetswitch cases in both the node-side and gateway-side handlers, the special-case in the aggregator, and the now-unusedcapRegistryfield fromGatewayHandler.vaultsecret) that callsruntime.GetSecret()to replace the previously commented-out gateway-based get test. The workflow is configured with a secret key, namespace, and anExpectNotFoundflag for negative testing.ExecuteVaultTest: creates two secrets with the same ID in namespaces"main"and"alt", then verifies that create, get (via workflow), update, list, and delete are all scoped to their respective namespace — updating or deleting in one namespace does not affect the other.