new secret store impl #2525
new secret store impl #2525
Conversation
WalkthroughThis PR adds end-to-end secrets management: a new CLI secret subcommand (get/set/list) with name validation; Electron safeStorage encrypt/decrypt RPC handlers; new RPC types and wrappers for encrypt/decrypt and secrets operations; a frontend RPC API surface and type definitions; server-side RPC handlers that use a new in-process secret store; and pkg/secretstore implementing an encrypted, debounced, persistently stored secrets map with Linux storage-backend discovery and concurrency controls. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| func writeSecretsToFile() error { | ||
| lock.Lock() | ||
| secretsCopy := make(map[string]string, len(secrets)+1) |
Check failure
Code scanning / CodeQL
Size computation for allocation may overflow High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
The best practice is to validate the size of untrusted inputs before using them in allocations. Since the issue is in computing len(secrets) (a possibly giant map if an attacker has crafted the secrets.enc file), we should reject/unload secrets whose size is excessive, both when reading and allocating.
- In
readSecretsFromFile(pkg/secretstore/secretstore.go), after unmarshaling, add a check: if the resulting map's length (len(decryptedSecrets)) exceeds some reasonable threshold (e.g., 1024, or a configurable constant), reject the secrets with an error and refuse to load them. This avoids poisoning the program state. - In
writeSecretsToFile, before allocating the new map withmake(map[string]string, len(secrets)+1), checklen(secrets). If it exceeds the threshold, return an error and refuse to allocate. - Define a
MaxSecretsCountconstant (in the secretstore file) and use it for both checks. - No new imports needed—standard Go code suffices.
| @@ -28,6 +28,7 @@ | ||
| InitRetryMs = 1000 | ||
| SecretNamePattern = `^[A-Za-z][A-Za-z0-9_]*$` | ||
| WriteTsKey = "wave:writets" | ||
| MaxSecretsCount = 1024 | ||
| ) | ||
|
|
||
| var lock sync.Mutex | ||
| @@ -118,6 +119,9 @@ | ||
| if err := json.Unmarshal([]byte(result.PlainText), &decryptedSecrets); err != nil { | ||
| return nil, fmt.Errorf("failed to parse secrets: %w", err) | ||
| } | ||
| if len(decryptedSecrets) > MaxSecretsCount { | ||
| return nil, fmt.Errorf("secrets file too large: %d secrets (max allowed %d)", len(decryptedSecrets), MaxSecretsCount) | ||
| } | ||
|
|
||
| return decryptedSecrets, nil | ||
| } | ||
| @@ -165,6 +169,10 @@ | ||
|
|
||
| func writeSecretsToFile() error { | ||
| lock.Lock() | ||
| if len(secrets) > MaxSecretsCount { | ||
| lock.Unlock() | ||
| return fmt.Errorf("too many secrets to write: %d (max allowed %d)", len(secrets), MaxSecretsCount) | ||
| } | ||
| secretsCopy := make(map[string]string, len(secrets)+1) | ||
| for k, v := range secrets { | ||
| secretsCopy[k] = v |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
cmd/wsh/cmd/wshcmd-secret.go(1 hunks)emain/emain-wsh.ts(2 hunks)frontend/app/store/wshclientapi.ts(3 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/secretstore/secretstore.go(1 hunks)pkg/wshrpc/wshclient/wshclient.go(3 hunks)pkg/wshrpc/wshrpctypes.go(4 hunks)pkg/wshrpc/wshserver/wshserver.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
pkg/wshrpc/wshserver/wshserver.gopkg/wshrpc/wshclient/wshclient.go
📚 Learning: 2025-11-01T00:57:23.025Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2504
File: frontend/app/aipanel/aipanel-contextmenu.ts:15-16
Timestamp: 2025-11-01T00:57:23.025Z
Learning: In the waveterm codebase, types defined in custom.d.ts are globally available and do not require explicit imports. Backend types defined in gotypes.d.ts are also globally available.
Applied to files:
frontend/types/gotypes.d.ts
🧬 Code graph analysis (8)
pkg/wshrpc/wshserver/wshserver.go (2)
pkg/wshrpc/wshclient/wshclient.go (4)
GetSecretsCommand(378-381)GetSecretsNamesCommand(390-393)SetSecretsCommand(602-605)GetSecretsLinuxStorageBackendCommand(384-387)pkg/secretstore/secretstore.go (4)
GetSecret(240-252)GetSecretNames(254-269)SetSecret(222-238)GetLinuxStorageBackend(271-292)
emain/emain-wsh.ts (1)
pkg/wshrpc/wshrpctypes.go (4)
CommandElectronEncryptData(1008-1010)CommandElectronEncryptRtnData(1012-1015)CommandElectronDecryptData(1017-1019)CommandElectronDecryptRtnData(1021-1024)
cmd/wsh/cmd/wshcmd-secret.go (5)
pkg/ijson/ijson.go (1)
Command(27-27)frontend/app/store/wshclientapi.ts (4)
GetSecretsCommand(311-313)GetSecretsLinuxStorageBackendCommand(316-318)SetSecretsCommand(501-503)GetSecretsNamesCommand(321-323)pkg/wshrpc/wshclient/wshclient.go (4)
GetSecretsCommand(378-381)GetSecretsLinuxStorageBackendCommand(384-387)SetSecretsCommand(602-605)GetSecretsNamesCommand(390-393)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(345-351)cmd/wsh/cmd/wshcmd-root.go (1)
WriteStdout(74-76)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (6)
ElectronEncryptCommand(156-158)ElectronDecryptCommand(151-153)GetSecretsCommand(311-313)GetSecretsNamesCommand(321-323)SetSecretsCommand(501-503)GetSecretsLinuxStorageBackendCommand(316-318)pkg/wshrpc/wshclient/wshclient.go (6)
ElectronEncryptCommand(195-198)ElectronDecryptCommand(189-192)GetSecretsCommand(378-381)GetSecretsNamesCommand(390-393)SetSecretsCommand(602-605)GetSecretsLinuxStorageBackendCommand(384-387)
frontend/app/store/wshclientapi.ts (1)
pkg/wshrpc/wshrpctypes.go (5)
CommandElectronDecryptData(1017-1019)RpcOpts(345-351)CommandElectronDecryptRtnData(1021-1024)CommandElectronEncryptData(1008-1010)CommandElectronEncryptRtnData(1012-1015)
pkg/secretstore/secretstore.go (5)
pkg/wshrpc/wshclient/barerpcclient.go (1)
GetBareRpcClient(30-39)pkg/wshrpc/wshrpctypes.go (3)
CommandElectronEncryptData(1008-1010)RpcOpts(345-351)CommandElectronDecryptData(1017-1019)pkg/wshutil/wshrouter.go (1)
ElectronRoute(26-26)pkg/wshrpc/wshclient/wshclient.go (2)
ElectronEncryptCommand(195-198)ElectronDecryptCommand(189-192)pkg/wavebase/wavebase.go (1)
GetWaveConfigDir(121-123)
frontend/types/gotypes.d.ts (1)
pkg/wshrpc/wshrpctypes.go (4)
CommandElectronDecryptData(1017-1019)CommandElectronDecryptRtnData(1021-1024)CommandElectronEncryptData(1008-1010)CommandElectronEncryptRtnData(1012-1015)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (6)
ElectronDecryptCommand(151-153)ElectronEncryptCommand(156-158)GetSecretsCommand(311-313)GetSecretsLinuxStorageBackendCommand(316-318)GetSecretsNamesCommand(321-323)SetSecretsCommand(501-503)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (5)
CommandElectronDecryptData(1017-1019)RpcOpts(345-351)CommandElectronDecryptRtnData(1021-1024)CommandElectronEncryptData(1008-1010)CommandElectronEncryptRtnData(1012-1015)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
| func (ws *WshServer) SetSecretsCommand(ctx context.Context, secrets map[string]string) error { | ||
| for name, value := range secrets { | ||
| err := secretstore.SetSecret(name, value) | ||
| if err != nil { | ||
| return fmt.Errorf("error setting secret %q: %w", name, err) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (ws *WshServer) GetSecretsLinuxStorageBackendCommand(ctx context.Context) (string, error) { | ||
| backend, err := secretstore.GetLinuxStorageBackend() | ||
| if err != nil { | ||
| return "", fmt.Errorf("error getting linux storage backend: %w", err) | ||
| } | ||
| return backend, nil | ||
| } |
There was a problem hiding this comment.
Block insecure storage backends on the server
SetSecretsCommand accepts secrets from every RPC client, but it never re-checks the Linux storage backend. The CLI currently guards against basic_text/unknown, yet any other caller (e.g., future Tsunami flows) can still hit this endpoint and persist secrets while Electron is falling back to plaintext. That’s a security regression. Please enforce the same backend validation here before writing.
func (ws *WshServer) SetSecretsCommand(ctx context.Context, secrets map[string]string) error {
+ backend, err := secretstore.GetLinuxStorageBackend()
+ if err != nil {
+ return fmt.Errorf("error getting linux storage backend: %w", err)
+ }
+ if backend == "basic_text" || backend == "unknown" {
+ return fmt.Errorf("linux storage backend %q does not support encrypted secrets", backend)
+ }
for name, value := range secrets {
- err := secretstore.SetSecret(name, value)
- if err != nil {
- return fmt.Errorf("error setting secret %q: %w", name, err)
- }
+ if err := secretstore.SetSecret(name, value); err != nil {
+ return fmt.Errorf("error setting secret %q: %w", name, err)
+ }
}
return nil
}🤖 Prompt for AI Agents
In pkg/wshrpc/wshserver/wshserver.go around lines 1267 to 1283,
SetSecretsCommand currently writes secrets without re-checking the Linux storage
backend; before looping and calling secretstore.SetSecret, call
secretstore.GetLinuxStorageBackend() and if it returns "basic_text" or "unknown"
(or any insecure/unsupported backend per secretstore policy) return an error
refusing to persist secrets, otherwise proceed to set them; ensure the error
message clearly states that writing secrets is blocked due to insecure Linux
storage backend.
…ssary since we always get it on read)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pkg/secretstore/secretstore.go (2)
93-111: Same context pattern issue as in getLinuxStorageBackend.The context timeout handling here has the same problem: it's checked after the RPC completes rather than being used to control the RPC's lifetime.
181-199: Same context pattern issue as in other functions.The context timeout handling here has the same problem described earlier.
🧹 Nitpick comments (3)
pkg/secretstore/secretstore.go (3)
145-149: Consider adding graceful shutdown mechanism for the writer goroutine.The
writerLoopgoroutine is started but there's no mechanism to stop it gracefully. This could complicate testing and clean shutdown scenarios.Consider adding a shutdown mechanism:
writeRequestChan = make(chan struct{}, 1) + // Consider adding: stopChan = make(chan struct{}) initialized = true lastInitErr = nil go writerLoop() return nilThen modify
writerLoopto select on both channels and provide aShutdown()function to close the stop channel.
152-164: Consider improving error handling in the writer loop.Write errors are only logged. Silent failures could lead to data loss without user awareness, especially for transient failures that might succeed on retry.
Consider:
- Implementing retry logic for transient errors
- Exposing write failures through a callback or error channel
- Adding metrics/alerting for persistent write failures
218-234: Consider blocking WriteTsKey as a reserved secret name.While
WriteTsKeyis a reserved internal key, there's no validation preventing users from setting it. This could interfere with the timestamp tracking mechanism.Add a check to block the reserved key:
func SetSecret(name string, value string) error { if name == "" { return fmt.Errorf("secret name cannot be empty") } + if name == WriteTsKey { + return fmt.Errorf("secret name is reserved for internal use") + } if !secretNameRegexp.MatchString(name) { return fmt.Errorf("secret name must start with a letter and contain only letters, numbers, and underscores") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/secretstore/secretstore.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/secretstore/secretstore.go (5)
pkg/wshrpc/wshclient/barerpcclient.go (1)
GetBareRpcClient(30-39)pkg/wshrpc/wshrpctypes.go (3)
CommandElectronEncryptData(1008-1010)RpcOpts(345-351)CommandElectronDecryptData(1017-1019)pkg/wshutil/wshrouter.go (1)
ElectronRoute(26-26)pkg/wshrpc/wshclient/wshclient.go (2)
ElectronEncryptCommand(195-198)ElectronDecryptCommand(189-192)pkg/wavebase/wavebase.go (1)
GetWaveConfigDir(121-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
pkg/secretstore/secretstore.go (6)
1-40: LGTM!The constants and package-level variables are well-structured. The use of a compiled regex and proper synchronization primitives demonstrates good practice.
166-209: Past review comment is no longer applicable.The previous review mentioned that
writeSecretsToFileupdateslinuxStorageBackendwithout holding the lock. However, this function no longer contains any code that updateslinuxStorageBackend, so that concern has been addressed.
211-216: LGTM!The non-blocking send pattern correctly ensures write requests are coalesced without blocking the caller.
236-248: LGTM!The function correctly handles the reserved
WriteTsKeyand follows proper locking conventions.
250-265: LGTM!The function correctly filters the reserved key and uses proper synchronization.
267-288: LGTM!The function correctly uses the lock throughout and properly calls
getLinuxStorageBackendwhile holding the required lock.
| ctx, cancel := context.WithTimeout(context.Background(), EncryptionTimeout*time.Millisecond) | ||
| defer cancel() | ||
|
|
||
| encryptData := wshrpc.CommandElectronEncryptData{ | ||
| PlainText: "hello", | ||
| } | ||
| rpcOpts := &wshrpc.RpcOpts{ | ||
| Route: wshutil.ElectronRoute, | ||
| Timeout: EncryptionTimeout, | ||
| } | ||
|
|
||
| result, err := wshclient.ElectronEncryptCommand(rpcClient, encryptData, rpcOpts) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get storage backend: %w", err) | ||
| } | ||
|
|
||
| if ctx.Err() != nil { | ||
| return fmt.Errorf("encryption timeout: %w", ctx.Err()) | ||
| } |
There was a problem hiding this comment.
Context is not used correctly for timeout handling.
The context is created with a timeout and checked for errors, but it's not actually used to control the RPC call's lifetime. The RPC timeout is handled separately via RpcOpts.Timeout. Checking ctx.Err() after the RPC completes (line 65) won't cancel the RPC if it exceeds the timeout.
Either remove the context entirely and rely on RpcOpts.Timeout, or pass the context to the RPC layer so it can properly cancel on timeout:
- ctx, cancel := context.WithTimeout(context.Background(), EncryptionTimeout*time.Millisecond)
- defer cancel()
-
encryptData := wshrpc.CommandElectronEncryptData{
PlainText: "hello",
}
rpcOpts := &wshrpc.RpcOpts{
Route: wshutil.ElectronRoute,
Timeout: EncryptionTimeout,
}
result, err := wshclient.ElectronEncryptCommand(rpcClient, encryptData, rpcOpts)
if err != nil {
return fmt.Errorf("failed to get storage backend: %w", err)
}
- if ctx.Err() != nil {
- return fmt.Errorf("encryption timeout: %w", ctx.Err())
- }
-
if result.StorageBackend != "" {
linuxStorageBackend = result.StorageBackend
}Note: This same pattern appears in readSecretsFromFile and writeSecretsToFile.
📝 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.
| ctx, cancel := context.WithTimeout(context.Background(), EncryptionTimeout*time.Millisecond) | |
| defer cancel() | |
| encryptData := wshrpc.CommandElectronEncryptData{ | |
| PlainText: "hello", | |
| } | |
| rpcOpts := &wshrpc.RpcOpts{ | |
| Route: wshutil.ElectronRoute, | |
| Timeout: EncryptionTimeout, | |
| } | |
| result, err := wshclient.ElectronEncryptCommand(rpcClient, encryptData, rpcOpts) | |
| if err != nil { | |
| return fmt.Errorf("failed to get storage backend: %w", err) | |
| } | |
| if ctx.Err() != nil { | |
| return fmt.Errorf("encryption timeout: %w", ctx.Err()) | |
| } | |
| encryptData := wshrpc.CommandElectronEncryptData{ | |
| PlainText: "hello", | |
| } | |
| rpcOpts := &wshrpc.RpcOpts{ | |
| Route: wshutil.ElectronRoute, | |
| Timeout: EncryptionTimeout, | |
| } | |
| result, err := wshclient.ElectronEncryptCommand(rpcClient, encryptData, rpcOpts) | |
| if err != nil { | |
| return fmt.Errorf("failed to get storage backend: %w", err) | |
| } |
🤖 Prompt for AI Agents
In pkg/secretstore/secretstore.go around lines 49 to 67, the context with
timeout is created but never passed into the RPC call so it does not actually
control cancellation; either remove the local context and rely solely on
RpcOpts.Timeout, or thread the ctx into the RPC layer (and into
readSecretsFromFile and writeSecretsToFile where the same pattern exists) so the
RPC call is cancellable — update the ElectronEncryptCommand call to accept ctx
(and propagate it through wshclient/wshrpc) or delete the unused ctx/cancel and
the ctx.Err() check and rely on rpcOpts.Timeout consistently across the three
functions.
|
|
||
| func writeSecretsToFile() error { | ||
| lock.Lock() | ||
| secretsCopy := make(map[string]string, len(secrets)+1) |
There was a problem hiding this comment.
Address potential integer overflow in size computation.
The GitHub Advanced Security bot flagged this line for a potential overflow in len(secrets)+1. While unlikely in practice (would require a map with MaxInt elements), this is worth addressing for completeness.
Consider using a capacity of 0 or checking for overflow:
- secretsCopy := make(map[string]string, len(secrets)+1)
+ // Avoid potential overflow - capacity is just a hint anyway
+ secretsCopy := make(map[string]string, len(secrets))🤖 Prompt for AI Agents
In pkg/secretstore/secretstore.go around line 168, the allocation
make(map[string]string, len(secrets)+1) can theoretically overflow when
len(secrets) == math.MaxInt; replace it with a safe allocation strategy: either
use make(map[string]string) or make(map[string]string, 0) to avoid computing
len(secrets)+1, or compute the capacity with a guard (if len(secrets) ==
math.MaxInt { cap = len(secrets) } else { cap = len(secrets)+1 }) and pass that
capped value to make; implement the chosen approach and ensure imports are added
if you use math.MaxInt.
| if err := os.WriteFile(secretsPath, []byte(result.CipherText), 0600); err != nil { | ||
| return fmt.Errorf("failed to write secrets file: %w", err) | ||
| } |
There was a problem hiding this comment.
File write lacks atomicity, risking corruption.
Writing directly to the target file with os.WriteFile can result in corruption if the process crashes or is interrupted mid-write. This is critical for secrets storage.
Use an atomic write pattern (write to temp file, then rename):
- if err := os.WriteFile(secretsPath, []byte(result.CipherText), 0600); err != nil {
- return fmt.Errorf("failed to write secrets file: %w", err)
- }
+ // Atomic write: write to temp file, then rename
+ tempPath := secretsPath + ".tmp"
+ if err := os.WriteFile(tempPath, []byte(result.CipherText), 0600); err != nil {
+ return fmt.Errorf("failed to write secrets file: %w", err)
+ }
+ if err := os.Rename(tempPath, secretsPath); err != nil {
+ os.Remove(tempPath) // Clean up temp file
+ return fmt.Errorf("failed to rename secrets file: %w", err)
+ }📝 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.
| if err := os.WriteFile(secretsPath, []byte(result.CipherText), 0600); err != nil { | |
| return fmt.Errorf("failed to write secrets file: %w", err) | |
| } | |
| // Atomic write: write to temp file, then rename | |
| tempPath := secretsPath + ".tmp" | |
| if err := os.WriteFile(tempPath, []byte(result.CipherText), 0600); err != nil { | |
| return fmt.Errorf("failed to write secrets file: %w", err) | |
| } | |
| if err := os.Rename(tempPath, secretsPath); err != nil { | |
| os.Remove(tempPath) // Clean up temp file | |
| return fmt.Errorf("failed to rename secrets file: %w", err) | |
| } |
🤖 Prompt for AI Agents
In pkg/secretstore/secretstore.go around lines 204 to 206, the current direct
os.WriteFile to secretsPath risks corruption; replace it with an atomic write:
create a secure temp file in the same directory (os.CreateTemp), write the
ciphertext to the temp file, set permissions to 0600 if needed, call file.Sync()
(and optionally fdatasync via syscall if available) and close the file, then
atomically rename the temp file to secretsPath using os.Rename; ensure you clean
up the temp file on errors and return any file/rename errors.
adds a new secret store protected by electron's safeStorage API. currently just hooked up to the CLI via a new wsh command:
wsh secret.will be used to power secrets in tsunami later (and for config + connections)