Resolve ls paths in safe's own syntax - #28
Merged
Conversation
safe prints paths escaped, but ls passed its argument straight to the list call, which talks to Vault in literal paths, and joined that raw argument to each child before reading it back. A folder whose name holds a colon was unreachable from safe's own output, and reachable in a way that lied from the literal name: the liveness check on a version 2 mount parsed the colon as a key separator, read a shorter path, missed, and dropped every child from the listing -- an empty listing at exit 0. Resolve the root the way the tree walks do, and escape the whole path rather than only the child before handing it to Read. A key or a version cannot scope a listing, so naming one is refused rather than looked up as part of the path. The names ls prints are escaped for the same reason paths and tree escape theirs: what safe prints has to be what safe accepts.
safe ls now resolves its argument like the other listings, so the path syntax rules cover it: it takes a path only, and the names it prints come back escaped.
The command reference skipped ls entirely, though it is one of the first commands anybody reaches for.
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 lswas the last listing command that did not speak safe's own pathsyntax. It passed its argument straight to the list call, which talks to Vault
in literal paths, and joined that raw argument to each child before reading it
back.
What that cost
Fixture: one secret at the literal path
secret/od:d/inner, on the KV v2secret/mount of a real Vault 1.13.2 dev server.Before:
The second one is the bad one.
innerexists and is alive; the listing isempty and the exit code is 0. The liveness check on a version 2 mount escapes
only the child before handing the path to
Read, sosecret/od:d+/innerarrives as
secret/od:d/inner,Readsplits it at the colon into the pathsecret/odand the keyd/inner, misses, andcontinues past the entry.--quickskips that check, which is why it prints the secret the defaultlisting hides.
After:
The change
Three parts, one behaviour:
Resolve the root through
walkRoot, the same helpertree,paths, andvalueshave used since Accept safe's own escaped paths as input #15. A key or a version cannot scope a listing, sonaming one is refused rather than looked up as part of the path.
Escape the whole path, not just the child, before handing it to
Read; themount-version lookup keeps the literal form, which is what it wants.
Escape the names
lsprints, for the same reasonpathsandtreeescapetheirs. Without this the fix is only half a round trip:
ls secretwouldprint a name that
lsitself now refuses.Plain names are untouched —
EscapePathSegmentis the identity on anythingwithout a
\,:, or^.Tests
internal/cli/ls_paths_test.go, seven tests over both mount versions:escaped root accepted on v1 and v2, the quiet-drop regression (the default
listing must agree with
--quick), printed names escaped, output fed back in,plain listings byte-identical in both
-1and default form, and key/versionrefused.
Mutation-tested on each axis independently, against a clean tree:
make checkandgo test -race ./...green. Every before/after pair above isfrom a live Vault, not the fake.
Docs
lsjoins the path-only bullet in the Secret Paths rules, the escaping sectiongains a short note on joining a printed name to the path you listed, and the
command reference gains an
### lssection — it had none, thoughlsis oneof the first commands anybody reaches for. Both examples were run against the
live server before being written down.