Repository navigation
fix: close the masking, put, store-lock, target-path, scan and HTTP-auth gaps - #3
Merged
Merged
Conversation
KeyNameIsSensitive existed but nothing called it, so a sensitive key holding a passphrase without digits (DB_PASSWORD=correcthorsebatterystaple) was returned verbatim by get/list, the MCP get tool and the MCP resource. MaskKey applies the key-name rule first and falls back to the value heuristics. store.List, the MCP get handler, the CLI get/set/vault commands, the Vault KV listing and the leak-scan report now use it.
put read any path the agent supplied and unlinked it afterwards, which made the tool an arbitrary-file read (store a file, then read it back with get when the value does not look secret) and an arbitrary-file delete. Value files now have to resolve inside <store>/inbox: symlink escapes, relative escapes and absolute paths elsewhere are refused. The inbox is created 0700, `keysmith inbox` prints it, and the tool description tells the agent where to write. e2e_test.py asserts the refusal and that the outside file survives.
Set/Delete/Rotate did a read-modify-write of the whole map with no lock. The MCP server and the CLI share the store, so an agent writing while a human runs `keysmith set` silently lost one of the writes: 20 concurrent `set` calls stored only 12 keys. Writers now take an exclusive flock on <store>/store.lock (unix; a no-op single-process fallback elsewhere), including key initialization, so two processes cannot generate competing identities. Temp files use a unique name per write, and randomSecret uses rejection sampling instead of a biased modulo, since the bias was easy to remove while touching the file.
--allow-path matched the parsed path with a string prefix, but url.Parse keeps dot segments, so a token bound to /services/ was redeemed for https://hooks.slack.com/services/../../admin. Encoded separators (%2f, %5c) and the '..;' path-parameter form were accepted the same way. ParseTarget now refuses '.'/'..' segments, encoded slashes, backslashes and dot segments hidden behind path parameters. '.well-known' still parses.
`keysmith scan` printed the matched line verbatim, so the secret it was warning about landed in the terminal, the CI log, and any issue the report was pasted into. Lines are now redacted: the key name and diff marker stay, the value is masked with mask.Redact, which always masks instead of consulting the heuristics. The test that asserted the value appears now asserts it does not.
The HTTP/SSE and Streamable transports had no authentication at all, while the tools include delete and rotate. Binding `-http :8080` (every interface), as the README showed, exposed them to the LAN. A non-loopback bind now requires `-http-token`; when set, every request must carry `Authorization: Bearer <token>` (constant-time comparison) or gets 401. The README examples bind 127.0.0.1 and document the flag.
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.
What this fixes
Reviewing
devturned up two gaps that contradict the README's "plaintext never reaches the agent" claim, plus four P1 hardening items. Each commit is self-contained.P0
fix(mask)— key-name masking was dead code.KeyNameIsSensitivehad no callers, andMask(value)only received the value.DB_PASSWORD=correcthorsebatterystaple(no digits, no prefix) was returned verbatim byget,list, the MCPgettool andsecret://secrets. NewMaskKey(key, value)applies the key-name rule first; all display paths use it.fix(mcp)—putwas an arbitrary-file read and unlink. The agent chosevalue_file; the server read it, thenos.Removed it. An injected agent could read e.g. a config file into the store and read it back viaget, and delete any file the user could write. Value files must now resolve inside<store>/inbox; symlink and..escapes are refused.keysmith inboxprints the path.P1
fix(store)— lost writes. Read-modify-write with no lock: 20 concurrentsetcalls stored 12 keys. Writers now take an exclusiveflockon<store>/store.lock(key init included). Temp files get a unique name.randomSecretswitched to rejection sampling (the modulo bias was a one-line fix while in the file).fix(redeem)—--allow-pathprefix escape.--target https://hooks.slack.com/services/../../adminsatisfied a/services/binding. Paths with./..segments, encoded slashes (%2f,%5c), backslashes and..;path parameters are now refused;.well-knownstill parses.fix(scan)— the leak report printed the leaked secret. Now redacted (+DB_PASSWORD= Su******ue) viamask.Redact, which always masks.fix(http)— unauthenticated HTTP transports.-http :8080(the README example) exposeddelete/rotateto the LAN. A non-loopback bind now requires-http-token; requests must carryAuthorization: Bearer <token>or get 401. README examples bind127.0.0.1.Verification
gofmt -l .clean,go vet ./...clean,go test ./...all packages pass (internal/mcpgained its first tests)python3 e2e_test.py— passes, including a new assertion that avalue_fileoutside the inbox is refused and the file survivespython3 verify_redeem.py— all 39 checks pass (43 on Linux with the/procchecks)python3 scripts/check-doc-parity.py— all 4 pairs in syncsetkept;services/../../adminrefused; anonymous HTTP request → 401; weak passphrase underDB_PASSWORDnow maskedBreaking changes (0.5.0)
putrefusesvalue_fileoutside<store>/inbox(agents should callkeysmith inbox)-httpon a non-loopback address requires-http-tokenkeysmith scanoutput no longer includes the matched valueCHANGELOG.md is updated under 0.5.0.