Conversation
|
""" WalkthroughThe changes refactor vault storage handling by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant VaultStorage
Client->>Server: Create/Update PluginPolicy (with publicKey, signature)
Server->>VaultStorage: GetVault(publicKey)
VaultStorage-->>Server: Encrypted vault data
Server->>Server: Decrypt vault with EncryptionSecret
Server->>Server: Derive public key from vault
Server->>Server: Verify policy signature using derived public key
Server-->>Client: Respond with verification result
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package session: could not load export data: no export data for "github.com/vultisig/go-wrappers/go-dkls/sessions"" ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the vault storage implementation throughout the codebase and cleans up related utilities and dependencies.
- Replaces
BlockStorageusage with a newvault.Storageabstraction in CLI and server layers - Introduces
EncryptionSecretin configuration and centralizes vault decryption in a shared helper - Cleans up unused utilities in
common/util.goand trims indirect dependencies ingo.mod
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/sigutil/signing.go | Renamed signature verifier to VerifyPolicySignature and removed unused imports |
| go.mod | Updated verifier/vultiserver versions and pruned indirect deps |
| config/config.go | Added EncryptionSecret field to top-level Config |
| common/util.go | Removed unused helpers and updated vaultBackupSuffix |
| cmd/vultisigner/main.go | Switched from storage.NewBlockStorage to vault.NewBlockStorageImp |
| api/server.go | Replaced blockStorage with vaultStorage and updated variable naming |
| api/plugin.go | Added getVault helper, updated policy signature flow |
Comments suppressed due to low confidence (3)
api/server.go:295
- [nitpick] The variable name
vis too short; consider renaming it tovaultDataor similar for better readability.
v, err := common.DecryptVaultFromBackup(passwd, content)
internal/sigutil/signing.go:71
- [nitpick] The parameter name
messageHexsuggests a hex-encoded string, but it takes raw bytes; consider renaming it tomessageBytesormessage.
func VerifyPolicySignature(publicKeyHex string, messageHex []byte, signature []byte) (bool, error) {
cmd/vultisigner/main.go:54
- [nitpick] The suffix
ImpinNewBlockStorageImpis unclear; consider renaming toNewBlockStorageorNewDefaultBlockStoragefor consistency.
vaultStorage, err := vault.NewBlockStorageImp(cfg.BlockStorage)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
api/plugin.go (1)
96-105: 🛠️ Refactor suggestionGuard against empty / nil vault bytes returned from storage
vaultStorage.GetVaultmay legitimately returnnil/[]byte{}together witherr == nil(e.g. zero-sized object, mis-configured bucket ACL, etc.).
Passing this straight toDecryptVaultFromBackupwill bubble up an opaque base-64 decode error.-content, err := s.vaultStorage.GetVault(filePathName) -if err != nil { +content, err := s.vaultStorage.GetVault(filePathName) +if err != nil { ... } + +if len(content) == 0 { + return fmt.Errorf("vault file %s is empty", filePathName) +}Repeating this check here and in
server.go’s handlers will save a trip down the call-stack and deliver clearer diagnostics.api/server.go (1)
331-341: 🛠️ Refactor suggestionNil vault bytes check (SignMessages)
Please replicate the same empty-content guard here to keep behaviour consistent across endpoints.
🧹 Nitpick comments (4)
api/plugin.go (2)
380-389: Verification path assumes ECDSA – will fail for EdDSA vaults
verifyPolicySignaturederives the public key with:derivedPublicKey, _ := tss.GetDerivedPubKey(vault.PublicKeyEcdsa, ...)If the vault happens to be an EdDSA one (
vault.PublicKeyEddsa),PublicKeyEcdsais empty andGetDerivedPubKeyreturns an error, causing the whole signature check to fail.Suggested defensive logic:
-var derivedPublicKey, err = tss.GetDerivedPubKey(vault.PublicKeyEcdsa, vault.HexChainCode, vcommon.Ethereum.GetDerivePath(), false) +pk := vault.PublicKeyEcdsa +isEdDSA := false +if pk == "" { + pk = vault.PublicKeyEddsa + isEdDSA = true +} +derivedPublicKey, err := tss.GetDerivedPubKey(pk, vault.HexChainCode, vcommon.Ethereum.GetDerivePath(), isEdDSA)Also, pass the same
isEdDSAflag down tosigutil.VerifyPolicySignatureif it supports EdDSA.
399-419: Improve error propagation & nil-content handling ingetVault
- The wrapped error from storage is discarded:
return nil, fmt.Errorf("failed to get vault")This loses the original cause, making troubleshooting harder.
- Empty content is only logged, but
DecryptVaultFromBackupwill panic on it.-vaultContent, err := s.vaultStorage.GetVault(fileName) -if err != nil { - ... - return nil, fmt.Errorf("failed to get vault") -} -if vaultContent == nil { - s.logger.Error("vault not found") - return nil, fmt.Errorf("vault not found") -} +vaultContent, err := s.vaultStorage.GetVault(fileName) +if err != nil { + return nil, fmt.Errorf("failed to get vault: %w", err) +} +if len(vaultContent) == 0 { + return nil, fmt.Errorf("vault %s is empty", fileName) +}This keeps the original stack-trace and prevents obscure base-64 errors later on.
api/server.go (2)
40-42: Cross-module coupling introduced viaverifier/vaultThe plugin server now depends on
github.com/vultisig/verifier/vault.
While technically fine, this increases compile-time coupling between the plugin and verifier repos and can result in circular-dependency headaches if both evolve independently.Consider extracting the minimal storage interface into a tiny, version-stable shared module (e.g.
github.com/vultisig/common/vaultiface) to decouple the two services.
398-401: HTTP semantics: return 404 instead of 400 when vault is absent
ExistVaultcurrently maps “file does not exist” to 400 Bad Request:exist, err := s.vaultStorage.Exist(filePathName) if err != nil || !exist { return c.NoContent(http.StatusBadRequest) }For a well-formed request where the resource simply isn’t found, 404 Not Found is semantically more accurate:
-if err != nil || !exist { - return c.NoContent(http.StatusBadRequest) +if err != nil { + s.logger.WithError(err).Error("failed to query vault storage") + return c.NoContent(http.StatusInternalServerError) +} +if !exist { + return c.NoContent(http.StatusNotFound) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
api/plugin.go(3 hunks)api/server.go(7 hunks)cmd/vultisigner/main.go(3 hunks)common/util.go(1 hunks)config/config.go(1 hunks)go.mod(1 hunks)internal/sigutil/signing.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/server.go (1)
common/util.go (1)
DecryptVaultFromBackup(137-166)
🔇 Additional comments (8)
go.mod (2)
22-23: Dependency updates look good.The updates to verifier and vultiserver dependencies support the vault storage refactoring. Removing indirect dependencies also helps clean up the codebase.
52-52: Addition of CORS dependency.The addition of
github.com/rs/cors v1.8.3as an indirect dependency suggests API changes. Ensure this is intentional and properly configured if the API needs to handle cross-origin requests.cmd/vultisigner/main.go (3)
9-9: LGTM: Added import for vault package.This import supports the refactoring of vault storage implementation.
54-54: Refactored block storage to vault storage.The change from
blockStoragetovaultStorageusingvault.NewBlockStorageImp()aligns with the PR objectives of refactoring vault storage implementation.
65-77:Details
✅ Verification successful
Removed pluginConfigs parameter from NewServer.
The call to
api.NewServerno longer includescfg.Plugin.PluginConfigsas an argument, which indicates changes to server initialization. Ensure that this was an intentional removal and that the server can function correctly without this parameter.Run the following script to verify this change:
🏁 Script executed:
#!/bin/bash # Check how NewServer is defined in the api package rg -A 5 "func NewServer" api/Length of output: 275
I’ll check the full
NewServersignature and search for anyPluginConfigsreferences to confirm its removal.
🏁 Script executed:
#!/bin/bash # Show full NewServer signature rg -A20 "func NewServer" api/server.go # Search for any PluginConfigs usage in the api package rg -n "PluginConfigs" api/Length of output: 598
pluginConfigs Parameter Removal Confirmed
TheNewServersignature inapi/server.gono longer accepts apluginConfigsargument, and all call sites—including incmd/vultisigner/main.go—have been updated to match. No further action is required.
api/server.goshows the new signature withoutpluginConfigs.- A search for
PluginConfigsin theapipackage yields no results.- The invocation in
cmd/vultisigner/main.goaligns with the updated parameters.internal/sigutil/signing.go (2)
71-71: Simplified signature verification function.Renaming
VerifySignaturetoVerifyPolicySignatureand simplifying the parameters is a good refactor. The function now directly accepts the public key hex string instead of deriving it, which improves code clarity and separation of concerns.
71-92:Details
✅ Verification successful
Verify key derivation logic is implemented elsewhere.
The removal of key derivation from this function suggests that this logic has been moved elsewhere. Ensure that the public key derivation is properly implemented in the vault decryption process to maintain security.
Let's check where the public key derivation is now implemented:
🏁 Script executed:
#!/bin/bash # Look for public key derivation in the codebase rg -A 5 "GetAddress|Derive" --include "*.go" # Check how VerifyPolicySignature is called rg -A 5 "VerifyPolicySignature" --include "*.go"Length of output: 979
🏁 Script executed:
#!/bin/bash # Search for public key derivation logic in Go files rg -g '*.go' -A 5 "Derive" rg -g '*.go' -A 5 "GetAddress" # Locate all calls to VerifyPolicySignature rg -g '*.go' -A 5 "VerifyPolicySignature"Length of output: 5243
Derivation logic verified elsewhere
The public‐key derivation has been moved out ofVerifyPolicySignatureinto:
common/util.go’sDeriveAddress(which wrapstss.GetDerivedPubKey)- The TSS layer calls in
api/server.go(GetDerivedPublicKey) andapi/plugin.gobefore invokingsigutil.VerifyPolicySignatureSince key derivation occurs correctly in those components and
VerifyPolicySignaturenow solely handles ECDSA verification, no further changes are needed.common/util.go (1)
30-32: Potential breaking change after renaming backup suffix
vaultBackupSuffixis now".bak"(was previously".bak.vult"according to commit message).
Any existing backups on disk that still carry the old extension will no longer be discoverable byGetVaultBackupFilename, which could lead to failed migrations or data-loss if operators upgrade without renaming the files.Consider one of the following:
-const ( - vaultBackupSuffix = ".bak" -) +// keep the new suffix but transparently fall back to the legacy one +const ( + vaultBackupSuffix = ".bak" + legacyBackupSuffix = ".bak.vult" +) + +func GetVaultBackupFilename(pubKey string) string { + return fmt.Sprintf("%s%s", pubKey, vaultBackupSuffix) +} + +// helper for readers – tries new first, then legacy +func ResolveBackupFilenames(pubKey string) []string { + return []string{ + fmt.Sprintf("%s%s", pubKey, vaultBackupSuffix), + fmt.Sprintf("%s%s", pubKey, legacyBackupSuffix), + } +}…and update callers (
vaultStorage.GetVault,Exist, etc.) to iterate through both names until one succeeds, then optionally migrate the file to the new name.Please verify whether backward compatibility is required in production before merging.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
api/plugin.go (1)
96-105: 💡 Verification agent🧩 Analysis chain
Use the new
getVaulthelper to avoid duplicated logic and secret / password drift
SignPluginMessagesre-implements vault-retrieval + decryption even though you just introducedgetVault.
Besides duplication, it now decrypts withreq.VaultPasswordwhereasverifyPolicySignaturedecrypts withcfg.EncryptionSecret. If those secrets can diverge (e.g., operator rotates the server‐side secret but a stale client keeps an old password) you risk hard-to-debug signing failures.Consider replacing lines 96-105 with:
- filePathName := common.GetVaultBackupFilename(req.PublicKey) - content, err := s.vaultStorage.GetVault(filePathName) - if err != nil { - wrappedErr := fmt.Errorf("fail to read file, err: %w", err) - ... - } - - _, err = common.DecryptVaultFromBackup(req.VaultPassword, content) + _, err := s.getVault(req.PublicKey) // re-uses centralised logic & cfg.EncryptionSecretand decide on one authoritative secret (probably the server-side
EncryptionSecret).
This keeps a single code-path, reduces I/O, and prevents inconsistencies between password sources.
🏁 Script executed:
#!/bin/bash # Locate the definition of getVault and inspect its implementation rg -n "func .*getVault" -C3 . # Locate verifyPolicySignature to confirm which secret it uses for decryption rg -n "verifyPolicySignature" -C3 .Length of output: 1974
🏁 Script executed:
#!/bin/bash # Display full getVault implementation for context rg -n "func (s \\*Server) getVault" -C30 api/plugin.goLength of output: 55
🏁 Script executed:
#!/bin/bash # Display full implementation of getVault in api/plugin.go sed -n '380,430p' api/plugin.go # Locate usages of DecryptVaultFromBackup to see how getVault handles decryption rg -n "DecryptVaultFromBackup" -C5 api/plugin.goLength of output: 2467
Simplify vault loading in SignPluginMessages
Use the sharedgetVaulthelper (which usess.cfg.EncryptionSecret) instead of re-implementing file I/O and decryption withreq.VaultPassword. This ensures a single decryption secret and removes duplicate code.—in api/plugin.go around lines 96–105—
Replace:filePathName := common.GetVaultBackupFilename(req.PublicKey) content, err := s.vaultStorage.GetVault(filePathName) if err != nil { wrappedErr := fmt.Errorf("fail to read file, err: %w", err) s.logger.Infof("fail to read file in SignPluginMessages, err: %v", err) s.logger.Error(wrappedErr) return wrappedErr } _, err = common.DecryptVaultFromBackup(req.VaultPassword, content) if err != nil { return fmt.Errorf("fail to decrypt vault from the backup, err: %w", err) }With:
vault, err := s.getVault(req.PublicKey) if err != nil { return fmt.Errorf("fail to load vault: %w", err) } // …use vault for signing…• Drops
req.VaultPasswordin favor of the single server‐sideEncryptionSecret
• Removes duplicated I/O and error handling
• Keeps one source of truth for vault decryption
🧹 Nitpick comments (1)
api/plugin.go (1)
399-415: Graceful handling when vaults are not encrypted & minor clean-ups
getVaultcurrently aborts ifEncryptionSecretis empty, even when the vault itself is stored unencrypted (IsEncrypted == false).
Users running in plain-text mode will be blocked. Suggest checking theIsEncryptedflag after reading the backup and only requiring the secret when needed.Double-logging the same error (
WithError(err).Error("fail to get vault")then wrapping and returning) clutters logs. Prefer either:return nil, fmt.Errorf("failed to get vault: %w", err) // let caller logor keep the log line but wrap with
%wonce – not both.Nit: the wrapping message has a punctuation typo (
"failed to decrypt vault,err: %w"). Add a space after the comma.- return nil, fmt.Errorf("failed to decrypt vault,err: %w", err) + return nil, fmt.Errorf("failed to decrypt vault: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
api/plugin.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/plugin.go (3)
internal/sigutil/signing.go (1)
VerifyPolicySignature(71-92)api/server.go (1)
Server(36-50)common/util.go (2)
GetVaultBackupFilename(265-267)DecryptVaultFromBackup(137-166)
| vault, err := s.getVault(policy.PublicKey) | ||
| if err != nil { | ||
| s.logger.WithError(err).Error("fail to get vault") | ||
| return false | ||
| } | ||
| derivedPublicKey, err := tss.GetDerivedPubKey(vault.PublicKeyEcdsa, vault.HexChainCode, vcommon.Ethereum.GetDerivePath(), false) | ||
| if err != nil { | ||
| s.logger.WithError(err).Error("failed to get derived public key") | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
Prefix handling & potential verification mismatch
derivedPublicKey may contain a 0x prefix while sigutil.VerifyPolicySignature expects a pure hex string (it later runs hex.DecodeString).
If the prefix is present, verification will always fail with “invalid byte” errors.
Add an explicit trim before calling the verifier:
-derivedPublicKey, err := tss.GetDerivedPubKey(...)
+derivedPublicKey, err := tss.GetDerivedPubKey(...)
+derivedPublicKey = strings.TrimPrefix(derivedPublicKey, "0x")Also, reuse strings.TrimPrefix consistently across the file for clarity.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vault, err := s.getVault(policy.PublicKey) | |
| if err != nil { | |
| s.logger.WithError(err).Error("fail to get vault") | |
| return false | |
| } | |
| derivedPublicKey, err := tss.GetDerivedPubKey(vault.PublicKeyEcdsa, vault.HexChainCode, vcommon.Ethereum.GetDerivePath(), false) | |
| if err != nil { | |
| s.logger.WithError(err).Error("failed to get derived public key") | |
| return false | |
| } | |
| vault, err := s.getVault(policy.PublicKey) | |
| if err != nil { | |
| s.logger.WithError(err).Error("fail to get vault") | |
| return false | |
| } | |
| derivedPublicKey, err := tss.GetDerivedPubKey(vault.PublicKeyEcdsa, vault.HexChainCode, vcommon.Ethereum.GetDerivePath(), false) | |
| derivedPublicKey = strings.TrimPrefix(derivedPublicKey, "0x") | |
| if err != nil { | |
| s.logger.WithError(err).Error("failed to get derived public key") | |
| return false | |
| } |
🤖 Prompt for AI Agents
In api/plugin.go around lines 380 to 390, the derivedPublicKey may include a
"0x" prefix which causes sigutil.VerifyPolicySignature to fail due to invalid
hex decoding. Fix this by explicitly trimming the "0x" prefix from
derivedPublicKey using strings.TrimPrefix before passing it to the verifier.
Also, ensure that strings.TrimPrefix is used consistently throughout the file
for handling hex string prefixes.
fixes #47
Summary by CodeRabbit
New Features
Refactor
Dependency Updates
Chores