script/lint-darwin (make lint-darwin; run by script/check, and its
commands by the Dockerfile lint stage) runs go vet and golangci-lint
with GOOS=darwin and cgo off. Compiling cgo for macOS needs Apple's SDK,
so the three functions that call go-keychain, which is cgo there, move
to keychainunlocker_cgo.go; a macOS build without cgo gets
keychainunlocker_nocgo.go and the macse stub, whose errors name the
missing macOS build with cgo. The rest of the keychain unlocker and its
plain-Go tests are now checked; their findings are fixed without
changing behaviour, and lines over 88 columns in the unchecked files
are wrapped.
Model: opus-5-5
A passphrase unlocker added to a vault that had one, and a PGP, keychain
or Secure Enclave unlocker added on the same day as another of its type,
were written into the existing unlocker's directory file by file, so a
crash part-way left a current unlocker whose files did not belong
together.
Unlocker directories, keychain items and Secure Enclave keys are now
named with the time to the nanosecond, and secret.WriteDir refuses a
directory that exists. Adding a passphrase unlocker writes the new one,
points current-unlocker at it, and only then removes the vault's other
passphrase unlockers.
Model: opus-5-5
init and vault create put the mnemonic into the process environment for
vault.CreateVault to read back, so every program they ran, gpg included,
inherited it, and SB_SECRET_MNEMONIC and SB_UNLOCK_PASSPHRASE were read
at 13 places and never unset. Each command that may need them now reads
both once, in its RunE, into locked buffers on the CLI Instance, and
unsets them at once. The buffers are passed down: vault.CreateVault
takes the mnemonic, a Vault carries Mnemonic and UnlockPassphrase, and
the PGP, keychain and Secure Enclave unlocker constructors take both;
CreatePGPUnlocker sets them on the vault it loads through SetMnemonic
and SetUnlockPassphrase, new in VaultInterface. README warns against
both variables.
Model: opus-5-5
CreatePGPUnlocker got the vault's long-term key from the keychain
unlocker's helper, which on every platform but macOS is a stub that
always fails. It now calls the vault's GetOrDeriveLongTermKey, as adding
a passphrase unlocker does: from the mnemonic, checked against the
vault, or else from the current unlocker. That method joins
VaultInterface. The test GPG key gains an encryption subkey, and a new
test adds a PGP unlocker with the long-term key from the mnemonic and
from a passphrase unlocker, then reads a secret through it.
Model: opus-5-5
A vault name may use only lowercase ASCII letters, digits, `.`, `-` and
`_`, and must not be empty, `.` or `..`; the error now states that rule.
`vault create`, `vault import`, `vault select`, `vault remove`, both
vault names of `mv` and shell completion of a `vault:secret` argument
check the name as typed before building any path from it. Before,
`vault import ..` wrote a long-term key and an unlocker into the state
directory itself, and `vault select ..` made that the current vault.
Model: opus-5-5
CreatePGPUnlocker looked up the GPG key's fingerprint, and the keychain
unlocker got the long-term key, only after writing part of the unlocker,
so a failure there left a directory with no metadata. Both now do every
step that can fail before writing anything. `secret unlocker add pgp`
looks the fingerprint up once, for its duplicate check, and passes it to
CreatePGPUnlocker to record. All four unlocker types write their files
through the new secret.WriteDir, which builds a new directory in a
temporary directory, renames it into place when complete and removes it
on a failure. A directory that already exists, as when an unlocker
replaces one of the same name, is written in place and never removed.
Model: opus-5-5
findUnlockerByID failed on the first unlocker directory whose metadata
could not be checked, read or parsed, so `secret unlocker select` and
`remove` failed when one sorted before the unlocker asked for. It now
skips such a directory with the warning ListUnlockers gives, through
the code both now share. A skipped directory is removed by its
directory name, with RemoveDirAtomic, and cannot be selected. Removing
one whose metadata file is missing or corrupt never counts as removing
the last unlocker; removing one whose metadata file cannot be checked
for or read does, since it may be the only working unlocker.
Model: opus-5-5
A failed command printed the whole usage text after its error, burying
it. The root command's PersistentPreRunE now turns usage off, so an
error from running the command is printed once on its own. Wrong arity,
an unknown flag, a bad flag value, a missing required flag and broken
flag groups still get usage: cobra checks arguments and flag values
before that hook but required flags and flag groups only after it, so
the hook checks those two first. Root SilenceUsage was not used: in
this cobra version it hides usage for argument and flag errors too.
Cobra still prints the error; Entry is unchanged.
Model: opus-5-5
Vault.GetSecret and Vault.GetSecretVersion return the decrypted value
as a *memguard.LockedBuffer instead of copying it into an ordinary
[]byte that nothing wiped. Every caller destroys the buffer, and
`secret get` writes its bytes straight to stdout, still with no
trailing newline. Instance.Print, which formatted through fmt and had
no other callers, is removed, and so is a debug log line in
`get --version` that held the plaintext value.
Model: opus-5-5
Adding a PGP unlocker checked unlockers.d for a duplicate and, when the
directory or an unlocker's metadata file could not be read, reported no
duplicate and went on. The check now reads unlockers.d itself and stops
with an error naming the path and cause; `unlocker list` keeps skipping
unlockers it cannot read.
The same flaw guarded removing the last unlocker and removing a vault
(an unreadable secrets directory counted as no secrets) and vault
import (an unreadable pub.age counted as no long-term key). Those now
stop with an error too.
`vault rm` and `unlocker rm` keep the state directory lock and now do
their work in an unexported function, as `vault import` does.
Model: opus-5-5
vault.CreateVault now checks for the vault before writing anything and
fails with "vault NAME already exists" (vault.ErrVaultExists). secret
init and secret vault create call it while holding the state directory
lock, so two creates at once cannot both pass the check. Before, either
command over an existing vault replaced its metadata, passphrase
unlocker and longterm.age, so none of its secrets could be decrypted.
Both commands now ask for the unlocker passphrase before creating the
vault, so one stopped at that prompt leaves no vault without an
unlocker behind, which they would then refuse to create again.
The lock tests set up the vault "work" instead of "default", which init
now refuses to create again.
Model: opus-5-5
The test that each changing command waits for the state directory lock
slept a fixed 100 ms per command. It now polls the goroutine stacks until
the command is parked in vault.LockStateDir, checks the state directory
is unchanged, and releases the lock; a command that takes no lock still
fails by finishing first.
newTwoVaultFs creates its two vaults, each with a passphrase unlocker,
once, and returns a fresh copy of them on every call, so the six path and
move tests no longer each pay for two passphrase key derivations.
Model: opus-5-5
On a case-insensitive filesystem (the macOS default) "Foo" and "foo" name
one secret, so `secret mv --force Foo foo` removed the destination, which
was the source, and lost the secret with every version. Between vaults the
copy replaced the source, and removing the source then removed the copy.
Both kinds of move now compare the two secret directories with
os.SameFile before changing anything and reject the move if they are one,
with or without --force. The tests give one secret two names with
symbolic links on the real filesystem.
Model: opus-5-5
PGPUnlocker.GetID() panicked when its metadata could not be read or
parsed, which took down `secret unlocker list` for every unlocker. It
now warns with the unlocker's directory and returns `pgp-unknown`;
metadata with an empty GPG key ID counts as corrupt too.
ListUnlockers now skips, with a warning, an unlocker whose metadata
file cannot be checked for, read or parsed, as it already did for a
missing one. The listing's ID lookup skips such a directory without
warning again.
This is the first half of the issue only. Passing the mnemonic in
memory moved to #60.
Model: opus-5-5
Co-authored-by: clawbot <sneak+clawbot@sneak.cloud>
`secret mv --force x x` deleted the secret: a move within one vault
removes an existing destination before renaming the source onto it. The
same happened for `work:x work:`, `work:x work` and `work:x ""`, and for
`work:x work/:x`, which named one vault two ways and so was taken for a
move between vaults.
A move whose two names are the same is now rejected before anything
changes. Every vault named with `vault:` must be one of the existing
vaults by exact name, checked before choosing between the two kinds of
move. A move within a named vault no longer makes it the current vault.
The test runs each rejected move on a copy of two in-memory vaults and
requires the exact error and an unchanged state directory.
Model: opus-5-5
Each command that changes the state directory holds one lock: flock(2)
on `lock` in the state directory, dropped by the kernel if the process
dies, or a process-wide mutex on the in-memory test filesystem. It
covers the state directory, not each vault, because `currentvault`,
`vault create` and cross-vault moves span vaults, and a lock file in a
vault would be deleted by `vault remove` under a waiting command.
Files go through `secret.WriteFileAtomic`; versions, new secrets and
cross-vault copies are built in a temporary directory and renamed into
place; removals rename out of the way first. Left for later: replacing
an unlocker (#71) and deleting
what an interrupted command leaves under a `.tmp-` name
(#75).
Model: opus-5-5
Co-authored-by: clawbot <sneak+clawbot@sneak.cloud>
version rm, version promote and get --version joined the version
argument into a path unchecked, so "", ".", "..", "../../.." removed or
read every version, the secret, the vault or directories above it.
A version is now accepted only if it is one of the versions
ListVersions lists for the secret, compared by name before any path is
built (secret.VersionExists, used by all three). An empty --version is
rejected instead of meaning the current version: GetSecretVersion no
longer treats "" as current, and GetSecret looks the current version
up itself.
Model: opus-5-5
Entry() now returns the exit code and only main calls os.Exit, so the
deferred memguard.Purge() in Entry() runs on success and on error;
before, os.Exit(1) skipped every deferred Destroy().
SIGINT and SIGTERM go through memguard's handler, which wipes every
buffer and exits with status 1. The passphrase prompt turns terminal
echo off until its read returns, and the handler exits before that, so
the handler first restores the terminal settings saved at startup, but
only when this process is in the terminal's foreground process group: a
background process that changes the terminal is stopped instead of
exiting.
Model: opus-5-5
`secret rm ..` deleted the whole vault; `secret rm .` and `secret rm ""`
deleted every secret. rm, mv, the version commands, encrypt and decrypt
built paths from the name unchecked; import checked it only after
reading the source file.
Each now calls vault.ValidateSecretName, which wraps the existing rule,
on the name as given, before building any path; MoveSecret checks both
names before switching the current vault. Its error and README.md state
the rule. The test-only copy of the rule in internal/secret is removed.
The regression test runs each rejected command on a copy of two
in-memory vaults and requires the exact error and an unchanged state
directory.
Model: opus-5-5
The passphrase protecting the keychain unlocker's age key was a plain
string passed through encoding/json, leaving copies in ordinary memory
when an unlocker was created and each time one was used.
It is now generated into a locked buffer, and KeychainData, moved to
keychaindata.go, which is not darwin-only so its tests run on Linux,
writes and reads the keychain JSON itself: encode copies the parts
straight into a locked buffer, and decodeKeychainData takes the
passphrase from a json.RawMessage that it wipes. The JSON field names
are unchanged. keychainunlocker.go only calls this code and stores the
item from the locked buffer without a string copy.
Model: opus-5-5
The size tests skip a case whose secret needs more locked memory than
the process can lock, found by locking a buffer of that size: memguard
panics otherwise, and a plain `docker build .` runs under an 8 MiB
RLIMIT_MEMLOCK. script/cibuild, or any process allowed to lock past the
limit, runs every case.
The build stage stamps the VERSION build argument, else
`git describe --tags --always`, and fails when .git is present but
yields no version. `make build` stamps `git describe` too instead of
the fixed 0.1.0. .dockerignore keeps .git/config out; script/docker is
now the canonical copy.
Model: opus-5-5
Co-authored-by: clawbot <sneak+clawbot@sneak.cloud>
Bumps golangci-lint from v2.1.6 (digest-only pin in the `Dockerfile` lint stage) to v2.12.2, pinned by tag and digest (Debian-based image).
Replaces `.golangci.yml` with the canonical strict config: all linters enabled except the standard disable list (`exhaustruct`, `depguard`, `godot`, `wsl`, `wrapcheck`, `varnamelen`), `lll` at 88, `funlen` 80/50, `cyclop` 15, `dupl` 100, and test files are now linted (the old config had `tests: false`, an enable-only list of ~20 linters, `lll` 120, and a blanket exclusion of `internal/macse`).
The stricter config surfaced ~1550 findings, all fixed:
- `wsl_v5` (439) / `nlreturn` (24): blank-line insertions
- `lll` (309): line wrapping at 88 columns; long literals split with `+` concatenation, values unchanged
- `noinlineerr` (130): `if err := ...` split into assignment plus check
- `paralleltest` (116): `t.Parallel()` added to tests without shared state; reasoned `//nolint` where `t.Setenv` or shared fixtures forbid it
- `err113` (97): package-level sentinel errors (new `internal/vault/errors.go`), `%w` wrapping, `errors.Is`
- `perfsprint` (74) / `modernize` (39) / `intrange`: `strconv`, `errors.New`, `slices.Contains`, `any`, `SplitSeq`
- `goconst` (40) / `dupword` (41) / `testifylint` (42) / `thelper` (33): constants, assertion fixes, `t.Helper()`
- `noctx` (22): `exec.CommandContext` for gpg/CLI invocations
- `testpackage` (18): black-box tests moved to `_test` packages where they use only exported identifiers; white-box files carry a reasoned `//nolint`
- `funlen`/`cyclop`/`gocognit`/`nestif`/`dupl`: behavior-preserving helper extraction
- assorted singletons: `gosec`, `gosmopolitan`, `funcorder`, `nonamedreturns`, `makezero`, `prealloc`, `godox`, `nolintlint`, `ireturn`, `nilnil`, `gochecknoinits`
## User-visible strings
**None changed.** Every error message this branch composes is byte-identical to the one `main` composes.
The `err113` sentinels are shaped so `fmt.Errorf` reassembles the original text around them: the sentinel carries the fixed words and the caller supplies the interpolated value in the position it has always occupied. Where the value sits mid-sentence the sentinel holds only a fragment (e.g. `vault.ErrVaultNotFound` is `"does not exist"`, composed by its caller as `vault <name> does not exist`); each such sentinel documents the message it participates in.
Verified mechanically, not by inspection: every `fmt.Errorf` and `errors.New` call site in both trees is parsed, the `Error()` text of any sentinel passed to `%w` is substituted in, and the resulting sets of composed message templates are compared. All 350 templates `main` produces are still produced, character for character. The set of lost or altered messages is empty.
## `unlocker list`
`findUnlockerIDByMetadata` returns `(string, error)` rather than signalling failure with an empty ID, so an unreadable `unlockers.d` is no longer indistinguishable from "no matching entry". `UnlockersList` skips such an entry with a warning naming the directory — its behavior before the scan was extracted into a helper — instead of emitting a row under a synthesized fallback ID that no `unlocker remove` or `unlocker select` can match and that suppresses the current-unlocker marker. The duplicate-check and shell-completion callers skip on the same condition, matching their pre-extraction behavior. Covered by `internal/cli/unlockers_list_test.go`.
`TODO.md` records the change plus follow-ups (version-completion TODOs formerly in code comments, darwin-gated files exceeding 88 columns that Linux CI does not lint).
`make check` is green and the pinned v2.12.2 image reports `0 issues.` Note the test suite needs the memlock ulimit from `script/cibuild` for the 10MB memguard test; that requirement is pre-existing.
Not changed: `script/bootstrap` installs golangci-lint via the system package manager (no version pin to bump), and `script/lint` invokes whatever `golangci-lint` is on PATH. golangci-lint v2.12 deprecates `gomodguard` in favor of `gomodguard_v2` (warning only); the canonical config owns that decision.
Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #29
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
Audit of the codebase found 9 locations where errors or anomalous
conditions were silently swallowed or only logged via Debug(). Users
should be informed when something unexpected happens, even if the
program can continue.
Changes:
- DetermineStateDir: warn on config dir fallback to ~/.config
- info_helper: warn when vault/secret stats cannot be read
- unlockers list: warn on metadata read/parse failures (fixes FIXMEs)
- unlockers list: warn on fallback ID generation
- checkUnlockerExists: warn on errors during duplicate checking
- completions: warn on unlocker metadata read/parse failures
- version list: upgrade metadata load failure from Debug to Warn
- secrets: upgrade file close failure from Debug to Warn
- version naming: warn on malformed version directory names
Closes#19
When an unlocker directory exists but is missing unlocker-metadata.json,
log a debug warning and skip it instead of returning a hard error that
crashes the entire 'unlocker ls' command.
Closes#1
Change NewCLIInstance() and NewCLIInstanceWithFs() to return
(*Instance, error) instead of panicking on DetermineStateDir failure.
Callers in RunE contexts propagate the error. Callers in command
construction (for shell completion) use log.Fatalf. Test callers
use t.Fatalf.
Addresses review feedback on PR #18.
Verifies that getLongTermPrivateKey reads the derivation index from
vault metadata instead of using hardcoded index 0. Test creates a
mock vault with DerivationIndex=5 and confirms the derived key
matches index 5.
Previously, getLongTermPrivateKey() always used derivation index 0 when
deriving the long-term key from a mnemonic. This caused wrong key
derivation for vaults with index > 0 (second+ vault from same mnemonic),
leading to silent data corruption in keychain unlocker creation.
Now reads the vault's actual DerivationIndex from vault-metadata.json.
- isValidSecretName() now rejects names with '..' path components (e.g. foo/../bar)
- GetSecretObject() now calls isValidSecretName() before building paths
- Added test cases for mid-path traversal patterns
When os.UserConfigDir() fails, DetermineStateDir falls back to
os.UserHomeDir(). Previously the error from UserHomeDir was discarded,
which could result in a dangerous root-relative path (/.config/...) if
both calls fail.
Now DetermineStateDir returns (string, error) and propagates failures
from both UserConfigDir and UserHomeDir.
Closes#14
The isValidSecretName() regex only allowed lowercase letters [a-z], rejecting
valid secret names containing uppercase characters (e.g. AWS access key IDs).
Changed regex from ^[a-z0-9\.\-\_\/]+$ to ^[a-zA-Z0-9\.\-\_\/]+$ and added
tests for uppercase secret names in both vault and secret packages.
Add isValidSecretName() check at the top of GetSecretVersion(), matching
the existing validation in AddSecret(). Without this, crafted secret names
containing path traversal sequences (e.g. '../../../etc/passwd') could be
used to read files outside the vault directory.
Add regression tests for both GetSecretVersion and GetSecret.
Closes#13
The stub previously panicked on all methods including NewKeychainUnlocker,
which is called from vault code when processing keychain-type unlocker
metadata. This caused crashes on Linux/Windows when a vault synced from
macOS contained keychain unlockers.
Now returns proper error values, allowing graceful degradation and
cross-platform vault portability.
CreatePassphraseUnlocker already encrypts and writes the long-term
private key to longterm.age. The Init command was doing this a second
time, overwriting the file with a functionally equivalent but
separately encrypted blob. This was wasteful and a maintenance hazard.
The decrypted data from io.ReadAll was copied into a memguard
LockedBuffer but the original byte slice was never zeroed, leaving
plaintext in swappable, dumpable heap memory.
sneak
clawbot
