Fix segfault on writes to a path naming a key - #14
Merged
Conversation
Read returned a nil *Secret alongside KeyNotFoundError, while the missing-secret branch returned an empty one. Callers that tolerate a not-found error and keep using the value therefore segfaulted on the first Secret method, since every accessor dereferences its receiver. Return the already-allocated empty secret so both not-found branches agree, and correct the doc comment, which claimed a nil secret and no error where the code does the opposite.
set, paste, ask, ssh, rsa, and dhparam write a whole secret and cannot honour a path naming a key or a version. Vault.Write said so, but only once it was reached: the value had already been prompted for and the key material already generated by then, and the work was discarded. Check the path before connecting, reusing Write's wording. Escaped colons and carets stay part of the path, so a secret whose name contains one is still writable.
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.
safe set secret/foo:bar k=vsegfaults. So dopaste,ask,ssh,rsa, anddhparamwhen the path names a key that does not yet exist.Cause
Vault.Readreturns a nil*SecretalongsideKeyNotFoundError(vault.go:158), while the missing-secret branch returns an empty one.
IsNotFoundcovers both (errors.go:29), so the caller shapefalls through holding nil, and every
Secretaccessor dereferences itsreceiver.
The doc comment above
Readclaimed the opposite of what the code did —"a nil
*Secretwill be returned, with no error" — on both clauses.The tell is that the same command works when the key does exist:
Readsucceeds, and
Writerejects the syntax properly. The correct behaviorwas already there; only the not-found path skipped it.
Measured, Vault 1.13.2 KV v2
Seed
safe set secret/foo user=admin, then:set secret/foo:bar k=vcannot write to paths in /path:key notation, rc=1paste secret/foo:nope k=vssh secret/foo:privatersa secret/foo:privatedhparam secret/foo:dhparam-pemset secret/foo:user k=v(key exists)gen/uuidonpath:keyWhat this does
Commit 1 returns the already-allocated empty secret instead of nil, so
both not-found branches agree, and corrects the doc comment. This alone
removes all five crashes.
Commit 2 is not a crash fix — it moves the complaint earlier. Without
it the error still arrives from
Write, but only after the value has beenprompted for and the key material generated.
safe dhparam 4096spun forthe full generation before failing; it now returns immediately. The guard
runs before
connect(), so no Vault round-trip happens either.Escaped colons and carets stay part of the path:
safe set 'secret/we\:ird' user=adminstill writes, verified against a live server with thevaultCLI confirming the literal
we:irdpath.Verification
make checkgreen. Three newpkg/vaulttests pin the never-nilinvariant; one reproduces the production crash in-process and fails with
the identical stack before the fix.
internal/clitests drive the real handlers, which is whatpins the guard ahead of
connect().from
develop.compares a
*Secretagainst nil, and no test asserted it.Note for reviewers
IsNotFounddeliberately keeps its umbrella meaning. Narrowing it wouldchange the
-fdelete paths (secrets.go:542,549) and the tree walks(tree.go:411,427,798,905), which rely on it covering both cases.
CreateSignedCertificate(vault.go:1044) has the same hazardous shape andis exported, but has no caller in the repo. Commit 1 makes it safe without
touching it.